ast: stop using Node::ptr_eq

This function hid a bug! It converted two Nodes to `*const ()` which discards
the vtable pointer, using only the data pointer; but two nodes can and do have
the same data pointer (e.g. if one node is the first item in another).

Add the (statically dispatched) is_same_node function with a warning comment,
and adopt that instead.

src/ast.rs

@@ -145,15 +145,51 @@ fn self_memory_size(&self) -> usize {
         std::mem::size_of_val(self)
     }
 
-    // The address of the object, for comparison.
-    fn as_ptr(&self) -> *const ();
-
-    fn pointer_eq(&self, rhs: &dyn Node) -> bool {
-        std::ptr::eq(self.as_ptr(), rhs.as_ptr())
-    }
+    /// Convert to the dynamic Node type.
     fn as_node(&self) -> &dyn Node;
 }
 
+/// Return true if two nodes are the same object.
+#[inline(always)]
+pub fn is_same_node(lhs: &dyn Node, rhs: &dyn Node) -> bool {
+    // There are two subtleties here:
+    //
+    // 1. Two &dyn Node may reference the same underlying object
+    //    with different vtables - for example if one is a &dyn Node
+    //    and the other is a &dyn NodeMut.
+    //
+    // 2. Two &dyn Node may reference different underlying objects
+    //    with the same base pointer - for example if a Node is a
+    //    the first field in another Node.
+    //
+    // Therefore we make the following assumptions, which seem sound enough:
+    //   1. If two nodes have different base pointers, they reference different objects
+    //      (though NOT true in C++).
+    //   2. If two nodes have the same base pointer and same vtable, they reference
+    //      the same object.
+    //   3. If two nodes have the same base pointer but different vtables, they are the same iff
+    //      their types are equal: a Node cannot have a Node of the same type at the same address
+    //      (unless it's a ZST, which does not apply here).
+    //
+    // Note this is performance-sensitive.
+
+    // Different base pointers => not the same.
+    let lptr = lhs as *const dyn Node as *const ();
+    let rptr = rhs as *const dyn Node as *const ();
+    if !std::ptr::eq(lptr, rptr) {
+        return false;
+    }
+
+    // Same base pointer and same vtable => same object.
+    if std::ptr::eq(lhs, rhs) {
+        return true;
+    }
+
+    // Same base pointer, but different vtables.
+    // Compare the types.
+    lhs.typ() == rhs.typ()
+}
+
 /// NodeMut is a mutable node.
 trait NodeMut: Node + AcceptorMut + ConcreteNodeMut {}
 
@@ -491,9 +527,6 @@ fn try_source_range(&self) -> Option<SourceRange> {
                     Some(visitor.total)
                 }
             }
-            fn as_ptr(&self) -> *const () {
-                (self as *const $name).cast()
-            }
             fn as_node(&self) -> &dyn Node {
                 self
             }

src/parse_util.rs

@@ -1,5 +1,5 @@
 //! Various mostly unrelated utility functions related to parsing, loading and evaluating fish code.
-use crate::ast::{self, Ast, Keyword, Leaf, List, Node, NodeVisitor, Token};
+use crate::ast::{self, is_same_node, Ast, Keyword, Leaf, List, Node, NodeVisitor, Token};
 use crate::builtins::shared::builtin_exists;
 use crate::common::{
     escape_string, unescape_string, valid_var_name, valid_var_name_char, EscapeFlags,
@@ -1538,7 +1538,7 @@ fn detect_errors_in_backgrounded_job(
         // Find the index of ourselves in the job list.
         let index = jlist
             .iter()
-            .position(|job| job.pointer_eq(job_conj))
+            .position(|job| is_same_node(job, job_conj))
             .expect("Should have found the job in the list");
 
         // Try getting the next job and check its decorator.
@@ -1601,7 +1601,7 @@ fn detect_errors_in_decorated_statement(
     // Check our pipeline position.
     let pipe_pos = if job.continuation.is_empty() {
         PipelinePosition::none
-    } else if job.statement.pointer_eq(st) {
+    } else if is_same_node(&job.statement, st) {
         PipelinePosition::first
     } else {
         PipelinePosition::subsequent

src/reader.rs

@@ -42,7 +42,7 @@
 use errno::{errno, Errno};
 
 use crate::abbrs::abbrs_match;
-use crate::ast::{self, Ast, Category, Traversal};
+use crate::ast::{self, is_same_node, Ast, Category, Traversal};
 use crate::builtins::shared::ErrorCode;
 use crate::builtins::shared::STATUS_CMD_ERROR;
 use crate::builtins::shared::STATUS_CMD_OK;
@@ -5357,7 +5357,7 @@ fn extract_tokens(s: &wstr) -> Vec<PositionedToken> {
         let mut cursor = Some(node);
         while let Some(cur) = cursor {
             if let Some(stmt) = cur.as_decorated_statement() {
-                if node.pointer_eq(&stmt.command) {
+                if is_same_node(node, &stmt.command) {
                     return true;
                 }
             }

src/tests/ast.rs

@@ -0,0 +1,46 @@
+use crate::ast::{is_same_node, Ast, Node};
+use crate::wchar::prelude::*;
+
+const FISH_FUNC: &str = r#"
+function stuff --description 'Stuff'
+    set -l log "/tmp/chaos_log.(random)"
+    set -x PATH /custom/bin $PATH
+
+    echo "[$USER] Hooray" | tee -a $log 2>/dev/null
+
+    time if test (count $argv) -eq 0
+        echo "No targets specified" >> $log 2>&1
+        return 1
+    end
+
+    for target in $argv
+        command bash -c "echo" >> $log 2> /dev/null
+        switch $status
+            case 0
+                echo "Success" | tee -a $log
+            case '*'
+                echo "Failure" >> $log
+        end
+    end
+    set_color green
+end
+"#;
+
+#[test]
+fn test_is_same_node() {
+    // is_same_node is pretty subtle! Let's check it.
+    let src = WString::from_str(FISH_FUNC);
+    let ast = Ast::parse(&src, Default::default(), None);
+    assert!(!ast.errored());
+    let all_nodes: Vec<&dyn Node> = ast.walk().collect();
+    for i in 0..all_nodes.len() {
+        for j in 0..all_nodes.len() {
+            let same = is_same_node(all_nodes[i], all_nodes[j]);
+            if i == j {
+                assert!(same, "Node {} should be the same as itself", i);
+            } else {
+                assert!(!same, "Node {} should not be the same as node {}", i, j);
+            }
+        }
+    }
+}

src/tests/mod.rs

@@ -1,4 +1,5 @@
 mod abbrs;
+mod ast;
 mod ast_bench;
 mod common;
 mod complete;