diff --git a/tamer/src/asg/graph.rs b/tamer/src/asg/graph.rs index 59aa4518..52a8c026 100644 --- a/tamer/src/asg/graph.rs +++ b/tamer/src/asg/graph.rs @@ -19,6 +19,7 @@ //! Abstract graph as the basis for concrete ASGs. +use super::object::ObjectContainer; use super::{ AsgError, FragmentText, Ident, IdentKind, Object, ObjectIndex, ObjectKind, Source, TransitionResult, @@ -48,11 +49,8 @@ pub type AsgResult = Result; /// There are currently no data stored on edges ("edge weights"). pub type AsgEdge = (); -/// Each node of the graph represents an object. -/// -/// Enclosed in an [`Option`] to permit moving owned values out of the -/// graph. -pub type Node = Option; +/// Each node of the graph. +pub type Node = ObjectContainer; /// Index size for Graph nodes and edges. type Ix = global::ProgSymSize; @@ -126,14 +124,15 @@ impl Asg { let mut graph = Graph::with_capacity(objects, edges); let mut index = Vec::with_capacity(objects); - // Exhaust the first index to be used as a placeholder. - let empty_node = graph.add_node(None); + // Exhaust the first index to be used as a placeholder + // (its value does not matter). + let empty_node = graph.add_node(Object::Root.into()); index.push(empty_node); // Automatically add the root which will be used to determine what // identifiers ought to be retained by the final program. // This is not indexed and is not accessable by name. - let root_node = graph.add_node(Some(Object::Root)); + let root_node = graph.add_node(Object::Root.into()); Self { graph, @@ -193,7 +192,7 @@ impl Asg { let sym = ident.symbol(); self.lookup(sym).unwrap_or_else(|| { - let index = self.graph.add_node(Some(Ident::declare(ident).into())); + let index = self.graph.add_node(Ident::declare(ident).into()); self.index_identifier(sym, index); ObjectIndex::new(index, ident.span()) @@ -236,22 +235,12 @@ impl Asg { where F: FnOnce(Ident) -> TransitionResult, { - let node = self.graph.node_weight_mut(identi.into()).unwrap(); + let container = self.graph.node_weight_mut(identi.into()).unwrap(); - let obj = node - .take() - .expect("internal error: missing object") - .unwrap_ident(); - - f(obj) - .and_then(|obj| { - node.replace(obj.into()); - Ok(identi) - }) - .or_else(|(orig, err)| { - node.replace(orig.into()); - Err(err.into()) - }) + container + .try_replace_with(f) + .map(|()| identi) + .map_err(Into::into) } // TODO: This is transitional; @@ -379,7 +368,7 @@ impl Asg { pub(super) fn create(&mut self, obj: O) -> ObjectIndex { let o = obj.into(); let span = o.span(); - let node_id = self.graph.add_node(Some(o)); + let node_id = self.graph.add_node(ObjectContainer::from(o.into())); ObjectIndex::new(node_id, span) } @@ -393,11 +382,9 @@ impl Asg { /// It is nevertheless wrapped in an [`Option`] just in case. #[inline] pub fn get(&self, index: ObjectIndex) -> Option<&O> { - self.graph.node_weight(index.into()).map(|node| { - node.as_ref() - .expect("internal error: Asg::get missing Node data") - .into() - }) + self.graph + .node_weight(index.into()) + .map(ObjectContainer::get) } /// Map over an inner [`Object`] referenced by [`ObjectIndex`]. @@ -441,24 +428,9 @@ impl Asg { "invalid ObjectIndex: data are missing from the ASG", ); - // Any function borrowing from the graph ought to also be responsible - // for returning it in all possible code paths, - // as we are. - // And error here means that this must not be the case, - // or that we're breaking encapsulation somewhere. - let cur_obj = obj_container.take().diagnostic_expect( - diagnostic_borrowed_node_desc(index), - "inaccessible ObjectIndex: object has not been returned to the ASG", - ); + obj_container.replace_with(f); - let new_obj: Object = f(cur_obj.into()).into(); - let new_span = new_obj.span(); - - // This is where we replace the object that we borrowed, - // as referenced in the above panic. - obj_container.replace(new_obj); - - index.overwrite(new_span) + index.overwrite(obj_container.get::().span()) } /// Retrieve the [`ObjectIndex`] to which the given `ident` is bound, @@ -511,18 +483,18 @@ impl Asg { /// (that allows for the invariant to be violated) /// or direct manipulation of the underlying graph. pub fn get_ident_obj(&self, ident: SPair) -> Option<&O> { - self.get_ident_obj_oi::(ident).map(|oi| { - let obj_container = - self.graph.node_weight(oi.into()).diagnostic_expect( - diagnostic_node_missing_desc(oi), - "invalid ObjectIndex: data are missing from the ASG", - ); + self.get_ident_obj_oi::(ident) + .map(|oi| self.expect_obj(oi)) + } - obj_container.as_ref().diagnostic_expect( - diagnostic_borrowed_node_desc(oi), - "inaccessible ObjectIndex: object has not been returned to the ASG", - ).into() - }) + pub(super) fn expect_obj(&self, oi: ObjectIndex) -> &O { + let obj_container = + self.graph.node_weight(oi.into()).diagnostic_expect( + diagnostic_node_missing_desc(oi), + "invalid ObjectIndex: data are missing from the ASG", + ); + + obj_container.get() } /// Attempt to retrieve the [`Object`] to which the given `ident` is bound, @@ -645,23 +617,6 @@ fn diagnostic_node_missing_desc( ] } -fn diagnostic_borrowed_node_desc( - index: ObjectIndex, -) -> Vec> { - vec![ - index.internal_error( - "this object was borrowed from the graph and \ - was not returned", - ), - index.help("this means that some operation used take() on the object"), - index.help(" container but never replaced it with an updated object"), - index.help( - " after the operation completed, which should not \ - be possible.", - ), - ] -} - fn diagnostic_opaque_ident_desc(ident: SPair) -> Vec> { vec![ ident.internal_error( diff --git a/tamer/src/asg/object.rs b/tamer/src/asg/object.rs index 4b99ab45..fa3d1158 100644 --- a/tamer/src/asg/object.rs +++ b/tamer/src/asg/object.rs @@ -61,15 +61,15 @@ //! [`ObjectIndex`] is associated with a span derived from the point of its //! creation to handle this diagnostic situation automatically. -use super::{Expr, Ident}; +use super::{Asg, Expr, Ident}; use crate::{ - diagnose::Annotate, + diagnose::{panic::DiagnosticPanic, Annotate, AnnotatedSpan}, diagnostic_panic, f::Functor, span::{Span, UNKNOWN_SPAN}, }; use petgraph::graph::NodeIndex; -use std::{fmt::Display, marker::PhantomData}; +use std::{convert::Infallible, fmt::Display, marker::PhantomData}; /// An object on the ASG. /// @@ -315,3 +315,110 @@ impl From> for Span { } } } + +/// A container for an [`Object`] allowing for owned borrowing of data. +/// +/// The purpose of allowing this owned borrowing is to permit a functional +/// style of object manipulation, +/// like the rest of the TAMER system, +/// despite the mutable underpinnings of the ASG. +/// This is accomplished by wrapping each object in an [`Option`] so that we +/// can [`Option::take`] its inner value temporarily. +/// +/// This container has a critical invariant: +/// the inner [`Option`] must _never_ be [`None`] after a method exits, +/// no matter what branches are taken. +/// Methods operating on owned data enforce this invariant by mapping over +/// data and immediately placing the new value to the container before the +/// method completes. +/// This container will panic if this variant is not upheld. +/// +/// TODO: Make this `pub(super)` when [`Asg`]'s public API is cleaned up. +#[derive(Debug, PartialEq)] +pub struct ObjectContainer(Option); + +impl ObjectContainer { + /// Retrieve an immutable reference to the inner [`Object`], + /// narrowed to expected type `O`. + /// + /// Panics + /// ====== + /// This will panic if the object on the graph is not the expected + /// [`ObjectKind`] `O`. + pub fn get(&self) -> &O { + let Self(container) = self; + + container + .as_ref() + .diagnostic_unwrap(container_oops()) + .into() + } + + /// Attempt to modify the inner [`Object`], + /// narrowed to expected type `O`, + /// returning any error. + /// + /// See also [`Self::replace_with`] if the operation is [`Infallible`]. + /// + /// Panics + /// ====== + /// This will panic if the object on the graph is not the expected + /// [`ObjectKind`] `O`. + pub fn try_replace_with( + &mut self, + f: impl FnOnce(O) -> Result, + ) -> Result<(), E> { + let ObjectContainer(container) = self; + + let obj = container.take().diagnostic_unwrap(container_oops()).into(); + + // NB: We must return the object to the container in all code paths! + let result = f(obj) + .map(|obj| { + container.replace(obj.into()); + }) + .map_err(|(orig, err)| { + container.replace(orig.into()); + err + }); + + debug_assert!(container.is_some()); + result + } + + /// Modify the inner [`Object`], + /// narrowed to expected type `O`. + /// + /// See also [`Self::try_replace_with`] if the operation can fail. + /// + /// Panics + /// ====== + /// This will panic if the object on the graph is not the expected + /// [`ObjectKind`] `O`. + pub fn replace_with(&mut self, f: impl FnOnce(O) -> O) { + let _ = self.try_replace_with::(|obj| Ok(f(obj))); + } +} + +impl> From for ObjectContainer { + fn from(obj: I) -> Self { + Self(Some(obj.into())) + } +} + +fn container_oops() -> Vec> { + // This used to be a real span, + // but since this invariant is easily verified and should absolutely + // never occur, + // there's no point in complicating the API. + let span = UNKNOWN_SPAN; + + vec![ + span.help("this means that some operation used take() on the object"), + span.help(" container but never replaced it with an updated object"), + span.help( + " after the operation completed, which should not \ + be possible.", + ), + ] +} diff --git a/tamer/src/ld/poc.rs b/tamer/src/ld/poc.rs index 75f7455d..632efafb 100644 --- a/tamer/src/ld/poc.rs +++ b/tamer/src/ld/poc.rs @@ -113,8 +113,8 @@ pub fn graphml(package_path: &str, output: &str) -> Result<(), TameldError> { GraphMl::new(&g) .pretty_print(true) .export_node_weights(Box::new(|node| { - let (name, kind, generated) = match node { - Some(Object::Ident(n)) => { + let (name, kind, generated) = match node.get() { + Object::Ident(n) => { let generated = match n.src() { Some(src) => src.generated, None => false, @@ -127,16 +127,11 @@ pub fn graphml(package_path: &str, output: &str) -> Result<(), TameldError> { ) } // TODO: We want these filtered. - Some(_) => ( + _ => ( String::from("non-ident"), "non-ident".into(), "false".into(), ), - None => ( - String::from("missing"), - "missing".into(), - "false".into(), - ), }; vec![