fix dfs iterator when same file imported multiple times into another

Author: Strum355

server/main/src/dfs.rs

@@ -63,25 +63,22 @@ impl<'a> Iterator for Dfs<'a> {
                 touch: 1,
             });
 
-            let mut children: Vec<NodeIndex> = self.graph.child_node_indexes(child).collect();
+            let mut children: Vec<_> = self
+                .graph
+                .get_all_child_positions(child)
+                .collect();
+            children.reverse();
 
             if !children.is_empty() {
-                // sort by line number in parent
-                children.sort_by(|x, y| {
-                    let graph = &self.graph.graph;
-                    let edge1 = graph.edge_weight(graph.find_edge(child, *x).unwrap()).unwrap();
-                    let edge2 = graph.edge_weight(graph.find_edge(child, *y).unwrap()).unwrap();
 
-                    edge2.line.cmp(&edge1.line)
-                });
-
-                match self.check_for_cycle(&children) {
+                let child_indexes: Vec<_> = children.iter().map(|c| c.0).collect();
+                match self.check_for_cycle(&child_indexes) {
                     Ok(_) => {}
                     Err(e) => return Some(Err(e)),
                 };
 
                 for child in children {
-                    self.stack.push(child);
+                    self.stack.push(child.0);
                 }
             } else {
                 self.reset_path_to_branch();
@@ -314,10 +311,10 @@ mod dfs_test {
             //     \ /  \
             //      6 - 7
 
-            assert!(is_cyclic_directed(&graph.graph));
-
             let next = dfs.next().unwrap();
             assert_that!(next, err());
+
+            assert!(is_cyclic_directed(&graph.graph));
         }
         {
             let mut graph = CachedStableGraph::new();

server/main/src/graph.rs

@@ -59,9 +59,9 @@ impl CachedStableGraph {
         PathBuf::from_str(&self.graph[node]).unwrap()
     }
 
-    /// returns an iterator over all the `IncludePosition`'s between a parent and its child for all the positions
+    /// Returns an iterator over all the `IncludePosition`'s between a parent and its child for all the positions
     /// that the child may be imported into the parent, in order of import.
-    pub fn get_edge_metas(&self, parent: NodeIndex, child: NodeIndex) -> impl Iterator<Item = IncludePosition> + '_ {
+    pub fn get_child_positions(&self, parent: NodeIndex, child: NodeIndex) -> impl Iterator<Item = IncludePosition> + '_ {
         let mut edges = self
             .graph
             .edges(parent)
@@ -77,6 +77,18 @@ impl CachedStableGraph {
         edges.into_iter()
     }
 
+    /// Returns an iterator over all the `(NodeIndex, IncludePosition)` tuples between a node and all its children, in order
+    /// of import.
+    pub fn get_all_child_positions(&self, node: NodeIndex) -> impl Iterator<Item = (NodeIndex, IncludePosition)> + '_ {
+        let mut edges = self.graph.edges(node).map(|edge| {
+            let child = self.graph.edge_endpoints(edge.id()).unwrap().1;
+            (child, self.graph[edge.id()])
+        })
+        .collect::<Vec<_>>();
+        edges.sort_by(|x, y| x.1.line.cmp(&y.1.line));
+        edges.into_iter()
+    }
+
     pub fn add_node(&mut self, name: &Path) -> NodeIndex {
         if let Some(idx) = self.cache.get(name) {
             return *idx;
@@ -99,14 +111,6 @@ impl CachedStableGraph {
             .and_then(|edge| self.graph.remove_edge(edge));
     }
 
-    pub fn child_node_metas(&self, node: NodeIndex) -> impl Iterator<Item = (PathBuf, IncludePosition)> + '_ {
-        self.graph.neighbors(node).map(move |n| {
-            let edge = self.graph.find_edge(node, n).unwrap();
-            let edge_meta = self.graph.edge_weight(edge).unwrap();
-            return (self.reverse_index.get(&n).unwrap().clone(), *edge_meta);
-        })
-    }
-
     pub fn child_node_indexes(&self, node: NodeIndex) -> impl Iterator<Item = NodeIndex> + '_ {
         self.graph.neighbors(node)
     }
@@ -236,9 +240,9 @@ mod graph_test {
         //    / \
         //   1   1
 
-        assert_eq!(2, graph.get_edge_metas(idx0, idx1).count());
+        assert_eq!(2, graph.get_child_positions(idx0, idx1).count());
 
-        let mut edge_metas = graph.get_edge_metas(idx0, idx1);
+        let mut edge_metas = graph.get_child_positions(idx0, idx1);
         assert_eq!(Some(IncludePosition { line: 2, start: 0, end: 0 }), edge_metas.next());
         assert_eq!(Some(IncludePosition { line: 4, start: 0, end: 0 }), edge_metas.next());
     }

server/main/src/main.rs

@@ -251,7 +251,9 @@ impl MinecraftShaderLanguageServer {
             Some(n) => n,
         };
 
-        let prev_children: HashSet<_> = HashSet::from_iter(self.graph.borrow().child_node_metas(idx));
+        let prev_children: HashSet<_> = HashSet::from_iter(self.graph.borrow().get_all_child_positions(idx).map(|tup| {
+            (self.graph.borrow().get_node(tup.0), tup.1)
+        }));
         let new_children: HashSet<_> = includes.iter().cloned().collect();
 
         let to_be_added = new_children.difference(&prev_children);
@@ -825,7 +827,7 @@ impl LanguageServerHandling for MinecraftShaderLanguageServer {
                 .child_node_indexes(node)
                 .filter_map::<Vec<DocumentLink>, _>(|child| {
                     let graph = self.graph.borrow();
-                    graph.get_edge_metas(node, child).map(|value| {
+                    graph.get_child_positions(node, child).map(|value| {
                         let path = graph.get_node(child);
                         let url = match Url::from_file_path(&path) {
                             Ok(url) => url,

server/main/src/merge_views.rs

@@ -8,6 +8,7 @@ use std::{
 use core::slice::Iter;
 
 use petgraph::stable_graph::NodeIndex;
+use slog_scope::debug;
 
 use crate::graph::CachedStableGraph;
 use crate::source_mapper::SourceMapper;
@@ -88,27 +89,16 @@ impl<'a> MergeViewBuilder<'a> {
         // );
 
         // last_offset_set.insert((first, None), version_char_offsets.1);
-        self.last_offset_set.insert(
-            FilialTuple {
-                child: first,
-                parent: None,
-            },
-            0,
-        );
+        self.set_last_offset_for_tuple(None, first, 0);
 
         // stack to keep track of the depth first traversal
         let mut stack = VecDeque::<NodeIndex>::new();
 
         self.create_merge_views(&mut merge_list, &mut extra_lines, &mut stack);
 
         // now we add a view of the remainder of the root file
-        let offset = *self
-            .last_offset_set
-            .get(&FilialTuple {
-                child: first,
-                parent: None,
-            })
-            .unwrap();
+
+        let offset = self.get_last_offset_for_tuple(None, first).unwrap();
 
         let len = first_source.len();
         merge_list.push_back(&first_source[min(offset, len)..]);
@@ -135,8 +125,8 @@ impl<'a> MergeViewBuilder<'a> {
                 .parent_child_edge_iterator
                 .entry(*n)
                 .or_insert_with(|| {
-                    let edge_metas = self.graph.get_edge_metas(parent, child);
-                    Box::new(edge_metas)
+                    let child_positions = self.graph.get_child_positions(parent, child);
+                    Box::new(child_positions)
                 })
                 .next()
                 .unwrap();
@@ -147,15 +137,16 @@ impl<'a> MergeViewBuilder<'a> {
             let (char_for_line, char_following_line) = self.char_offset_for_line(edge.line, parent_source);
 
             let offset = *self
-                .last_offset_set
-                .insert(
-                    FilialTuple {
-                        child: parent,
-                        parent: stack.back().copied(),
-                    },
-                    char_following_line,
-                )
+                .set_last_offset_for_tuple(stack.back().copied(), parent, char_following_line)
                 .get_or_insert(0);
+
+            debug!("creating view to start child file";
+                "parent" => parent_path.to_str().unwrap(), "child" => child_path.to_str().unwrap(),
+                "grandparent" => stack.back().copied().map(|g| self.graph.get_node(g).to_str().unwrap().to_string()), // self.graph.get_node().to_str().unwrap(),
+                "last_parent_offset" => offset, "line" => edge.line, "char_for_line" => char_for_line,
+                "char_following_line" => char_following_line,
+            );
+
             merge_list.push_back(&parent_source[offset..char_for_line]);
             self.add_opening_line_directive(&child_path, child, merge_list, extra_lines);
 
@@ -173,13 +164,7 @@ impl<'a> MergeViewBuilder<'a> {
                             }
                         };
                         merge_list.push_back(&child_source[..offset]);
-                        self.last_offset_set.insert(
-                            FilialTuple {
-                                child,
-                                parent: Some(parent),
-                            },
-                            0,
-                        );
+                        self.set_last_offset_for_tuple(Some(parent), child, 0);
                         // +2 because edge.line is 0 indexed but #line is 1 indexed and references the *following* line
                         self.add_closing_line_directive(edge.line + 2, &parent_path, parent, merge_list, extra_lines);
                         // if the next pair's parent is not the current pair's parent, we need to bubble up
@@ -193,29 +178,17 @@ impl<'a> MergeViewBuilder<'a> {
                     self.create_merge_views(merge_list, extra_lines, stack);
                     stack.pop_back();
 
-                    let offset = *self
-                        .last_offset_set
-                        .get(&FilialTuple {
-                            child,
-                            parent: Some(parent),
-                        })
-                        .unwrap();
+                    let offset = self.get_last_offset_for_tuple(Some(parent), child).unwrap();
                     let child_source = self.sources.get(&child_path).unwrap();
                     // this evaluates to false once the file contents have been exhausted aka offset = child_source.len() + 1
                     let end_offset = match child_source.ends_with('\n') {
-                        true => 1,  /* child_source.len()-1 */
-                        false => 0, /* child_source.len() */
+                        true => 1,
+                        false => 0,
                     };
                     if offset < child_source.len() - end_offset {
                         // if ends in \n\n, we want to exclude the last \n for some reason. Ask optilad
-                        merge_list.push_back(&child_source[offset../* std::cmp::max( */child_source.len()-end_offset/* , offset) */]);
-                        self.last_offset_set.insert(
-                            FilialTuple {
-                                child,
-                                parent: Some(parent),
-                            },
-                            0,
-                        );
+                        merge_list.push_back(&child_source[offset..child_source.len() - end_offset]);
+                        self.set_last_offset_for_tuple(Some(parent), child, 0);
                     }
 
                     // +2 because edge.line is 0 indexed but #line is 1 indexed and references the *following* line
@@ -234,20 +207,26 @@ impl<'a> MergeViewBuilder<'a> {
                         false => child_source.len(),
                     };
                     merge_list.push_back(&child_source[..offset]);
-                    self.last_offset_set.insert(
-                        FilialTuple {
-                            child,
-                            parent: Some(parent),
-                        },
-                        0,
-                    );
+                    self.set_last_offset_for_tuple(Some(parent), child, 0);
                     // +2 because edge.line is 0 indexed but #line is 1 indexed and references the *following* line
                     self.add_closing_line_directive(edge.line + 2, &parent_path, parent, merge_list, extra_lines);
                 }
             }
         }
     }
 
+    fn set_last_offset_for_tuple(&mut self, parent: Option<NodeIndex>, child: NodeIndex, offset: usize) -> Option<usize> {
+        debug!("inserting last offset";
+            "parent" => parent.map(|p| self.graph.get_node(p).to_str().unwrap().to_string()),
+            "child" => self.graph.get_node(child).to_str().unwrap().to_string(),
+            "offset" => offset);
+        self.last_offset_set.insert(FilialTuple { child, parent }, offset)
+    }
+
+    fn get_last_offset_for_tuple(&self, parent: Option<NodeIndex>, child: NodeIndex) -> Option<usize> {
+        self.last_offset_set.get(&FilialTuple { child, parent }).copied()
+    }
+
     // returns the character offset + 1 of the end of line number `line` and the character
     // offset + 1 for the end of the line after the previous one
     fn char_offset_for_line(&self, line_num: usize, source: &str) -> (usize, usize) {