diff --git a/tamer/src/asg/air/expr.rs b/tamer/src/asg/air/expr.rs index ae135770..4c87279b 100644 --- a/tamer/src/asg/air/expr.rs +++ b/tamer/src/asg/air/expr.rs @@ -92,12 +92,8 @@ impl ParseState for AirExprAggregate { } (BuildingExpr(es, poi), AirExpr(ExprStart(op, span))) => { - match poi.create_subexpr(ctx.asg_mut(), Expr::new(op, span)) { - Ok(oi) => { - Transition(BuildingExpr(es.push(poi), oi)).incomplete() - } - Err(e) => Transition(BuildingExpr(es, poi)).err(e), - } + let oi_child = ctx.asg_mut().create(Expr::new(op, span)); + Transition(BuildingExpr(es.push(poi), oi_child)).incomplete() } (BuildingExpr(es, oi), AirExpr(ExprEnd(end))) => { @@ -107,13 +103,19 @@ impl ParseState for AirExprAggregate { let oi_root = ctx.dangling_expr_oi(); match (es.pop(), dangling) { - ((es, Some(poi)), _) => { - Transition(BuildingExpr(es, poi)).incomplete() - } + // This was a child expression + ((es, Some(poi)), _) => poi + .add_completed_subexpr(ctx.asg_mut(), oi) + .map(|_| ()) + .transition(BuildingExpr(es, poi)), + + // Topmost expression, dangling. ((es, None), true) => { Self::hold_dangling(ctx.asg_mut(), oi_root, oi) .transition(Ready(es.done())) } + + // Topmost expression, reachable. ((es, None), false) => { Transition(Ready(es.done())).incomplete() } diff --git a/tamer/src/asg/air/expr/test.rs b/tamer/src/asg/air/expr/test.rs index 87237df5..04c2c2b4 100644 --- a/tamer/src/asg/air/expr/test.rs +++ b/tamer/src/asg/air/expr/test.rs @@ -47,10 +47,15 @@ pub fn collect_subexprs( asg: &Asg, oi: ObjectIndex, ) -> Vec<(ObjectIndex, &Expr)> { - oi.edges(&asg) + let mut exprs = oi + .edges(&asg) .filter_map(|rel| rel.narrow::()) .map(|oi| (oi, oi.resolve(&asg))) - .collect::>() + .collect::>(); + + // Edge order is reversed. + exprs.reverse(); + exprs } #[test] @@ -931,7 +936,7 @@ fn expr_referencing_missing_without_abstract_is_unknown() { // but throw in a known reference to show that it doesn't override the // missing one. #[test] -fn expr_referencing_missing_with_concrete_without_abstract_is_unknown() { +fn expr_referencing_missing_with_concrete_without_abstract_is_maybe_concrete() { #[rustfmt::skip] let toks = [ ExprStart(ExprOp::Sum, S1), @@ -959,3 +964,73 @@ fn expr_referencing_missing_with_concrete_without_abstract_is_unknown() { // so only one is missing. assert_eq!(expr.meta_state(), MetaState::MaybeConcrete(1.unwrap_into())); } + +// If any child expression is not known to be concrete, +// then we cannot reasonably call the parent expression concrete either, +// because we must decend into the parent's tree in order to expand +// children. +#[test] +fn expr_is_maybe_concrete_if_any_child_is_maybe_concrete() { + #[rustfmt::skip] + let toks = [ + ExprStart(ExprOp::Sum, S1), // A = 2 // ----------------. + BindIdent(spair("expr", S2)), // | + // | + // This should be MaybeConcrete because // | + // there is an inner missing reference. // | + ExprStart(ExprOp::Sum, S3), // B = 1 // ---------. <-: A1 + ExprStart(ExprOp::Sum, S4), // C = 2 // --. <-' B1 | + // A couple children deep to make sure // | | + // this property is inherited by all // | | + // ancestors (B, A). // | | + RefIdent(spair("missing", S5)), // <-: C1 | + // | | + // A second missing reference. // | | + // Even though _this_ expression has two, // | | + // we want the parent B to see only a // | | + // single missing edge to C. // | | + RefIdent(spair("missing", S6)), // <-' C2 | + ExprEnd(S6), // | + ExprEnd(S7), // | + // | + // Concrete, // | + // to make sure our count isn't affected. // | + ExprStart(ExprOp::Sum, S8), // | + ExprEnd(S9), // | + // | + // A second abstract child for A. // | + ExprStart(ExprOp::Sum, S10), // D = 1 // --. <--------' A2 + RefIdent(spair("missing", S11)), // <-' D1 + ExprEnd(S12), + ExprEnd(S13), + ]; + + let ctx = air_ctx_from_pkg_body_toks(toks); + let asg = ctx.asg_ref(); + + let oi_a = pkg_expect_ident_oi::(&ctx, spair("expr", S20)); + let a = oi_a.resolve(&asg); + + let ae = collect_subexprs(asg, oi_a); + assert_eq!(ae.len(), 3); // abstract, concrete, abstract + let (oi_b, b) = ae[0]; + let (_, d) = ae[2]; + + // Sanity check to make sure we have the exprs that we're expecting. + assert_eq!(S3.merge(S7).unwrap(), b.span()); + assert_eq!(S10.merge(S12).unwrap(), d.span()); + + let be = collect_subexprs(asg, oi_b); + assert_eq!(be.len(), 1); + let (_, c) = be[0]; + + // We'll assert according to the visualization above, + // from inside out. + // Asserting individually helps to guide debugging with a narrow focus, + // and we want to fail on inner exprs first since effects compose + // outward. + assert_eq!(d.meta_state(), MetaState::MaybeConcrete(1.unwrap_into())); + assert_eq!(c.meta_state(), MetaState::MaybeConcrete(2.unwrap_into())); + assert_eq!(b.meta_state(), MetaState::MaybeConcrete(1.unwrap_into())); + assert_eq!(a.meta_state(), MetaState::MaybeConcrete(2.unwrap_into())); +} diff --git a/tamer/src/asg/air/tpl/test.rs b/tamer/src/asg/air/tpl/test.rs index 2b0bc797..571f1c95 100644 --- a/tamer/src/asg/air/tpl/test.rs +++ b/tamer/src/asg/air/tpl/test.rs @@ -161,7 +161,6 @@ fn tpl_within_expr() { collect_subexprs(&asg, oi_expr) .iter() .map(|(_, expr)| expr.span()) - .rev() .collect::>(), ); } diff --git a/tamer/src/asg/graph/object/expr.rs b/tamer/src/asg/graph/object/expr.rs index 49bb33e8..ced44fc8 100644 --- a/tamer/src/asg/graph/object/expr.rs +++ b/tamer/src/asg/graph/object/expr.rs @@ -26,6 +26,50 @@ //! so expressions both naturally compose and are able to be replaced with //! the value that they represent without affecting the meaning of the //! program. +//! +//! An expression is [_concrete_](`MetaState::Concrete`) if it requires no +//! expansion by the template system. +//! If an expression or any of its children reference any +//! [metavariables](super::Meta) +//! (template parameters), +//! then the expression will be [_abstract_](MetaState::Abstract). +//! Expressions' static bindings together with their referential +//! transparency means that a concrete expression is able to be moved and +//! copied to any other point in the program without changing its +//! meaning; +//! this includes the act of copying via template expansion. +//! +//! A _reference_ to another expression does not have any influence over +//! whether an expression is abstract or not. +//! In graph terms: +//! tree edges influence an expression's [`MetaState`], +//! but not cross edges. +//! Consider the following expression in XML notation to help with intuition +//! on this. +//! Assume that this is the body of some template with a single template +//! parameter identified as `@foo@`: +//! +//! ```xml +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! +//! ``` use super::{ ident::IdentDefinition, prelude::*, Doc, Ident, ObjectIndexToTree, Tpl, @@ -44,6 +88,8 @@ use super::ObjectKind; /// The [`Span`] of an expression should be expanded to encompass not only /// all child expressions, /// but also any applicable closing span. +/// +/// See the [parent module](self) for more information. #[derive(Debug, PartialEq, Eq)] pub struct Expr { op: ExprOp, @@ -348,6 +394,20 @@ impl MetaState { } } } + + /// Determine how a child expression should impact whether this + /// expression is abstract. + fn observe_child(self, state: MetaState, span: Span) -> Self { + match state { + Self::Concrete => self, + Self::Abstract => self.found_abstract(), + + // Since we track the number of missing edges to direct children, + // we treat the child subgraph as if it were a single node on + // the graph. + Self::MaybeConcrete(_) => self.found_missing(span), + } + } } impl Display for MetaState { @@ -384,10 +444,27 @@ object_rel! { ); }, - // Non-meta identifiers are just references. - // We don't care what they are as long as they're not - // metavariables. - Some(IdentDefinition::Expr(_) | IdentDefinition::Tpl(_)) => (), + // This is a _reference_ to another expression tree. + // Only tree edges influence our abstract status. + Some(IdentDefinition::Expr(_)) => (), + + // Unlike the XSLT-based TAME, + // this reference can act as a template application, + // just as the `tree Tpl` edge below. + // TODO: We can expand closed expr templates here, + // since it's no different than referencing the inner + // expression. + Some(IdentDefinition::Tpl(_)) => diagnostic_todo!( + vec![ + rel.to_oi.error("this references a template"), + rel.to_oi.help( + "only closed expression templates will be \ + supported in this context" + ) + ], + "template references in an expression context are + not yet supported" + ), None => { rel.from_oi.map_obj_inner(asg, |meta: MetaState| { @@ -404,10 +481,28 @@ object_rel! { } }, - tree Expr, - tree Doc, + tree Expr { + fn pre_add_edge( + asg: &mut Asg, + rel: ProposedRel, + commit: impl FnOnce(&mut Asg), + ) -> Result<(), AsgError> { + let to = rel.to_oi.resolve(asg); - // Template application + let child_state = to.meta_state(); + let span = to.span(); + + rel.from_oi.map_obj_inner(asg, |state: MetaState| { + state.observe_child(child_state, span) + }); + + Ok(commit(asg)) + } + }, + + tree Doc, + + // Deferred template application tree Tpl, } } @@ -419,18 +514,20 @@ impl ObjectIndex { self.map_obj_inner(asg, |span: Span| span.merge(end).unwrap_or(span)) } - /// Create a new subexpression as the next child of this expression and - /// return the [`ObjectIndex`] of the new subexpression. + /// Add a completed subexpression as a child of a parent expression. + /// + /// It is important that the subexpression has _completed parsing_ so + /// that edge hooks are able to conduct inference on the entirety of + /// the subexpression. /// /// Sub-expressions maintain relative order to accommodate /// non-associative and non-commutative expressions. - pub fn create_subexpr( + pub fn add_completed_subexpr( self, asg: &mut Asg, - expr: Expr, + oi_sub: ObjectIndex, ) -> Result, AsgError> { - let oi_subexpr = asg.create(expr); - oi_subexpr.add_tree_edge_from(asg, self) + self.add_tree_edge_to(asg, oi_sub) } /// Reference the value of the expression identified by `oi_ident` as if @@ -451,8 +548,8 @@ impl ObjectIndex { /// If this is not true, /// consider using: /// - /// 1. [`Self::create_subexpr`] to create and assign ownership of - /// expressions contained within other expressions; or + /// 1. [`Self::add_completed_subexpr`] to create and assign ownership + /// of expressions contained within other expressions; or /// 2. [`ObjectIndex::bind_definition`] if this expression is to /// be assigned to an identifier. pub fn held_by(