Fix symbol display exception when handling incomplete class #44936
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
When a class declaration lacks a name, don't throw an exception when
producing the display parts (e.g. for QuickInfo).
Remaining issues:
1. The name shows as "__missing", the name of the underlying symbol,
rather than "(Missing)", as it is for the corresponding function
declaration case (because the parse constructs a missing identifier
node for the function declaration).
2. "(Missing)" is hard-coded, rather than being a localizable resource
string.
3. When an anonymous class declaration is a default export, the
corresponding symbol is named "default", resulting in the confusing
display string "class default".
Since display parts are built using existing `symbolToString`
functionality, it wasn't clear whether detecting special symbol names
and replacing them with user-friendly strings could be done without
breaking other functionality.
Similarly, changing the shape of the parse tree seemed riskier than the
problem justified (the user experience is just not getting QuickInfo for
the incomplete declaration, which seems acceptable).
typescript-bot
added
Author: Team
For Uncommitted Bug
Member
Author
|
Stack was |
champpp96
approved these changes
Jul 8, 2021
sandersn
approved these changes
Jul 14, 2021
| }, | ||
| "displayParts": [ | ||
| { | ||
| "text": "class", |
Member
There was a problem hiding this comment.
class default is weird but correct, at least according to commonjs:
- assignment to a variable gives an anonymous function the name of the variable, eg
var f = function () { } - JS classes are just functions.
export default xtranslates toexports.default = xin commonjs.
I think this is in the standard, but at least node and TS agree on this.
There was a problem hiding this comment.
Node appears to agree even in ESM mode:
foo.mjs
export default class {}bar.mjs
import foo from './foo.mjs';
console.log(foo);> node bar.mjs
[class default]
| // for class and function declarations, use the `default` modifier | ||
| // when the declaration is unnamed. | ||
| const defaultModifier = find(node.modifiers!, isDefaultModifier); | ||
| const defaultModifier = node.modifiers && find(node.modifiers, isDefaultModifier); |
Member
There was a problem hiding this comment.
Could also do
Suggested change
| const defaultModifier = node.modifiers && find(node.modifiers, isDefaultModifier); | |
| const defaultModifier = forEach(node.modifiers, isDefaultModifier); |
or
Suggested change
| const defaultModifier = node.modifiers && find(node.modifiers, isDefaultModifier); | |
| const defaultModifier = node.modifiers?.find(isDefaultModifier); |
DanielRosenwasser
approved these changes
Aug 6, 2021
BobobUnicorn
pushed a commit
to BobobUnicorn/TypeScript
that referenced
this pull request
Oct 24, 2021
…t#44936) When a class declaration lacks a name, don't throw an exception when producing the display parts (e.g. for QuickInfo). Remaining issues: 1. The name shows as "__missing", the name of the underlying symbol, rather than "(Missing)", as it is for the corresponding function declaration case (because the parse constructs a missing identifier node for the function declaration). 2. "(Missing)" is hard-coded, rather than being a localizable resource string. 3. When an anonymous class declaration is a default export, the corresponding symbol is named "default", resulting in the confusing display string "class default". Since display parts are built using existing `symbolToString` functionality, it wasn't clear whether detecting special symbol names and replacing them with user-friendly strings could be done without breaking other functionality. Similarly, changing the shape of the parse tree seemed riskier than the problem justified (the user experience is just not getting QuickInfo for the incomplete declaration, which seems acceptable).
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 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.
When a class declaration lacks a name, don't throw an exception when
producing the display parts (e.g. for QuickInfo).
Remaining issues:
rather than "(Missing)", as it is for the corresponding function
declaration case (because the parse constructs a missing identifier
node for the function declaration).
string.
corresponding symbol is named "default", resulting in the confusing
display string "class default".
Since display parts are built using existing
symbolToStringfunctionality, it wasn't clear whether detecting special symbol names
and replacing them with user-friendly strings could be done without
breaking other functionality.
Similarly, changing the shape of the parse tree seemed riskier than the
problem justified (the user experience is just not getting QuickInfo for
the incomplete declaration, which seems acceptable).