From deede5ff21b3245bd0df57b361d475926b898c6b Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Mon, 31 Jul 2023 13:54:40 -0400 Subject: [PATCH] tamer: asg::graph::object::tpl: Infer shape from referenced identifiers Oh, identifiers. What a mess you make of the graph. As indirection tends to do, I suppose. This still needs some cleanup; I wanted to get something working first. In particular: - Common operations need refactoring; - Having all this code in the edge definition makes for a mess; - `Ident` references really ought not be returned by `Ident::definition` since an identifier will never be a definition. ...I'm sure there's more. DEV-13163 --- tamer/src/asg/air/tpl/test/apply.rs | 181 +++++++++++++++++++++++++--- tamer/src/asg/graph.rs | 1 - tamer/src/asg/graph/object/ident.rs | 2 + tamer/src/asg/graph/object/tpl.rs | 129 +++++++++++++++++--- 4 files changed, 280 insertions(+), 33 deletions(-) diff --git a/tamer/src/asg/air/tpl/test/apply.rs b/tamer/src/asg/air/tpl/test/apply.rs index 6ccf9ee1..a2004cc0 100644 --- a/tamer/src/asg/air/tpl/test/apply.rs +++ b/tamer/src/asg/air/tpl/test/apply.rs @@ -149,18 +149,18 @@ fn tpl_apply_nested_missing() { // Inner template application (Missing) TplStart(S3), - RefIdent(ref_tpl_inner_pre), - TplEndRef(S5), - - // Define the template above - TplStart(S6), - BindIdent(id_tpl_inner), - TplEnd(S8), - - // Apply again, - // this time _after_ having been defined. - TplStart(S9), - RefIdent(ref_tpl_inner_post), + RefIdent(ref_tpl_inner_pre), // -. + TplEndRef(S5), // | + // | + // Define the template above // | + TplStart(S6), // | + BindIdent(id_tpl_inner), // <: + TplEnd(S8), // | + // | + // Apply again, // | + // this time _after_ having been defined. // | + TplStart(S9), // | + RefIdent(ref_tpl_inner_post), // -' TplEndRef(S11), TplEnd(S12), ]; @@ -171,11 +171,17 @@ fn tpl_apply_nested_missing() { let oi_tpl_outer = pkg_expect_ident_oi::(&ctx, id_tpl_outer); assert_eq!(S1.merge(S12).unwrap(), oi_tpl_outer.resolve(&asg).span()); - // We apply two template, - // both of which are empty, - // and so the outer shape is still empty. + // TODO: It should be the case that we apply two templates, + // both of which are empty, + // and so the outer shape is still empty. + // But until we notify templates of `Missing` ident resolution, + // we don't know the shape of the template by the time we reach the + // first reference. + // Consequently, + // `TplShape::Unknown` must take precedence to reflect this + // uncertainty. let tpl_outer = oi_tpl_outer.resolve(&asg); - assert_eq!(TplShape::Empty, tpl_outer.shape()); + assert_eq!(TplShape::Unknown(S3.merge(S5).unwrap()), tpl_outer.shape()); // The inner template should be contained within the outer and so not // globally resolvable. @@ -378,3 +384,146 @@ fn tpl_inner_apply_expr_alongside_another_apply_expr() { .collect::>() ); } + +// A simpler version of the above test that asserts against the explicit +// case of sibling expressions in a template body, +// with no wrapping or indirection. +#[test] +fn tpl_multi_expr() { + #[rustfmt::skip] + let toks = [ + TplStart(S1), + BindIdent(spair("_tpl_", S2)), + + // First one is okay. + ExprStart(ExprOp::Sum, S3), + ExprEnd(S4), + + // But the second is in error and will be dropped. + 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 and changes the template's shape. + Ok(Parsed::Incomplete), // ExprStart + Ok(Parsed::Incomplete), // ExprEnd + + // But this one runs afoul of that new shape. + Ok(Parsed::Incomplete), // ExprStart + Err(ParseError::StateError( + AsgError::TplShapeExprMulti( + Some(spair("_tpl_", S2)), + S5.merge(S6).unwrap(), + S3.merge(S4).unwrap(), + ) + )), + // RECOVERY: We ignore the expression by not adding its + // edge. + 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, spair("_tpl_", S8)); + + assert_eq!( + TplShape::Expr(S3.merge(S4).unwrap()), + oi_tpl.resolve(&asg).shape() + ); + + // The second Expr should have been omitted. + assert_eq!( + vec![S3.merge(S4).unwrap()], + oi_tpl + .edges_filtered::(&asg) + .map(ObjectIndex::cresolve(&asg)) + .map(Expr::span) + .collect::>() + ); +} + +// Identifiers are a layer of indirection, +// but the object that they identify should influence the shape of the +// template all the same. +#[test] +fn tpl_ref_expr_shape() { + #[rustfmt::skip] + let toks = [ + TplStart(S1), + BindIdent(spair("_tpl-pre_", S2)), + + RefIdent(spair("expr", S3)), // --, + TplEnd(S4), // | + // | U + ExprStart(ExprOp::Sum, S5), // | + BindIdent(spair("expr", S6)), // <=: + ExprEnd(S7), // | + // | E + TplStart(S8), // | + BindIdent(spair("_tpl-post_", S9)), // <-+-. + // | | + RefIdent(spair("expr", S10)), // --' | + TplEnd(S11), // | + // | + // Similarly, // | + // if we have a template that references // | + // one of the above templates, // | + // it too should have the same // | E + // Expr shape, // | + // since the reference represents // | + // application. // | + TplStart(S12), // | + BindIdent(spair("_tpl-post-tpl-ref_", S13)),// | + // | + RefIdent(spair("_tpl-post_", S14)), // ----' + TplEnd(S14), + ]; + + let ctx = air_ctx_from_pkg_body_toks(toks); + let asg = ctx.asg_ref(); + + let oi_tpl_pre = pkg_expect_ident_oi::(&ctx, spair("_tpl-pre_", S15)); + let oi_tpl_post = + pkg_expect_ident_oi::(&ctx, spair("_tpl-post_", S16)); + let oi_tpl_post_tpl_ref = + pkg_expect_ident_oi::(&ctx, spair("_tpl-post-tpl-ref_", S17)); + + // TODO: We haven't yet handled notification when an identifier has been + // resolved, + // so the shape will be unknown until then. + assert_eq!(TplShape::Unknown(S3), oi_tpl_pre.resolve(&asg).shape(),); + + // But the template defined _after_ the expression will be immediately + // resolved and its shape known. + assert_eq!( + // Note that the span is that of the _reference_, + // since that is what appears within the template body and that + // which we want to emphasize in diagnostics. + TplShape::Expr(S10), + oi_tpl_post.resolve(&asg).shape(), + ); + + // And the final template applies the preceding one, + // and should inherit its shape since. + assert_eq!( + // Just as above, + // the span is the topmost _reference_, + // which is the application. + TplShape::Expr(S14), + oi_tpl_post_tpl_ref.resolve(&asg).shape(), + ); +} diff --git a/tamer/src/asg/graph.rs b/tamer/src/asg/graph.rs index b7a0a538..ca849ef3 100644 --- a/tamer/src/asg/graph.rs +++ b/tamer/src/asg/graph.rs @@ -533,7 +533,6 @@ impl AsgRelMut for OA { _rel: ProposedRel, commit: impl FnOnce(&mut Asg), ) -> Result<(), AsgError> { - let _ = _rel.ctx_span; // TODO: remove when used (dead_code) Ok(commit(asg)) } } diff --git a/tamer/src/asg/graph/object/ident.rs b/tamer/src/asg/graph/object/ident.rs index 95249a4f..4fcc1a10 100644 --- a/tamer/src/asg/graph/object/ident.rs +++ b/tamer/src/asg/graph/object/ident.rs @@ -1291,6 +1291,8 @@ impl ObjectIndex { &self, asg: &Asg, ) -> Option<::Rel> { + // XXX: This could return an abstract binding metavar reference + // depending on undefined edge ordering! self.edges(asg).next() } diff --git a/tamer/src/asg/graph/object/tpl.rs b/tamer/src/asg/graph/object/tpl.rs index bb15ccf6..340b42cf 100644 --- a/tamer/src/asg/graph/object/tpl.rs +++ b/tamer/src/asg/graph/object/tpl.rs @@ -21,8 +21,13 @@ use std::fmt::Display; -use super::{prelude::*, Doc, Expr, Ident}; -use crate::{asg::graph::ProposedRel, f::Map, parse::util::SPair, span::Span}; +use super::{ident::IdentRel, prelude::*, Doc, Expr, Ident}; +use crate::{ + asg::graph::ProposedRel, + f::Map, + parse::{prelude::Annotate, util::SPair}, + span::Span, +}; /// Template with associated name. #[derive(Debug, PartialEq, Eq)] @@ -123,7 +128,10 @@ pub enum TplShape { /// completed. /// Note that a definition is not complete until all missing identifiers /// have been defined. - Unknown, + /// + /// The associated span represents the location that resulted in + /// uncertainty. + Unknown(Span), /// The template can be expanded inline into a single [`Expr`]. /// @@ -158,18 +166,35 @@ impl TplShape { )), // Higher levels of specificity take precedence. - (shape @ TplShape::Expr(_), TplShape::Empty) - | (TplShape::Empty, shape @ TplShape::Expr(_)) - | (shape @ TplShape::Empty, TplShape::Empty) => Ok(shape), - - // Unknown is not yet handled. + // This pattern is designed to be very clear in what shape takes + // precedence over another. + // It should be clear enough that there is no value in writing + // unit test against this method since those tests' examples + // would simply reiterate this table + // (but tests for AIR should still be written to test more + // complex interactions). + #[rustfmt::skip] ( - TplShape::Unknown, - TplShape::Empty | TplShape::Unknown | TplShape::Expr(_), + TplShape::Empty, + give_precedence_to @ ( + TplShape::Empty + | TplShape::Unknown(_) + | TplShape::Expr(_) + ), ) - | (TplShape::Empty | TplShape::Expr(_), TplShape::Unknown) => { - todo!("TplShape::Unknown") - } + | ( + TplShape::Unknown(_), + give_precedence_to @ TplShape::Expr(_), + ) + | ( + give_precedence_to @ TplShape::Unknown(_), + TplShape::Empty | TplShape::Unknown(_), + ) + | ( + give_precedence_to @ TplShape::Expr(_), + TplShape::Empty | TplShape::Unknown(_), + ) + => Ok(give_precedence_to), } } @@ -183,7 +208,8 @@ impl TplShape { /// its own body. fn overwrite_span_if_any(self, span: Span) -> Self { match self { - TplShape::Empty | TplShape::Unknown => self, + TplShape::Empty => self, + TplShape::Unknown(_) => TplShape::Unknown(span), TplShape::Expr(_) => TplShape::Expr(span), } } @@ -204,8 +230,8 @@ impl Display for TplShape { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { // phrase as "template with ..." match self { - TplShape::Unknown => write!(f, "unknown shape"), TplShape::Empty => write!(f, "empty shape"), + TplShape::Unknown(_) => write!(f, "unknown shape"), TplShape::Expr(_) => write!(f, "shape of a single expression"), } } @@ -237,7 +263,78 @@ object_rel! { // Identifiers are used for both references and identifiers that // will expand into an application site. - dyn Ident, + dyn Ident { + fn pre_add_edge( + asg: &mut Asg, + rel: ProposedRel, + commit: impl FnOnce(&mut Asg), + ) -> Result<(), AsgError> { + let tpl_name = rel.from_oi.name(asg); + + match (rel.ctx_span, rel.to_oi.definition(asg)) { + // Missing definition results in shape uncertainty that + // will have to be resolved when (if) a definition + // becomes available. + (Some(ref_span), None) => { + rel.from_oi.try_map_obj_inner( + asg, + try_adapt_to(TplShape::Unknown(ref_span), tpl_name), + )?; + } + + // TAME is referentally transparent, + // so a reference to an Expr is no different than + // inlining that Expr. + (Some(ref_span), Some(IdentRel::Expr(_))) => { + rel.from_oi.try_map_obj_inner( + asg, + try_adapt_to(TplShape::Expr(ref_span), tpl_name), + )?; + } + + // This is the same as the `Tpl` tree edge below, + // but a named template instead of an anonymous one. + (Some(ref_span), Some(IdentRel::Tpl(to_oi))) => { + // TODO: Factor common logic between this and the + // `Tpl->Tpl` edge below. + let tpl_name = to_oi.name(asg); + let apply = to_oi.resolve(asg); + let apply_shape = apply + .shape() + .overwrite_span_if_any(ref_span); + + rel.from_oi.try_map_obj_inner( + asg, + try_adapt_to(apply_shape, tpl_name), + )?; + } + + // TODO: Filter this out (Ident -> Ident) + (Some(span), Some(IdentRel::Ident(_))) => { + diagnostic_todo!( + vec![span.internal_error("while parsing this reference")], + "opaque identifier or abstract binding" + ) + } + + // The mere _existence_ of metavariables (template + // params) do not influence the expansion shape. + (Some(_), Some(IdentRel::Meta(_))) => (), + + // Lack of span means that this is not a cross edge, + // and so not a reference; + // this means that the object is identified and will + // be hoisted into the rooting context of the + // application site, + // which does not impact template shape. + // TODO: Let's make that span assumption explicit in the + // `ProposeRel` abstraction. + (None, _) => (), + } + + Ok(commit(asg)) + } + }, // Template application. tree Tpl {