Skip to content

fix(51225): Go-to-definition on case or default should jump to the containing switch statement if available. #51236

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jan 11, 2024
Merged

fix(51225): Go-to-definition on case or default should jump to the containing switch statement if available. #51236

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7721e83
fix(51225): Go-to-definition on case or default should jump to the co…
sviat9440 Oct 20, 2022
1c1599f
Merge branch 'main' into fix/51225
sviat9440 Nov 24, 2022
1995f33
fix(51225): Fix context span
sviat9440 Nov 24, 2022
f09b5b6
Merge branch 'main' into fix/51225
sandersn Dec 5, 2023
03d4b40
port tests to baselineGoToDefinition
sandersn Dec 5, 2023
d692bbf
dprint fmt
sandersn Dec 5, 2023
a97f704
Revert context span change
sandersn Dec 5, 2023
77c28cb
Revert context span again, add close paren this time
sandersn Dec 15, 2023
e11f00e
Better way of including close paren
sandersn Dec 15, 2023
ffe7d40
Merge branch 'main' into fix/51225
sandersn Jan 3, 2024
279ce44
Move context span calculation to central location
sandersn Jan 4, 2024
dd9d9f1
move createDefinitionInfoFromStatement closer to others
sandersn Jan 4, 2024
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
56 changes: 53 additions & 3 deletions src/services/goToDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import {
isClassStaticBlockDeclaration,
isConstructorDeclaration,
isDeclarationFileName,
isDefaultClause,
isExternalModuleNameRelative,
isFunctionLike,
isFunctionLikeDeclaration,
Expand All @@ -69,9 +70,11 @@ import {
isPropertyName,
isRightSideOfPropertyAccess,
isStaticModifier,
isSwitchStatement,
isTypeAliasDeclaration,
isTypeReferenceNode,
isVariableDeclaration,
KeywordSyntaxKind,
last,
map,
mapDefined,
Expand All @@ -91,6 +94,7 @@ import {
skipTrivia,
some,
SourceFile,
SwitchStatement,
Symbol,
SymbolDisplay,
SymbolFlags,
Expand Down Expand Up @@ -133,9 +137,33 @@ export function getDefinitionAtPosition(program: Program, sourceFile: SourceFile
return label ? [createDefinitionInfoFromName(typeChecker, label, ScriptElementKind.label, node.text, /*containerName*/ undefined!)] : undefined; // TODO: GH#18217
}

if (node.kind === SyntaxKind.ReturnKeyword) {
const functionDeclaration = findAncestor(node.parent, n => isClassStaticBlockDeclaration(n) ? "quit" : isFunctionLikeDeclaration(n)) as FunctionLikeDeclaration | undefined;
return functionDeclaration ? [createDefinitionFromSignatureDeclaration(typeChecker, functionDeclaration)] : undefined;
switch (node.kind) {
case SyntaxKind.ReturnKeyword:
const functionDeclaration = findAncestor(node.parent, n =>
isClassStaticBlockDeclaration(n)
? "quit"
: isFunctionLikeDeclaration(n)) as FunctionLikeDeclaration | undefined;
return functionDeclaration
? [createDefinitionFromSignatureDeclaration(typeChecker, functionDeclaration)]
: undefined;
case SyntaxKind.DefaultKeyword:
if (!isDefaultClause(node.parent)) {
break;
}
// falls through
case SyntaxKind.CaseKeyword:
const switchStatement = findAncestor(node.parent, isSwitchStatement);
if (switchStatement) {
return [
createDefinitionInfoFromStatement(
switchStatement,
SyntaxKind.SwitchKeyword,
sourceFile,
"switch",
),
];
}
break;
}

if (node.kind === SyntaxKind.AwaitKeyword) {
Expand Down Expand Up @@ -710,3 +738,25 @@ function isConstructorLike(node: Node): boolean {
return false;
}
}

function createDefinitionInfoFromStatement(
statement: SwitchStatement,
keywordKind: KeywordSyntaxKind,
sourceFile: SourceFile,
name: string,
): DefinitionInfo {
const keyword = find(statement.getChildren(sourceFile), node => node.kind === keywordKind)!;
return {
fileName: sourceFile.fileName,
textSpan: createTextSpanFromNode(keyword, sourceFile),
kind: ScriptElementKind.keyword,
name,
containerKind: undefined!,
containerName: "",
contextSpan: createTextSpanFromBounds(keyword.getStart(sourceFile), statement.caseBlock.getFullStart()),
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved to getContextNode which handled SwitchStatementthen ...FindAllReferences.getContextSpan() so that all places where we will use context span are in single place.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at createDefinitionInfoFromName above, it looks like the right way to use this is first to call getContextNode to get the start/end tokens, then to use toContextSpan to turn it into a context span (see code below). Unfortunately, my getFullStart hack (which grabs the closing paren) gets lost since getContextNode returns a node pair, not a location pair. And toContextSpan currently always calls getEnd, but my hack uses getFullStart.

    const keyword = FindAllReferences.getContextNode(statement)!;
    const textSpan = createTextSpanFromNode(isContextWithStartAndEndNode(keyword) ? keyword.start : keyword, sourceFile)
    return {
        fileName: sourceFile.fileName,
        textSpan,
        kind: ScriptElementKind.keyword,
        name: "switch",
        containerKind: undefined!,
        containerName: "",
        ...FindAllReferences.toContextSpan(textSpan, sourceFile, keyword),
        isLocal: true,
        isAmbient: false,
        unverified: false,
        failedAliasResolution: undefined,
    };

Any ideas? I'm going to try hacking toContextSpan to special case switches to see how bad that is.

Copy link
Member

Choose a reason for hiding this comment

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

OK, the hack isn't too bad; there's a special case for strin gliterals there already, so I added it after that.

isLocal: true,
isAmbient: false,
unverified: false,
failedAliasResolution: undefined,
};
}
17 changes: 17 additions & 0 deletions tests/baselines/reference/goToDefinitionSwitchCase1.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase1.ts ===
// <|[|switch|] (null )|> {
// /*GOTO DEF*/case null: break;
// }

// === Details ===
[
{
"kind": "keyword",
"name": "switch",
"containerName": "",
"isLocal": true,
"isAmbient": false,
"unverified": false
}
]
17 changes: 17 additions & 0 deletions tests/baselines/reference/goToDefinitionSwitchCase2.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase2.ts ===
// <|[|switch|] (null)|> {
// /*GOTO DEF*/default: break;
// }

// === Details ===
[
{
"kind": "keyword",
"name": "switch",
"containerName": "",
"isLocal": true,
"isAmbient": false,
"unverified": false
}
]
45 changes: 45 additions & 0 deletions tests/baselines/reference/goToDefinitionSwitchCase3.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase3.ts ===
// <|[|switch|] (null)|> {
// /*GOTO DEF*/default: {
// switch (null) {
// default: break;
// }
// };
// }

// === Details ===
[
{
"kind": "keyword",
"name": "switch",
"containerName": "",
"isLocal": true,
"isAmbient": false,
"unverified": false
}
]



// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase3.ts ===
// switch (null) {
// default: {
// <|[|switch|] (null)|> {
// /*GOTO DEF*/default: break;
// }
// };
// }

// === Details ===
[
{
"kind": "keyword",
"name": "switch",
"containerName": "",
"isLocal": true,
"isAmbient": false,
"unverified": false
}
]
21 changes: 21 additions & 0 deletions tests/baselines/reference/goToDefinitionSwitchCase4.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase4.ts ===
// switch (null) {
// case null: break;
// }
//
// <|[|switch|] (null)|> {
// /*GOTO DEF*/case null: break;
// }

// === Details ===
[
{
"kind": "keyword",
"name": "switch",
"containerName": "",
"isLocal": true,
"isAmbient": false,
"unverified": false
}
]
16 changes: 16 additions & 0 deletions tests/baselines/reference/goToDefinitionSwitchCase5.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase5.ts ===
// [|export /*GOTO DEF*/default {}|]

// === Details ===
[
{
"kind": "property",
"name": "default",
"containerName": "\"/tests/cases/fourslash/goToDefinitionSwitchCase5\"",
"isLocal": true,
"isAmbient": false,
"unverified": false,
"failedAliasResolution": false
}
]
47 changes: 47 additions & 0 deletions tests/baselines/reference/goToDefinitionSwitchCase6.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase6.ts ===
// export default { /*GOTO DEF*/[|{| textSpan: true |}case|] };
// default;
// case 42;

// === Details ===
[
{
"kind": "property",
"name": "case",
"containerName": "__object",
"isLocal": true,
"isAmbient": false,
"unverified": false,
"failedAliasResolution": false
}
]



// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase6.ts ===
// [|export default { case };
// /*GOTO DEF*/default;
// case 42;|]

// === Details ===
[
{
"kind": "module",
"name": "\"/tests/cases/fourslash/goToDefinitionSwitchCase6\"",
"containerName": "",
"isLocal": false,
"isAmbient": false,
"unverified": false,
"failedAliasResolution": false
}
]



// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase6.ts ===
// export default { case };
// default;
// /*GOTO DEF*/case 42;
18 changes: 18 additions & 0 deletions tests/baselines/reference/goToDefinitionSwitchCase7.baseline.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// === goToDefinition ===
// === /tests/cases/fourslash/goToDefinitionSwitchCase7.ts ===
// switch (null) {
// case null:
// [|export /*GOTO DEF*/default 123;|]

// === Details ===
[
{
"kind": "var",
"name": "default",
"containerName": "",
"isLocal": true,
"isAmbient": false,
"unverified": false,
"failedAliasResolution": false
}
]
7 changes: 7 additions & 0 deletions tests/cases/fourslash/goToDefinitionSwitchCase1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference path="fourslash.ts" />

////switch (null ) {
//// [|/*start*/case|] null: break;
////}

verify.baselineGoToDefinition("start");
7 changes: 7 additions & 0 deletions tests/cases/fourslash/goToDefinitionSwitchCase2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference path="fourslash.ts" />

////switch (null) {
//// [|/*start*/default|]: break;
////}

verify.baselineGoToDefinition("start");
11 changes: 11 additions & 0 deletions tests/cases/fourslash/goToDefinitionSwitchCase3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path="fourslash.ts" />

////switch (null) {
//// [|/*start1*/default|]: {
//// switch (null) {
//// [|/*start2*/default|]: break;
//// }
//// };
////}

verify.baselineGoToDefinition("start1", "start2");
11 changes: 11 additions & 0 deletions tests/cases/fourslash/goToDefinitionSwitchCase4.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path="fourslash.ts" />

//// switch (null) {
//// case null: break;
//// }
////
//// switch (null) {
//// [|/*start*/case|] null: break;
//// }

verify.baselineGoToDefinition("start");
5 changes: 5 additions & 0 deletions tests/cases/fourslash/goToDefinitionSwitchCase5.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/// <reference path="fourslash.ts" />

////export [|/*start*/default|] {}

verify.baselineGoToDefinition("start");
7 changes: 7 additions & 0 deletions tests/cases/fourslash/goToDefinitionSwitchCase6.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference path="fourslash.ts" />

////export default { [|/*a*/case|] };
////[|/*b*/default|];
////[|/*c*/case|] 42;

verify.baselineGoToDefinition("a", "b", "c");
7 changes: 7 additions & 0 deletions tests/cases/fourslash/goToDefinitionSwitchCase7.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference path="fourslash.ts" />

////switch (null) {
//// case null:
//// export [|/*start*/default|] 123;

verify.baselineGoToDefinition("start");