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

@@ -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<Signal>,
 
-    // 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<LineCounter<ast::JobPipeline>>,
+    pipeline_node: &'a ScopedRefCell<Option<NodeRef<ast::JobPipeline>>>,
 
     /// 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<LineCounter<ast::JobPipeline>>,
+        pipeline_node: &'a ScopedRefCell<Option<NodeRef<ast::JobPipeline>>>,
         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();

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<NodeType: Node>(self: &Arc<Self>) -> LineCounter<NodeType> {
-        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<Self>) -> NodeRef<JobList> {
         NodeRef::new(Arc::clone(self), self.ast.top())
@@ -193,6 +185,16 @@ pub fn child_ref<ChildType: Node>(
             node: func(self),
         }
     }
+
+    // Return the source offset of this node, if any.
+    pub fn source_offset(&self) -> Option<usize> {
+        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<NodeType: Node> Clone for NodeRef<NodeType> {
@@ -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<NodeType: Node> {
-    /// The parse tree containing the node.
-    /// This is pinned because we hold a pointer into it.
-    parsed_source: Pin<Arc<ParsedSource>>,
+#[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<NodeType: Node> LineCounter<NodeType> {
-    // 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<usize> {
-        // 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<NodeRef<NodeType>> {
-        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);
     }
 }

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<LineCounter<ast::JobPipeline>>,
+    /// The currently executing Node.
+    current_node: ScopedRefCell<Option<NodeRef<ast::JobPipeline>>>,
 
     /// The jobs associated with this parser.
     job_list: RefCell<JobList>,
@@ -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<T: 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<T: 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<NonZeroU32> {
-        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<NodeRef<ast::JobPipeline>> {
-        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);

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)