deleteDeclaration: don't crash on the top node.
#42968
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Milestone Bug
andrewbranch
requested changes
Feb 26, 2021
Member
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
But moreover, we know that this is not from Emacs or fringe editors or general API misuse, because the telemetry comes to us via VS Code. I’m generally not a fan of silencing bugs we don’t understand. I’m not sure this is any better than crashing, because either way, the result is that we don’t generate a codefix. In either case, the user experience is either broken or completely unnoticeable (hard to say which); the only thing this PR does is prevents us from getting telemetry for it anymore.
elibarzilay
force-pushed
the
33726
branch
2 times, most recently
from
063e360 to
b5cc450
Compare
andrewbranch
requested changes
Mar 22, 2021
andrewbranch
approved these changes
Mar 23, 2021
A misbehaved client can sometimes cause the server to reach `deleteDeclaration` with the SourceFile, and it will crash due to no `node.parent`. I couldn't find a good way to create a test for it, but I could trigger it manually by having a file with just a `,`, and sending an explicit `getCodeFixes` command to the server with `errorCodes: [6133]`. Do three things to improve this: 1. `textChanges.ts`: if we get here with the root node, delete it instead of failing. 2. `fixUnusedIdentifier.ts`: check that we don't `delete` a node that is the whole source file, so the error is more focused (should have more similar failure stacks). 3. `session.ts`: when there was any failure in `getCodeFixes`, check if the input had a diag code that does not appear in the requested text range, and throw an error saying that the failure is probably a result of a bad request. Closes microsoft#33726 (probably not fixing it, but making it easier to find the cause)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A misbehaved client can sometimes cause the server to reach
deleteDeclarationwith the SourceFile, and it will crash due to nonode.parent. I couldn't find a good way to create a test for it, butI could trigger it manually by having a file with just a
,, andsending an explicit
getCodeFixescommand to the server witherrorCodes: [6133].Do three things to improve this:
textChanges.ts: if we get here with the root node, delete itinstead of failing.
fixUnusedIdentifier.ts: check that we don'tdeletea node that isthe whole source file, so the error is more focused (should have more
similar failure stacks).
session.ts: when there was any failure ingetCodeFixes, check ifthe input had a diag code that does not appear in the requested text
range, and throw an error saying that the failure is probably a
result of a bad request.
Closes #33726 (probably not fixing it, but making it easier to find the cause)