From b97598fa6c237362f2551534583a4314d8788d81 Mon Sep 17 00:00:00 2001 From: Peter Ammon Date: Fri, 27 Dec 2024 16:42:38 -0800 Subject: [PATCH] Clean up some logic around handling the parser blocks Fix a todo. Enforce reverse iteration order. --- src/builtins/return.rs | 2 +- src/builtins/shared.rs | 2 +- src/event.rs | 2 +- src/parse_execution.rs | 5 +-- src/parser.rs | 81 ++++++++++++++++++++++-------------------- 5 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/builtins/return.rs b/src/builtins/return.rs index 489ba60c7..cb8abe907 100644 --- a/src/builtins/return.rs +++ b/src/builtins/return.rs @@ -50,7 +50,7 @@ pub fn r#return(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> Err(e) => return e, }; - let has_function_block = parser.blocks().iter().any(|b| b.is_function_call()); + let has_function_block = parser.blocks_iter_rev().any(|b| b.is_function_call()); // *nix does not support negative return values, but our `return` builtin happily accepts being // called with negative literals (e.g. `return -1`). diff --git a/src/builtins/shared.rs b/src/builtins/shared.rs index 51c7034fa..25e9fc342 100644 --- a/src/builtins/shared.rs +++ b/src/builtins/shared.rs @@ -873,7 +873,7 @@ fn builtin_break_continue( // Paranoia: ensure we have a real loop. // This is checked in the AST but we may be invoked dynamically, e.g. just via "eval break". let mut has_loop = false; - for b in parser.blocks().iter().rev() { + for b in parser.blocks_iter_rev() { if [BlockType::while_block, BlockType::for_block].contains(&b.typ()) { has_loop = true; break; diff --git a/src/event.rs b/src/event.rs index 2ae9ee8cc..75303a09e 100644 --- a/src/event.rs +++ b/src/event.rs @@ -264,7 +264,7 @@ impl Event { /// Test if specified event is blocked. fn is_blocked(&self, parser: &Parser) -> bool { - for block in parser.blocks().iter().rev() { + for block in parser.blocks_iter_rev() { if block.event_blocks { return true; } diff --git a/src/parse_execution.rs b/src/parse_execution.rs index 62c66e5b4..35b7595dc 100644 --- a/src/parse_execution.rs +++ b/src/parse_execution.rs @@ -198,10 +198,7 @@ impl<'a> ExecutionContext { // Check for stack overflow in case of function calls (regular stack overflow) or string // substitution blocks, which can be recursively called with eval (issue #9302). - let block_type = { - let blocks = ctx.parser().blocks(); - blocks.get(associated_block).unwrap().typ() - }; + let block_type = ctx.parser().block_with_id(associated_block).typ(); if (block_type == BlockType::top && ctx.parser().function_stack_is_overflowing()) || (block_type == BlockType::subst && ctx.parser().is_eval_depth_exceeded()) { diff --git a/src/parser.rs b/src/parser.rs index b6ca73835..e6a2b4a83 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -356,7 +356,10 @@ pub enum ParserStatusVar { count_, } -pub type BlockId = usize; +/// A newtype for the block index. +/// This is the naive position in the block list. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct BlockId(usize); /// Controls the behavior when fish itself receives a signal and there are /// no blocks on the stack. @@ -447,9 +450,7 @@ impl Parser { /// Return whether we are currently evaluating a function. pub fn is_function(&self) -> bool { - self.blocks() - .iter() - .rev() + self.blocks_iter_rev() // If a function sources a file, don't descend further. .take_while(|b| b.typ() != BlockType::source) .any(|b| b.is_function_call()) @@ -457,9 +458,7 @@ impl Parser { /// Return whether we are currently evaluating a command substitution. pub fn is_command_substitution(&self) -> bool { - self.blocks() - .iter() - .rev() + self.blocks_iter_rev() // If a function sources a file, don't descend further. .take_while(|b| b.typ() != BlockType::source) .any(|b| b.typ() == BlockType::subst) @@ -737,7 +736,7 @@ impl Parser { /// This supports 'status is-block'. pub fn is_block(&self) -> bool { // Note historically this has descended into 'source', unlike 'is_function'. - self.blocks().iter().rev().any(|b| { + self.blocks_iter_rev().any(|b| { ![ BlockType::top, BlockType::subst, @@ -749,37 +748,51 @@ impl Parser { /// Return whether we have a breakpoint block. pub fn is_breakpoint(&self) -> bool { - self.blocks() - .iter() - .rev() + self.blocks_iter_rev() .any(|b| b.typ() == BlockType::breakpoint) } - /// Return the list of blocks. The first block is at the top. - /// todo!("this RAII object should only be used for iterating over it (in reverse). Maybe enforce this") - pub fn blocks(&self) -> Ref<'_, Vec> { - self.block_list.borrow() + // Return an iterator over the blocks, in reverse order. + // That is, the first block is the innermost block. + pub fn blocks_iter_rev<'a>(&'a self) -> impl Iterator> { + let blocks = self.block_list.borrow(); + let mut indices = (0..blocks.len()).rev(); + std::iter::from_fn(move || { + let last = indices.next()?; + // note this clone is cheap + Some(Ref::map(Ref::clone(&blocks), |bl| &bl[last])) + }) } + + // Return the block at a given index, where 0 is the innermost block. pub fn block_at_index(&self, index: usize) -> Option> { - let block_list = self.blocks(); - if index >= block_list.len() { + let block_list = self.block_list.borrow(); + let block_count = block_list.len(); + if index >= block_count { None } else { - Some(Ref::map(block_list, |bl| &bl[bl.len() - 1 - index])) + Some(Ref::map(block_list, |bl| &bl[block_count - 1 - index])) } } + + // Return the block at a given index, where 0 is the innermost block. pub fn block_at_index_mut(&self, index: usize) -> Option> { let block_list = self.block_list.borrow_mut(); - if index >= block_list.len() { + let block_count = block_list.len(); + if index >= block_count { None } else { Some(RefMut::map(block_list, |bl| { - let len = bl.len(); - &mut bl[len - 1 - index] + &mut bl[block_count - 1 - index] })) } } + // Return the block with the given id, asserting it exists. Note ids are recycled. + pub fn block_with_id(&self, id: BlockId) -> Ref<'_, Block> { + Ref::map(self.block_list.borrow(), |bl| &bl[id.0]) + } + pub fn blocks_size(&self) -> usize { self.block_list.borrow().len() } @@ -871,14 +884,14 @@ impl Parser { let mut block_list = self.block_list.borrow_mut(); block_list.push(block); - block_list.len() - 1 + BlockId(block_list.len() - 1) } /// Remove the outermost block, asserting it's the given one. pub fn pop_block(&self, expected: BlockId) { let block = { let mut block_list = self.block_list.borrow_mut(); - assert!(expected == block_list.len() - 1); + assert!(expected.0 == block_list.len() - 1); block_list.pop().unwrap() }; if block.wants_pop_env() { @@ -893,9 +906,7 @@ impl Parser { // isn't one return the function name for the current level. // Walk until we find a breakpoint, then take the next function. return self - .blocks() - .iter() - .rev() + .blocks_iter_rev() .skip_while(|b| b.typ() != BlockType::breakpoint) .find_map(|b| match b.data() { Some(BlockData::Function { name, .. }) => Some(name.clone()), @@ -903,9 +914,7 @@ impl Parser { }); } - self.blocks() - .iter() - .rev() + self.blocks_iter_rev() // Historical: If we want the topmost function, but we are really in a file sourced by a // function, don't consider ourselves to be in a function. .take_while(|b| !(level == 1 && b.typ() == BlockType::source)) @@ -1052,9 +1061,7 @@ impl Parser { /// reader_current_filename, e.g. if we are evaluating a function defined in a different file /// than the one currently read. pub fn current_filename(&self) -> Option { - self.blocks() - .iter() - .rev() + self.blocks_iter_rev() .find_map(|b| match b.data() { Some(BlockData::Function { name, .. }) => { function::get_props(name).and_then(|props| props.definition_file.clone()) @@ -1073,16 +1080,14 @@ impl Parser { /// Return a string representing the current stack trace. pub fn stack_trace(&self) -> WString { - self.blocks() - .iter() - .rev() + self.blocks_iter_rev() // Stop at event handler. No reason to believe that any other code is relevant. // It might make sense in the future to continue printing the stack trace of the code // that invoked the event, if this is a programmatic event, but we can't currently // detect that. .take_while(|b| b.typ() != BlockType::event) .fold(WString::new(), |mut trace, b| { - append_block_description_to_stack_trace(self, b, &mut trace); + append_block_description_to_stack_trace(self, &b, &mut trace); trace }) } @@ -1098,9 +1103,7 @@ impl Parser { } // Count the functions. let depth = self - .blocks() - .iter() - .rev() + .blocks_iter_rev() .filter(|b| b.is_function_call()) .count(); depth > FISH_MAX_STACK_DEPTH