Fix cycle detection logic, and add tests for productive paths

Author: Hendrik van Antwerpen

stack-graphs/src/cycles.rs

@@ -34,10 +34,12 @@ use std::collections::{HashMap, VecDeque};
 use smallvec::SmallVec;
 
 use crate::arena::Handle;
+use crate::graph::Edge;
 use crate::graph::Node;
 use crate::graph::StackGraph;
 use crate::partial::PartialPath;
 use crate::partial::PartialPaths;
+use crate::paths::Extend;
 use crate::paths::Path;
 use crate::stitching::Database;
 use crate::stitching::OwnedOrDatabasePath;
@@ -148,34 +150,48 @@ impl AppendingCycleDetector {
         &mut self,
         graph: &StackGraph,
         partials: &mut PartialPaths,
-        node: Handle<Node>,
-        new_path: &PartialPath,
+        appended_node: Handle<Node>,
     ) -> bool {
-        let i = match self.nodes.iter().position(|n| *n == node) {
+        // find last occurrence of the appended node
+        let i = self.nodes.iter().rposition(|n| *n == appended_node);
+
+        // add new node
+        self.nodes.push(appended_node);
+
+        // check if the appended node was found
+        let i = match i {
             Some(i) => i,
             None => return true,
         };
-        let mut rhs = PartialPath::from_node(graph, partials, node);
+
+        // construct cyclic path
+        let mut cyclic_path = PartialPath::from_node(graph, partials, self.nodes[i]);
         for node in self.nodes.range(i + 1..) {
-            graph[*node]
-                .apply_to_partial_stacks(
+            if cyclic_path.ends_in_jump(graph) {
+                // restore jump target
+                cyclic_path
+                    .scope_stack_precondition
+                    .push_back(partials, *node);
+                cyclic_path
+                    .scope_stack_postcondition
+                    .push_front(partials, *node);
+                cyclic_path.resolve(graph, partials).unwrap();
+            }
+            cyclic_path
+                .append(
                     graph,
                     partials,
-                    &mut rhs.symbol_stack_precondition,
-                    &mut rhs.scope_stack_precondition,
-                    &mut rhs.symbol_stack_postcondition,
-                    &mut rhs.scope_stack_postcondition,
+                    Edge {
+                        source: cyclic_path.end_node,
+                        sink: *node,
+                        precedence: 0,
+                    },
                 )
                 .unwrap();
         }
-        let mut loop_path = new_path.clone();
-        if loop_path.concatenate(graph, partials, &rhs).is_ok()
-            && loop_path.symbol_stack_postcondition.len()
-                > new_path.symbol_stack_postcondition.len()
-        {
-            return false;
-        }
-        true
+
+        // process path if it is productive
+        cyclic_path.is_productive(graph, partials)
     }
 }
 
@@ -191,7 +207,7 @@ impl JoiningCycleDetector {
         path: PartialPath,
     ) -> Self {
         Self {
-            paths: vec![OwnedOrDatabasePath::Owned(path)].into(),
+            paths: vec![path.into()].into(),
         }
     }
 
@@ -202,7 +218,7 @@ impl JoiningCycleDetector {
         path: Handle<PartialPath>,
     ) -> Self {
         Self {
-            paths: vec![OwnedOrDatabasePath::Db(path)].into(),
+            paths: vec![path.into()].into(),
         }
     }
 
@@ -211,25 +227,43 @@ impl JoiningCycleDetector {
         graph: &StackGraph,
         partials: &mut PartialPaths,
         db: &Database,
-        new_path: &PartialPath,
+        joined_path: OwnedOrDatabasePath,
     ) -> bool {
-        if let Some(i) = self
+        // add new fragment
+        self.paths.push(joined_path.into());
+        let joined_path = self.paths.back().unwrap().get(db);
+
+        // find a fragment starting with our end node
+        // (includes new fragment, in case that is cyclic)
+        let end_node = joined_path.end_node;
+        let i = match self
             .paths
             .iter()
-            .rposition(|p| p.get(db).start_node == new_path.end_node)
+            .rposition(|p| p.get(db).start_node == end_node)
         {
-            let mut rhs = self.paths[i].get(db).clone();
-            for path in self.paths.range(i + 1..) {
-                rhs.concatenate(graph, partials, path.get(db)).unwrap();
-            }
-            let mut loop_path = new_path.clone();
-            if loop_path.concatenate(graph, partials, &rhs).is_ok()
-                && loop_path.symbol_stack_postcondition.len()
-                    > new_path.symbol_stack_postcondition.len()
-            {
-                return false;
+            Some(i) => i,
+            None => return true,
+        };
+
+        // construct the cyclic path
+        let mut cyclic_path = self.paths[i].get(db).clone();
+        for path in self.paths.range(i + 1..) {
+            let path = path.get(db);
+            if cyclic_path.ends_in_jump(graph) {
+                // restore jump target
+                cyclic_path
+                    .scope_stack_precondition
+                    .push_back(partials, path.start_node);
+                cyclic_path
+                    .scope_stack_postcondition
+                    .push_front(partials, path.start_node);
+                cyclic_path.resolve(graph, partials).unwrap();
             }
+            cyclic_path.ensure_no_overlapping_variables(partials, path);
+            cyclic_path.concatenate(graph, partials, path).unwrap();
         }
-        true
+
+        // process path if it is productive
+        cyclic_path.is_productive(graph, partials)
     }
 }

stack-graphs/src/partial.rs

@@ -2068,26 +2068,82 @@ impl PartialPath {
         graph[self.end_node].is_jump_to()
     }
 
-    /// Returns whether a partial path is "productive" — that is, whether it adds useful
-    /// information to a path.  Non-productive paths are ignored.
-    pub fn is_productive(&self, partials: &mut PartialPaths) -> bool {
+    /// Returns whether a partial path is "productive" --- that is, it is not cyclic, or it consumes
+    /// symbols from the stack, possible replacing them by different symbols.
+    pub fn is_productive(&self, graph: &StackGraph, partials: &mut PartialPaths) -> bool {
         // StackGraph ensures that there are no nodes with duplicate IDs, so we can do a simple
         // comparison of node handles here.
         if self.start_node != self.end_node {
             return true;
         }
-        if !self
+
+        let lhs = self;
+        let mut rhs = self.clone();
+        rhs.ensure_no_overlapping_variables(partials, lhs);
+
+        // If the join node operates on the stack, the effect is present in both sides.
+        // For correct joining, we must undo the effect on one of the sides. We match
+        // the end node of the lhs, and the start node of the rhs separately, depending
+        // on whether we must manipulate the lhs postcondition or rhs precondition,
+        // respectively. The reason we cannot use only one of the lhs end or rhs start
+        // node is that the variables used in them may differ.
+        let mut lhs_symbol_stack_postcondition = lhs.symbol_stack_postcondition;
+        let mut lhs_scope_stack_postcondition = lhs.scope_stack_postcondition;
+        let mut rhs_symbol_stack_precondition = rhs.symbol_stack_precondition;
+        let mut rhs_scope_stack_precondition = rhs.scope_stack_precondition;
+        graph[lhs.end_node].halfopen_closed_postcondition(
+            partials,
+            &mut lhs_symbol_stack_postcondition,
+            &mut lhs_scope_stack_postcondition,
+        );
+        graph[rhs.start_node].halfopen_closed_precondition(
+            partials,
+            &mut rhs_symbol_stack_precondition,
+            &mut rhs_scope_stack_precondition,
+        );
+
+        let mut symbol_bindings = PartialSymbolStackBindings::new();
+        let mut scope_bindings = PartialScopeStackBindings::new();
+        if lhs_symbol_stack_postcondition
+            .unify(
+                partials,
+                rhs_symbol_stack_precondition,
+                &mut symbol_bindings,
+                &mut scope_bindings,
+            )
+            .is_err()
+        {
+            // symbol stacks are incompatible
+            return true;
+        }
+        if lhs_scope_stack_postcondition
+            .unify(partials, rhs_scope_stack_precondition, &mut scope_bindings)
+            .is_err()
+        {
+            // scope stacks are incompatible
+            return true;
+        }
+
+        if lhs
             .symbol_stack_precondition
-            .matches(partials, self.symbol_stack_postcondition)
+            .variable
+            .into_option()
+            .map_or(false, |v| symbol_bindings.get(v).iter().len() > 0)
         {
+            // symbol stack precondition strengthened
             return true;
         }
-        if !self
+
+        if lhs
             .scope_stack_precondition
-            .matches(partials, self.scope_stack_postcondition)
+            .variable
+            .into_option()
+            .map_or(false, |v| scope_bindings.get(v).iter().len() > 0)
         {
+            // scope stack precondition strengthened
             return true;
         }
+
         false
     }
 
@@ -2350,17 +2406,25 @@ impl PartialPath {
         let extensions = graph.outgoing_edges(self.end_node);
         result.reserve(extensions.size_hint().0);
         for extension in extensions {
+            copious_debugging!(
+                "      -> with edge {} -> {}",
+                extension.source.display(graph),
+                extension.sink.display(graph)
+            );
             if !graph[extension.sink].is_in_file(file) {
+                copious_debugging!("         * outside file");
                 continue;
             }
             let mut new_path = self.clone();
             let mut new_cycle_detector = path_cycle_detector.clone();
             // If there are errors adding this edge to the partial path, or resolving the resulting
             // partial path, just skip the edge — it's not a fatal error.
             if new_path.append(graph, partials, extension).is_err() {
+                copious_debugging!("         * invalid extension");
                 continue;
             }
-            if !new_cycle_detector.appended(graph, partials, extension.sink, &new_path) {
+            if !new_cycle_detector.appended(graph, partials, extension.sink) {
+                copious_debugging!("         * cycle");
                 continue;
             }
             result.push((new_path, new_cycle_detector));

stack-graphs/src/stitching.rs

@@ -903,23 +903,23 @@ impl ForwardPartialPathStitcher {
         self.next_iteration.0.reserve(extension_count);
         self.next_iteration.1.reserve(extension_count);
         for extension in &self.candidate_partial_paths {
-            let mut extension = db[*extension].clone();
+            let mut extension_path = db[*extension].clone();
             copious_debugging!("    Extend {}", partial_path.display(graph, partials));
-            copious_debugging!("      with {}", extension.display(graph, partials));
-            extension.ensure_no_overlapping_variables(partials, partial_path);
-            copious_debugging!("        -> {}", extension.display(graph, partials));
+            copious_debugging!("      with {}", extension_path.display(graph, partials));
+            extension_path.ensure_no_overlapping_variables(partials, partial_path);
+            copious_debugging!("        -> {}", extension_path.display(graph, partials));
 
             let mut new_partial_path = partial_path.clone();
             let mut new_cycle_detector = cycle_detector.clone();
             // If there are errors concatenating these partial paths, or resolving the resulting
             // partial path, just skip the extension — it's not a fatal error.
             #[cfg_attr(not(feature = "copious-debugging"), allow(unused_variables))]
             {
-                if let Err(err) = new_partial_path.concatenate(graph, partials, &extension) {
+                if let Err(err) = new_partial_path.concatenate(graph, partials, &extension_path) {
                     copious_debugging!("        is invalid: {:?}", err);
                     continue;
                 }
-                if !new_cycle_detector.joined(graph, partials, db, &extension) {
+                if !new_cycle_detector.joined(graph, partials, db, extension.into()) {
                     copious_debugging!("        is invalid: cyclic");
                     continue;
                 }
@@ -1032,7 +1032,7 @@ impl ForwardPartialPathStitcher {
 }
 
 #[derive(Clone)]
-pub(crate) enum OwnedOrDatabasePath {
+pub enum OwnedOrDatabasePath {
     Db(Handle<PartialPath>),
     Owned(PartialPath),
 }
@@ -1045,3 +1045,21 @@ impl OwnedOrDatabasePath {
         }
     }
 }
+
+impl From<Handle<PartialPath>> for OwnedOrDatabasePath {
+    fn from(value: Handle<PartialPath>) -> Self {
+        Self::Db(value)
+    }
+}
+
+impl From<&Handle<PartialPath>> for OwnedOrDatabasePath {
+    fn from(value: &Handle<PartialPath>) -> Self {
+        Self::Db(*value)
+    }
+}
+
+impl From<PartialPath> for OwnedOrDatabasePath {
+    fn from(value: PartialPath) -> Self {
+        Self::Owned(value)
+    }
+}