address review comments

- refactor serde module
- rename InvalidNodeID error variant
- better variable names in loops
- reduce error handling code in `load_files`
- delete old serialization code
- serialize Position::containing_line and Position::trimmed_line

Author: oppiliappan

lsp-positions/src/lib.rs

@@ -53,11 +53,9 @@ pub struct Position {
     pub column: Offset,
     /// The UTF-8 byte indexes (within the file) of the start and end of the line containing the
     /// character
-    #[cfg_attr(feature = "serde", serde(skip))]
     pub containing_line: Range<usize>,
     /// The UTF-8 byte indexes (within the file) of the start and end of the line containing the
     /// character, with any leading and trailing whitespace removed
-    #[cfg_attr(feature = "serde", serde(skip))]
     pub trimmed_line: Range<usize>,
 }
 

stack-graphs/src/json.rs

@@ -5,7 +5,6 @@
 // Please see the LICENSE-APACHE or LICENSE-MIT files in this distribution for license details.
 // ------------------------------------------------------------------------------------------------
 
-use itertools::Itertools;
 use lsp_positions::Offset;
 use lsp_positions::Position;
 use lsp_positions::Span;
@@ -35,60 +34,14 @@ use crate::partial::PartialScopedSymbol;
 use crate::partial::PartialSymbolStack;
 use crate::partial::ScopeStackVariable;
 use crate::partial::SymbolStackVariable;
+use crate::serde::ImplicationFilter;
 pub use crate::serde::{Filter, NoFilter};
 use crate::stitching::Database;
 
 #[derive(Debug, Error)]
 #[error(transparent)]
 pub struct JsonError(#[from] serde_json::error::Error);
 
-/// Filter implementation that enforces all implications of another filter.
-/// For example, that nodes frome excluded files are not included, etc.
-pub(crate) struct ImplicationFilter<'a>(pub &'a dyn Filter);
-
-impl Filter for ImplicationFilter<'_> {
-    fn include_file(&self, graph: &StackGraph, file: &Handle<File>) -> bool {
-        self.0.include_file(graph, file)
-    }
-
-    fn include_node(&self, graph: &StackGraph, node: &Handle<Node>) -> bool {
-        graph[*node]
-            .id()
-            .file()
-            .map_or(true, |f| self.include_file(graph, &f))
-            && self.0.include_node(graph, node)
-    }
-
-    fn include_edge(&self, graph: &StackGraph, source: &Handle<Node>, sink: &Handle<Node>) -> bool {
-        self.include_node(graph, source)
-            && self.include_node(graph, sink)
-            && self.0.include_edge(graph, source, sink)
-    }
-
-    fn include_partial_path(
-        &self,
-        graph: &StackGraph,
-        paths: &PartialPaths,
-        path: &PartialPath,
-    ) -> bool {
-        let super_ok = self.0.include_partial_path(graph, paths, path);
-        if !super_ok {
-            return false;
-        }
-        let all_included_edges = path
-            .edges
-            .iter_unordered(paths)
-            .map(|e| graph.node_for_id(e.source_node_id).unwrap())
-            .chain(std::iter::once(path.end_node))
-            .tuple_windows()
-            .all(|(source, sink)| self.include_edge(graph, &source, &sink));
-        if !all_included_edges {
-            return false;
-        }
-        true
-    }
-}
-
 //-----------------------------------------------------------------------------
 // InStackGraph
 
@@ -107,61 +60,6 @@ impl<'a, T> InStackGraph<'a, T> {
     }
 }
 
-//-----------------------------------------------------------------------------
-// StackGraph
-
-impl<'a> StackGraph {
-    pub fn to_json(&'a self, f: &'a dyn Filter) -> JsonStackGraph {
-        JsonStackGraph(self, f)
-    }
-
-    pub fn to_serializable(&self) -> crate::serde::StackGraph {
-        self.to_serializable_filter(&NoFilter)
-    }
-
-    pub fn to_serializable_filter(&self, f: &'a dyn Filter) -> crate::serde::StackGraph {
-        crate::serde::StackGraph::from_graph_filter(self, f)
-    }
-}
-
-pub struct JsonStackGraph<'a>(&'a StackGraph, &'a dyn Filter);
-
-impl<'a> JsonStackGraph<'a> {
-    pub fn to_value(&self) -> Result<Value, JsonError> {
-        Ok(serde_json::to_value(self)?)
-    }
-
-    pub fn to_string(&self) -> Result<String, JsonError> {
-        Ok(serde_json::to_string(self)?)
-    }
-
-    pub fn to_string_pretty(&self) -> Result<String, JsonError> {
-        Ok(serde_json::to_string_pretty(self)?)
-    }
-}
-
-impl Serialize for JsonStackGraph<'_> {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        let mut ser = serializer.serialize_struct("stack_graph", 2)?;
-        ser.serialize_field(
-            "files",
-            &InStackGraph(self.0, &Files, &ImplicationFilter(self.1)),
-        )?;
-        ser.serialize_field(
-            "nodes",
-            &InStackGraph(self.0, &Nodes, &ImplicationFilter(self.1)),
-        )?;
-        ser.serialize_field(
-            "edges",
-            &InStackGraph(self.0, &Edges, &ImplicationFilter(self.1)),
-        )?;
-        ser.end()
-    }
-}
-
 //-----------------------------------------------------------------------------
 // Files
 

stack-graphs/src/serde/filter.rs

@@ -0,0 +1,137 @@
+use crate::{
+    arena::Handle,
+    graph::{File, Node, StackGraph},
+    partial::{PartialPath, PartialPaths},
+};
+use itertools::Itertools;
+
+pub trait Filter {
+    /// Return whether elements for the given file must be included.
+    fn include_file(&self, graph: &StackGraph, file: &Handle<File>) -> bool;
+
+    /// Return whether the given node must be included.
+    /// Nodes of excluded files are always excluded.
+    fn include_node(&self, graph: &StackGraph, node: &Handle<Node>) -> bool;
+
+    /// Return whether the given edge must be included.
+    /// Edges via excluded nodes are always excluded.
+    fn include_edge(&self, graph: &StackGraph, source: &Handle<Node>, sink: &Handle<Node>) -> bool;
+
+    /// Return whether the given path must be included.
+    /// Paths via excluded nodes or edges are always excluded.
+    fn include_partial_path(
+        &self,
+        graph: &StackGraph,
+        paths: &PartialPaths,
+        path: &PartialPath,
+    ) -> bool;
+}
+
+impl<F> Filter for F
+where
+    F: Fn(&StackGraph, &Handle<File>) -> bool,
+{
+    fn include_file(&self, graph: &StackGraph, file: &Handle<File>) -> bool {
+        self(graph, file)
+    }
+
+    fn include_node(&self, _graph: &StackGraph, _node: &Handle<Node>) -> bool {
+        true
+    }
+
+    fn include_edge(
+        &self,
+        _graph: &StackGraph,
+        _source: &Handle<Node>,
+        _sink: &Handle<Node>,
+    ) -> bool {
+        true
+    }
+
+    fn include_partial_path(
+        &self,
+        _graph: &StackGraph,
+        _paths: &PartialPaths,
+        _path: &PartialPath,
+    ) -> bool {
+        true
+    }
+}
+
+// Filter implementation that includes everything.
+pub struct NoFilter;
+
+impl Filter for NoFilter {
+    fn include_file(&self, _graph: &StackGraph, _file: &Handle<File>) -> bool {
+        true
+    }
+
+    fn include_node(&self, _graph: &StackGraph, _node: &Handle<Node>) -> bool {
+        true
+    }
+
+    fn include_edge(
+        &self,
+        _graph: &StackGraph,
+        _source: &Handle<Node>,
+        _sink: &Handle<Node>,
+    ) -> bool {
+        true
+    }
+
+    fn include_partial_path(
+        &self,
+        _graph: &StackGraph,
+        _paths: &PartialPaths,
+        _path: &PartialPath,
+    ) -> bool {
+        true
+    }
+}
+
+/// Filter implementation that enforces all implications of another filter.
+/// For example, that nodes frome excluded files are not included, etc.
+pub(crate) struct ImplicationFilter<'a>(pub &'a dyn Filter);
+
+impl Filter for ImplicationFilter<'_> {
+    fn include_file(&self, graph: &StackGraph, file: &Handle<File>) -> bool {
+        self.0.include_file(graph, file)
+    }
+
+    fn include_node(&self, graph: &StackGraph, node: &Handle<Node>) -> bool {
+        graph[*node]
+            .id()
+            .file()
+            .map_or(true, |f| self.include_file(graph, &f))
+            && self.0.include_node(graph, node)
+    }
+
+    fn include_edge(&self, graph: &StackGraph, source: &Handle<Node>, sink: &Handle<Node>) -> bool {
+        self.include_node(graph, source)
+            && self.include_node(graph, sink)
+            && self.0.include_edge(graph, source, sink)
+    }
+
+    fn include_partial_path(
+        &self,
+        graph: &StackGraph,
+        paths: &PartialPaths,
+        path: &PartialPath,
+    ) -> bool {
+        let super_ok = self.0.include_partial_path(graph, paths, path);
+        if !super_ok {
+            return false;
+        }
+        let all_included_edges = path
+            .edges
+            .iter_unordered(paths)
+            .map(|e| graph.node_for_id(e.source_node_id).unwrap())
+            .chain(std::iter::once(path.end_node))
+            .tuple_windows()
+            .all(|(source, sink)| self.include_edge(graph, &source, &sink));
+        if !all_included_edges {
+            return false;
+        }
+        true
+    }
+}