From 0330c2f255e5e4bf862fcf6076fb54a7f16c4b42 Mon Sep 17 00:00:00 2001 From: Tony Solomonik Date: Sat, 12 Sep 2026 17:23:35 +0300 Subject: [PATCH 1/2] Break loops between non-consuming `set`s The loop guard only watches a non-consuming push followed by a non-consuming pop, and the comment next to it admitted that a non-consuming `set` can loop the same way but left it alone. A `set` is neither the push that arms the guard nor the pop that trips it, and it leaves the stack depth alone, so two contexts that `set` each other at the same byte spin in `parse_next_token` forever, with a flat stack and no allocation to show for it. Older JavaScript packages hit this on a line like `x /*`, where `expression-statement-continuation` and the "rest of the line is blanks and comments" rule both match empty and hand the position back to each other; current Sublime packages dodge it with a `branch` instead, which is why nothing in `testdata` ever caught it. The fix remembers the `(byte, context stack)` a non-consuming `set` was taken from and, on a second visit to the same state, advances one character like the push/pop guard does. The stack has to be in the key because `include:` shares contexts, so the same rule legitimately fires at one byte from many states, and keying on the rule alone costs hundreds of syntest assertions. Both syntest baselines are unchanged. --- src/parsing/parser.rs | 87 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 14 deletions(-) diff --git a/src/parsing/parser.rs b/src/parsing/parser.rs index f8102e32..7ef84cd0 100644 --- a/src/parsing/parser.rs +++ b/src/parsing/parser.rs @@ -17,7 +17,7 @@ use crate::parsing::syntax_definition::ContextId; use crate::parsing::syntax_set::{SyntaxReference, SyntaxSet}; use fnv::FnvHasher; use regex_syntax::escape; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::hash::BuildHasherDefault; /// Errors that can occur while parsing. @@ -188,6 +188,24 @@ struct RegexMatch<'a> { /// Maps the pattern to the start index, which is -1 if not found. type SearchCache = HashMap<*const MatchPattern, Option, BuildHasherDefault>; +/// The `(byte, context stack)` states a non-consuming `set` was already taken +/// from, used to break `set` cycles. See [`ParseState::parse_next_token`]. +type SetStates = HashSet<(usize, Vec), BuildHasherDefault>; + +/// Moves `start` past one character, the way Sublime Text breaks a match loop. +/// Byte indices, so `+= 1` would split a multi-byte character. Returns false at +/// the end of the line, where there is nothing left to advance over and no +/// point trying more patterns. +fn advance_one_char(line: &str, start: &mut usize) -> bool { + match line[*start..].char_indices().nth(1) { + Some((i, _)) => { + *start += i; + true + } + None => false, + } +} + /// Build the ordered Push/Pop ops for a match's `captures:` mapping over /// its regex `regions`. Captures can appear in arbitrary source order /// (e.g. `((bob)|(hi))*` matching `hibob` — the outer group must Push @@ -444,6 +462,9 @@ impl ParseState { let mut search_cache: SearchCache = HashMap::with_capacity_and_hasher(128, fnv); // Used for detecting loops with push/pop, see long comment above. let mut non_consuming_push_at = (0, 0); + // Used for detecting loops between non-consuming `set`s, see + // `parse_next_token`. + let mut set_states = SetStates::default(); while self.parse_next_token( line, @@ -452,6 +473,7 @@ impl ParseState { &mut search_cache, &mut regions, &mut non_consuming_push_at, + &mut set_states, &mut res, )? {} @@ -467,6 +489,7 @@ impl ParseState { search_cache: &mut SearchCache, regions: &mut Region, non_consuming_push_at: &mut (usize, usize), + set_states: &mut SetStates, ops: &mut Vec<(usize, ScopeStackOp)>, ) -> Result { let check_pop_loop = { @@ -521,17 +544,7 @@ impl ParseState { // println!("pop_would_loop for match {:?}, start {}", reg_match, *start); - // nth(1) gets the next character if there is one. Need to do - // this instead of just += 1 because we have byte indices and - // unicode characters can be more than 1 byte. - if let Some((i, _)) = line[*start..].char_indices().nth(1) { - *start += i; - return Ok(true); - } else { - // End of line, no character to advance and no point trying - // any more patterns. - return Ok(false); - } + return Ok(advance_one_char(line, start)); } let match_end = reg_match.regions.pos(0).unwrap().1; @@ -560,8 +573,7 @@ impl ParseState { if !consuming { // The match doesn't consume any characters. If this is a // "push", remember the position and stack size so that we can - // check the next "pop" for loops. Otherwise leave the state, - // e.g. non-consuming "set" could also result in a loop. + // check the next "pop" for loops. if matches!( match_pattern.operation, MatchOperation::Push(_) @@ -570,6 +582,21 @@ impl ParseState { ) { *non_consuming_push_at = (match_end, self.stack.len() + 1); } + // A non-consuming "set" loops too, and the guard above cannot + // see it: a "set" is neither the push that arms it nor the pop + // that trips it, and the stack depth never changes, so two + // contexts that "set" each other spin here forever. Taking a + // "set" from a byte and stack we already took one from can + // only repeat the same work, so do what Sublime Text does with + // the push/pop loop and move on by a character. The stack has + // to be part of the key: an included context is shared, so the + // same rule legitimately fires at one byte from many states. + if matches!(match_pattern.operation, MatchOperation::Set { .. }) { + let state = self.stack.iter().map(|lvl| lvl.context).collect(); + if !set_states.insert((*start, state)) { + return Ok(advance_one_char(line, start)); + } + } } *start = match_end; @@ -2480,6 +2507,38 @@ contexts: expect_scope_stacks(line, &expect, syntax); } + #[test] + fn can_parse_non_consuming_sets_that_would_loop() { + let syntax = r#" +name: test +scope: source.test +contexts: + main: + # This makes us go into "a" without consuming any characters + - match: (?=test) + set: a + a: + # And this one into "b", still without consuming anything + - match: (?=t) + set: b + - match: \w+ + scope: test.matched + b: + # Which sends us straight back to "a". Neither of the two is a push or a + # pop, so the stack depth never moves and the push/pop loop check above + # never sees a thing. ST stops taking a "set" that already fired at this + # position and advances instead. + - match: (?=t) + set: a + - match: \w+ + scope: test.matched +"#; + + let line = "test"; + let expect = [", "]; + expect_scope_stacks(line, &expect, syntax); + } + #[test] fn can_parse_non_consuming_set_after_consuming_push_that_does_not_loop() { let syntax = r#" From 087bd9bda0d0808f6d7d5f9fafafb2f8ddf30c5d Mon Sep 17 00:00:00 2001 From: Tony Solomonik Date: Tue, 27 Oct 2026 16:56:06 +0200 Subject: [PATCH 2/2] Key the `set` loop guard on a hash of the stack, prototypes included Two changes to the key the non-consuming `set` guard remembers, both asked for in review. The key was the byte and a `Vec` of the stack's contexts, so every non-consuming `set` allocated a copy of the stack. It is now the byte and a `u64` hash of the stack. A collision can only make the guard skip a `set` and advance a character, never hang. The key also ignored `with_prototype`s, so two levels with the same context but different prototypes, which match differently, counted as one state and the second `set` was skipped. Each level now hashes its prototypes too. Hashing the prototypes as they are would have reopened the hang: a `set` keeps the prototypes of the level it replaces and appends its own `with_prototype`, so a `set` cycle with a `with_prototype` grows the list on every lap, every lap looks new, and the guard never fires. A repeated prototype can never win a match over its first occurrence, so each level hashes every prototype once, which keeps the number of states finite. `can_parse_non_consuming_sets_that_would_loop_with_prototype` covers both: it fails with the contexts-only key and hangs if every copy of a prototype is hashed. Both syntest baselines are unchanged, and clippy reports nothing new. Co-authored-by: Tobias Oetiker --- src/parsing/parser.rs | 99 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 84 insertions(+), 15 deletions(-) diff --git a/src/parsing/parser.rs b/src/parsing/parser.rs index 7ef84cd0..6129ab59 100644 --- a/src/parsing/parser.rs +++ b/src/parsing/parser.rs @@ -18,7 +18,7 @@ use crate::parsing::syntax_set::{SyntaxReference, SyntaxSet}; use fnv::FnvHasher; use regex_syntax::escape; use std::collections::{HashMap, HashSet}; -use std::hash::BuildHasherDefault; +use std::hash::{BuildHasherDefault, Hash, Hasher}; /// Errors that can occur while parsing. #[derive(Debug, thiserror::Error)] @@ -188,9 +188,42 @@ struct RegexMatch<'a> { /// Maps the pattern to the start index, which is -1 if not found. type SearchCache = HashMap<*const MatchPattern, Option, BuildHasherDefault>; -/// The `(byte, context stack)` states a non-consuming `set` was already taken +/// The `(byte, stack hash)` states a non-consuming `set` was already taken /// from, used to break `set` cycles. See [`ParseState::parse_next_token`]. -type SetStates = HashSet<(usize, Vec), BuildHasherDefault>; +type SetStates = HashSet<(usize, u64), BuildHasherDefault>; + +/// Hashes the stack a non-consuming `set` is taken from, for [`SetStates`]. +/// +/// A hash instead of a copy of the stack, so the guard never allocates. A +/// collision can only make the guard skip a `set` it should have taken and +/// advance a character instead, it can never make the parser hang. +/// +/// Each level hashes its context and its prototypes, because two levels with +/// the same context but different `with_prototype`s match differently. A +/// prototype is hashed only once per level: a `set` keeps the prototypes of +/// the level it replaces and appends its own `with_prototype`, so a cycle of +/// `set`s with a `with_prototype` grows the list on every lap. A repeated +/// prototype can never win over its first occurrence, so the state really is +/// the same, and hashing it once keeps the number of states finite. Hashing +/// every copy would make each lap look new and the cycle would never break. +/// +/// `captures` are left out because `Region` isn't `Hash`. Two states that only +/// differ in them are treated as one, which again can only skip a `set`. +fn set_state_hash(stack: &[StateLevel]) -> u64 { + let mut hasher = FnvHasher::default(); + for level in stack { + level.context.hash(&mut hasher); + let mut unique_prototypes = 0; + for (i, prototype) in level.prototypes.iter().enumerate() { + if !level.prototypes[..i].contains(prototype) { + prototype.hash(&mut hasher); + unique_prototypes += 1; + } + } + hasher.write_usize(unique_prototypes); + } + hasher.finish() +} /// Moves `start` past one character, the way Sublime Text breaks a match loop. /// Byte indices, so `+= 1` would split a multi-byte character. Returns false at @@ -584,18 +617,18 @@ impl ParseState { } // A non-consuming "set" loops too, and the guard above cannot // see it: a "set" is neither the push that arms it nor the pop - // that trips it, and the stack depth never changes, so two - // contexts that "set" each other spin here forever. Taking a - // "set" from a byte and stack we already took one from can - // only repeat the same work, so do what Sublime Text does with - // the push/pop loop and move on by a character. The stack has - // to be part of the key: an included context is shared, so the - // same rule legitimately fires at one byte from many states. - if matches!(match_pattern.operation, MatchOperation::Set { .. }) { - let state = self.stack.iter().map(|lvl| lvl.context).collect(); - if !set_states.insert((*start, state)) { - return Ok(advance_one_char(line, start)); - } + // that trips it, and a "set" of one context leaves the stack + // depth alone, so two contexts that "set" each other spin here + // forever. Taking a "set" from a byte and stack we already took + // one from can only repeat the same work, so do what Sublime + // Text does with the push/pop loop and move on by a character. + // The stack has to be part of the key: an included context is + // shared, so the same rule legitimately fires at one byte from + // many states. + if matches!(match_pattern.operation, MatchOperation::Set { .. }) + && !set_states.insert((*start, set_state_hash(&self.stack))) + { + return Ok(advance_one_char(line, start)); } } @@ -2539,6 +2572,42 @@ contexts: expect_scope_stacks(line, &expect, syntax); } + #[test] + fn can_parse_non_consuming_sets_that_would_loop_with_prototype() { + let syntax = r#" +name: test +scope: source.test +contexts: + main: + - match: (?=x) + set: a + a: + # Sets "b" without consuming anything. A "set" keeps the prototypes of + # the context it replaces and adds its own, so every lap through here + # adds another copy of this prototype. + - match: (?=x) + set: b + with_prototype: + - match: z + scope: test.prototype + - match: y + scope: test.a + b: + # And straight back to "a", which now has the prototype. That is not the + # state the first "set" was taken from, so it is taken again. The loop is + # caught one lap later, on "b" with the prototype twice, which matches + # exactly like "b" with it once. + - match: (?=x) + set: a + - match: y + scope: test.b +"#; + + let line = "xyz"; + let expect = [", ", ", "]; + expect_scope_stacks(line, &expect, syntax); + } + #[test] fn can_parse_non_consuming_set_after_consuming_push_that_does_not_loop() { let syntax = r#"