From 7d98cc88507b632bddd6660e38d78119ff847ed4 Mon Sep 17 00:00:00 2001 From: Peter Ammon Date: Sun, 13 Apr 2025 11:49:06 -0700 Subject: [PATCH] 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 | 51 ++++++++++++++++++++++++++++++++++++++--------- src/parse_util.rs | 6 +++--- src/reader.rs | 4 ++-- src/tests/ast.rs | 46 ++++++++++++++++++++++++++++++++++++++++++ src/tests/mod.rs | 1 + 5 files changed, 94 insertions(+), 14 deletions(-) create mode 100644 src/tests/ast.rs diff --git a/src/ast.rs b/src/ast.rs index 0a9380c9c..a8c82da8c 100644 --- a/src/ast.rs +++ b/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 { Some(visitor.total) } } - fn as_ptr(&self) -> *const () { - (self as *const $name).cast() - } fn as_node(&self) -> &dyn Node { self } diff --git a/src/parse_util.rs b/src/parse_util.rs index 3107fec98..6a2938fef 100644 --- a/src/parse_util.rs +++ b/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 diff --git a/src/reader.rs b/src/reader.rs index 50a26333d..56906c1eb 100644 --- a/src/reader.rs +++ b/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 { 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; } } diff --git a/src/tests/ast.rs b/src/tests/ast.rs new file mode 100644 index 000000000..e43f4c2eb --- /dev/null +++ b/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); + } + } + } +} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index dba77503f..61bd1945f 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,4 +1,5 @@ mod abbrs; +mod ast; mod ast_bench; mod common; mod complete;