Skip to content

Prioritize case and default keywords in switch statement completions #62126

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 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
87 changes: 86 additions & 1 deletion src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
canHaveDecorators,
canUsePropertyAccess,
CaseBlock,
CaseClause,
cast,
CharacterCodes,
ClassElement,
Expand Down Expand Up @@ -48,6 +49,7 @@ import {
Debug,
Declaration,
Decorator,
DefaultClause,
Diagnostics,
diagnosticToString,
displayPart,
Expand Down Expand Up @@ -355,6 +357,7 @@ import {
startsWith,
stringToToken,
stripQuotes,
SwitchStatement,
Symbol,
SymbolDisplay,
SymbolDisplayPart,
Expand Down Expand Up @@ -1269,6 +1272,83 @@ function getOptionalReplacementSpan(location: Node | undefined) {
return location?.kind === SyntaxKind.Identifier ? createTextSpanFromNode(location) : undefined;
}

function shouldPrioritizeCaseAndDefaultKeywords(contextToken: Node | undefined, position: number): boolean {
if (!contextToken) return false;

// Check if we're in a switch statement context
const switchStatement = findAncestor(contextToken, node =>
node.kind === SyntaxKind.SwitchStatement ? true :
isFunctionLikeDeclaration(node) || isClassLike(node) ? "quit" :
false) as SwitchStatement | undefined;

if (!switchStatement) return false;

const sourceFile = contextToken.getSourceFile();
const { line: currentLine, character: currentColumn } = getLineAndCharacterOfPosition(sourceFile, position);

// Case 1: Cursor is directly inside the switch block
// switch (thing) {
// /*cursor*/
// }
if (contextToken.parent === switchStatement.caseBlock) {
return true;
}

// Case 2: Cursor is at the same column as a case/default keyword but on a different line,
// with at least one statement in the previous clause that meets certain conditions
const caseBlock = switchStatement.caseBlock;
if (!caseBlock) return false;

// Find the last case/default clause before the cursor position
let lastClause: CaseClause | DefaultClause | undefined;
for (const clause of caseBlock.clauses) {
if (clause.pos >= position) break;
lastClause = clause;
}

if (!lastClause) return false;

// Check if cursor is at the same column as the last clause's keyword
const clauseKeywordPos = lastClause.kind === SyntaxKind.CaseClause ?
lastClause.getFirstToken(sourceFile)!.getStart(sourceFile) :
lastClause.getFirstToken(sourceFile)!.getStart(sourceFile);
const { line: clauseLine, character: clauseColumn } = getLineAndCharacterOfPosition(sourceFile, clauseKeywordPos);

if (currentLine === clauseLine || currentColumn !== clauseColumn) {
return false;
}

// Check if there's at least one statement in the clause
if (!lastClause.statements || lastClause.statements.length === 0) {
return false;
}

const lastStatement = lastClause.statements[lastClause.statements.length - 1];

// Get position of the last statement
const { line: stmtLine, character: stmtColumn } = getLineAndCharacterOfPosition(sourceFile, lastStatement.getStart(sourceFile));

// Check if it's a jump statement
const isJumpStatement = isBreakOrContinueStatement(lastStatement) ||
lastStatement.kind === SyntaxKind.ReturnStatement ||
lastStatement.kind === SyntaxKind.ThrowStatement;

if (isJumpStatement) {
// For jump statements: prioritize if on same line as case, or on different line with different indentation
if (stmtLine === clauseLine || (stmtLine !== clauseLine && stmtColumn !== clauseColumn)) {
return true;
}
}
else {
// For non-jump statements: prioritize only if on different line and different column
if (stmtLine !== clauseLine && stmtColumn !== clauseColumn) {
return true;
}
}

return false;
}

function completionInfoFromData(
sourceFile: SourceFile,
host: LanguageServiceHost,
Expand Down Expand Up @@ -1369,14 +1449,19 @@ function completionInfoFromData(
);

if (keywordFilters !== KeywordCompletionFilters.None) {
const shouldPrioritizeCaseDefault = shouldPrioritizeCaseAndDefaultKeywords(contextToken, position);
for (const keywordEntry of getKeywordCompletions(keywordFilters, !insideJsDocTagTypeExpression && isSourceFileJS(sourceFile))) {
if (
isTypeOnlyLocation && isTypeKeyword(stringToToken(keywordEntry.name)!) ||
!isTypeOnlyLocation && isContextualKeywordInAutoImportableExpressionSpace(keywordEntry.name) ||
!uniqueNames.has(keywordEntry.name)
) {
uniqueNames.add(keywordEntry.name);
insertSorted(entries, keywordEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
// Create a modified keyword entry with prioritized sort text for case/default in switch contexts
const modifiedKeywordEntry = shouldPrioritizeCaseDefault && (keywordEntry.name === "case" || keywordEntry.name === "default")
? { ...keywordEntry, sortText: SortText.LocalDeclarationPriority }
: keywordEntry;
insertSorted(entries, modifiedKeywordEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
}
}
}
Expand Down
126 changes: 126 additions & 0 deletions tests/cases/fourslash/switchCaseCompletionPriority.ts
Copy link
Member

Choose a reason for hiding this comment

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

The diff is huge, so maybe this needs to run with noLib or some similar thing to trim down the full completion list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @noLib: true and switched from baseline testing to targeted verify.completions() assertions. This reduced the test from 256k+ lines to a focused test that validates the specific sortText values ("10" for prioritized, "15" for normal) without the massive global completion list. Fixed in c8b6d8c.

Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/// <reference path="fourslash.ts" />

// @noLib: true

//// declare const thing: string;

//// // Basic switch block - should prioritize case/default
//// switch (thing) {
//// /*basic*/
//// }

//// // Same - show all completions (not at switch body level)
//// switch (thing) {
//// case 42:
//// /*sameAll1*/
//// }

//// // Same - show all completions (after break at same column as case)
//// switch (thing) {
//// case 42:
//// break;
//// /*sameAll2*/
//// }

//// // Same - show all completions (complex nested structure)
//// switch (thing) {
//// case 42:
//// if (Math.random()) {
//// }
//// else {
//// }
//// /*sameAll3*/
//// }

//// // After break statement with proper indentation
//// switch (thing) {
//// case 42:
//// break;
//// /*newPrio1*/
//// }

//// // After same-line break statement
//// switch (thing) {
//// case 42: break;
//// /*newPrio2*/
//// }

//// // After return statement with proper indentation
//// switch (thing) {
//// case 42:
//// return 1;
//// /*newPrio3*/
//// }

//// // After throw statement with proper indentation
//// switch (thing) {
//// case 42:
//// throw new Error();
//// /*newPrio4*/
//// }

// Scenarios where case/default should be prioritized
goTo.marker("basic");
verify.completions({
includes: [
{ name: "case", sortText: "10" },
{ name: "default", sortText: "10" }
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be tested that thing is still in the completion list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added explicit verification that thing is still in the completion list with proper sortText ("11") across all test scenarios. The test now uses both baseline testing to show relative ordering and targeted verify.completions() to confirm thing availability. Fixed in 75fcd1d.

]
});

goTo.marker("newPrio1");
verify.completions({
Copy link
Member

Choose a reason for hiding this comment

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

Go back to baselining if the baselines aren't massive, and show some characteristics about the ordering. The key thing the tests need to witness is whether case, default, and thing appear in the list and what their relative order would be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched back to baseline testing with @nolib to keep baselines manageable (~548KB). The baseline now clearly shows the relative ordering of case, default, and thing in different contexts:

  • Prioritized contexts: case/default sortText "10", thing sortText "11"
  • Non-prioritized contexts: case/default sortText "15", thing sortText "11"

Fixed in 75fcd1d.

includes: [
{ name: "case", sortText: "10" },
{ name: "default", sortText: "10" }
]
});

goTo.marker("newPrio2");
verify.completions({
includes: [
{ name: "case", sortText: "10" },
{ name: "default", sortText: "10" }
]
});

goTo.marker("newPrio3");
verify.completions({
includes: [
{ name: "case", sortText: "10" },
{ name: "default", sortText: "10" }
]
});

goTo.marker("newPrio4");
verify.completions({
includes: [
{ name: "case", sortText: "10" },
{ name: "default", sortText: "10" }
]
});

// Scenarios where case/default should NOT be prioritized
goTo.marker("sameAll1");
verify.completions({
includes: [
{ name: "case", sortText: "15" },
{ name: "default", sortText: "15" }
]
});

goTo.marker("sameAll2");
verify.completions({
includes: [
{ name: "case", sortText: "15" },
{ name: "default", sortText: "15" }
]
});

goTo.marker("sameAll3");
verify.completions({
includes: [
{ name: "case", sortText: "15" },
{ name: "default", sortText: "15" }
]
});