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 2 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
171 changes: 128 additions & 43 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
CancellationToken,
canHaveDecorators,
canUsePropertyAccess,
CaseBlock,
CaseBlock,
CaseClause,
cast,
CharacterCodes,
ClassElement,
Expand Down Expand Up @@ -44,9 +45,10 @@ import {
createSortedArray,
createTextSpanFromBounds,
createTextSpanFromNode,
createTextSpanFromRange,
Debug,
Declaration,
createTextSpanFromRange,
Debug,
Declaration,
DefaultClause,
Decorator,
Diagnostics,
diagnosticToString,
Expand Down Expand Up @@ -349,12 +351,13 @@ import {
skipAlias,
SnippetKind,
some,
SortedArray,
SourceFile,
SpreadAssignment,
startsWith,
stringToToken,
stripQuotes,
SortedArray,
SourceFile,
SpreadAssignment,
startsWith,
stringToToken,
stripQuotes,
SwitchStatement,
Symbol,
SymbolDisplay,
SymbolDisplayPart,
Expand Down Expand Up @@ -1264,11 +1267,88 @@ function keywordFiltersFromSyntaxKind(keywordCompletion: TokenSyntaxKind): Keywo
}
}

function getOptionalReplacementSpan(location: Node | undefined) {
// StringLiteralLike locations are handled separately in stringCompletions.ts
return location?.kind === SyntaxKind.Identifier ? createTextSpanFromNode(location) : undefined;
}

function getOptionalReplacementSpan(location: Node | undefined) {
// StringLiteralLike locations are handled separately in stringCompletions.ts
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 @@ -1368,17 +1448,22 @@ function completionInfoFromData(
includeSymbol,
);

if (keywordFilters !== KeywordCompletionFilters.None) {
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);
}
}
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);
// 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);
}
}
}

for (const keywordEntry of getContextualKeywords(contextToken, position)) {
Expand Down Expand Up @@ -4812,23 +4897,23 @@ function getCompletionData(
return undefined;
}

function tryGetFunctionLikeBodyCompletionContainer(contextToken: Node): FunctionLikeDeclaration | undefined {
if (contextToken) {
let prev: Node;
const container = findAncestor(contextToken.parent, (node: Node) => {
if (isClassLike(node)) {
return "quit";
}
if (isFunctionLikeDeclaration(node) && prev === node.body) {
return true;
}
prev = node;
return false;
});
return container && container as FunctionLikeDeclaration;
}
}
function tryGetFunctionLikeBodyCompletionContainer(contextToken: Node): FunctionLikeDeclaration | undefined {
if (contextToken) {
let prev: Node;
const container = findAncestor(contextToken.parent, (node: Node) => {
if (isClassLike(node)) {
return "quit";
}
if (isFunctionLikeDeclaration(node) && prev === node.body) {
return true;
}
prev = node;
return false;
});
return container && container as FunctionLikeDeclaration;
}
}

function tryGetContainingJsxElement(contextToken: Node): JsxOpeningLikeElement | undefined {
if (contextToken) {
const parent = contextToken.parent;
Expand Down
125 changes: 125 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,125 @@
/// <reference path="fourslash.ts" />

//// 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*/
//// }

//// // NEW - prioritize case/default (after break on different column)
//// switch (thing) {
//// case 42:
//// break;
//// /*newPrio1*/
//// }

//// // NEW - prioritize case/default (break on same line)
//// switch (thing) {
//// case 42: break;
//// /*newPrio2*/
//// }

//// // NEW - prioritize case/default (after return)
//// switch (thing) {
//// case 42:
//// return 1;
//// /*newPrio3*/
//// }

//// // NEW - prioritize case/default (after throw)
Copy link
Member

Choose a reason for hiding this comment

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

Remove these NEW labels, don't claim what to expect. Instead, describe the positioning of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all "NEW" labels and improved the descriptions to focus on positioning rather than claiming expectations. Comments now describe the context (e.g., "After break statement with proper indentation") instead of predicting behavior. Fixed in 7f7a30e.

//// switch (thing) {
//// case 42:
//// throw new Error();
//// /*newPrio4*/
//// }

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.

Switch this to baselining

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 to baseline testing approach using verify.baselineCompletions() for all markers instead of explicit verify.completions() assertions. The baseline now clearly shows case/default prioritization (sortText "10") in appropriate contexts and normal ordering (sortText "15") elsewhere. Fixed in 7f7a30e.

{
marker: "basic",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.LocalDeclarationPriority },
{ name: "default", sortText: completion.SortText.LocalDeclarationPriority }
]
},
{
marker: "sameAll1",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.GlobalsOrKeywords },
{ name: "default", sortText: completion.SortText.GlobalsOrKeywords }
]
},
{
marker: "sameAll2",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.GlobalsOrKeywords },
{ name: "default", sortText: completion.SortText.GlobalsOrKeywords }
]
},
{
marker: "sameAll3",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.GlobalsOrKeywords },
{ name: "default", sortText: completion.SortText.GlobalsOrKeywords }
]
},
{
marker: "newPrio1",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.LocalDeclarationPriority },
{ name: "default", sortText: completion.SortText.LocalDeclarationPriority }
]
},
{
marker: "newPrio2",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.LocalDeclarationPriority },
{ name: "default", sortText: completion.SortText.LocalDeclarationPriority }
]
},
{
marker: "newPrio3",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.LocalDeclarationPriority },
{ name: "default", sortText: completion.SortText.LocalDeclarationPriority }
]
},
{
marker: "newPrio4",
isNewIdentifierLocation: false,
includes: [
{ name: "case", sortText: completion.SortText.LocalDeclarationPriority },
{ name: "default", sortText: completion.SortText.LocalDeclarationPriority }
]
}
);