diff --git a/src/parsing/parser.rs b/src/parsing/parser.rs index f8102e32..6129ab59 100644 --- a/src/parsing/parser.rs +++ b/src/parsing/parser.rs @@ -17,8 +17,8 @@ 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::hash::BuildHasherDefault; +use std::collections::{HashMap, HashSet}; +use std::hash::{BuildHasherDefault, Hash, Hasher}; /// Errors that can occur while parsing. #[derive(Debug, thiserror::Error)] @@ -188,6 +188,57 @@ 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, 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, 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 +/// 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 +495,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 +506,7 @@ impl ParseState { &mut search_cache, &mut regions, &mut non_consuming_push_at, + &mut set_states, &mut res, )? {} @@ -467,6 +522,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 +577,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 +606,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 +615,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 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)); + } } *start = match_end; @@ -2480,6 +2540,74 @@ 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_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#"