Variable sealing rework (#154)

* add tests and trial a few options as a fix

* made the change to sealing which asserts if an op adds variables or not and it is passing pretty much all tests, very promising!

* all tests are passing, solution is viable

* document variable items

* add release note

RELEASE.md

@@ -14,6 +14,8 @@
 - Fix _locals graph needs updating_ bug (https://github.com/kdr-aus/ogma/pull/148)
 - Provide more verbose parsing errors (https://github.com/kdr-aus/ogma/pull/151)
 - Fix CPU spinning with completion prompt open (https://github.com/kdr-aus/ogma/pull/152)
+- Fix an uncommon variable shadowing bug by reworking the variable sealing system
+    (https://github.com/kdr-aus/ogma/pull/154)
 
 **✨ Other Updates**
 - `ogma` crate API documentation is now published at https://kdr-aus.github.io/ogma/ogma/ (https://github.com/kdr-aus/ogma/commit/cf5cc7979c399b609e2e0605ffe176e70e474ac2)

ogma/src/eng/arg.rs

@@ -13,14 +13,14 @@ pub struct ArgBuilder<'a> {
 
     compiler: &'a Compiler<'a>,
     blk_in_ty: Option<Type>,
-    chgs: Chgs<'a>,
+    chgs: &'a mut Chgs,
 }
 
 impl<'a> ArgBuilder<'a> {
     pub(super) fn new(
         node: ArgNode,
         compiler: &'a Compiler<'a>,
-        chgs: Chgs<'a>,
+        chgs: &'a mut Chgs,
         blk_in_ty: Option<Type>,
     ) -> Box<Self> {
         // see if the input and/or output types are known
@@ -42,7 +42,7 @@ impl<'a> ArgBuilder<'a> {
     fn follow_local_arg_ptr(mut self: Box<Self>) -> Box<Self> {
         let new_node = self.compiler.ag[self.node.idx()]
             .var()
-            .and_then(|t| self.compiler.lg.get(self.node.idx(), t.str()))
+            .and_then(|t| self.compiler.lg.get_opt(self.node.idx(), t.str()).1)
             .and_then(|l| match l {
                 Local::Ptr { to, tag: _ } => Some(to),
                 _ => None,
@@ -66,6 +66,20 @@ impl<'a> ArgBuilder<'a> {
         self.node
     }
 
+    /// Flag that this argument does not require it's parent op to be sealed for variable
+    /// resolution.
+    ///
+    /// This method is only required when variables are being introduced, see
+    /// [`Block::assert_adds_vars`] for more information and usage practices.
+    pub fn decouple_op_seal(self: Box<Self>) -> Box<Self> {
+        let arg = self.node();
+        let op = arg.op(self.compiler.ag());
+        self.chgs
+            .chgs
+            .push(locals_graph::Chg::BreakEdge { op, arg }.into());
+        self
+    }
+
     /// Assert that this argument will be supplied an input of type `ty`.
     ///
     /// > Since the **blocks's** input type is used often, and trying to pass this through is
@@ -99,6 +113,7 @@ impl<'a> ArgBuilder<'a> {
                     "expecting TypesSet to not be empty or a single set"
                 );
                 self.chgs
+                    .chgs
                     .push(tygraph::Chg::KnownInput(self.node.idx(), ty).into());
                 Err(Error::unknown_arg_input_type(self.tag()))
             }
@@ -132,6 +147,7 @@ impl<'a> ArgBuilder<'a> {
                     "expecting TypesSet to not be empty or a single set"
                 );
                 self.chgs
+                    .chgs
                     .push(tygraph::Chg::ObligeOutput(self.node.idx(), ty).into());
                 Err(Error::unknown_arg_output_type(self.tag()))
             }
@@ -244,7 +260,7 @@ impl<'a> ArgBuilder<'a> {
                 Ok(Hold::Expr(stack))
             }
             Var(tag) => lg
-                .get_checked(node.idx(), tag.str(), tag)
+                .get(node.idx(), tag.str(), tag)
                 .and_then(|local| match local {
                     Local::Var(var) => Ok(Hold::Var(var.clone())),
                     Local::Ptr { .. } => {
@@ -271,24 +287,13 @@ impl<'a> ArgBuilder<'a> {
     /// - `Ok(Some(true))`: The variable exists in the locals,
     /// - `Err(_)`: The variable does not exist in the locals.
     pub fn assert_var_exists(&self) -> Result<Option<bool>> {
-        let Compiler { ag, lg, .. } = self.compiler;
-
-        match &ag[self.node.idx()] {
-            astgraph::AstNode::Var(tag) => {
-                lg.get(self.node.idx(), tag.str())
-                    // Ok(Some(true)) if variable exists
-                    .map(|_| true)
-                    // if NOT sealed, return Ok(Some(false)) -- lg should be updated
-                    // NOTE we check the parent op for seal, not the argument itself.
-                    .or_else(|| {
-                        let sealed = lg.sealed(self.node.op(ag).idx());
-                        (!sealed).then(|| false)
-                    })
-                    // Error with a HARD error
-                    .ok_or_else(|| Error::var_not_found(tag))
-                    .map(Some)
-            }
-            _ => Ok(None), // not a variable
+        match self.node.var(self.compiler.ag()) {
+            Some(var) => match self.compiler.lg.get_opt(self.node.idx(), var.str()) {
+                (true, Some(_)) => Ok(Some(true)),
+                (true, None) => Err(Error::var_not_found(var)),
+                (false, _) => Ok(Some(false)),
+            },
+            None => Ok(None), // not a variable
         }
     }
 }

ogma/src/eng/blk.rs

@@ -66,14 +66,15 @@ impl<'a> Block<'a> {
         }
 
         self.chgs
+            .chgs
             .push(graphs::tygraph::Chg::KnownOutput(self.node.idx(), ty).into());
     }
 
     /// Inserts a new, anonymous type into the compiler.
     ///
     /// Note that this will not affect already resolved nodes, only inferred nodes.
     pub fn insert_anon_type_into_compiler(&mut self, ty: Type) {
-        self.chgs.push(graphs::tygraph::Chg::AnonTy(ty).into());
+        self.chgs.chgs.push(graphs::tygraph::Chg::AnonTy(ty).into());
     }
 
     /// Gets the flag that matches a given name.
@@ -102,6 +103,39 @@ impl<'a> Block<'a> {
         self.args.get(0).copied()
     }
 
+    /// Assert that this operation will be adding variables.
+    ///
+    /// This should be called early on to flag to the compiler that it cannot eagerly seal this
+    /// node.
+    /// Usually, `retain_op_coupling: false`, unless one or more arguments cannot be considered
+    /// sealed until this operation is (see the `fold` intrinsic as an example).
+    /// If `retain_op_coupling: true`, any arguments that **do not require** the op to be
+    /// considered sealed should flag this with [`ArgBuilder::decouple_op_seal`].
+    ///
+    /// Note that this function should be paired with a subsequent [`Block::assert_vars_added`]
+    /// call once all required variables have been added.
+    pub fn assert_adds_vars(&mut self, retain_op_coupling: bool) {
+        self.chgs.adds_vars = true;
+        if !retain_op_coupling {
+            let op = self.node;
+            self.chgs.chgs.extend(
+                self.args
+                    .iter()
+                    .map(|&arg| locals_graph::Chg::BreakEdge { op, arg }.into()),
+            );
+        }
+    }
+
+    /// Asserts that all the variables have been added, allowing the compiler to seal this node.
+    ///
+    /// This should be paired with a [`Block::assert_adds_vars`] call, and invoked _after_ the
+    /// variables have been added.
+    pub fn assert_vars_added(&mut self) {
+        self.chgs
+            .chgs
+            .push(locals_graph::Chg::Seal(self.node).into());
+    }
+
     /// Assert this argument is a variable and construct a reference to it.
     ///
     /// If the block does not contain a reference to an up-to-date locals, and error is returned.
@@ -124,7 +158,7 @@ impl<'a> Block<'a> {
                 Some(next) => lg
                     .new_var(next.idx(), Str::new(var.str()), ty, var.clone())
                     .map_err(|chg| {
-                        chgs.push(chg.into());
+                        chgs.chgs.push(chg.into());
                         Error::update_locals_graph(var)
                     }),
                 None => Ok(Variable::noop(var.clone(), ty)),
@@ -168,7 +202,7 @@ impl<'a> Block<'a> {
             .lg
             .new_var(arg.idx(), name, ty, tag.clone())
             .map_err(|chg| {
-                self.chgs.push(chg.into());
+                self.chgs.chgs.push(chg.into());
                 Error::update_locals_graph(tag)
             })
     }

ogma/src/eng/comp/infer.rs

@@ -35,7 +35,7 @@ impl<'d> Compiler<'d> {
             (self.lg.sealed(n) || !self.ag.detect_var(OpNode(n)))
         });
 
-        let mut chgs = Vec::new();
+        let mut chgs = Vec::default();
 
         for op in infer_nodes.into_iter().map(OpNode) {
             trial_inferred_types(op, self, &mut chgs);
@@ -143,8 +143,8 @@ enum Error {
 
 /// Tries to compile the `op` with each type in the inferred types set.
 /// Pushes `RemoveInput` if failed.
-fn trial_inferred_types(op: OpNode, compiler: &Compiler, chgs: Chgs) {
-    let sink1 = &mut Default::default();
+fn trial_inferred_types(op: OpNode, compiler: &Compiler, chgs: &mut Vec<graphs::Chg>) {
+    let mut sink1 = Default::default();
 
     let set = compiler.tg[op.idx()]
         .input
@@ -155,10 +155,19 @@ fn trial_inferred_types(op: OpNode, compiler: &Compiler, chgs: Chgs) {
         .iter()
         .cloned()
         .filter(|ty| {
-            let infer_output = &mut false;
-            let compiled = compiler
-                .compile_block(op, ty.clone(), sink1, infer_output)
-                .is_ok();
+            let mut chgs = Chgs {
+                chgs: std::mem::take(&mut sink1),
+                ..Chgs::default()
+            };
+
+            let compiled = compiler.compile_block(op, ty.clone(), &mut chgs).is_ok();
+
+            let Chgs {
+                chgs,
+                infer_output,
+                adds_vars: _,
+            } = chgs;
+            sink1 = chgs;
 
             let lg_chg_req = sink1.drain(..).any(|chg| chg.is_lg_chg());
 
@@ -167,7 +176,7 @@ fn trial_inferred_types(op: OpNode, compiler: &Compiler, chgs: Chgs) {
             // we keep in if infer_output is true, since we need more information about types
             // before we can compile this block, and removing input inferences will just reduce it
             // to an empty set.
-            !compiled && !*infer_output && !lg_chg_req
+            !compiled && !infer_output && !lg_chg_req
         })
         .map(|ty| RemoveInput(op.into(), ty))
         .map(Into::into);
@@ -316,7 +325,7 @@ impl<'a> Block<'a> {
         let Block {
             node: opnode,
             compiler: Compiler { ag, tg, .. },
-            infer_output,
+            chgs: Chgs { infer_output, .. },
             ..
         } = self;
 
@@ -340,7 +349,7 @@ impl<'a> Block<'a> {
             .cloned();
 
         if ret.is_none() {
-            **infer_output = true;
+            *infer_output = true;
         }
 
         ret