Skip to content
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

Merged
merged 3 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/services/goToDefinition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
createTextSpanFromBounds,
createTextSpanFromNode,
createTextSpanFromRange,
Debug,
Declaration,
DefinitionInfo,
DefinitionInfoAndBoundSpan,
Expand Down Expand Up @@ -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();
Copy link
Contributor Author

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

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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

return node.kind === SyntaxKind.SuperKeyword ? [sigInfo, ...defs] : [...defs, sigInfo];

Copy link
Member

Choose a reason for hiding this comment

The 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 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.

}
}

Expand Down
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
}
]
17 changes: 17 additions & 0 deletions tests/cases/fourslash/goToDefinitionExpandoClass1.ts
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");
19 changes: 19 additions & 0 deletions tests/cases/fourslash/goToDefinitionExpandoClass2.ts
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");