From 393ca63f4cc05453c73ebfbff44fa6819c92214c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 28 Jul 2026 18:18:54 +0200 Subject: [PATCH] Fix two LSH issues --- crates/lsh/src/compiler/mod.rs | 72 ++++++++++++-------------------- crates/lsh/src/compiler/regex.rs | 14 ++++++- 2 files changed, 39 insertions(+), 47 deletions(-) diff --git a/crates/lsh/src/compiler/mod.rs b/crates/lsh/src/compiler/mod.rs index d6b37238735..9004581176a 100644 --- a/crates/lsh/src/compiler/mod.rs +++ b/crates/lsh/src/compiler/mod.rs @@ -21,7 +21,6 @@ use std::path::Path; use stdext::arena::Arena; use stdext::collections::BString; -use stdext::opt_ptr_eq; pub use self::charset::{Charset, SerializedCharset}; use self::frontend::*; @@ -147,51 +146,37 @@ impl<'a> Compiler<'a> { fn visit_nodes_from(&self, root: IRCell<'a>) -> TreeVisitor<'a> { let mut stack = VecDeque::new(); stack.push_back(root); - TreeVisitor { current: None, stack, visited: Default::default() } + TreeVisitor { current: None, stack, visited: Default::default(), follow_then: true } + } + + /// Same as [`Self::visit_nodes_from`], but doesn't traverse `then` edges. + /// A `then` target is still visited if some node's `next` edge points at it too. + fn visit_fallthrough_nodes_from(&self, root: IRCell<'a>) -> TreeVisitor<'a> { + let mut visitor = self.visit_nodes_from(root); + visitor.follow_then = false; + visitor } /// Collect all "interesting" characters from conditions in a loop body. /// Returns a charset where true = interesting character that should be checked. fn collect_interesting_charset(&self, loop_body: IRCell<'a>) -> Charset { - let mut iter = self.visit_nodes_from(loop_body); let mut charset = Charset::no(); - #[allow(clippy::while_let_loop)] - loop { - // Can't use `while let`, because that borrows `iter` - // and that prevents us from calling `skip_node()`. - let Some(node) = iter.next() else { - break; - }; - + // Only conditions reachable without consuming input matter here, + // so we don't descend into if bodies. Let's assume: + // loop { if /a/ { loop { if /b/ {} } } } + // The inner loop's inverted charset includes "a", and merging it into the + // outer loop would cover all characters and defeat fast-skips entirely. + // + // Conditions that a preceding optional/alternation can fall through to, as in + // if /[ab]?c/ {} + // --> if "a" .then -> if "c" + // .else -> if "b" .then -> if "c" + // .else -> if "c" + // are still reached, because `if "c"` sits on the `next` chain. + for node in self.visit_fallthrough_nodes_from(loop_body) { let node = node.borrow(); - if let IRI::If { condition, then } = node.instr { - // For the purpose of computing fast-skips the contents of if conditions are irrelevant, - // so skip the subtree. This is actually quite important. This this as an example: - // loop { - // if /a/ { - // loop { - // if /b/ { - // } - // } - // } - // } - // The inverted charset of the inner /b/ includes "a". If we merge that into the outer - // loop's charset we get one that covers all characters, making fast-skips impossible. - // --> Skip the "then" subtree. - // - // HOWEVER, imagine a condition like this: - // if /a?b/ {} - // This compiles to something like: - // if "a" - // .then -> if "b" {} - // .else -> if "b" {} (aka: .next) - // In other words, "then" and "next" point to the same thing. - // --> Only skip "then" if it's not the same as "next". - if !opt_ptr_eq(Some(then), node.next) { - iter.skip_node(then); - } - + if let IRI::If { condition, .. } = node.instr { match condition { Condition::Cmp { .. } => {} Condition::EndOfLine => {} @@ -351,12 +336,7 @@ struct TreeVisitor<'a> { current: Option>, stack: VecDeque>, visited: HashSet<*const RefCell>>, -} - -impl<'a> TreeVisitor<'a> { - fn skip_node(&mut self, node: IRCell<'a>) { - self.visited.insert(node as *const _); - } + follow_then: bool, } impl<'a> Iterator for TreeVisitor<'a> { @@ -366,7 +346,9 @@ impl<'a> Iterator for TreeVisitor<'a> { if let Some(cell) = self.current.take() { { let ir = cell.borrow(); - if let IRI::If { then, .. } = ir.instr { + if let IRI::If { then, .. } = ir.instr + && self.follow_then + { self.stack.push_back(then); } if let Some(next) = ir.next { diff --git a/crates/lsh/src/compiler/regex.rs b/crates/lsh/src/compiler/regex.rs index e2c6cd11f77..9cc3370e25a 100644 --- a/crates/lsh/src/compiler/regex.rs +++ b/crates/lsh/src/compiler/regex.rs @@ -402,7 +402,7 @@ impl<'a> RegexParser<'a> { } Atom::Meta(']') => break, Atom::Meta(c) | Atom::Char(c) => { - if !c.is_ascii() { + if c as u32 > 0xff { return unexpected_unicode(c); } @@ -425,7 +425,7 @@ impl<'a> RegexParser<'a> { end = b'>'; } Atom::Meta(c) | Atom::Char(c) => { - if !c.is_ascii() { + if c as u32 > 0xff { return unexpected_unicode(c); } end = c as u8; @@ -483,6 +483,16 @@ impl<'a> RegexParser<'a> { Ok(Atom::Class(cs)) } 't' => Ok(Atom::Char('\t')), + 'x' => { + if let Some(byte) = + self.rest().get(..2).and_then(|hex| u8::from_str_radix(hex, 16).ok()) + { + self.pos += 2; + Ok(Atom::Char(byte as char)) + } else { + Err("invalid hex escape sequence".to_string()) + } + } c if !c.is_ascii_alphanumeric() => Ok(Atom::Char(c)), c => Err(format!("unknown escape sequence '\\{c}'")), }