-
Notifications
You must be signed in to change notification settings - Fork 12.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed crash in go to definition related to expando classes in JS files #57628
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -9,7 +9,6 @@ import { | |||
createTextSpanFromBounds, | ||||
createTextSpanFromNode, | ||||
createTextSpanFromRange, | ||||
Debug, | ||||
Declaration, | ||||
DefinitionInfo, | ||||
DefinitionInfoAndBoundSpan, | ||||
|
@@ -581,16 +580,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(); | ||||
if (signatureDefinition) { | ||||
return signatureDefinition; | ||||
} | ||||
const withoutExpandos = filter(filteredDeclarations, d => !isExpandoDeclaration(d)); | ||||
const results = some(withoutExpandos) ? withoutExpandos : filteredDeclarations; | ||||
return getConstructSignatureDefinition() || getCallSignatureDefinition() || map(results, declaration => createDefinitionInfo(declaration, typeChecker, symbol, node, /*unverified*/ false, failedAliasResolution)); | ||||
return map(results, declaration => createDefinitionInfo(declaration, typeChecker, symbol, node, /*unverified*/ false, failedAliasResolution)); | ||||
|
||||
function getConstructSignatureDefinition(): DefinitionInfo[] | undefined { | ||||
// Applicable only if we are in a new expression, or we are on a constructor declaration | ||||
// and in either case the symbol has a construct signature definition, i.e. class | ||||
if (symbol.flags & SymbolFlags.Class && !(symbol.flags & (SymbolFlags.Function | SymbolFlags.Variable)) && (isNewExpressionTarget(node) || node.kind === SyntaxKind.ConstructorKeyword)) { | ||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this skip it crashes while resolving what becomes TypeScript/src/services/goToDefinition.ts Line 239 in 9d8f812
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, that makes sense. I guess I thought of Test so strongly as a class in |
||||
} | ||||
} | ||||
|
||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// === goToDefinition === | ||
// === /tests/cases/fourslash/index.js === | ||
// const Core = {} | ||
// | ||
// <|Core.[|Test|]|> = class { } | ||
// | ||
// Core.Test.prototype.foo = 10 | ||
// | ||
// new Core.Tes/*GOTO DEF*/t() | ||
|
||
// === Details === | ||
[ | ||
{ | ||
"kind": "property", | ||
"name": "Test", | ||
"containerName": "Core", | ||
"isLocal": true, | ||
"isAmbient": false, | ||
"unverified": false, | ||
"failedAliasResolution": false | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// === goToDefinition === | ||
// === /tests/cases/fourslash/index.js === | ||
// const Core = {} | ||
// | ||
// <|Core.[|{| defId: 0 |}Test|]|> = class { | ||
// [|{| defId: 1 |}constructor() { }|] | ||
// } | ||
// | ||
// Core.Test.prototype.foo = 10 | ||
// | ||
// new Core.Tes/*GOTO DEF*/t() | ||
|
||
// === Details === | ||
[ | ||
{ | ||
"defId": 0, | ||
"kind": "property", | ||
"name": "Test", | ||
"containerName": "Core", | ||
"isLocal": true, | ||
"isAmbient": false, | ||
"unverified": false, | ||
"failedAliasResolution": false | ||
}, | ||
{ | ||
"defId": 1, | ||
"kind": "constructor", | ||
"name": "__constructor", | ||
"containerName": "Test", | ||
"isLocal": true, | ||
"isAmbient": false, | ||
"unverified": false, | ||
"failedAliasResolution": false | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/// <reference path="fourslash.ts" /> | ||
|
||
// @strict: true | ||
// @allowJs: true | ||
// @checkJs: true | ||
|
||
// @filename: index.js | ||
|
||
//// const Core = {} | ||
//// | ||
//// Core.Test = class { } | ||
//// | ||
//// Core.Test.prototype.foo = 10 | ||
//// | ||
//// new Core.Tes/*1*/t() | ||
|
||
verify.baselineGoToDefinition("1"); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/// <reference path="fourslash.ts" /> | ||
|
||
// @strict: true | ||
// @allowJs: true | ||
// @checkJs: true | ||
|
||
// @filename: index.js | ||
|
||
//// const Core = {} | ||
//// | ||
//// Core.Test = class { | ||
//// constructor() { } | ||
//// } | ||
//// | ||
//// Core.Test.prototype.foo = 10 | ||
//// | ||
//// new Core.Tes/*1*/t() | ||
|
||
verify.baselineGoToDefinition("1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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