diff --git a/RELEASES.md b/RELEASES.md index 4c64ca62..c9e8e75c 100644 --- a/RELEASES.md +++ b/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 diff --git a/tamer/src/ir/asg/base.rs b/tamer/src/ir/asg/base.rs index 9e72f2e8..b7759759 100644 --- a/tamer/src/ir/asg/base.rs +++ b/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>, fail_extern: RefCell>, fail_set_fragment: RefCell>, + fail_resolved: RefCell>, } 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(()) + } } diff --git a/tamer/src/ir/asg/graph.rs b/tamer/src/ir/asg/graph.rs index ebadf4fc..d5c36841 100644 --- a/tamer/src/ir/asg/graph.rs +++ b/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], @@ -292,6 +298,16 @@ impl From for AsgError { /// sorting process. #[derive(Debug, PartialEq)] pub enum SortableAsgError { + /// 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 { Cycles(Vec>>), } +impl From for SortableAsgError { + fn from(err: UnresolvedError) -> Self { + Self::UnresolvedObject(err) + } +} + impl std::fmt::Display for SortableAsgError { 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 std::fmt::Display for SortableAsgError { impl std::error::Error for SortableAsgError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - None + match self { + Self::UnresolvedObject(err) => Some(err), + _ => None, + } } } diff --git a/tamer/src/ir/asg/mod.rs b/tamer/src/ir/asg/mod.rs index 9f54569f..1b66d92e 100644 --- a/tamer/src/ir/asg/mod.rs +++ b/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}; diff --git a/tamer/src/ir/asg/object.rs b/tamer/src/ir/asg/object.rs index e67799d7..658bb5a0 100644 --- a/tamer/src/ir/asg/object.rs +++ b/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; + /// 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, + }, +} + +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(&"".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 ``")); + 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"); diff --git a/tamer/src/obj/xmle/writer/xmle.rs b/tamer/src/obj/xmle/writer/xmle.rs index 3d2e606d..9f57ce0c 100644 --- a/tamer/src/obj/xmle/writer/xmle.rs +++ b/tamer/src/obj/xmle/writer/xmle.rs @@ -294,7 +294,10 @@ impl XmleWriter { self.writer.write_event(Event::Empty(sym))?; } - _ => unreachable!("filtered out during sorting"), + _ => unreachable!( + "identifier should have been filtered out during sorting: {:?}", + ident, + ), } }