(cpp) Fix highlighting of unterminated raw strings

[davidben]

@yyyc514, thoughts?

PR #1897 switched C++ raw strings to use backreferences, however this breaks souce files where raw strings are truncated. Like comments, it would be preferable to highlight them.

Instead, go back to using separate begin and end regexps, but introduce an endFilter feature to filter out false positive matches. This internally works similarly to endSameAsBegin.

See also issue #2259.

(Also, I have to say, this C++ raw string syntax is a little absurd...)

[joshgoebel]

I'll play around with a bit. Nice to see this was so easy to accomplish. I'm going to explore going broader though:

       { // heredocs
-        begin: /<<[-~]?'?(\w+)(?:.|\n)*?\n\s*\1\b/,
-        returnBegin: true,
-        contains: [
-          { begin: /<<[-~]?'?/ },
-          { begin: /\w+/,
-            endSameAsBegin: true,
-            contains: [hljs.BACKSLASH_ESCAPE, SUBST],
-          }
-        ]
+        begin: /<<[-~]?'?(\w+)/,
+        end: /(\w+)\b/,
+        onBegin: function(m, state) { state.heredoc = m[1] },
+        onEnd: function(m, state) { return state.heredoc === m[1] },
+        contains: [hljs.BACKSLASH_ESCAPE, SUBST],
       }
     ]
   };

Though now I'm wondering if instead of more keys our keys should get more complex:

      { // heredocs
        begin: {
          re: /<<[-~]?'?(\w+)/,
          callback: function(m, state) { state.heredoc = m[1] }, 
        },
// ...
        end: hljs.withCallback(/(\w+)\b/, function(m, state) { return state.heredoc === m[1] });
        contains: [hljs.BACKSLASH_ESCAPE, SUBST],
      }

[joshgoebel]

      {
        begin: /(?:u8?|U|L)?R"([^()\\ ]{0,16})\(/,
        end: /\)([^()\\ ]{0,16})"/,
        onBegin: function(m, state) { state.heredoc = m[1] },
        endWhen: function(m, state) { return state.heredoc === m[1] },
      }

Very open to thoughts on naming. The PR isn't quite right, we'd need lots of thought and additional regarding how this works with endsParent, endsWithParent, etc... endWhen probably needs to happen inside endOfMode so that if the first endWhen failed it would continue up the parent chain looking for a end that might match... vs right now it would find the first end that possibly matches, and then endWhen could cancel that match.

And already you have some good questions about nesting and when both end rules are the same (other than the callback)... if the first callback returns "false" is that a blocker, or does the chain of possible ends continue to be inspected moving up the parents until it finds one that might hit?

These same type of questions would likely apply to the endFilter also... whether a false is a HARD no, or a soft no, etc...

I MUCH prefer a generic solution here though because I very much dislike the idea of adding specific solutions to the parser each time we come across some weird language nuance. For example that's what we did with endSameAsBegin. A VERY specific grammar feature for only TWO languages (pgsql and Ruby)... when that exact problem could have been solved with the more generic solutions being proposed here.

@egor-rogov Any high-level thoughts? Not looking for code critiques at this point but more of the idea conceptually... of adding callbacks to allow for this type of thing, etc...

The callback itself

The callback being proposed takes a normal [almost] regex match object (so it can look at sub matches, etc)... the second variable starts as an empty object and can be used by rules to preserve state between the begin and end rules, such as saving the magic "token" in this case.

[joshgoebel]

that's what we did with endSameAsBegin. A VERY specific grammar feature for only TWO languages (pgsql and Ruby)... when that exact problem could have been solved with the more generic solutions being proposed here.

Also note this here is really almost EXACTLY the same problem as "endSameAsBegin" but because the solution was hard-coded it can't be applied to this use case... just another point in why I dislike one off grammar "hack" rules vs more generic solutions that can be used more widely.

[egor-rogov]

I very much dislike the idea of adding specific solutions to the parser each time we come across some weird language nuance.

Absolutely. I'd gladly sacrifice endSameAsBagin in favour of something more general. I was thinking (back when it was invented) about a way to reference begin's matching groups from end regexp, but couldn't come to a good solution.

Any high-level thoughts? Not looking for code critiques at this point but more of the idea conceptually... of adding callbacks to allow for this type of thing, etc...

I like the idea! Callbacks look promising.
For now I can't see what the use cases for onBegin/onEnd may be. Single endFilter is not enough?

[joshgoebel]

For now I can't see what the use cases for onBegin/onEnd may be. Single endFilter is not enough?

• Not all matches result in an actual end match... the file could end before an actual ending is reached.
• It should probably be possible to reject/ignore a match completely at begin... (hence needing a begin callback of some sort)
• Perhaps the onBegin wants to simply change the end regex directly at the time the match starts... (not currently allowed, but I'm not opposed to the idea conceptually). But again, this would be trigger by begin (ie, how endSameAsBegin is currently implemented)
• Or perhaps you're counting tags because you only want to highlight the first occurrence of something or highlight additional occurrences as invalid?

The idea is to build out a flexible plugin/event system. I feel like this comment you're making the same mistake as when endSameAsBegin was added... you're only imagining a single use case (filtering) instead of more generic system that people could build on top of.

[egor-rogov]

* Or perhaps you're counting tags because you only want to highlight the first occurrence of something or highlight additional occurrences as invalid?

Yes, this makes sense.

The idea is to build out a flexible plugin/event system. I feel like this comment you're making the same mistake as when endSameAsBegin was added... you're only imagining a single use case (filtering) instead of more generic system that people could build on top of.

I understand the intent, and I'm in no way against it. Just wanted to imagine a situation in which onBegin/onEnd can be useful.

[joshgoebel]

My latest thoughts on this include a response object:

{
        begin: /(?:u8?|U|L)?R"([^()\\ ]{0,16})\(/,
        end: /\)([^()\\ ]{0,16})"/,
        onBegin: function(m, state, resp) { state.heredoc = m[1] },
        onEnd: function(m, state, resp) { if (state.heredoc === m[1]) resp.ignoreMatch()  },
      }

It's possible you could have resp.data and then you'd be back to two params... but then I come to a naming issue. Technically when you have a potential match (remember our callback is going to decide what to do with the match) you have two main possibilities:

• "abort"... this is an indication of a failed match but you STILL advance the cursor, ie skipping the content in question
• "ignore"... failed match, but the cursor is not advanced.

I think I could use help with the naming. Examples of both:

beginKeywords in the past used an "abort" style match... So if you have:

B.class = A.class

And beginKeywords: 'class'... the .class would have been matched, but the . would be detected and then the entire match ignored AND the cursor moved to right before the =... ie. any later modes you have that might have matched ".cl" or "las" would fail because the cursor had already moved past.

Meanwhile an "ignore" match would ignore class (because of the .) but your cursor would still be at the .... so .cl WOULD match... or las WOULD match.

I'm thinking perhaps we only should expose the ignore behavior, but still I could use some better names just for internal use and talking about these things.

---

Abort is very strange IMHO because typically rules that FAIL to match do NOT eat content... but the prior behavior of beginKeyword (and maybe future behavior) means this is something we have to think about or at least name conceptually.

There is also the word skip but our mode definitions already give it a very particular meaning, so not sure if it would be good to reuse it here.

[joshgoebel]

Example of usage:

// in Ruby
Object.assign({
  begin: /(\w+)/, end: /(\w+)/,
  contains: [hljs.BACKSLASH_ESCAPE, SUBST],
}, hljs.END_FIRST_MATCH_SAME_AS_BEGIN)

// or perhaps
hljs.END_SAME_AS_BEGIN({
  begin: /(\w+)/, end: /(\w+)/,
  contains: [hljs.BACKSLASH_ESCAPE, SUBST],
});

// in C++
Object.assign({
  begin: /(?:u8?|U|L)?R"([^()\\ ]{0,16})\(/,
  end: /\)([^()\\ ]{0,16})"/,
}, hljs.END_FIRST_MATCH_SAME_AS_BEGIN)

And the "logic" can then simply become a shared mode snippet:

export const END_FIRST_MATCH_SAME_AS_BEGIN = {
   'after:begin': (m, resp) => { resp.data.heredoc = m[1]; },
   'before:end': (m, resp) => { if (resp.data.heredoc !== m[1]) resp.ignoreMatch(); }
 };

Not quite as pretty as endSameAsBegin: true, but completely explicit, no "magic" mode flags (ala, endSameasBegin) required.

I'm open to better naming than Response. My HTTP/web background here is showing... but technically it is designed to be the response object of the callback...

[egor-rogov]

I really like it. Very flexible and concise.
Response sound good, but too general. Maybe CallbackResponse or something like that?
I consider this PR mergeable, as soon as these new callbacks are described in the docs.
Btw, what do you think about deprecating endSameAsBegin?

[joshgoebel]

Btw, what do you think about deprecating endSameAsBegin?

That would be the next step. Though I'm not sure what that would mean... that we could rip it out in say v11?

[joshgoebel]

export const END_SAME_AS_BEGIN = function(mode) {
  return Object.assign(mode,
    {
    'after:begin': (m, resp) => { resp.data._beginMatch = m[1]; },
    'before:end': (m, resp) => { if (resp.data._beginMatch !== m[1]) resp.ignoreMatch() }
    });
};

Now I'm wondering if defaulting to the first match group is assuming too much? Hmmm...

[joshgoebel]

@egor-rogov Check out the docs. Now that I had to document them I'm thinking perhaps on:begin and on:end might be better names...

I'm not sure we ever need such granularity as before:begin and after:begin... and if not I'm not sure the prior naming makes good sense. Its kind of weird to remember one is before and one is after, etc... and begs the question why isn't is "before begin" (and maybe it should be, lol).

I guess I could see some crazy data transforms if we allowed that, but we could do the same thing by adding more flexibility to response... ie response.addContent() or some such...

Does the on naming seem clearer to you? Plus I think on still allows for adding after later IF it proved necessary...

[egor-rogov]

Btw, what do you think about deprecating endSameAsBegin?

That would be the next step. Though I'm not sure what that would mean... that we could rip it out in say v11?

Just so.

Does the on naming seem clearer to you?

Yes, I have the same feeling that on is better.

Maybe that END_SAME_AS_BEGIN snippet worth mentioning in plugin-recipes?

[joshgoebel]

Maybe that END_SAME_AS_BEGIN snippet worth mentioning in plugin-recipes?

Are all the other MODE helpers documented anywhere? It'd probably be good if they were, but if you document one you have to document all...

[egor-rogov]

Maybe that END_SAME_AS_BEGIN snippet worth mentioning in plugin-recipes?

Are all the other MODE helpers documented anywhere? It'd probably be good if they were, but if you document one you have to document all...

They are not, but END_SAME_AS_BEGIN is the first one that uses plugins and it is a good example. Anyway, that's a separate story.

[egor-rogov]

I was sure that I've already approved this, but it turns out that no...

[joshgoebel]

They are not, but END_SAME_AS_BEGIN is the first one that uses plugins and it is a good example. Anyway, that's a separate story.

It uses callbacks, not plugins. :-) But yes separate story.

[egor-rogov]

It uses callbacks, not plugins. :-)

Oh, right... 🤦 There are no callback recipes yet (:

[joshgoebel]

@davidben Sorry for the delay on this one - and it's not exactly what you originally submitted - but I still gave you the byline for the bug fix. :-) Merging now.