Fixed crash in go to definition related to expando classes in JS files #57628
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
typescript-bot
added
the
For Uncommitted Bug
| @@ -583,16 +583,20 @@ function isExpandoDeclaration(node: Declaration): boolean { | |||
|
|
|||
| function getDefinitionFromSymbol(typeChecker: TypeChecker, symbol: Symbol, node: Node, failedAliasResolution?: boolean, excludeDeclaration?: Node): DefinitionInfo[] | undefined { | |||
| const filteredDeclarations = filter(symbol.declarations, d => d !== excludeDeclaration); | |||
| const signatureDefinition = getConstructSignatureDefinition() || getCallSignatureDefinition(); | |||
There was a problem hiding this comment.
Changes in this function are not needed. I just tweaked it while trying to understand what happens here. This still acts as a short-circuit for calls below it so I'd call it a micro-optimization and keep the change ;p
| const cls = find(filteredDeclarations, isClassLike) || Debug.fail("Expected declaration to have at least one class-like declaration"); | ||
| return getSignatureDefinition(cls.members, /*selectConstructors*/ true); | ||
| const cls = find(filteredDeclarations, isClassLike); | ||
| return cls && getSignatureDefinition(cls.members, /*selectConstructors*/ true); |
There was a problem hiding this comment.
there is a question in this case - should this be treated more like a class or more like a property? I decided that it's better to ignore class-ness here as the expando property acts as a common root for other things and as such it makes sense to prefer showing it here in my opinion.
There was a problem hiding this comment.
What is the difference in behaviour? The baselines for goToDefinitionExpandoClass2 show that the constructor is found when present, so what are we skipping here?
There was a problem hiding this comment.
Without this skip it crashes while resolving what becomes defId: 0 in goToDefinitionExpandoClass2 with the fix. The resolved constructor (defId: 1) comes from sigInfo here:
TypeScript/src/services/goToDefinition.ts
Line 239 in 9d8f812
There was a problem hiding this comment.
OK, that makes sense. I guess I thought of Test so strongly as a class in Core.Test = class { ... that I didn't even consider that it might not appear because it's a property. In other words, I think that defId: 0 definitely makes sense to include.
| const cls = find(filteredDeclarations, isClassLike) || Debug.fail("Expected declaration to have at least one class-like declaration"); | ||
| return getSignatureDefinition(cls.members, /*selectConstructors*/ true); | ||
| const cls = find(filteredDeclarations, isClassLike); | ||
| return cls && getSignatureDefinition(cls.members, /*selectConstructors*/ true); |
There was a problem hiding this comment.
What is the difference in behaviour? The baselines for goToDefinitionExpandoClass2 show that the constructor is found when present, so what are we skipping here?
fixes a crash reported here