Showcase off-by-one error in language-html

[winstliu]

This PR adds a failing test to showcase an off-by-one error that can occasionally occur. It was originally discovered in atom/atom#14982, though it can now be reproduced by using atom/language-html#170 as well. Note: I had to bring in a more recent version of language-css so that the proper rules could be hooked up. html-with-css.cson is a very minimal example grammar that will reproduce the issue, given <span style="s:"></span>.

Will attempt to revert the caching changes to see if that was the cause of this regression.

/cc @maxbrunsfeld

[winstliu]

Interestingly, this test still seems to fail in the 6.x versions (and 5.x throws errors when trying to start specs). I'll try copying over the repro case from atom/atom#14982 to see if that one's any different.

[Ingramz]

The cause for atom/atom#14982 seems to be 7c16504#diff-30b5df382fb36749d0d1d282b2df9a53

Rule::getNextTags doesn't like if there is a newline appended down this path. If you are saying that the html test fails with versions 7.0.2 and lower, these issues must be different.

[winstliu]

Ok, I'll open a new PR for that one.

[maxbrunsfeld]

⚡ Thanks so much for moving this forward @50Wliu! The theory about the appended newline seems very promising. You're sure it's failing in the same way w/ #94 reverted?

[Ingramz]

@maxbrunsfeld the following patch fixes both cases for me, rest of the specs pass. I am not sure what it breaks though.

diff --git a/src/grammar.coffee b/src/grammar.coffee
index 82fd12c..7880b8b 100644
--- a/src/grammar.coffee
+++ b/src/grammar.coffee
@@ -98,7 +98,7 @@ class Grammar
   # * `ruleStack` An {Array} of rules representing the tokenized state at the
   #   end of the line. These should be passed back into this method when
   #   tokenizing the next line in the file.
-  tokenizeLine: (inputLine, ruleStack, firstLine=false, compatibilityMode=true) ->
+  tokenizeLine: (inputLine, ruleStack, firstLine=false, compatibilityMode=true, withNewLine=true) ->
     tags = []
 
     truncatedLine = false
@@ -108,7 +108,8 @@ class Grammar
     else
       line = inputLine
 
-    string = new OnigString(line + '\n')
+    string = new OnigString(line)
+    stringWithNewLine = if withNewLine then new OnigString(line + '\n') else string
 
     if ruleStack?
       ruleStack = ruleStack.slice()
@@ -139,7 +140,7 @@ class Grammar
         truncatedLine = true
         break
 
-      if match = _.last(ruleStack).rule.getNextTags(ruleStack, string, position, firstLine)
+      if match = _.last(ruleStack).rule.getNextTags(ruleStack, string, stringWithNewLine, position, firstLine)
         {nextTags, tagsStart, tagsEnd} = match
 
         # Unmatched text before next tags
diff --git a/src/pattern.coffee b/src/pattern.coffee
index 2aff1d4..30a2c91 100644
--- a/src/pattern.coffee
+++ b/src/pattern.coffee
@@ -171,7 +171,7 @@ class Pattern
 
   tagsForCaptureRule: (rule, line, captureStart, captureEnd, stack) ->
     captureText = line.substring(captureStart, captureEnd)
-    {tags} = rule.grammar.tokenizeLine(captureText, [stack..., {rule}])
+    {tags} = rule.grammar.tokenizeLine(captureText, [stack..., {rule}], false, true, false)
 
     # only accept non empty tokens that don't exceed the capture end
     openScopes = []
diff --git a/src/rule.coffee b/src/rule.coffee
index 3301060..84d7528 100644
--- a/src/rule.coffee
+++ b/src/rule.coffee
@@ -96,8 +96,8 @@ class Rule
       @normalizeCaptureIndices(lineWithNewline, result.captureIndices)
       result
 
-  getNextTags: (ruleStack, line, position, firstLine) ->
-    result = @findNextMatch(ruleStack, line, position, firstLine)
+  getNextTags: (ruleStack, line, lineWithNewline, position, firstLine) ->
+    result = @findNextMatch(ruleStack, lineWithNewline, position, firstLine)
     return null unless result?
 
     {index, captureIndices, scanner} = result

[maxbrunsfeld]

@50Wliu Just so I understand your unit test, will the error reproduce if we put valid CSS in the style value, like top: 1px or something? Or does it only happen if we have some partial content like s:?

[maxbrunsfeld]

@Ingramz Awesome! Want to open a PR that applies that patch with this branch as the base branch?

[winstliu]

I'm fairly confident that #94 was not the change, because I checked out v6.3.0, nuked node_modules, and the tests still failed.

And the off-by-one error only reproduces when there's no property-value present. Once you add a non-space after the colon, the error disappears. So top: 1px is fine.