[DEV-8000] ir::asg: Error types for unresolved identifiers during sorting

This checks explicitly for unresolved objects while sorting and provides an
explicit error for them.  For example, this will catch externs that have no
concrete resolution.

This previously fell all the way through to the unreachable! block.  The old
POC implementation was catching unresolved objects, albeit with a debug
error.

RELEASES.md

@@ -14,6 +14,15 @@ commits that introduce the changes.  To make a new release, run
 `tools/mkrelease`, which will handle updating the heading for you.
 
 
+NEXT
+====
+Linker
+------
+- Provide useful error for unresolved identifiers.
+  - This was previously falling through to an `unreachable!` block,
+    producing a very opaque and useless internal error message.
+
+
 v17.4.2 (2020-05-13)
 ====================
 This release adds GraphML output for linked objects to allow us to

tamer/src/ir/asg/base.rs

@@ -295,7 +295,7 @@ where
         }
 
         while let Some(index) = dfs.next(&self.graph) {
-            let ident = self.get(index).expect("missing node");
+            let ident = self.get(index).expect("missing node").resolved()?;
 
             match ident.kind() {
                 Some(kind) => match kind {
@@ -386,7 +386,9 @@ where
 mod test {
     use super::super::graph::AsgError;
     use super::*;
-    use crate::ir::asg::{Dim, IdentObject, TransitionError, TransitionResult};
+    use crate::ir::asg::{
+        Dim, IdentObject, TransitionError, TransitionResult, UnresolvedError,
+    };
     use crate::ir::legacyir::SymDtype;
     use crate::sym::SymbolIndex;
     use std::cell::RefCell;
@@ -400,6 +402,7 @@ mod test {
         fail_redeclare: RefCell<Option<TransitionError>>,
         fail_extern: RefCell<Option<TransitionError>>,
         fail_set_fragment: RefCell<Option<TransitionError>>,
+        fail_resolved: RefCell<Option<UnresolvedError>>,
     }
 
     impl<'i> IdentObjectData<'i> for StubIdentObject<'i> {
@@ -446,6 +449,14 @@ mod test {
             Ok(self)
         }
 
+        fn resolved(&self) -> Result<&StubIdentObject<'i>, UnresolvedError> {
+            if self.fail_resolved.borrow().is_some() {
+                return Err(self.fail_resolved.replace(None).unwrap());
+            }
+
+            Ok(self)
+        }
+
         fn extern_(
             mut self,
             kind: IdentKind,
@@ -1514,4 +1525,36 @@ mod test {
 
         Ok(())
     }
+
+    /// A graph containing unresolved objects cannot be sorted.
+    #[test]
+    fn graph_sort_fail_unresolved() -> SortableAsgResult<(), u8> {
+        let mut sut = Sut::new();
+
+        let sym = symbol_dummy!(1, "unresolved");
+        let node = sut
+            .declare(&sym, IdentKind::Meta, Default::default())
+            .unwrap();
+        let ident = sut.get(node).unwrap();
+
+        let expected = UnresolvedError::Missing {
+            name: sym.to_string(),
+        };
+
+        // Cause resolved() to fail.
+        ident.fail_resolved.replace(Some(expected.clone()));
+
+        let result = sut
+            .sort(&vec![node])
+            .expect_err("expected sort failure due to unresolved identifier");
+
+        match result {
+            SortableAsgError::UnresolvedObject(err) => {
+                assert_eq!(expected, err);
+            }
+            _ => panic!("expected SortableAsgError::Unresolved: {:?}", result),
+        }
+
+        Ok(())
+    }
 }

tamer/src/ir/asg/graph.rs

@@ -22,6 +22,7 @@
 use super::ident::IdentKind;
 use super::object::{
     FragmentText, IdentObjectData, IdentObjectState, Source, TransitionError,
+    UnresolvedError,
 };
 use super::Sections;
 use crate::sym::Symbol;
@@ -186,6 +187,11 @@ where
     O: IdentObjectData<'i>,
     Ix: IndexType,
 {
+    /// Sort graph into [`Sections`].
+    ///
+    /// Sorting will fail if the graph contains unresolved objects,
+    ///   or identifiers whose kind cannot be determined
+    ///   (see [`UnresolvedError`]).
     fn sort(
         &'i self,
         roots: &[ObjectRef<Ix>],
@@ -292,6 +298,16 @@ impl From<TransitionError> for AsgError {
 ///   sorting process.
 #[derive(Debug, PartialEq)]
 pub enum SortableAsgError<Ix: IndexType> {
+    /// An unresolved object was encountered during sorting.
+    ///
+    /// An unresolved object means that the graph has an incomplete picture
+    ///   of the program,
+    ///     and so sorting cannot be reliably performed.
+    /// Since all objects are supposed to be resolved prior to sorting,
+    ///   this represents either a problem with the program being compiled
+    ///   or a bug in the compiler itself.
+    UnresolvedObject(UnresolvedError),
+
     /// The kind of an object encountered during sorting could not be
     ///   determined.
     ///
@@ -305,9 +321,16 @@ pub enum SortableAsgError<Ix: IndexType> {
     Cycles(Vec<Vec<ObjectRef<Ix>>>),
 }
 
+impl<Ix: IndexType> From<UnresolvedError> for SortableAsgError<Ix> {
+    fn from(err: UnresolvedError) -> Self {
+        Self::UnresolvedObject(err)
+    }
+}
+
 impl<Ix: IndexType> std::fmt::Display for SortableAsgError<Ix> {
     fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
         match self {
+            Self::UnresolvedObject(err) => std::fmt::Display::fmt(err, fmt),
             Self::MissingObjectKind(name) => write!(
                 fmt,
                 "internal error: missing object kind for object `{}` (this may be a compiler bug!)",
@@ -320,7 +343,10 @@ impl<Ix: IndexType> std::fmt::Display for SortableAsgError<Ix> {
 
 impl<Ix: IndexType> std::error::Error for SortableAsgError<Ix> {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        None
+        match self {
+            Self::UnresolvedObject(err) => Some(err),
+            _ => None,
+        }
     }
 }
 

tamer/src/ir/asg/mod.rs

@@ -203,7 +203,7 @@ pub use graph::{
 pub use ident::{DataType, Dim, IdentKind, IdentKindError};
 pub use object::{
     FragmentText, IdentObject, IdentObjectData, IdentObjectState, Source,
-    TransitionError, TransitionResult,
+    TransitionError, TransitionResult, UnresolvedError,
 };
 pub use section::{Section, SectionIterator, Sections};
 

tamer/src/ir/asg/object.rs

@@ -212,6 +212,23 @@ where
     ///   since rules may vary between implementations.
     fn resolve(self, kind: IdentKind, src: Source<'i>) -> TransitionResult<T>;
 
+    /// Assertion to return self if identifier is resolved,
+    ///   otherwise failing with [`UnresolvedError`].
+    ///
+    /// This simplifies working with identifiers without having to match on
+    ///   specific variants,
+    ///     and will continue to work if new variants are added in the
+    ///     future that are considered to be unresolved.
+    ///
+    /// Since this does not cause a state transition and is useful in
+    ///   contexts where ownership over the identifier is not possible,
+    ///     this accepts and returns a reference to the identifier.
+    ///
+    /// At present,
+    ///   both [`IdentObject::Missing`] and [`IdentObject::Extern`] are
+    ///   considered to be unresolved.
+    fn resolved(&self) -> Result<&T, UnresolvedError>;
+
     /// Resolve identifier against an extern declaration or produce an
     ///   extern.
     ///
@@ -373,6 +390,25 @@ impl<'i> IdentObjectState<'i, IdentObject<'i>> for IdentObject<'i> {
         }
     }
 
+    fn resolved(&self) -> Result<&IdentObject<'i>, UnresolvedError> {
+        match self {
+            IdentObject::Missing(name) => Err(UnresolvedError::Missing {
+                name: name.to_string(),
+            }),
+
+            IdentObject::Extern(name, ref kind, ref src) => {
+                Err(UnresolvedError::Extern {
+                    name: name.to_string(),
+                    kind: kind.clone(),
+                    pkg_name: src.pkg_name.map(|s| s.to_string()),
+                })
+            }
+
+            IdentObject::Ident(_, _, _)
+            | IdentObject::IdentFragment(_, _, _, _) => Ok(self),
+        }
+    }
+
     fn extern_(
         self,
         kind: IdentKind,
@@ -524,6 +560,52 @@ impl std::error::Error for TransitionError {
     }
 }
 
+/// Resolved identifier was expected.
+#[derive(Clone, Debug, PartialEq)]
+pub enum UnresolvedError {
+    /// Expected identifier is missing and nothing about it is known.
+    Missing { name: String },
+
+    /// Expected identifier has not yet been resolved with a concrete
+    ///   definition.
+    Extern {
+        /// Identifier name.
+        name: String,
+        /// Expected identifier type.
+        kind: IdentKind,
+        /// Name of package where the extern was defined.
+        pkg_name: Option<String>,
+    },
+}
+
+impl std::fmt::Display for UnresolvedError {
+    fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
+        match self {
+            UnresolvedError::Missing { name } => {
+                write!(fmt, "missing expected identifier `{}`", name,)
+            }
+
+            UnresolvedError::Extern {
+                name,
+                kind,
+                pkg_name,
+            } => write!(
+                fmt,
+                "unresolved extern `{}` of type `{}`, declared in `{}`",
+                name,
+                kind,
+                pkg_name.as_ref().unwrap_or(&"<unknown>".into()),
+            ),
+        }
+    }
+}
+
+impl std::error::Error for UnresolvedError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        None
+    }
+}
+
 /// Compiled fragment for identifier.
 ///
 /// This represents the text associated with an identifier.
@@ -782,8 +864,24 @@ mod test {
         }
 
         #[test]
-        fn ident_object_ident() {
+        fn resolved_on_missing() {
             let sym = symbol_dummy!(1, "missing");
+
+            let result = IdentObject::declare(&sym)
+                .resolved()
+                .expect_err("expected error asserting resolved() on missing");
+
+            match result {
+                UnresolvedError::Missing { name: e_name } => {
+                    assert_eq!(sym.to_string(), e_name);
+                }
+                _ => panic!("expected UnresolvedError {:?}", result),
+            }
+        }
+
+        #[test]
+        fn ident_object_ident() {
+            let sym = symbol_dummy!(1, "ident");
             let kind = IdentKind::Meta;
             let src = Source {
                 desc: Some("ident ctor".into()),
@@ -798,6 +896,25 @@ mod test {
             );
         }
 
+        #[test]
+        fn resolved_on_ident() {
+            let sym = symbol_dummy!(1, "ident resolve");
+            let kind = IdentKind::Meta;
+            let src = Source {
+                desc: Some("ident ctor".into()),
+                ..Default::default()
+            };
+
+            assert_eq!(
+                &IdentObject::Ident(&sym, kind.clone(), src.clone()),
+                IdentObject::declare(&sym)
+                    .resolve(kind.clone(), src.clone())
+                    .unwrap()
+                    .resolved()
+                    .unwrap(),
+            );
+        }
+
         // Note that we don't care about similar sources.  It's expected
         // that the system populating the ASG will only resolve local
         // symbols, and so redeclarations should represent that multiple
@@ -834,7 +951,7 @@ mod test {
 
             #[test]
             fn ident_object() {
-                let sym = symbol_dummy!(1, "missing");
+                let sym = symbol_dummy!(1, "extern");
                 let kind = IdentKind::Class(Dim::from_u8(1));
                 let src = Source {
                     desc: Some("extern".into()),
@@ -847,6 +964,72 @@ mod test {
                 );
             }
 
+            #[test]
+            fn resolved_on_extern() {
+                let sym = symbol_dummy!(1, "extern resolved");
+                let kind = IdentKind::Class(Dim::from_u8(1));
+                let pkg_name = symbol_dummy!(2, "pkg/name");
+                let src = Source {
+                    pkg_name: Some(&pkg_name),
+                    desc: Some("extern".into()),
+                    ..Default::default()
+                };
+
+                let result =
+                    IdentObject::Extern(&sym, kind.clone(), src.clone())
+                        .resolved()
+                        .expect_err(
+                            "expected error asserting resolved() on extern",
+                        );
+
+                match result {
+                    UnresolvedError::Extern {
+                        name: e_name,
+                        kind: e_kind,
+                        pkg_name: e_pkg_name,
+                    } => {
+                        assert_eq!(sym.to_string(), e_name);
+                        assert_eq!(kind, e_kind);
+                        assert_eq!(Some(pkg_name.to_string()), e_pkg_name);
+                    }
+                    _ => panic!("expected UnresolvedError: {:?}", result),
+                }
+            }
+
+            #[test]
+            fn resolved_on_extern_error_fmt_without_pkg() {
+                let meta = IdentKind::Meta;
+                let err = UnresolvedError::Extern {
+                    name: "foo".into(),
+                    kind: IdentKind::Meta,
+                    pkg_name: None,
+                };
+
+                let msg = format!("{}", err);
+
+                assert!(msg.contains("`foo`"));
+                assert!(msg.contains("in `<unknown>`"));
+                assert!(msg.contains(&format!("`{}`", meta)));
+            }
+
+            #[test]
+            fn resolved_on_extern_error_fmt_with_pkg() {
+                let meta = IdentKind::Meta;
+                let pkg = "pkg".to_string();
+
+                let err = UnresolvedError::Extern {
+                    name: "foo".into(),
+                    kind: IdentKind::Meta,
+                    pkg_name: Some(pkg.clone()),
+                };
+
+                let msg = format!("{}", err);
+
+                assert!(msg.contains("`foo`"));
+                assert!(msg.contains(&format!("in `{}`", pkg)));
+                assert!(msg.contains(&format!("`{}`", meta)));
+            }
+
             // Extern first, then identifier
             #[test]
             fn redeclare_compatible_resolves() {
@@ -1020,6 +1203,27 @@ mod test {
             );
         }
 
+        #[test]
+        fn resolved_on_fragment() {
+            let sym = symbol_dummy!(1, "tofrag resolved");
+            let src = Source {
+                generated: true,
+                ..Default::default()
+            };
+
+            let kind = IdentKind::Meta;
+            let ident = IdentObject::declare(&sym)
+                .resolve(kind.clone(), src.clone())
+                .unwrap();
+            let text = FragmentText::from("a fragment for resolved()");
+            let ident_with_frag = ident.set_fragment(text.clone());
+
+            assert_eq!(
+                Ok(&IdentObject::IdentFragment(&sym, kind, src, text)),
+                ident_with_frag.unwrap().resolved(),
+            );
+        }
+
         #[test]
         fn add_fragment_to_fragment_fails() {
             let sym = symbol_dummy!(1, "badsym");

tamer/src/obj/xmle/writer/xmle.rs

@@ -294,7 +294,10 @@ impl<W: Write> XmleWriter<W> {
 
                     self.writer.write_event(Event::Empty(sym))?;
                 }
-                _ => unreachable!("filtered out during sorting"),
+                _ => unreachable!(
+                    "identifier should have been filtered out during sorting: {:?}",
+                    ident,
+                ),
             }
         }