From 08a9f5e6831fbccb969c256216f7a2ce7a72c39f Mon Sep 17 00:00:00 2001 From: Peter Ammon Date: Thu, 29 Jan 2026 19:55:10 -0800 Subject: [PATCH] Remove LineCounter LineCounter was a funny type which aided in eagerly computing line numbers. It's no longer needed as we lazily compute lines, so remove it. --- src/parse_execution.rs | 15 ++++--- src/parse_tree.rs | 75 ++++++++++++----------------------- src/parser.rs | 36 ++++++++--------- tests/checks/line-number.fish | 7 ++++ 4 files changed, 56 insertions(+), 77 deletions(-) diff --git a/src/parse_execution.rs b/src/parse_execution.rs index 54aa10ea6..56b4e3807 100644 --- a/src/parse_execution.rs +++ b/src/parse_execution.rs @@ -30,7 +30,7 @@ ParseError, ParseErrorCode, ParseErrorList, ParseKeyword, ParseTokenType, StatementDecoration, parse_error_offset_source_start, }; -use crate::parse_tree::{LineCounter, NodeRef, ParsedSourceRef}; +use crate::parse_tree::{NodeRef, ParsedSourceRef}; use crate::parse_util::{ MaybeParentheses::CommandSubstitution, locate_cmdsubst_range, unescape_wildcards, }; @@ -84,9 +84,9 @@ pub struct ExecutionContext<'a> { // unwinding. cancel_signal: Option, - // Helper to count lines. + // The currently executing pipeline node. // This is shared with the Parser so that the Parser can access the current line. - line_counter: &'a ScopedRefCell>, + pipeline_node: &'a ScopedRefCell>>, /// The block IO chain. /// For example, in `begin; foo ; end < file.txt` this would have the 'file.txt' IO. @@ -131,13 +131,13 @@ impl<'a> ExecutionContext<'a> { pub fn new( pstree: ParsedSourceRef, block_io: IoChain, - line_counter: &'a ScopedRefCell>, + pipeline_node: &'a ScopedRefCell>>, test_only_suppress_stderr: bool, ) -> Self { Self { pstree, cancel_signal: None, - line_counter, + pipeline_node, block_io, test_only_suppress_stderr, } @@ -1530,9 +1530,8 @@ fn run_1_job( let _saved_eval_level = ctx.parser().push_scope(|s| s.eval_level += 1); // Save the executing node. - let _saved_node = self - .line_counter - .scoped_set(std::ptr::from_ref(job_node), |s| &mut s.node); + let executing_node = NodeRef::new(Arc::clone(self.pstree()), job_node); + let _saved_node = self.pipeline_node.scoped_replace(Some(executing_node)); // Profiling support. let profile_item_id = ctx.parser().create_profile_item(); diff --git a/src/parse_tree.rs b/src/parse_tree.rs index 38f063baa..7318a03ad 100644 --- a/src/parse_tree.rs +++ b/src/parse_tree.rs @@ -147,14 +147,6 @@ pub fn new(src: WString, ast: Ast) -> Self { ParsedSource { src, ast } } - // Return a line counter over this source. - pub fn line_counter(self: &Arc) -> LineCounter { - LineCounter { - parsed_source: Pin::new(Arc::clone(self)), - node: std::ptr::null(), - } - } - // Return the top NodeRef for the parse tree, which is of type JobList. pub fn top_job_list(self: &Arc) -> NodeRef { NodeRef::new(Arc::clone(self), self.ast.top()) @@ -193,6 +185,16 @@ pub fn child_ref( node: func(self), } } + + // Return the source offset of this node, if any. + pub fn source_offset(&self) -> Option { + self.try_source_range().map(|r| r.start()) + } + + // Return the source, as a string. + pub fn source_str(&self) -> &wstr { + &self.parsed_source.src + } } impl Clone for NodeRef { @@ -253,50 +255,23 @@ pub fn parse_source( } } -/// A type which assists in returning line numbers. -/// This is a somewhat strange type which both counts line numbers and also holds -/// a reference to a "current" node; this matches the expected usage from parse_execution. -pub struct LineCounter { - /// The parse tree containing the node. - /// This is pinned because we hold a pointer into it. - parsed_source: Pin>, +#[cfg(test)] +mod tests { + use super::*; - /// The node itself. This points into the parsed source, or it may be null. - pub node: *const NodeType, -} + #[test] + fn test_lineno_for_offset() { + let src = L!("a\nb\nc\nd").to_owned(); + let ps = ParsedSource::new(src, ast::parse(L!(""), ParseTreeFlags::default(), None)); + let mut cache = SourceLineCache::default(); -impl LineCounter { - // Return a line counter for empty source. - pub fn empty() -> Self { - let parsed_source = - Pin::new(parse_source(WString::new(), ParseTreeFlags::default(), None).unwrap()); - LineCounter { - parsed_source, - node: std::ptr::null(), - } - } + // Forward progression + assert_eq!(ps.lineno_for_offset(0, &mut cache), 1); + assert_eq!(ps.lineno_for_offset(4, &mut cache), 3); + assert_eq!(ps.lineno_for_offset(6, &mut cache), 4); - // Return the 0 based character offset of the node. - pub fn source_offset_of_node(&mut self) -> Option { - // Safety: any node is valid for the lifetime of the source. - let node = unsafe { self.node.as_ref()? }; - let range = node.try_source_range()?; - Some(range.start()) - } - - // Return the source. - pub fn get_source(&self) -> &wstr { - &self.parsed_source.src - } - - /// Return a NodeRef for the current node, if any. - pub fn node_ref(&self) -> Option> { - if self.node.is_null() { - return None; - } - Some(NodeRef::new( - Pin::into_inner(self.parsed_source.clone()), - self.node, - )) + // Backward progression + assert_eq!(ps.lineno_for_offset(2, &mut cache), 2); + assert_eq!(ps.lineno_for_offset(0, &mut cache), 1); } } diff --git a/src/parser.rs b/src/parser.rs index ff0bddf2e..77de30aa0 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -26,7 +26,7 @@ SOURCE_LOCATION_UNKNOWN, }; use crate::parse_execution::{EndExecutionReason, ExecutionContext}; -use crate::parse_tree::{LineCounter, NodeRef, ParsedSourceRef, SourceLineCache, parse_source}; +use crate::parse_tree::{NodeRef, ParsedSourceRef, SourceLineCache, parse_source}; use crate::portable_atomic::AtomicU64; use crate::prelude::*; use crate::proc::{JobGroupRef, JobList, JobRef, Pid, ProcStatus, job_reap}; @@ -396,9 +396,8 @@ pub enum CancelBehavior { pub struct Parser { pub interactive_initialized: RelaxedAtomicBool, - /// A shared line counter. This is handed out to each execution context - /// so they can communicate the line number back to this Parser. - line_counter: ScopedRefCell>, + /// The currently executing Node. + current_node: ScopedRefCell>>, /// The jobs associated with this parser. job_list: RefCell, @@ -460,7 +459,7 @@ impl Parser { pub fn new(variables: EnvStack, cancel_behavior: CancelBehavior) -> Parser { let result = Self { interactive_initialized: RelaxedAtomicBool::new(false), - line_counter: ScopedRefCell::new(LineCounter::empty()), + current_node: ScopedRefCell::new(None), job_list: RefCell::default(), wait_handles: RefCell::default(), block_list: RefCell::default(), @@ -704,15 +703,14 @@ pub fn eval_node( let cancel_checker: CancelChecker = Box::new(move || check_cancel_signal().is_some()); op_ctx.cancel_checker = cancel_checker; - // Restore the line counter. - let ps = node.parsed_source_ref(); - let restore_line_counter = self.line_counter.scoped_replace(ps.line_counter()); + // Restore the current pipeline node. + let restore_current_node = self.current_node.scoped_replace(None); // Create a new execution context. let mut execution_context = ExecutionContext::new( - ps, + node.parsed_source_ref(), block_io.clone(), - &self.line_counter, + &self.current_node, test_only_suppress_stderr, ); @@ -723,7 +721,7 @@ pub fn eval_node( let new_exec_count = self.libdata().exec_count; let new_status_count = self.libdata().status_count; - ScopeGuarding::commit(restore_line_counter); + ScopeGuarding::commit(restore_current_node); self.pop_block(scope_block); job_reap(self, false, Some(block_io)); // reap again @@ -777,7 +775,10 @@ pub fn expand_argument_list( /// /// init.fish (line 127): ls|grep pancake pub fn current_line(&self) -> WString { - let Some(source_offset) = self.line_counter.borrow_mut().source_offset_of_node() else { + let Some(node_ref) = self.current_node.borrow().clone() else { + return WString::new(); + }; + let Some(source_offset) = node_ref.source_offset() else { return WString::new(); }; @@ -810,7 +811,7 @@ pub fn current_line(&self) -> WString { }; let mut line_info = empty_error.describe_with_prefix( - self.line_counter.borrow().get_source(), + node_ref.source_str(), &prefix, self.is_interactive(), skip_caret, @@ -825,10 +826,7 @@ pub fn current_line(&self) -> WString { /// Returns the current line number, indexed from 1. pub fn get_lineno(&self) -> Option { - self.line_counter - .borrow() - .node_ref() - .and_then(|n| n.lineno()) + self.current_node.borrow().as_ref().and_then(|n| n.lineno()) } /// Returns the current line number, indexed from 1, or zero if not sourced. @@ -839,7 +837,7 @@ pub fn get_lineno_for_display(&self) -> u32 { /// Returns a NodeRef to the current node being executed, if any. /// This can be used for lazy line number computation. pub fn current_node_ref(&self) -> Option> { - self.line_counter.borrow().node_ref() + self.current_node.borrow().clone() } /// Return whether we are currently evaluating a "block" such as an if statement. @@ -1031,7 +1029,7 @@ pub fn sync_uvars_and_fire(&self, always: bool) { /// Pushes a new block. Returns an id (index) of the block, which is stored in the parser. pub fn push_block(&self, mut block: Block) -> BlockId { block.src_filename = self.current_filename(); - block.src_node = self.line_counter.borrow().node_ref(); + block.src_node.clone_from(&self.current_node.borrow()); if block.typ() != BlockType::top { let new_scope = block.typ() == BlockType::function_call { shadows: true }; self.vars().push(new_scope); diff --git a/tests/checks/line-number.fish b/tests/checks/line-number.fish index da30a0541..ea2b77d52 100644 --- a/tests/checks/line-number.fish +++ b/tests/checks/line-number.fish @@ -25,3 +25,10 @@ end line-number # CHECK: 23 + +bind -M +# CHECKERR: bind: -M: option requires an argument +# CHECKERR: {{.*}} (line 29): +# CHECKERR: bind -M +# CHECKERR: ^ +# CHECKERR: (Type 'help bind' for related documentation)