From e414782def6bfa892850facc205a6e30a781b8dd Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Thu, 20 Jul 2023 12:37:59 -0400 Subject: [PATCH] tamer: asg::graph: Encapsulate edge additions AIR is no longer able to explicitly add edges without going through an object-specific `ObjectIndex` API. `Asg::add_edge` was already private, but `ObjectIndex::add_edge_{to,from}` was not. The problem is that I want to augment the graph with other invariants, such as caches. I'd normally have this built into the graph system itself, but I don't have the time for the engineering effort to extend or replace Petgraph, so I'm going to build atop of it. To have confidence in any sort of caching, I need assurances that the graph can't change out from underneath an object. This gets _close_ to accomplishing that, but I'm still uncomfortable: - We're one `pub` addition away from breaking these invariants; and - Other `Object` types can still manipulates one-anothers' edges. So this is a first step that at least proves encapsulation within `asg::graph`, but ideally we'd have the system enforce, statically, that `Objects` own their _outgoing_ edges, and no other `Object` is able to manipulate them. This would ensure that any accidental future changes, or bugs, will cause compilation failures rather than e.g. allowing caches to get out of sync with the graph. DEV-13163 --- tamer/src/asg/air.rs | 10 +++---- tamer/src/asg/air/expr.rs | 12 ++++----- tamer/src/asg/graph.rs | 5 ++++ tamer/src/asg/graph/object.rs | 42 ++++++++++++++++++++++++++++- tamer/src/asg/graph/object/expr.rs | 22 ++++++++++++++- tamer/src/asg/graph/object/ident.rs | 14 ++++++++++ tamer/src/asg/graph/object/rel.rs | 28 ------------------- 7 files changed, 90 insertions(+), 43 deletions(-) diff --git a/tamer/src/asg/air.rs b/tamer/src/asg/air.rs index f28fbb64..839778af 100644 --- a/tamer/src/asg/air.rs +++ b/tamer/src/asg/air.rs @@ -1005,11 +1005,9 @@ impl AirAggregateCtx { .rooting_oi() .ok_or(AsgError::InvalidBindContext(binding_name))?; - Ok(self.lookup_lexical_or_missing(binding_name).add_edge_from( - self.asg_mut(), - oi_root, - None, - )) + Ok(self + .lookup_lexical_or_missing(binding_name) + .defined_by(self.asg_mut(), oi_root)) } /// Define an abstract identifier within the context of a container that @@ -1058,7 +1056,7 @@ impl AirAggregateCtx { Ok(oi_meta_ident .new_abstract_ident(self.asg_mut(), meta_name.span()) - .add_edge_from(self.asg_mut(), oi_root, None)) + .defined_by(self.asg_mut(), oi_root)) } } } diff --git a/tamer/src/asg/air/expr.rs b/tamer/src/asg/air/expr.rs index a6e1497c..e93fd92a 100644 --- a/tamer/src/asg/air/expr.rs +++ b/tamer/src/asg/air/expr.rs @@ -30,10 +30,7 @@ use super::{ AirAggregate, AirAggregateCtx, }; use crate::{ - asg::{ - graph::object::{ObjectIndexRelTo, ObjectIndexTo}, - ObjectKind, - }, + asg::{graph::object::ObjectIndexTo, ObjectKind}, f::Functor, parse::prelude::*, }; @@ -207,9 +204,10 @@ impl AirExprAggregate { oi_root: Option>, oi_expr: ObjectIndex, ) -> Result<(), AsgError> { - oi_root - .ok_or(AsgError::DanglingExpr(oi_expr.resolve(asg).span()))? - .add_edge_to(asg, oi_expr, None); + let oi_container = oi_root + .ok_or(AsgError::DanglingExpr(oi_expr.resolve(asg).span()))?; + + oi_expr.held_by(asg, oi_container); Ok(()) } diff --git a/tamer/src/asg/graph.rs b/tamer/src/asg/graph.rs index 7142ce01..79488c4c 100644 --- a/tamer/src/asg/graph.rs +++ b/tamer/src/asg/graph.rs @@ -197,6 +197,11 @@ impl Asg { /// /// For more information on how the ASG's ontology is enforced statically, /// see [`ObjectRelTo`](object::ObjectRelTo). + /// + /// Callers external to this module should use [`ObjectIndex`] APIs to + /// manipulate the graph; + /// this allows those objects to uphold their own invariants + /// relative to the state of the graph. fn add_edge( &mut self, from_oi: impl ObjectIndexRelTo, diff --git a/tamer/src/asg/graph/object.rs b/tamer/src/asg/graph/object.rs index 250eb3e0..9f36bc79 100644 --- a/tamer/src/asg/graph/object.rs +++ b/tamer/src/asg/graph/object.rs @@ -587,13 +587,44 @@ impl ObjectIndex { } } + /// Add an edge from `self` to `to_oi` on the provided [`Asg`]. + /// + /// Since the only invariant asserted by [`ObjectIndexRelTo`] is that + /// it may be related to `OB`, + /// this method will only permit edges to `OB`; + /// nothing else about the inner object is statically known. + /// + /// See also [`Self::add_edge_from`]. + /// + /// _This method must remain private_, + /// forcing callers to go through APIs for specific operations that + /// allow objects to enforce their own invariants. + /// This is also the reason why this method is defined here rather than + /// on [`ObjectIndexRelTo`]. + fn add_edge_to( + self, + asg: &mut Asg, + to_oi: ObjectIndex, + ctx_span: Option, + ) -> Self + where + Self: ObjectIndexRelTo, + { + asg.add_edge(self, to_oi, ctx_span); + self + } + /// Add an edge from `from_oi` to `self` on the provided [`Asg`]. /// /// An edge can only be added if ontologically valid; /// see [`ObjectRelTo`] for more information. /// /// See also [`Self::add_edge_to`]. - pub fn add_edge_from>( + /// + /// _This method must remain private_, + /// forcing callers to go through APIs for specific operations that + /// allow objects to enforce their own invariants. + fn add_edge_from>( self, asg: &mut Asg, from_oi: OF, @@ -831,6 +862,15 @@ impl ObjectIndex { self.incoming_edges_filtered(asg).next() } + /// Indicate that the given identifier `oi` is defined by this object. + pub fn defines(self, asg: &mut Asg, oi: ObjectIndex) -> Self + where + Self: ObjectIndexRelTo, + { + oi.defined_by(asg, self); + self + } + /// Describe this expression using a short independent clause. /// /// This is intended to be a concise description for use either as a diff --git a/tamer/src/asg/graph/object/expr.rs b/tamer/src/asg/graph/object/expr.rs index acd23554..6064c0d1 100644 --- a/tamer/src/asg/graph/object/expr.rs +++ b/tamer/src/asg/graph/object/expr.rs @@ -19,7 +19,7 @@ //! Expressions on the ASG. -use super::{prelude::*, Doc, Ident, Tpl}; +use super::{prelude::*, Doc, Ident, ObjectIndexTo, Tpl}; use crate::{f::Functor, num::Dim, span::Span}; use std::fmt::Display; @@ -243,4 +243,24 @@ impl ObjectIndex { pub fn ref_expr(self, asg: &mut Asg, oi_ident: ObjectIndex) -> Self { self.add_edge_to(asg, oi_ident, Some(oi_ident.span())) } + + /// The expression is held by the container `oi_container`. + /// + /// This is intended to convey that an expression would otherwise be + /// dangling (unreachable) were it not for the properties + /// of `oi_container`. + /// If this is not true, + /// consider using: + /// + /// 1. [`Self::create_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( + &self, + asg: &mut Asg, + oi_container: ObjectIndexTo, + ) -> Self { + self.add_edge_from(asg, oi_container, None) + } } diff --git a/tamer/src/asg/graph/object/ident.rs b/tamer/src/asg/graph/object/ident.rs index 5eafedb7..7c492ba3 100644 --- a/tamer/src/asg/graph/object/ident.rs +++ b/tamer/src/asg/graph/object/ident.rs @@ -1316,6 +1316,20 @@ impl ObjectIndex { self.incoming_edges_filtered(asg).next() } + /// Root this identifier into the provided object, + /// as if making the statement "`oi_root` defines `self`". + /// + /// This causes `oi_root` to act as an owner of this identifier. + /// An identifier really only ought to have one owner, + /// but this is not enforced here. + pub fn defined_by( + &self, + asg: &mut Asg, + oi_root: impl ObjectIndexRelTo, + ) -> Self { + self.add_edge_from(asg, oi_root, None) + } + /// Declare that `oi_dep` is an opaque dependency of `self`. pub fn add_opaque_dep( &self, diff --git a/tamer/src/asg/graph/object/rel.rs b/tamer/src/asg/graph/object/rel.rs index 01495de5..367370a2 100644 --- a/tamer/src/asg/graph/object/rel.rs +++ b/tamer/src/asg/graph/object/rel.rs @@ -826,39 +826,11 @@ pub trait ObjectIndexRelTo: Sized + Clone + Copy { /// See [`ObjectIndex::widen`] for more information. fn widen(&self) -> ObjectIndex; - /// Add an edge from `self` to `to_oi` on the provided [`Asg`]. - /// - /// Since the only invariant asserted by [`ObjectIndexRelTo`] is that - /// it may be related to `OB`, - /// this method will only permit edges to `OB`; - /// nothing else about the inner object is statically known. - /// To create edges to other types of objects, - /// and for more information about this operation - /// (including `ctx_span`), - /// see [`ObjectIndex::add_edge_to`]. - fn add_edge_to( - self, - asg: &mut Asg, - to_oi: ObjectIndex, - ctx_span: Option, - ) -> Self { - asg.add_edge(self, to_oi, ctx_span); - self - } - /// Check whether an edge exists from `self` to `to_oi`. fn has_edge_to(&self, asg: &Asg, to_oi: ObjectIndex) -> bool { asg.has_edge(*self, to_oi) } - /// Indicate that the given identifier `oi` is defined by this object. - fn defines(self, asg: &mut Asg, oi: ObjectIndex) -> Self - where - Self: ObjectIndexRelTo, - { - self.add_edge_to(asg, oi, None) - } - /// Iterate over the [`ObjectIndex`]es of the outgoing edges of `self` /// that match the [`ObjectKind`] `OB`. ///