Refine cycle detection logic

Author: Hendrik van Antwerpen

stack-graphs/src/cycles.rs

@@ -39,6 +39,7 @@ use crate::arena::ListArena;
 use crate::graph::Edge;
 use crate::graph::Node;
 use crate::graph::StackGraph;
+use crate::partial::Cyclicity;
 use crate::partial::PartialPath;
 use crate::partial::PartialPaths;
 use crate::paths::Path;
@@ -173,16 +174,23 @@ impl<A: Appendable + Clone> AppendingCycleDetector<A> {
         result
     }
 
-    pub fn append(
-        &mut self,
+    pub fn append(&mut self, appendables: &mut Appendables<A>, appendage: A) {
+        self.appendages.push_front(appendables, appendage);
+    }
+
+    pub fn is_cyclic(
+        &self,
         graph: &StackGraph,
         partials: &mut PartialPaths,
         ctx: &mut A::Ctx,
         appendables: &mut Appendables<A>,
-        appendage: A,
-    ) -> Result<(), PathResolutionError> {
-        let end_node = appendage.end_node(ctx);
-        self.appendages.push_front(appendables, appendage);
+    ) -> Vec<Cyclicity> {
+        let mut cycles = Vec::new();
+
+        let end_node = match self.appendages.clone().pop_front(appendables) {
+            Some(appendage) => appendage.end_node(ctx),
+            None => return cycles,
+        };
 
         let mut maybe_cyclic_path = None;
         let mut appendages = self.appendages;
@@ -199,7 +207,7 @@ impl<A: Appendable + Clone> AppendingCycleDetector<A> {
                             break;
                         }
                     }
-                    None => return Ok(()),
+                    None => return cycles,
                 }
             }
 
@@ -224,8 +232,8 @@ impl<A: Appendable + Clone> AppendingCycleDetector<A> {
             prefix_path
                 .concatenate(graph, partials, &cyclic_path)
                 .expect("concatenating cyclic path failed ");
-            if !prefix_path.is_productive(graph, partials) {
-                return Err(PathResolutionError::DisallowedCycle);
+            if let Some(cyclicity) = prefix_path.is_cyclic(graph, partials) {
+                cycles.push(cyclicity);
             }
             maybe_cyclic_path = Some(prefix_path);
         }

stack-graphs/src/partial.rs

@@ -2069,13 +2069,14 @@ impl PartialPath {
         graph[self.end_node].is_jump_to()
     }
 
-    /// Returns whether a partial path is "productive" --- that is, it is not cyclic, or its
-    /// pre- and postcondition are incompatible.
-    pub fn is_productive(&self, graph: &StackGraph, partials: &mut PartialPaths) -> bool {
+    /// Returns whether a partial path is cyclic---that is, it starts and ends at the same node,
+    /// and its postcondition is compatible with its precondition.  If the path is cyclic, a
+    /// tuple is returned indicating whether cycle requires strengthening the pre- or postcondition.
+    pub fn is_cyclic(&self, graph: &StackGraph, partials: &mut PartialPaths) -> Option<Cyclicity> {
         // 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;
+            return None;
         }
 
         let lhs = self;
@@ -2084,30 +2085,36 @@ impl PartialPath {
 
         let join = match Self::compute_join(graph, partials, lhs, &rhs) {
             Ok(join) => join,
-            Err(_) => return true,
+            Err(_) => return None,
         };
 
         if lhs
             .symbol_stack_precondition
             .variable
             .into_option()
             .map_or(false, |v| join.symbol_bindings.get(v).iter().len() > 0)
+            || lhs
+                .scope_stack_precondition
+                .variable
+                .into_option()
+                .map_or(false, |v| join.scope_bindings.get(v).iter().len() > 0)
         {
-            // symbol stack precondition strengthened
-            return true;
-        }
-
-        if lhs
-            .scope_stack_precondition
+            Some(Cyclicity::StrengthensPrecondition)
+        } else if rhs
+            .symbol_stack_postcondition
             .variable
             .into_option()
-            .map_or(false, |v| join.scope_bindings.get(v).iter().len() > 0)
+            .map_or(false, |v| join.symbol_bindings.get(v).iter().len() > 0)
+            || rhs
+                .scope_stack_postcondition
+                .variable
+                .into_option()
+                .map_or(false, |v| join.scope_bindings.get(v).iter().len() > 0)
         {
-            // scope stack precondition strengthened
-            return true;
+            Some(Cyclicity::StrengthensPostcondition)
+        } else {
+            Some(Cyclicity::Free)
         }
-
-        false
     }
 
     /// Ensures that the content of this partial path is available in both forwards and backwards
@@ -2222,6 +2229,16 @@ impl PartialPath {
     }
 }
 
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub enum Cyclicity {
+    /// The path can be freely concatenated to itself.
+    Free,
+    /// Concatenating the path to itself strengthens the precondition---symbols are eliminated from the stack.
+    StrengthensPrecondition,
+    /// Concatenating the path to itself strengthens the postcondition---symbols are introduced on the stack.
+    StrengthensPostcondition,
+}
+
 impl<'a> DisplayWithPartialPaths for &'a PartialPath {
     fn prepare(&mut self, graph: &StackGraph, partials: &mut PartialPaths) {
         self.symbol_stack_precondition
@@ -2407,13 +2424,7 @@ impl PartialPath {
                 copious_debugging!("         * invalid extension");
                 continue;
             }
-            if new_cycle_detector
-                .append(graph, partials, &mut (), edges, extension)
-                .is_err()
-            {
-                copious_debugging!("         * cycle");
-                continue;
-            }
+            new_cycle_detector.append(edges, extension);
             result.push((new_path, new_cycle_detector));
         }
     }
@@ -2676,10 +2687,6 @@ impl PartialPaths {
     ///  (a) is minimal, no path can be constructed by stitching other paths in the set, and
     ///  (b) covers all complete paths, from references to definitions, when used for path stitching
     ///
-    /// Callers are advised _not_ to filter this set in the visitor using functions like
-    /// [`PartialPath::is_productive`][] as that may interfere with implementation changes of this
-    /// function.
-    ///
     /// This function will not return until all reachable partial paths have been processed, so
     /// `graph` must already contain a complete stack graph.  If you have a very large stack graph
     /// stored in some other storage system, and want more control over lazily loading only the
@@ -2734,6 +2741,11 @@ impl PartialPaths {
                     copious_debugging!("    * visit");
                     visit(graph, self, path);
                 }
+            } else if !path_cycle_detector
+                .is_cyclic(graph, self, &mut (), &mut edges)
+                .is_empty()
+            {
+                copious_debugging!("    * cycle");
             } else {
                 copious_debugging!("    * extend");
                 path.extend_from_file(

stack-graphs/src/stitching.rs

@@ -54,6 +54,7 @@ use crate::cycles::SimilarPathDetector;
 use crate::graph::Node;
 use crate::graph::StackGraph;
 use crate::graph::Symbol;
+use crate::partial::Cyclicity;
 use crate::partial::PartialPath;
 use crate::partial::PartialPaths;
 use crate::partial::PartialSymbolStack;
@@ -931,15 +932,12 @@ impl ForwardPartialPathStitcher {
                     copious_debugging!("        is invalid: {:?}", err);
                     continue;
                 }
-                if new_cycle_detector
-                    .append(
-                        graph,
-                        partials,
-                        db,
-                        &mut self.appended_paths,
-                        extension.into(),
-                    )
-                    .is_err()
+                new_cycle_detector.append(&mut self.appended_paths, extension.into());
+                let cycles =
+                    new_cycle_detector.is_cyclic(graph, partials, db, &mut self.appended_paths);
+                if !cycles
+                    .into_iter()
+                    .all(|c| c == Cyclicity::StrengthensPrecondition)
                 {
                     copious_debugging!("        is invalid: cyclic");
                     continue;