Rules overflowing since version 5.0 (aka atom 1.0.15)

[basarat]

I don't have an easy repro. But here is an example

This rule:

https://github.com/TypeStrong/atom-typescript/blob/c45213fb6619ed39bc9a36eae3390199a8e7776b/grammars/typescript.cson#L1220-L1317

causes everything after the following piece of code to be marked as meta.function.js:

function statusBar(state: StatusBarState = initialStatusBarState, action: StatusBarAction): StatusBarState {
    ////////////////EVERYTHING HERE/////////////
        if (action.activeProject) {

Sample screenshot: (some highlighting due to surrounding function context, but just blue and white)

This did not happen in atom 1.0.15.

refs a lot of reports in atom-typescript (different rules though), the first report being TypeStrong/atom-typescript#611 (TypeStrong/atom-typescript#614, TypeStrong/atom-typescript#613, TypeStrong/atom-typescript#626)

[zcbenz]

Probably related to atom/node-oniguruma#40.

[sgal]

Hi guys,

I can confirm, that this issue is related to changes in node-oniguruma. I tried to build atom@master with [email protected] and it has correct parsing/highlighting behaviour. Next version (5.0.0) breaks the highlighting.

Still haven't figured the exact place that is causing the error since I'm not really familiar with v8 API. Any help would be very much appreciated.

[alexdima]

I think in this case the grammar loops at position 42 in the string
-> it pushes & pops a rule without advancing
-> and I think the rule that gets pushed and popped without advancing is:
https://github.com/TypeStrong/atom-typescript/blob/c45213fb6619ed39bc9a36eae3390199a8e7776b/grammars/typescript.cson#L718

@zcbenz Now that I think about it I am stating to have doubts about the StrictEqual changes in OnigStringContext::IsSame from atom/node-oniguruma#40... What if two consecutive lines are equal, will the Scanners correctly drop the cache?

[alexdima]

To clarify: I don't think this issue is related to atom/node-oniguruma#40 , it is simply a bug in the grammar that causes a loop.

But I think the intent of OnigStringContext::IsSame was to do pointer location comparison on the strings to figure out if the OnigScanner is on the exact same line or on a different line. I suggest that OnigScanner::FindNextMatchSync also check if v8StartLocation === 0 and use that as well as a hint that the cache should be dropped in the following if statement:

Handle<Value> OnigScanner::FindNextMatchSync(Handle<String> v8String, Handle<Number> v8StartLocation) {
  if (!lastSource || !lastSource->IsSame(v8String))
    lastSource = shared_ptr<OnigStringContext>(new OnigStringContext(v8String));

What do you think?

[zcbenz]

I'm cc-ing @kevinsawicki here for help, I'm not very familiar with how oniguruma's cache works.

[kevinsawicki]

What do you think?

That seems like a solid approach, would you be able to start a pull request for this?

[basarat]

it is simply a bug in the grammar that causes a loop.

@alexandrudima you mean a bug in atom-typescript's grammar. If so can you be more specific on what fix is needed (or exactly what key/value pair is causing the loop). Thanks 🌹