From 343f5b34b39f6c6e77a0b58dd684e7981cbc11af Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 8 Mar 2023 23:44:40 -0500 Subject: [PATCH] tamer: asg::air: Template support for dangling expressions The intent was to have a very simple implementation of `hold_dangling` and have everything work. But, I had a nasty surprise when the system tests caught bug caused by some interesting depth interactions as it relates to `xmli` and auto-closing. I added an extra test/example in `asg::graph::visit::test` to illustrate the situation; it was difficult to derive from the traces, but trivially obvious once I wrote it out as an example. With that, templates can now aggregate tokens for dangling expressions. DEV-13708 --- tamer/src/asg/air/expr.rs | 10 ++-- tamer/src/asg/air/tpl/test.rs | 37 +++++++++++++ tamer/src/asg/graph/visit/test.rs | 86 +++++++++++++++++++++++++++++-- tamer/src/asg/graph/xmli.rs | 13 ++++- tamer/tests/xmli/expected.xml | 26 ++++++++++ tamer/tests/xmli/src.xml | 42 +++++++++++++++ 6 files changed, 205 insertions(+), 9 deletions(-) diff --git a/tamer/src/asg/air/expr.rs b/tamer/src/asg/air/expr.rs index 8fa02629..df8702a9 100644 --- a/tamer/src/asg/air/expr.rs +++ b/tamer/src/asg/air/expr.rs @@ -480,11 +480,13 @@ mod root { fn hold_dangling( &self, - _asg: &mut Asg, - _oi_expr: ObjectIndex, + asg: &mut Asg, + oi_expr: ObjectIndex, ) -> Result<(), AsgError> { - // This will be used for templates. - todo!("hold_dangling") + let Self(oi_root) = self; + + oi_root.add_edge_to(asg, oi_expr, None); + Ok(()) } } } diff --git a/tamer/src/asg/air/tpl/test.rs b/tamer/src/asg/air/tpl/test.rs index c8abe625..def8b588 100644 --- a/tamer/src/asg/air/tpl/test.rs +++ b/tamer/src/asg/air/tpl/test.rs @@ -229,6 +229,43 @@ fn tpl_with_reachable_expression() { ); } +// Templates can expand into many contexts, +// including other expressions, +// and so must be able to contain expressions that, +// while dangling now, +// will become reachable in its expansion context. +#[test] +fn tpl_holds_dangling_expressions() { + let id_tpl = SPair("_tpl_".into(), S2); + + #[rustfmt::skip] + let toks = vec![ + Air::TplOpen(S1), + Air::BindIdent(id_tpl), + + // Dangling + Air::ExprOpen(ExprOp::Sum, S3), + Air::ExprClose(S4), + + // Dangling + Air::ExprOpen(ExprOp::Sum, S5), + Air::ExprClose(S6), + Air::TplClose(S7), + ]; + + let asg = asg_from_toks(toks); + let oi_tpl = asg.expect_ident_oi::(id_tpl); + + assert_eq!( + vec![S5.merge(S6).unwrap(), S3.merge(S4).unwrap(),], + oi_tpl + .edges_filtered::(&asg) + .map(ObjectIndex::cresolve(&asg)) + .map(Expr::span) + .collect::>() + ); +} + #[test] fn close_tpl_mid_open() { let id_expr = SPair("expr".into(), S3); diff --git a/tamer/src/asg/graph/visit/test.rs b/tamer/src/asg/graph/visit/test.rs index 3013fd8b..c4ef245f 100644 --- a/tamer/src/asg/graph/visit/test.rs +++ b/tamer/src/asg/graph/visit/test.rs @@ -32,6 +32,10 @@ use std::fmt::Debug; use Air::*; +// More concise values for tables below. +use ObjectRelTy::*; +const SU: Span = UNKNOWN_SPAN; + fn asg_from_toks>(toks: I) -> Asg where I::IntoIter: Debug, @@ -83,10 +87,8 @@ fn traverses_ontological_tree() { let sut = tree_reconstruction(&asg); // We need more concise expressions for the below table of values. - use ObjectRelTy::*; let d = DynObjectRel::new; let m = |a: Span, b: Span| a.merge(b).unwrap(); - const SU: Span = UNKNOWN_SPAN; // Note that the `Depth` beings at 1 because the actual root of the // graph is not emitted. @@ -95,7 +97,8 @@ fn traverses_ontological_tree() { // language doesn't have such nesting. #[rustfmt::skip] assert_eq!( - vec![ + // A -|-> B | A span -|-> B span | espan | depth + vec![//-----|-------|-----------|-----------|--------|----------------- (d(Root, Pkg, SU, m(S1, S11), None ), Depth(1)), (d(Pkg, Ident, m(S1, S11), S3, None ), Depth(2)), (d(Ident, Expr, S3, m(S2, S7), None ), Depth(3)), @@ -113,3 +116,80 @@ fn traverses_ontological_tree() { )).collect::>(), ); } + +// This is a variation of the above test, +// focusing on the fact that templates may contain odd constructions that +// wouldn't necessarily be valid in other contexts. +// This merely establishes a concrete example to re-enforce intuition and +// serve as an example of the system's behavior in a laboratory setting, +// as opposed to having to scan through real-life traces and all the +// complexity and noise therein. +// +// This also serves as an integration test to ensure that templates produce +// the expected result on the graph. +// Just as was mentioned above, +// that makes this test very fragile, +// and you should look at other failing tests before assuming that this +// one is broken; +// let this help to guide your reasoning of the system rather than +// your suspicion. +#[test] +fn traverses_ontological_tree_tpl_with_sibling_at_increasing_depth() { + let id_tpl = SPair("_tpl_".into(), S3); + let id_expr = SPair("expr".into(), S7); + + #[rustfmt::skip] + let toks = vec![ + PkgOpen(S1), + TplOpen(S2), + BindIdent(id_tpl), + + // Dangling + ExprOpen(ExprOp::Sum, S4), + ExprClose(S5), + + // Reachable + ExprOpen(ExprOp::Sum, S6), + BindIdent(id_expr), + ExprClose(S8), + TplClose(S9), + PkgClose(S10), + ]; + + let asg = asg_from_toks(toks); + let sut = tree_reconstruction(&asg); + + // We need more concise expressions for the below table of values. + let d = DynObjectRel::new; + let m = |a: Span, b: Span| a.merge(b).unwrap(); + + // Writing this example helped to highlight how the system is + // functioning and immediately obviated a bug downstream in the + // lowering pipeline (xmli derivation) at the time of writing. + // The `Tpl->Ident` was ignored along with its `Depth` because it + // produced no output, + // and therefore the final expression was interpreted as being a + // child of its sibling. + // This traversal was always correct; + // the problem manifested in the integration of these systems and + // was caught by system tests. + #[rustfmt::skip] + assert_eq!( + // A -|-> B | A span -|-> B span | espan| depth + vec![//-----|-------|-----------|-----------|------|----------------- + (d(Root, Pkg, SU, m(S1, S10), None), Depth(1)), + (d(Pkg, Ident, m(S1, S10), S3, None), Depth(2)), + (d(Ident, Tpl, S3, m(S2, S9), None), Depth(3)), + (d(Tpl, Expr, m(S2, S9), m(S4, S5), None), Depth(4)), // --, + (d(Tpl, Ident, m(S2, S9), S7, None), Depth(4)), // | + (d(Ident, Expr, S7, m(S6, S8), None), Depth(5)), // <' + ], + sut.map(|TreeWalkRel(rel, depth)| ( + rel.map(|(soi, toi)| ( + soi.resolve(&asg).span(), + toi.resolve(&asg).span() + )), + depth + )).collect::>(), + ); +} diff --git a/tamer/src/asg/graph/xmli.rs b/tamer/src/asg/graph/xmli.rs index d211b2c8..be752bab 100644 --- a/tamer/src/asg/graph/xmli.rs +++ b/tamer/src/asg/graph/xmli.rs @@ -49,7 +49,7 @@ use crate::{ xir::{ flat::{Text, XirfToken}, st::qname::*, - OpenSpan, QName, + CloseSpan, OpenSpan, QName, }, }; use arrayvec::ArrayVec; @@ -179,7 +179,16 @@ impl<'a> TreeContext<'a> { // Identifiers will be considered in context; // pass over it for now. - Object::Ident(..) => None, + // But we must not skip over its depth, + // otherwise we parent a following sibling at a matching + // depth; + // this close will force the auto-closing system to close + // any siblings in preparation for the object to follow. + Object::Ident((ident, _)) => Some(Xirf::Close( + None, + CloseSpan::without_name_span(ident.span()), + depth, + )), Object::Expr((expr, _)) => { self.emit_expr(expr, paired_rel.source(), depth) diff --git a/tamer/tests/xmli/expected.xml b/tamer/tests/xmli/expected.xml index 3e7eaa40..fe2dcdee 100644 --- a/tamer/tests/xmli/expected.xml +++ b/tamer/tests/xmli/expected.xml @@ -44,4 +44,30 @@ + + + + diff --git a/tamer/tests/xmli/src.xml b/tamer/tests/xmli/src.xml index 5c0f4633..54bc9303 100644 --- a/tamer/tests/xmli/src.xml +++ b/tamer/tests/xmli/src.xml @@ -51,4 +51,46 @@ + + + +