Compare Node by content and position

src/dijkstra.rs

@@ -1,11 +1,8 @@
-
-
 use std::cmp::Ordering;
 use std::collections::HashSet;
 use std::collections::{BinaryHeap, HashMap};
 use std::hash::{Hash, Hasher};
 
-use crate::positions::SingleLineSpan;
 use crate::tree_diff::{ChangeKind, Node};
 use typed_arena::Arena;
 use Edge::*;
@@ -16,33 +13,24 @@ struct Vertex<'a> {
     rhs_node: Option<&'a Node<'a>>,
 }
 
-fn node_pos<'a>(node: &'a Node<'a>) -> (Option<Vec<SingleLineSpan>>, Option<Vec<SingleLineSpan>>) {
+// When walking the graph, we want to consider nodes distinct if they
-    // TODO: get first SingleLineSpan rather than cloning the whole vec.
+// have different content or if they have different positions. There
-    match node {
+// may be multiple nodes with same content at different positions.
-        Node::List {
-            open_position,
-            close_position,
-            ..
-        } => (Some(open_position.clone()), Some(close_position.clone())),
-        Node::Atom { position, .. } => (Some(position.clone()), None),
-    }
-}
-
-// Compare nodes by position. If we have multiple atoms with the same
-// content, we don't want to think that we've found a route to all of
-// them.
 impl<'a> PartialEq for Vertex<'a> {
     fn eq(&self, other: &Self) -> bool {
-        self.lhs_node.map(node_pos) == other.lhs_node.map(node_pos)
+        match (self.lhs_node, other.lhs_node) {
-            && self.rhs_node.map(node_pos) == other.rhs_node.map(node_pos)
+            (Some(lhs_node), Some(rhs_node)) => lhs_node.equal_content_and_pos(rhs_node),
+            (None, None) => true,
+            _ => false,
+        }
     }
 }
 impl<'a> Eq for Vertex<'a> {}
 
 impl<'a> Hash for Vertex<'a> {
     fn hash<H: Hasher>(&self, state: &mut H) {
-        self.lhs_node.map(node_pos).hash(state);
+        self.lhs_node.hash(state);
-        self.rhs_node.map(node_pos).hash(state);
+        self.rhs_node.hash(state);
     }
 }
 

src/tree_diff.rs

@@ -250,6 +250,61 @@ impl<'a> Node<'a> {
             _ => false,
         }
     }
+
+    /// Does this `Node` have the same position in all its subnodes?
+    ///
+    /// Nodes with different numbers of children return false
+    /// regardless of top-level positions.
+    fn equal_pos(&self, other: &Self) -> bool {
+        match (&self, other) {
+            (
+                Atom {
+                    position: lhs_position,
+                    ..
+                },
+                Atom {
+                    position: rhs_position,
+                    ..
+                },
+            ) => lhs_position == rhs_position,
+            (
+                List {
+                    open_position: lhs_open_position,
+                    close_position: lhs_close_position,
+                    children: lhs_children,
+                    ..
+                },
+                List {
+                    open_position: rhs_open_position,
+                    close_position: rhs_close_position,
+                    children: rhs_children,
+                    ..
+                },
+            ) => {
+                if lhs_open_position != rhs_open_position
+                    || lhs_close_position != rhs_close_position
+                {
+                    return false;
+                }
+                if lhs_children.len() != rhs_children.len() {
+                    return false;
+                }
+
+                for (lhs_child, rhs_child) in lhs_children.iter().zip(rhs_children.iter()) {
+                    if !lhs_child.equal_pos(rhs_child) {
+                        return false;
+                    }
+                }
+
+                true
+            }
+            _ => false,
+        }
+    }
+
+    pub fn equal_content_and_pos(&self, other: &Self) -> bool {
+        self.equal_pos(other) && self.equal_content(other)
+    }
 }
 
 pub fn set_next<'a>(node: &'a Node<'a>) {
@@ -276,7 +331,7 @@ fn set_next_<'a>(node: &'a Node<'a>, new_next: Option<&'a Node<'a>>) {
 
 impl<'a> PartialEq for Node<'a> {
     fn eq(&self, other: &Self) -> bool {
-        self.equal_content(other)
+        self.equal_content_and_pos(other)
     }
 }
 impl<'a> Eq for Node<'a> {}
@@ -285,18 +340,23 @@ impl<'a> Hash for Node<'a> {
     fn hash<H: Hasher>(&self, state: &mut H) {
         match self {
             List {
+                open_position,
                 open_delimiter,
+                close_position,
                 close_delimiter,
                 children,
                 ..
             } => {
+                open_position.hash(state);
                 open_delimiter.hash(state);
                 close_delimiter.hash(state);
+                close_position.hash(state);
                 for child in children {
                     child.hash(state);
                 }
             }
-            Atom { content, .. } => {
+            Atom { position, content, .. } => {
+                position.hash(state);
                 content.hash(state);
             }
         }
@@ -475,7 +535,7 @@ mod tests {
     }
 
     #[test]
-    fn test_atom_equality_ignores_change_and_pos() {
+    fn test_atom_equality_ignores_change() {
         assert_eq!(
             Atom {
                 next: Cell::new(None),
@@ -492,9 +552,9 @@ mod tests {
                 next: Cell::new(None),
                 change: Cell::new(None),
                 position: vec![SingleLineSpan {
-                    line: 10.into(),
+                    line: 1.into(),
-                    start_col: 20,
+                    start_col: 2,
-                    end_col: 30
+                    end_col: 3
                 }],
                 content: "foo".into(),
                 kind: Other,