Merge pull request #286 from github/unified-path-finding

Unify path finding and path stitching

Author: Hendrik van Antwerpen

stack-graphs/CHANGELOG.md

@@ -5,6 +5,27 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+
+### Changed
+
+- The `Appendable` trait has been simplified. Its `Ctx` type parameter is gone, in favor of a separate trait `ToAppendable` that is used to find appendables for a handle. The type itself moved from the `cycles` to the `stitching` module.
+- The `ForwardPartialPathStitcher` has been generalized so that it can be used to build paths from a database or from graph edges. It now takes a type parameter indicating the type of candidates it uses. Instead of a `Database` instance, it expects a value that implements the `Candidates` and `ToAppendable` traits. The `ForwardPartialPathStitcher::process_next_phase` expects an additional `extend_until` closure that controls whether the extended paths are considered for further extension or not (using `|_,_,_| true` retains old behavior).
+
+### Fixed
+
+- A panic in `AppendingCycleDetector::is_cyclic` that occurred because variables were not always renamed before attempting to concatenate partial paths.
+- An inverted condition in `PartialSymbolStack::has_variable` that resulted in incorrect return values.
+- A bug in `Partial*Stack::unify` that resulted in recursive bindings (`$1 => SYMBOL,$1`). Any unification that would result in a recursive binding now returns an error.
+- A bug in `PartialPath::append` that would incorrectly allow appending edges that added symbols to the precondition symbol stack, even if that stack had no variable.
+
+### Removed
+
+- The `ForwardPartialPathStitcher::from_nodes` function has been removed. Callers are responsible for creating the right initial paths, which can be done using `PartialPath::from_node`.
+- The `PartialPaths::find_minimal_partial_path_set_in_file` method has been removed in favor of `ForwardPartialPathStitcher::find_minimal_partial_path_set_in_file`.
+- The `PartialPaths::find_all_complete_paths` method has been removed in favor of `ForwardPartialPathStitcher::find_all_complete_partial_paths` using `GraphEdges(None)` for the `db` argument.
+- The `OwnedOrDatabasePath` has been removed because it was obsolete with the changes to `Appendable`.
+
 ## v0.11.0 -- 2023-06-08
 
 ### Added

stack-graphs/include/stack-graphs.h

@@ -801,7 +801,7 @@ void sg_partial_path_database_mark_local_nodes(struct sg_partial_path_database *
 struct sg_node_handle_set sg_partial_path_database_local_nodes(const struct sg_partial_path_database *db);
 
 // Creates a new forward partial path stitcher that is "seeded" with a set of starting stack graph
-// nodes.
+// nodes. The path stitcher will be set up to find complete paths only.
 struct sg_forward_partial_path_stitcher *sg_forward_partial_path_stitcher_from_nodes(const struct sg_stack_graph *graph,
                                                                                      struct sg_partial_path_arena *partials,
                                                                                      size_t count,

stack-graphs/src/c.rs

@@ -30,6 +30,7 @@ use crate::partial::PartialScopedSymbol;
 use crate::partial::PartialSymbolStack;
 use crate::stitching::Database;
 use crate::stitching::ForwardPartialPathStitcher;
+use crate::stitching::GraphEdges;
 use crate::CancellationError;
 use crate::CancellationFlag;
 
@@ -1179,17 +1180,18 @@ pub extern "C" fn sg_partial_path_arena_find_partial_paths_in_file(
     let partial_path_list = unsafe { &mut *partial_path_list };
     let cancellation_flag: Option<&AtomicUsize> =
         unsafe { std::mem::transmute(cancellation_flag.as_ref()) };
-    partials
-        .find_minimal_partial_path_set_in_file(
-            graph,
-            file,
-            &AtomicUsizeCancellationFlag(cancellation_flag),
-            |_graph, partials, mut path| {
-                path.ensure_both_directions(partials);
-                partial_path_list.partial_paths.push(path);
-            },
-        )
-        .into()
+    ForwardPartialPathStitcher::find_minimal_partial_path_set_in_file(
+        graph,
+        partials,
+        file,
+        &AtomicUsizeCancellationFlag(cancellation_flag),
+        |_graph, partials, path| {
+            let mut path = path.clone();
+            path.ensure_both_directions(partials);
+            partial_path_list.partial_paths.push(path);
+        },
+    )
+    .into()
 }
 
 /// Finds all complete paths reachable from a set of starting nodes, placing the result into the
@@ -1215,18 +1217,19 @@ pub extern "C" fn sg_partial_path_arena_find_all_complete_paths(
     let path_list = unsafe { &mut *path_list };
     let cancellation_flag: Option<&AtomicUsize> =
         unsafe { std::mem::transmute(cancellation_flag.as_ref()) };
-    partials
-        .find_all_complete_paths(
-            graph,
-            starting_nodes.iter().copied().map(sg_node_handle::into),
-            &AtomicUsizeCancellationFlag(cancellation_flag),
-            |graph, _partials, path| {
-                if path.is_complete(graph) {
-                    path_list.partial_paths.push(path);
-                }
-            },
-        )
-        .into()
+    ForwardPartialPathStitcher::find_all_complete_partial_paths(
+        graph,
+        partials,
+        &mut GraphEdges(None),
+        starting_nodes.iter().copied().map(sg_node_handle::into),
+        &AtomicUsizeCancellationFlag(cancellation_flag),
+        |graph, _partials, path| {
+            if path.is_complete(graph) {
+                path_list.partial_paths.push(path.clone());
+            }
+        },
+    )
+    .into()
 }
 
 /// A handle to a partial path in a partial path database.  A zero handle represents a missing
@@ -1404,12 +1407,12 @@ struct InternalForwardPartialPathStitcher {
     previous_phase_partial_paths: *const PartialPath,
     previous_phase_partial_paths_length: usize,
     is_complete: bool,
-    stitcher: ForwardPartialPathStitcher,
+    stitcher: ForwardPartialPathStitcher<Handle<PartialPath>>,
 }
 
 impl InternalForwardPartialPathStitcher {
     fn new(
-        stitcher: ForwardPartialPathStitcher,
+        stitcher: ForwardPartialPathStitcher<Handle<PartialPath>>,
         partials: &mut PartialPaths,
     ) -> InternalForwardPartialPathStitcher {
         let mut this = InternalForwardPartialPathStitcher {
@@ -1434,7 +1437,7 @@ impl InternalForwardPartialPathStitcher {
 }
 
 /// Creates a new forward partial path stitcher that is "seeded" with a set of starting stack graph
-/// nodes.
+/// nodes. The path stitcher will be set up to find complete paths only.
 #[no_mangle]
 pub extern "C" fn sg_forward_partial_path_stitcher_from_nodes(
     graph: *const sg_stack_graph,
@@ -1445,11 +1448,17 @@ pub extern "C" fn sg_forward_partial_path_stitcher_from_nodes(
     let graph = unsafe { &(*graph).inner };
     let partials = unsafe { &mut (*partials).inner };
     let starting_nodes = unsafe { std::slice::from_raw_parts(starting_nodes, count) };
-    let stitcher = ForwardPartialPathStitcher::from_nodes(
-        graph,
-        partials,
-        starting_nodes.iter().copied().map(sg_node_handle::into),
-    );
+    let initial_paths = starting_nodes
+        .iter()
+        .copied()
+        .map(sg_node_handle::into)
+        .map(|n| {
+            let mut p = PartialPath::from_node(graph, partials, n);
+            p.eliminate_precondition_stack_variables(partials);
+            p
+        })
+        .collect::<Vec<_>>();
+    let stitcher = ForwardPartialPathStitcher::from_partial_paths(graph, partials, initial_paths);
     Box::into_raw(Box::new(InternalForwardPartialPathStitcher::new(
         stitcher, partials,
     ))) as *mut _
@@ -1526,7 +1535,9 @@ pub extern "C" fn sg_forward_partial_path_stitcher_process_next_phase(
     let partials = unsafe { &mut (*partials).inner };
     let db = unsafe { &mut (*db).inner };
     let stitcher = unsafe { &mut *(stitcher as *mut InternalForwardPartialPathStitcher) };
-    stitcher.stitcher.process_next_phase(graph, partials, db);
+    stitcher
+        .stitcher
+        .process_next_phase(graph, partials, db, |_, _, _| true);
     stitcher.update_previous_phase_partial_paths(partials);
 }