From 24ee0413738a744486104e536b3623de422fd30f Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Mon, 10 Jul 2023 16:24:36 -0400 Subject: [PATCH] tamer: asg::air: Support abstract biding of `Expr`s This produces a representation of abstract identifiers on the graph, for `Expr`s at least. The next step will probably be to get this working end-to-end in the xmli output before extending it to the other remaining bindable contexts. DEV-13163 --- tamer/src/asg/air/expr.rs | 59 +++++++++++++---- tamer/src/asg/air/expr/test.rs | 70 +++++++++++++++++++++ tamer/src/asg/air/tpl/test.rs | 49 +++++++++++++++ tamer/src/asg/error.rs | 46 ++++++++++++-- tamer/src/asg/graph/object/ident.rs | 98 ++++++++++++++++++++++++++--- 5 files changed, 297 insertions(+), 25 deletions(-) diff --git a/tamer/src/asg/air/expr.rs b/tamer/src/asg/air/expr.rs index 7aad237e..a187c1ba 100644 --- a/tamer/src/asg/air/expr.rs +++ b/tamer/src/asg/air/expr.rs @@ -138,17 +138,54 @@ impl ParseState for AirExprAggregate { } } - (BuildingExpr(_, oi), AirBind(BindIdentAbstract(meta_name))) => { - diagnostic_todo!( - vec![ - oi.note("for this expression"), - meta_name.note( - "attempting to bind an abstract identifier with \ - this metavariable" - ), - ], - "attempt to bind abstract identifier to expression", - ) + (BuildingExpr(es, oi), AirBind(BindIdentAbstract(meta_name))) => { + // To help mitigate potentially cryptic errors down the line, + // let's try to be helpful and notify the user when + // they're trying to do something that almost certainly + // will not succeed. + match ctx.dangling_expr_oi() { + // The container does not support dangling expressions + // and so there is no chance that this expression will + // be expanded in the future. + None => { + let rooting_span = ctx + .rooting_oi() + .map(|oi| oi.widen().resolve(ctx.asg_ref()).span()); + + // Note that we _discard_ the attempted bind token + // and so remain in a dangling state. + Transition(BuildingExpr(es, oi)).err( + AsgError::InvalidAbstractBindContext( + meta_name, + rooting_span, + ), + ) + } + + // We don't care what our container is, + // only that the above check passed. + // That is: + // the above check is entirely optional and intended + // only as a debugging aid for users. + Some(_) => { + let oi_meta_ident = + ctx.lookup_lexical_or_missing(meta_name); + + let oi_abstract = oi_meta_ident.new_abstract_ident( + ctx.asg_mut(), + meta_name.span(), + ); + + // Note that we are still dangling, + // since the identifier is abstract and is + // therefore not yet reachable via its + // yet-to-be-determined identifier. + oi_abstract + .bind_definition(ctx.asg_mut(), meta_name, oi) + .map(|_| ()) + .transition(BuildingExpr(es, oi)) + } + } } (BuildingExpr(es, oi), AirBind(RefIdent(name))) => { diff --git a/tamer/src/asg/air/expr/test.rs b/tamer/src/asg/air/expr/test.rs index 01660905..a2ae2bac 100644 --- a/tamer/src/asg/air/expr/test.rs +++ b/tamer/src/asg/air/expr/test.rs @@ -813,3 +813,73 @@ fn expr_doc_short_desc() { oi_docs.collect::>(), ); } + +// Binding an abstract identifier to an expression means that the expression +// may _eventually_ be reachable after expansion, +// but it is not yet. +// They must therefore only be utilized within the context of a container +// that supports dangling expressions, +// like a template. +#[test] +fn abstract_bind_without_dangling_container() { + let id_meta = SPair("@foo@".into(), S2); + let id_ok = SPair("concrete".into(), S5); + + #[rustfmt::skip] + let toks = vec![ + Air::ExprStart(ExprOp::Sum, S1), + // This expression is bound to an _abstract_ identifier, + // which will be expanded at a later time. + // Consequently, + // this expression is still dangling. + Air::BindIdentAbstract(id_meta), + + // Since the expression is still dangling, + // attempting to close it will produce an error. + Air::ExprEnd(S3), + + // RECOVERY: Since an attempt at identification has been made, + // we shouldn't expect that another attempt will be made. + // The sensible thing to do is to move on to try to find other + // errors, + // leaving the expression alone and unreachable. + Air::ExprStart(ExprOp::Sum, S4), + // This is intended to demonstrate that we can continue on to the + // next expression despite the prior error. + Air::BindIdent(id_ok), + Air::ExprEnd(S6), + ]; + + let mut sut = parse_as_pkg_body(toks); + + assert_eq!( + #[rustfmt::skip] + vec![ + Ok(Parsed::Incomplete), // PkgStart + Ok(Parsed::Incomplete), // ExprStart + + // This provides an _abstract_ identifier, + // which is not permitted in this context. + Err(ParseError::StateError(AsgError::InvalidAbstractBindContext( + id_meta, + Some(S1), // Pkg + ))), + + // RECOVERY: Ignore the bind and move to close. + // The above identifier was rejected and so we are still dangling. + Err(ParseError::StateError(AsgError::DanglingExpr( + S1.merge(S3).unwrap() + ))), + + // RECOVERY: This observes that we're able to continue parsing + // the package after the above identification problem. + Ok(Parsed::Incomplete), // ExprStart + Ok(Parsed::Incomplete), // BindIdent (ok) + Ok(Parsed::Incomplete), // ExprEnd + Ok(Parsed::Incomplete), // PkgEnd + ], + sut.by_ref().collect::>(), + ); + + let _ = sut.finalize().unwrap(); +} diff --git a/tamer/src/asg/air/tpl/test.rs b/tamer/src/asg/air/tpl/test.rs index af08e9c9..7b3c0206 100644 --- a/tamer/src/asg/air/tpl/test.rs +++ b/tamer/src/asg/air/tpl/test.rs @@ -785,3 +785,52 @@ fn metavars_within_exprs_hoisted_to_parent_tpl() { assert_eq!(S11.merge(S13).unwrap(), span_inner); } + +#[test] +fn expr_abstract_bind_produces_cross_edge_from_ident_to_meta() { + let id_tpl = SPair("_tpl_".into(), S2); + let id_meta = SPair("@foo@".into(), S4); + + #[rustfmt::skip] + let toks = vec![ + Air::TplStart(S1), + // This identifier is concrete; + // the abstract identifier will be the _expression_. + Air::BindIdent(id_tpl), + + Air::ExprStart(ExprOp::Sum, S3), + // This expression is bound to an _abstract_ identifier, + // which will be expanded at a later time. + // This does _not_ change the dangling status, + // and so can only occur within an expression that acts as a + // container for otherwise-dangling expressions. + Air::BindIdentAbstract(id_meta), + Air::ExprEnd(S5), + Air::TplEnd(S6), + ]; + + let ctx = air_ctx_from_pkg_body_toks(toks); + let asg = ctx.asg_ref(); + + // The expression is dangling and so we must find it through a + // traversal. + let oi_tpl = pkg_expect_ident_oi::(&ctx, id_tpl); + let oi_expr = oi_tpl + .edges_filtered::(asg) + .next() + .expect("could not locate child Expr"); + + // The abstract identifier that should have been bound to the expression. + let oi_ident = oi_expr + .ident(asg) + .expect("abstract identifier was not bound to Expr"); + + assert_eq!( + None, + oi_ident.resolve(asg).name(), + "abstract identifier must not have a concrete name", + ); + + // The metavariable referenced by the abstract identifier + assert_eq!(id_meta, oi_ident.name_or_meta(asg)); +} diff --git a/tamer/src/asg/error.rs b/tamer/src/asg/error.rs index a930652e..7d383e30 100644 --- a/tamer/src/asg/error.rs +++ b/tamer/src/asg/error.rs @@ -117,13 +117,20 @@ pub enum AsgError { /// delimiter. UnbalancedTpl(Span), - /// Attempted to bind an identifier to an object while not in a context - /// that can receive an identifier binding. + /// Attempted to bind a concrete identifier to an object while not in a + /// context that can receive an identifier binding. /// /// Note that the user may encounter an error from a higher-level IR /// instead of this one. InvalidBindContext(SPair), + /// Attempted to bind an abstract identifier in a context where + /// expansion will not take place. + /// + /// This is intended to preempt future errors when we know that the + /// context does not make sense for an abstract binding. + InvalidAbstractBindContext(SPair, Option), + /// Attempted to reference an identifier while not in a context that can /// receive an identifier reference. /// @@ -202,8 +209,20 @@ impl Display for AsgError { ), UnbalancedExpr(_) => write!(f, "unbalanced expression"), UnbalancedTpl(_) => write!(f, "unbalanced template definition"), - InvalidBindContext(_) => { - write!(f, "invalid identifier binding context") + InvalidBindContext(name) => { + write!( + f, + "invalid identifier binding context for {}", + TtQuote::wrap(name), + ) + } + InvalidAbstractBindContext(name, _) => { + write!( + f, + "invalid abstract identifier binding context for \ + metavariable {}", + TtQuote::wrap(name), + ) } InvalidRefContext(ident) => { write!( @@ -351,6 +370,25 @@ impl Diagnostic for AsgError { InvalidBindContext(name) => vec![name .error("an identifier binding is not valid in this context")], + InvalidAbstractBindContext(name, parent_span) => parent_span + .map(|span| { + span.note( + "this definition context does not support metavariable \ + expansion" + ) + }) + .into_iter() + .chain(vec![ + name.error("this metavariable will never be expanded"), + name.help(format!( + "this identifier must have its name derived from \ + the metavariable {}, + but that metavariable will never be expanded here", + TtQuote::wrap(name), + )), + ]) + .collect(), + InvalidRefContext(ident) => vec![ident.error( "cannot reference the value of an expression from outside \ of an expression context", diff --git a/tamer/src/asg/graph/object/ident.rs b/tamer/src/asg/graph/object/ident.rs index 5ea92bd9..30a539f9 100644 --- a/tamer/src/asg/graph/object/ident.rs +++ b/tamer/src/asg/graph/object/ident.rs @@ -21,7 +21,7 @@ use super::{prelude::*, Expr, Meta, Pkg, Tpl}; use crate::{ - diagnose::{Annotate, Diagnostic}, + diagnose::{panic::DiagnosticPanic, Annotate, Diagnostic}, diagnostic_todo, f::Functor, fmt::{DisplayWrapper, TtQuote}, @@ -271,6 +271,17 @@ impl Ident { Missing(ident) } + /// Create a new abstract identifier at the given location. + /// + /// The provided [`Span`] is the only way to uniquely identify this + /// identifier since it does not yet have a name. + /// Note that this just _represents_ an abstract identifier; + /// it is given meaning only when given the proper relationships on + /// the ASG. + pub fn new_abstract>(at: S) -> Self { + Abstract(at.into()) + } + /// Attempt to redeclare an identifier with additional information. /// /// If an existing identifier is an [`Ident::Extern`], @@ -1106,7 +1117,10 @@ object_rel! { /// This is a legacy feature expected to be removed in the future; /// see [`ObjectRel::can_recurse`] for more information. Ident -> { - tree Ident, + // Could represent an opaque dependency or an abstract identifier's + // metavariable reference. + dyn Ident, + tree Expr, tree Tpl, tree Meta, @@ -1224,11 +1238,22 @@ impl ObjectIndex { ) } - // e.g. in a template body - Abstract(span) => diagnostic_todo!( - vec![span.note("abstract defintion bind here")], - "bind definition to abstract identifier", - ), + // An abstract identifier will become `Transparent` during + // expansion. + // This does not catch multiple definitions, + // but this is hopefully not a problem in practice since there + // is no lookup mechanism in source languages for abstract + // identifiers since this has no name yet and cannot be + // indexed in the usual way. + // Even so, + // multiple definitions can be caught at expansion-time if + // they somehow are able to slip through + // (which would be a compiler bug); + // it is not worth complicating `Ident`'s API or variants + // even further, + // and not worth the cost of a graph lookup here when + // we'll have to do it later anyway. + Abstract(span) => Ok(Abstract(span)), // We are okay to proceed to add an edge to the `definition`. // Discard the original span @@ -1305,13 +1330,66 @@ impl ObjectIndex { pub fn name_or_meta(&self, asg: &Asg) -> SPair { let ident = self.resolve(asg); + // It would be nice if this could be built more into the type system + // in the future, + // if it's worth the effort of doing so. + // This is a simple lookup; + // the robust internal diagnostic messages make it look + // more complicated than it is. ident.name().unwrap_or_else(|| { - diagnostic_todo!( - vec![ident.span().internal_error("for this abstract ident")], - "metavariable lookup not yet supported", + let oi_meta_ident = + self.edges_filtered::(asg).next().diagnostic_expect( + || { + vec![ + self.internal_error( + "this abstract identifier has no Ident edge", + ), + self.help( + "the compiler created an `Ident::Abstract` \ + object but did not produce an edge to the \ + Ident of the Meta from which its name \ + will be derived", + ), + ] + }, + "invalid ASG representation of abstract identifier", + ); + + oi_meta_ident.resolve(asg).name().diagnostic_expect( + || { + vec![ + self.note( + "while trying to find the Meta name of this abstract \ + identifier" + ), + oi_meta_ident.internal_error( + "encountered another abstract identifier" + ), + oi_meta_ident.help( + "an abstract identifier must reference a concrete \ + `Ident`" + ), + ] + }, + "abstract identifier references another abstract identifier", ) }) } + + /// Create a new abstract identifier whose name will be derived from + /// this one during expansion. + /// + /// It is expected that `self` defines a [`Meta`], + /// but this is not enforced here and will be checked during + /// expansion. + pub fn new_abstract_ident( + self, + asg: &mut Asg, + at: Span, + ) -> ObjectIndex { + asg.create(Ident::new_abstract(at)) + .add_edge_to(asg, self, Some(at)) + } } #[cfg(test)]