Break loops between non-consuming sets

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<Region>, BuildHasherDefault<FnvHasher>>;
 
+/// 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<FnvHasher>>;
+
+/// 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<bool, ParsingError> {
         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 = ["<source.test>, <test.matched>"];
+        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 = ["<source.test>, <test.b>", "<source.test>, <test.prototype>"];
+        expect_scope_stacks(line, &expect, syntax);
+    }
+
     #[test]
     fn can_parse_non_consuming_set_after_consuming_push_that_does_not_loop() {
         let syntax = r#"