Incremental line-by-line highlighting breaks some languages

[davidben]

highlight.js supports highlighting text incrementally via the continuation parameter to the highlight function. Gerrit uses this to highlight line-by-line. However, this causes some features, notably C++ raw string matching, to break. See the discussion here.

(Ironically, raw string matching partially worked in #1825 and then I broke it again for us when I tried to make it more featureful in #1897.)

I changed the tests to feed all the markup samples line-by-line to demonstrate the issue. Things mostly work, but when a begin or end element crosses a line boundary, things break.
master...davidben:line-by-line-tests

This suggests that the strategy in #1897 was wrong and probably C++ raw strings should have separate begin and end units. But endSameAsBegin doesn't seem quite flexible enough, unless there's some trick I'm missing. One thought is begin matches R"<blah>( and then we have a endFromBegin attribute which takes a callback and will compute )<blah>" for the end pattern given a begin.

But it's not entirely clear what the general API contract here is. Supposing incremental highlight is a thing highlight.js wants to support (I assume yes, given the continuation parameter), it seems there's basically no hope in allowing a begin or end unit to span an input boundary. The API needs to return something for the text that was passed in, so you can't lookahead across chunks. That means highlight.js needs to have opinions about what input boundaries need to be supported by languages (One call per character obviously cannot do anything useful. Line-by-line seems reasonable?), or we change the incremental highlight API altogether.

Thoughts?

[joshgoebel]

But endSameAsBegin doesn't seem quite flexible enough, unless there's some trick I'm missing. One thought is begin matches R"( and then we have a endFromBegin attribute which takes a callback and will compute )" for the end pattern given a begin.

Currently you can do this by nesting, but yes it's annoying. You might also see my work on Elixir:

https://github.com/highlightjs/highlight.js/pull/2257/files

It doesn't use endSameAsBegin, but same concept with first matching on the prefix and then the child matchers can focus on just the begin/end syntax, not the prefix.

---

continuation really is NOT meant as a way to do line-by-line processing. This would be better handled by a post-highlight or post-parse plugin as mentioned over in the plugin thread.

it seems there's basically no hope in allowing a begin or end unit to span an input boundary. The API needs to return something for the text that was passed in, so you can't lookahead across chunks.

I'm not sure I completely follow, I will follow your link and see if it's helpful. I've found the "outside" edge of the continuation stuff to be more robust than I expected... ie it remembers the state the parser is in re-opens those spans and continues the processing, even after handling a sublanguage in the middle... but again I'm speaking for what continuation were INTENDED for... they weren't intended for line-by-line processing.

[joshgoebel]

  // When marking up an input line-by-line, elements which span multiple lines
  // will be divided up into several spans. For instance, a multi-line C++
  // comment may be marked up as:
  //
  //   <span class="hljs-comment">/*</span>
  //   <span class="hljs-comment">Hello</span>
  //   <span class="hljs-comment">*/</span>

This looks like the behavior I've come to expect with continuations.

[joshgoebel]

Perhaps it'd be useful to show a "failing case" and what happens and what you would hope to happen, etc...

[joshgoebel]

I'd suggest you take a look at what's going on here:

#1086

I think doing anything line-by-line would be done by walking the tree and dealing with \n inside text nodes however you saw fit... Doing something simple like wrapping every line in it's own span would probably be pretty trivial one that foundation.

There has also has been mention of a line-by-line callback especially, but honestly I'd probably rather see this first done by a plugin... no reason plugins couldn't extend the callback system or even have their own.

[davido]

FWIW, the tracking issue in Gerrit Code Review project is here: [1]. The current repro is: [2].

[1] https://bugs.chromium.org/p/gerrit/issues/detail?id=7748
[2] https://boringssl-review.googlesource.com/c/boringssl/+/38665/1/util/fipstools/acvp/modulewrapper/modulewrapper.cc

[joshgoebel]

Commented on that thread just to clear up the fact that we do NOT have a line by line mode. In fact the original author was actually pretty against any type of "line number" feature and such things. And it's still mentioned in our docs today, etc... not that I think we're still THAT hostile to it today, but definitely sounds like a good use for a plugin.

We just need better hooks to make these type of things easier to do.

[davidben]

Ah, interesting. Hopefully one of the Gerrit devs will respond either here or on the Gerrit bug you commented on. (I don't actually work on Gerrit. I work on Chromium and BoringSSL, which use Gerrit, and highlight.js was easy enough to hack on, so I'd been poking at the highlighting bugs I was running into. :-) )

You mentioned in the Gerrit bug that the continuation feature is primarily for internal use. Should it be removed from the documentation? The documentation reads to me like feeding in chunks of partial data is supposed to work.

It doesn't use endSameAsBegin, but same concept with first matching on the prefix and then the child matchers can focus on just the begin/end syntax, not the prefix.

Hrm. I toyed with that a bit but it seemed there'd be issues with C++'s raw strings ending in )<delimiter>", so merely looking for <delimiter> might end the string early.

Of course, if line-by-line isn't supposed to work anyway, that's no longer a reason to change the current C++ definition. That said, hljs currently won't match R"(half a raw string, whereas it will match /* half a block comment, so perhaps there's something to be said for changing the C++ mode anyway?

This looks like the behavior I've come to expect with continuations.

It seems the issue may have since been clarified, but in case there's still confusion, yeah that behavior makes sense. The "issue" is the lineByLineExceptions list of tests that don't work even after accounting for it. (I say "issue" in quotes because it sounds like line-by-line mode isn't meant to work anyway.)

[joshgoebel]

You mentioned in the Gerrit bug that the continuation feature is primarily for internal use. Should it be removed from the documentation? The documentation reads to me like feeding in chunks of partial data is supposed to work.

I'd actually be for that if @egor-rogov had no objections. It really has all sorts of caveats and really it's more of a limited thing to work with sublanguages. I was not aware we were encouraging its broader use. Either way I'll still say: Not really intended for line to line.

Of course, if line-by-line isn't supposed to work anyway, that's no longer a reason to change the current C++ definition.

Continuation (in the core library) is intended to support subLanguages (pretty sure that's the only reason it's there, but I'm open to being corrected)... and personally I don't think we should REQUIRE that any languages parse in a certain way if they naturally find it beneficial to parse in larger chunks. IE, we shouldn't do things that limits peoples choices in writing grammars without VERY good reasons to do so.

[joshgoebel]

That said, hljs currently won't match R"(half a raw string, whereas it will match /* half a block comment, so perhaps there's something to be said for changing the C++ mode anyway?

You mean when parsing tiny bits at a time only, yes?

[davidben]

Nah, maybe the file is truncated or the author forgot to close the string. Usually syntax highlighters will highlight the unfinished thing.

[joshgoebel]

Ah, now that might be worth thinking about. That would be one case where doing the full match would not turn out so well...

[egor-rogov]

You mentioned in the Gerrit bug that the continuation feature is primarily for internal use. Should it be removed from the documentation? The documentation reads to me like feeding in chunks of partial data is supposed to work.

I'd actually be for that if @egor-rogov had no objections.

I'd rather not remove it ('cause it is a part of the API), but explicitly explain that this is intended for internal use and may not behave as one might expect.

[davidben]

Ah, now that might be worth thinking about. That would be one case where doing the full match would not turn out so well...

What are your thoughts on something like:

endCallback: function(begin) {
  // Compute ')<delimiter>"' from 'R"<delimiter>('
},

Returning a match object for the begin regex would be smoother, but probably tricky given how the regexes get all joined up.

[joshgoebel]

What are your thoughts on something like:

I'm receptive to the idea, though it'd probably be a more generic match callback that fires on match or perhaps start/finish... then you could do what you wanted. Ie, hook "start" to generate end on the fly, this would allow the current endSameAsBegin to be built with only a plug-in, as it's a much simpler version of what you're suggesting.

Although it's actually not that simple because the begin/end matchers are all compiled together and are not modified at run-time. endSameAsBegin currently only works because it's a MORE SPECIFIC match of the same type... IE, end (a copy of begin) will always match, but then we double check to see if the more specific end match was triggered.

Otherwise we might have to recursively recompile the regexs every time the regexes were dynamically changed... not sure what type of performance implications that would have.

Returning a match object for the begin regex would be smoother, but probably tricky given how the regexes get all joined up.

Huh? Not sure I follow? Accessing the invividual capture groups isn't that hard... but I'm not sure exactly what you're suggesting with this comment.

[davidben]

Although it's actually not that simple because the begin/end matchers are all compiled together and are not modified at run-time. endSameAsBegin currently only works because it's a MORE SPECIFIC match of the same type... IE, end (a copy of begin) will always match, but then we double check to see if the more specific end match was triggered.

Ah, interesting. Okay, I think I can do something with that. I'll try my hand at this and we'll see if you like the result.

Huh? Not sure I follow? Accessing the invividual capture groups isn't that hard... but I'm not sure exactly what you're suggesting with this comment.

I mean that you could imagine the function(begin) { ... } above being passed in a regex match object rather than the string. My recollection was that, since you combine all the regexes into one jumbo regex, the match groups are all shifted and such. Or maybe I'm entirely confused and misremembering. Anyway, not important, I'll poke around and see what's cleanest to do.