From c19ecba6ef86d43849551e8e6081771e0878b147 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 26 Jul 2023 03:43:24 -0400 Subject: [PATCH] tamer: asg::air::object::tpl: Reject multi-expression shape This enforces the new constraint that templates expanding into an `Expr` context must only inline a single `Expr`. Perhaps in the future we'll support explicit splicing, like `,@` in Lisp. But this new restriction is intended for two purposes: - To make templates more predictable (if you have a list of expressions inlined then they will act differently depending on the type of expression that they are inlined into, which means that more defensive programming would otherwise be required); and - To make expansion easier, since we're going to have to set aside an expansion workspace ahead of time to ensure ordering (Petgraph can't replace edges in-place). If we support multi-expansion, we'd have to handle associativity in all expression contexts. This'll become more clear in future commits. It's nice to see all this hard work coming together now, though; it's easy now to perform static analysis on the system, and any part of the graph construction can throw errors with rich diagnostic information and still recover properly. And, importantly, the system enforces its own state, and the compiler helps us with that (the previous commits). DEV-13163 --- tamer/build-aux/asg-ontviz.awk | 32 ++++++-- tamer/src/asg/air/test.rs | 19 +++-- tamer/src/asg/air/tpl/test.rs | 104 +++++++++++++++++++++---- tamer/src/asg/error.rs | 66 ++++++++++++++++ tamer/src/asg/graph/object/tpl.rs | 17 +++- tamer/tests/xmli/template/expected.xml | 32 +++----- tamer/tests/xmli/template/src.xml | 12 +-- 7 files changed, 226 insertions(+), 56 deletions(-) diff --git a/tamer/build-aux/asg-ontviz.awk b/tamer/build-aux/asg-ontviz.awk index 32637dc2..6bf776e6 100644 --- a/tamer/build-aux/asg-ontviz.awk +++ b/tamer/build-aux/asg-ontviz.awk @@ -32,6 +32,24 @@ # } # } # +# But edges also support arbitrary code definitions: +# +# object_rel! { +# Source -> { +# tree TargetA { +# fn pre_add_edge(...) { +# // ... +# } +# }, +# tree TargetB, +# cross TargetC, +# } +# } +# +# And because of that, +# this script hard-codes the expected level of nesting just to make life +# easier. +# # This script expects to receive a list of files containing such # definitions. # It will output, @@ -81,7 +99,8 @@ BEGINFILE { /^object_rel! {$/, /^}$/ { in_block = 1 } # `Foo -> {` line declares the source of the relation. -in_block && /->/ { +# We hard-code the expected depth to simplify parsing. +in_block && /^ \w+ -> \{$/ { block_src = $1 printf " # `%s` from `%s:%d`\n", block_src, FILENAME, FNR @@ -92,7 +111,7 @@ in_block && /->/ { # A closing curly brace always means that we've finished with the current # source relation, # since we're at the innermost level of nesting. -block_src && /}/ { +block_src && /^ }$/ { block_src = "" print "" } @@ -116,7 +135,12 @@ block_src && /^ *\/\/ empty$/ { # we must independently define each one. # But that's okay; # the output is quite legible. -block_src && $NF ~ /\w+,$/ { +block_src && /^ \w+ \w+(,| \{)$/ { + # Clean up the end of the string _before_ pulling information out of + # fields, + # since the number of fields can vary. + gsub(/(,| \{)$/, "") + # Edge type (cross, tree) ty = $(NF-1) @@ -139,8 +163,6 @@ block_src && $NF ~ /\w+,$/ { break; } - gsub(/,$/, "") - # This may need updating over time as object names in Rust sources # exceed the fixed-width definition here. # This output is intended to form a table that is easy to read and diff --git a/tamer/src/asg/air/test.rs b/tamer/src/asg/air/test.rs index 48ccdf98..8ea14618 100644 --- a/tamer/src/asg/air/test.rs +++ b/tamer/src/asg/air/test.rs @@ -30,7 +30,7 @@ use crate::{ span::dummy::*, }; -type Sut = AirAggregate; +pub type Sut = AirAggregate; use Air::*; use Parsed::Incomplete; @@ -686,16 +686,23 @@ fn resume_previous_parsing_context() { pub fn parse_as_pkg_body>( toks: I, ) -> Parser + Debug> +where + ::IntoIter: Debug, +{ + Sut::parse(as_pkg_body(toks)) +} + +pub fn as_pkg_body>( + toks: I, +) -> impl Iterator + Debug where ::IntoIter: Debug, { use std::iter; - Sut::parse( - iter::once(PkgStart(S1, spair("/incidental-pkg", S1))) - .chain(toks.into_iter()) - .chain(iter::once(PkgEnd(S1))), - ) + iter::once(PkgStart(S1, spair("/incidental-pkg", S1))) + .chain(toks.into_iter()) + .chain(iter::once(PkgEnd(S1))) } pub(super) fn asg_from_pkg_body_toks>( diff --git a/tamer/src/asg/air/tpl/test.rs b/tamer/src/asg/air/tpl/test.rs index d4562a76..ad997c9b 100644 --- a/tamer/src/asg/air/tpl/test.rs +++ b/tamer/src/asg/air/tpl/test.rs @@ -18,6 +18,7 @@ // along with this program. If not, see . use super::*; +use crate::asg::air::test::{as_pkg_body, Sut}; use crate::span::dummy::*; use crate::{ asg::{ @@ -333,18 +334,9 @@ fn tpl_holds_dangling_expressions() { BindIdent(id_tpl), // Dangling expression. - // This would be inlined at an application site, - // and so this changes the shape of the template. ExprStart(ExprOp::Sum, S3), ExprEnd(S4), - - // Dangling - // (TODO: This won't be valid; - // extract into separate test case to check for a new - // AsgError variant.) - ExprStart(ExprOp::Sum, S5), - ExprEnd(S6), - TplEnd(S7), + TplEnd(S5), ]; let ctx = air_ctx_from_pkg_body_toks(toks); @@ -353,12 +345,96 @@ fn tpl_holds_dangling_expressions() { let oi_tpl = pkg_expect_ident_oi::(&ctx, id_tpl); let tpl = oi_tpl.resolve(&asg); - // TODO: Until the above is invalid, - // the second is overwriting the first. - assert_eq!(TplShape::Expr(S5.merge(S6).unwrap()), tpl.shape()); + // The above `Expr` would be inlined at an application site, + // and so this changes the shape of the template. + assert_eq!(TplShape::Expr(S3.merge(S4).unwrap()), tpl.shape()); assert_eq!( - vec![S5.merge(S6).unwrap(), S3.merge(S4).unwrap(),], + vec![S3.merge(S4).unwrap()], + oi_tpl + .edges_filtered::(&asg) + .map(ObjectIndex::cresolve(&asg)) + .map(Expr::span) + .collect::>() + ); +} + +// As of TAMER, +// a new restriction is that templates can't just inline whatever they +// want into their expression expansion context. +#[test] +fn multi_dangling_invalid_expr_shape() { + let id_tpl = spair("_tpl_", S2); + + #[rustfmt::skip] + let toks = [ + TplStart(S1), + BindIdent(id_tpl), + + // We have an expression to be expanded inline. + // We cannot have another. + ExprStart(ExprOp::Sum, S3), + ExprEnd(S4), + + // ...but we're provided another! + // This should error. + ExprStart(ExprOp::Sum, S5), + ExprEnd(S6), + TplEnd(S7), + ]; + + let mut sut = Sut::parse(as_pkg_body(toks)); + + assert_eq!( + #[rustfmt::skip] + vec![ + Ok(Parsed::Incomplete), // PkgStart + Ok(Parsed::Incomplete), // TplStart + Ok(Parsed::Incomplete), // BindIdent + + // This one's okay. + Ok(Parsed::Incomplete), // ExprStart + Ok(Parsed::Incomplete), // ExprEnd + + // We start out okay, + // because an edge for a dangling expression is not added + // until after the expression ends + // (we have until then to bind an identifier). + Ok(Parsed::Incomplete), // ExprStart + // But the ending token will trigger the error. + Err(ParseError::StateError( + AsgError::TplShapeExprMulti( + Some(id_tpl), + S5.merge(S6).unwrap(), + S3.merge(S4).unwrap() + ) + )), + + // RECOVERY: We do not add the edge, + // but the object otherwise continues to exist on the + // graph, + // unreachable. + Ok(Parsed::Incomplete), // TplEnd + Ok(Parsed::Incomplete), // PkgEnd + ], + sut.by_ref().collect::>(), + ); + + let ctx = sut.finalize().unwrap().into_private_context(); + let asg = ctx.asg_ref(); + + let oi_tpl = pkg_expect_ident_oi::(&ctx, id_tpl); + let tpl = oi_tpl.resolve(&asg); + + // The error above should be evidence for this, + // but let's make sure the error didn't somehow cause the shape to + // change. + assert_eq!(TplShape::Expr(S3.merge(S4).unwrap()), tpl.shape()); + + // The template should have only one inline expression; + // it should have discarded the other. + assert_eq!( + vec![S3.merge(S4).unwrap()], oi_tpl .edges_filtered::(&asg) .map(ObjectIndex::cresolve(&asg)) diff --git a/tamer/src/asg/error.rs b/tamer/src/asg/error.rs index 7d383e30..a8bd129d 100644 --- a/tamer/src/asg/error.rs +++ b/tamer/src/asg/error.rs @@ -167,8 +167,26 @@ pub enum AsgError { /// The provided [`Span`] indicates the location of the start of the /// metavariable definition. UnexpectedMeta(Span), + + /// A template already has a shape of + /// [`TplShape::Expr`](super::graph::object::tpl::TplShape::Expr), + /// but another expression was found that would violate this shape + /// constraint. + /// + /// The template name is provided if it is known at the time of the + /// error. + TplShapeExprMulti(Option, ErrorOccurrenceSpan, FirstOccurrenceSpan), } +/// A [`Span`] representing the subject of this error. +type ErrorOccurrenceSpan = Span; + +/// A [`Span`] representing the first occurrence of an object related to the +/// subject of this error. +/// +/// This should be paired with [`ErrorOccurrenceSpan`]. +type FirstOccurrenceSpan = Span; + impl Display for AsgError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { use AsgError::*; @@ -250,6 +268,18 @@ impl Display for AsgError { UnexpectedMeta(_) => { write!(f, "unexpected metavariable definition") } + + TplShapeExprMulti(Some(name), _, _) => write!( + f, + "definition of template {} would produce multiple inline \ + expressions when expanded", + TtQuote::wrap(name), + ), + TplShapeExprMulti(None, _, _) => write!( + f, + "template definition would produce multiple inline \ + expressions when expanded" + ), } } } @@ -445,6 +475,42 @@ impl Diagnostic for AsgError { span.help("metavariables are expected to occur in a template context"), ] } + + // TODO: Perhaps we should have a documentation page that can be + // referenced with examples and further rationale, + // like Rust does. + TplShapeExprMulti(oname, err, first) => oname + .map(|name| name.note("for this template")) + .into_iter() + .chain(vec![ + first.note( + "this is the first expression that would be inlined \ + at an expansion site", + ), + err.error( + "template cannot inline more than one expression \ + into an expansion site", + ), + err.help( + "this restriction is intended to ensure that \ + templates expand in ways that are consistent \ + given any expansion context; consider either:", + ), + err.help( + " - wrapping both expressions in a parent \ + expression; or", + ), + err.help( + " - giving at least one expression an id to prevent \ + inlining", + ), + // in case they were wondering + err.help( + "this restriction did not exist in versions of \ + TAME prior to TAMER", + ), + ]) + .collect(), } } } diff --git a/tamer/src/asg/graph/object/tpl.rs b/tamer/src/asg/graph/object/tpl.rs index e81b97ff..feecee87 100644 --- a/tamer/src/asg/graph/object/tpl.rs +++ b/tamer/src/asg/graph/object/tpl.rs @@ -176,7 +176,22 @@ object_rel! { commit: impl FnOnce(&mut Asg), ) -> Result<(), AsgError> { let span = to_oi.resolve(asg).span(); - from_oi.map_obj(asg, |tpl| tpl.overwrite(TplShape::Expr(span))); + let tpl_name = from_oi + .ident(asg) + .and_then(|oi| oi.resolve(asg).name()); + + from_oi.try_map_obj(asg, |tpl| match tpl.shape() { + TplShape::Expr(first) => { + Err(( + tpl, + AsgError::TplShapeExprMulti(tpl_name, span, first) + )) + }, + + TplShape::Unknown | TplShape::Empty => { + Ok(tpl.overwrite(TplShape::Expr(span))) + }, + })?; Ok(commit(asg)) } diff --git a/tamer/tests/xmli/template/expected.xml b/tamer/tests/xmli/template/expected.xml index 3e725c2c..a85d5f58 100644 --- a/tamer/tests/xmli/template/expected.xml +++ b/tamer/tests/xmli/template/expected.xml @@ -20,19 +20,11 @@ - - - - @@ -100,9 +92,9 @@