In getSuggestionForNonexistentSymbol, guard name against undefined#19039
Conversation
|
Unfortunately, the public API always calls |
| const result = resolveNameHelper(location, name, meaning, /*nameNotFoundMessage*/ undefined, name, /*isUse*/ false, (symbols, name, meaning) => { | ||
| // `name` from the callback === the outer `name` | ||
| // NOTE: `name` from the callback is supposed to === the outer `name`, but is undefined in some cases | ||
| if (name === undefined) { |
There was a problem hiding this comment.
would not it be better to put this in getActionsForCorrectSpelling? i find it hard to see how resolveName can return undefined
|
I agree with @Andy-MS, lets wait on this one and see if we get new reports post 2.6.0 |
|
Looks like @aozgaa found post-2.6 crashes. Should I merge this? I could also ask him how to log so we could figure out how this happens. |
In both fixSpelling and getSuggestionForNonexistentSymbol
|
I switched to asserts instead of undefined checks, so we should be able to see whether the undefined is introduced. |
|
The original issue was confirmed fixed #18379 (comment) |
|
@aozgaa still see crash reports for it on 2.6.1 |
|
so we might have addressed some but not all cases (not sure how that would happen still though). |
| // `name` from the callback === the outer `name` | ||
| function getSuggestionForNonexistentSymbol(location: Node, outerName: __String, meaning: SymbolFlags): string { | ||
| const result = resolveNameHelper(location, outerName, meaning, /*nameNotFoundMessage*/ undefined, outerName, /*isUse*/ false, (symbols, name, meaning) => { | ||
| Debug.assert(name !== undefined, "name should always be defined, and equal to " + outerName); |
There was a problem hiding this comment.
Debug.assertEqual(name, outerName) and at the top Debug.assert(outerName !== undefined)
| const result = resolveNameHelper(location, name, meaning, /*nameNotFoundMessage*/ undefined, name, /*isUse*/ false, (symbols, name, meaning) => { | ||
| // `name` from the callback === the outer `name` | ||
| function getSuggestionForNonexistentSymbol(location: Node, outerName: __String, meaning: SymbolFlags): string { | ||
| Debug.assert(outerName !== undefined, "outername should always be defined"); |
There was a problem hiding this comment.
The description is optional -- in this case you're simply repeating what the code says.
There was a problem hiding this comment.
Should make the assert less mysterious in logs, etc, though, since Javascript can only print false, not outerName !== undefined, otherwise.
Fixes #19002 (probably)
This came from an aggregate of crash stacks, and from my reading of the code, it doesn't look like
namecan ever be null (see below for analysis). So:I'll poke around the unit tests to see if there's a good place to write a test.
Analysis
unescapeLeadingUnderscores(name), called fromgetSuggestionForNonexistentSymbol.nameis a parameter of an lambda, and is provided byresolveNameHelper.nameis an argument toresolveNameHelper, and is supposed to pass through unchanged.resolveNameHelper, I see no assignments toname, nothing that shadowsname, and that the lambda is always called withnameas its argument. So it looks like it does indeed pass through unchanged.getSuggestionForNonexistentSymbolisfixSpelling.ts:getActionsForCorrectSpelling.nameisgetTextOfNode(node).getTextOfNodecallsgetSourceTextOfNodeFromSourceFilegetSourceTextOfNodeFromSourceFileeither returns""if the node is missing or callsString.substringotherwise. So it never returnsundefined.I might be missing some call locations, but they shouldn't matter since the call stack from #19002 follows the path that I laid out above.