From 96ffd5f6e5d7b26b74758640482de2d614bcc633 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 1 Jul 2020 15:40:21 -0400 Subject: [PATCH] [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 | 9 ++ tamer/src/ir/asg/base.rs | 47 ++++++- tamer/src/ir/asg/graph.rs | 28 +++- tamer/src/ir/asg/mod.rs | 2 +- tamer/src/ir/asg/object.rs | 208 +++++++++++++++++++++++++++++- tamer/src/obj/xmle/writer/xmle.rs | 5 +- 6 files changed, 292 insertions(+), 7 deletions(-) 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, + ), } }