Allow spaces in null conditional classification

[am11]

Before (3237bab) vs. After (c724205).

Fixes #207

[JoeRobich]

Thanks @am11!

[Gerrit0]

Unless I'm missing something, this change is going to break VSCode once it lands in a release there.

The Lightshow demo you showed uses PCRE for its regular expressions, which permits non-fixed-sized lookbehind expressions.
TextMate uses Oniguruma for its regular expressions, which does not, so the modified expression is invalid and will result in failures when trying to use the grammar.

https://github.com/kkos/oniguruma/blob/86bbff83f1d66ed76dce26fa47db2542debbb746/doc/RE#L549-L550

[am11]

@Gerrit0, interesting, that was certainly not my intention. Thanks for bringing it up!

but first was it "all wrong"? Lets see..

• This repository 'csharp-tmLanguage' has many consumers: GitHub linguist, VSCode are the huge ones.
  • linguist (supports PCRE and treesitter grammars) is responsible for syntax highlighting all code in comments and git repos on GitHub (which includes C# code)
  • VSCode (supports Oniguruma grammar only?) is used by millions of uses.
• Lightshow is the tool from linguist repo to test and demo grammar files conveniently before it lands on linguist. (they use it in their PRs)
• We previously converted grammar to PCRE: 4919838
  • my understanding was: Oniguruma generally offsets PCRE (Oniguruma supports hyphen and underscore in keywords, PCRE only supports underscore and so on..).
  • from your message it sounds like it is not the case, i.e. Oniguruma does not support all PCRE features.

Given the above, we should use subset of features supported by both PCRE and Oniguruma. Ideally there should be PR validation to catch it.. otherwise it is gonna be a never ending chase.

Before (3237bab) vs. After (c724205)

is going to break

btw, is it going to or is this example really broken in VSCode vNext?

[Gerrit0]

The latest insiders build I was able to get was:

Version: 1.61.0-insider (user setup)
Commit: 87260247dc7b912db6e6a21a366e6f50dca759ba
Date: 2021-09-03T20:58:36.778Z
Electron: 13.1.8
Chrome: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Windows_NT x64 10.0.19043

This was supposedly made 2 days ago, and this change merged in 5 days ago with microsoft/vscode@bd24934, so theoretically this fix should be in the build, I browsed through the files to find the appropriate grammar, and it does include this change, but it certainly doesn't highlight correctly!

I don't actually do much C#, ran into this because Shiki's auto update script broke because of this change. This could be because Onigasm (a WebAssembly port of Oniguruma) doesn't support non-fixed lookbehinds, but which looking for the spec, I found the page that I linked before, which made it seem like this was not supported, not a bug in Onigasm.

[am11]

but it certainly doesn't highlight correctly!

Your screenshot is showing it has fixed the issue described in #207, i.e. if keyword on line 20 has the correct color. Am I missing something?

Then it seems to be a limitation in Onigasm which neither VSCode nor linguist are using.

[Gerrit0]

The if does not have the correct color, it ought to be purple, like the first one is. Here's that same file with the troublesome whitespace stripped out

[am11]

Should be fixed by #220.

[Gerrit0]

Unless I'm missing something, this change is going to break VSCode once it lands in a release there.

3 months later... Yep. I missed something. Variable length look behind is allowed by a more recent version of Oniguruma. Shiki is still using a random commit from 2018 which doesn't support it, while VSCode (at least today, and I think possibly 3 months ago too) is using a release from 2020. So... if the original code was better, the reason I complained in the first place has gone away :)