Upgrade language-javascript to 0.111.0 #10999
Conversation
|
Well, it seems like I managed to crash Electron. That's pretty impressive. On a different note, I have no idea how to fix the core specs this time around. |
|
This crash started occurring after running specs a few times locally: Dump is attached. 😬 |
# Conflicts: # package.json
|
Summarizing my findings after working on this for a good couple of hours: The screen lines should contain this after the deletion on line 837 of display-buffer-spec.coffee: var quicksort = function () {
var sort = function(items) {
if (items.length <= 1) return items;
current = items.shift();
}
return sort(left).concat(pivot).concat(sort(right));
};
return sort(Array.apply(this, arguments));
};but instead, they contain this: var quicksort = function () {
var sort = function(items) {
if (items.length <= 1) return items;
current = items.shift();
}
return sort(Array.apply(this, arguments));
return sort(Array.apply(this, arguments));
}; |
|
@as-cii Can you take a look at this? I was able to replicate the test failures (though not the crash) and there seems to be some weirdness in the display-buffer. |
|
The crash was probably a one-time thing, as I haven't been able to reproduce it after that either. |
|
After discussion with @as-cii the problem seems to be unbalanced rules somewhere caused by the refactor in 0.111.0. I'll be adding extra scopes later to make sure everything's being closed correctly. |
|
So, as @50Wliu mentioned, this seems to be a problem with having unbalanced stacks (after atom/language-javascript#312), either because of the grammar or @50Wliu: could you post the findings you mentioned in chat here? I think I won't be able to follow up on this next week, and it would be nice to have a summary of what we think is happening on this issue. |
|
As a recap: So now this works fine for normal files, but what the failing spec is doing is deleting a line which has an opening bracket on it. This results in an unbalanced bracket scenario which is illustrated in the following snippet: var quicksort = function () {
var sort = function(items) {
if (items.length <= 1) return items;
current = items.shift();
} //<-- this bracket is matched with the one on line 2
return sort(left).concat(pivot).concat(sort(right));
}; //<-- and this one with the one on line 1
return sort(Array.apply(this, arguments));
}; //<-- this bracket isn't assigned a scopeYou can see this behavior visually through Bracket Matcher. I assume this scenario is causing unbalanced stacks, and completely confusing the display buffer. Still working on figuring out which part of the code is to blame though. |
|
@50Wliu the snippet you posted for the illustration works fine: var quicksort = function () { // 1
var sort = function(items) { // 2
if (items.length <= 1)
return items;
current = items.shift();
// } <- extra bracket
return sort(left).concat(pivot).concat(sort(right));
}; // 2
return sort(Array.apply(this, arguments));
}; // 1Any progress on this? |
…ate-language-javascript
|
I've updated this PR to [email protected] and pulled in the latest changes from master*, but the failures remain. The brighter news is that #11414 seems to fix this issue. * Once Github Desktop decides to finish syncing the changes |
|
@as-cii I think I've "fixed" the remaining TokenizedBuffer spec failures. These failures are closely related to the dangling bracket scenario above: all but one of the failures comment out an opening bracket, which causes changes to cascade down to the next one. Fixing those specs has the unfortunate side effect of not really being able to tell if everything works as intended though, since the default tokenization chunk is also 5 :/. |
|
@as-cii What do we need to do to get this into master before the next railcar release? |
|
I was somewhat concerned about performance, because Thanks, @50Wliu! ✨ |
|
Thanks @as-cii! I'll be happy to work with you to fix any performance problems that show up. |
Time to update some more specs!