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

Don’t offer import statement completions at from position #44125

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 38 additions & 9 deletions src/services/completions.ts
Expand Up @@ -174,6 +174,8 @@ namespace ts.Completions {
return jsdocCompletionInfo(JsDoc.getJSDocTagCompletions());
case CompletionDataKind.JsDocParameterName:
return jsdocCompletionInfo(JsDoc.getJSDocParameterNameCompletions(completionData.tag));
case CompletionDataKind.Keywords:
return specificKeywordCompletionInfo(completionData.keywords);
default:
return Debug.assertNever(completionData);
}
Expand All @@ -183,6 +185,20 @@ namespace ts.Completions {
return { isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: false, entries };
}

function specificKeywordCompletionInfo(keywords: readonly SyntaxKind[]): CompletionInfo {
Copy link
Member Author

Choose a reason for hiding this comment

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

Making a way to return specific keywords is extra credit beyond just fixing the obvious regression; the behavior in 4.2 was not great:

image

return {
isGlobalCompletion: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried making this false but the snippets for for loops and stuff still showed up 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Should it be false then?

@mjbvz any idea why that might be happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

Set to false now despite seeing no difference locally

isMemberCompletion: false,
isNewIdentifierLocation: false,
entries: keywords.map(k => ({
name: tokenToString(k)!,
kind: ScriptElementKind.keyword,
kindModifiers: ScriptElementKindModifier.none,
sortText: SortText.GlobalsOrKeywords,
})),
};
}

function getOptionalReplacementSpan(location: Node | undefined) {
// StringLiteralLike locations are handled separately in stringCompletions.ts
return location?.kind === SyntaxKind.Identifier ? createTextSpanFromNode(location) : undefined;
Expand Down Expand Up @@ -802,6 +818,8 @@ namespace ts.Completions {
return JsDoc.getJSDocTagCompletionDetails(name);
case CompletionDataKind.JsDocParameterName:
return JsDoc.getJSDocParameterNameCompletionDetails(name);
case CompletionDataKind.Keywords:
return request.keywords.some(k => tokenToString(k) === name) ? createSimpleDetails(name, ScriptElementKind.keyword, SymbolDisplayPartKind.keyword) : undefined;
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
default:
return Debug.assertNever(request);
}
Expand Down Expand Up @@ -893,7 +911,7 @@ namespace ts.Completions {
return completion.type === "symbol" ? completion.symbol : undefined;
}

const enum CompletionDataKind { Data, JsDocTagName, JsDocTag, JsDocParameterName }
const enum CompletionDataKind { Data, JsDocTagName, JsDocTag, JsDocParameterName, Keywords }
/** true: after the `=` sign but no identifier has been typed yet. Else is the Identifier after the initializer. */
type IsJsxInitializer = boolean | Identifier;
interface CompletionData {
Expand All @@ -918,7 +936,10 @@ namespace ts.Completions {
readonly isJsxIdentifierExpected: boolean;
readonly importCompletionNode?: Node;
}
type Request = { readonly kind: CompletionDataKind.JsDocTagName | CompletionDataKind.JsDocTag } | { readonly kind: CompletionDataKind.JsDocParameterName, tag: JSDocParameterTag };
type Request =
| { readonly kind: CompletionDataKind.JsDocTagName | CompletionDataKind.JsDocTag }
| { readonly kind: CompletionDataKind.JsDocParameterName, tag: JSDocParameterTag }
| { readonly kind: CompletionDataKind.Keywords, keywords: readonly SyntaxKind[] };

export const enum CompletionKind {
ObjectPropertyDeclaration,
Expand Down Expand Up @@ -1101,13 +1122,17 @@ namespace ts.Completions {
let location = getTouchingPropertyName(sourceFile, position);

if (contextToken) {
const importCompletionCandidate = getImportCompletionNode(contextToken);
if (importCompletionCandidate === SyntaxKind.FromKeyword) {
return { kind: CompletionDataKind.Keywords, keywords: [SyntaxKind.FromKeyword] };
}
// Import statement completions use `insertText`, and also require the `data` property of `CompletionEntryIdentifier`
// added in TypeScript 4.3 to be sent back from the client during `getCompletionEntryDetails`. Since this feature
// is not backward compatible with older clients, the language service defaults to disabling it, allowing newer clients
// to opt in with the `includeCompletionsForImportStatements` user preference.
importCompletionNode = preferences.includeCompletionsForImportStatements && preferences.includeCompletionsWithInsertText
? getImportCompletionNode(contextToken)
: undefined;
if (importCompletionCandidate && preferences.includeCompletionsForImportStatements && preferences.includeCompletionsWithInsertText) {
importCompletionNode = importCompletionCandidate;
}
// Bail out if this is a known invalid completion location
if (!importCompletionNode && isCompletionListBlocker(contextToken)) {
log("Returning an empty list because completion was requested in an invalid position.");
Expand Down Expand Up @@ -3041,17 +3066,21 @@ namespace ts.Completions {

function getImportCompletionNode(contextToken: Node) {
const candidate = getCandidate();
return candidate && rangeIsOnSingleLine(candidate, candidate.getSourceFile()) ? candidate : undefined;
return candidate === SyntaxKind.FromKeyword || candidate && rangeIsOnSingleLine(candidate, candidate.getSourceFile()) ? candidate : undefined;

function getCandidate() {
const parent = contextToken.parent;
if (isImportEqualsDeclaration(parent)) {
return isModuleSpecifierMissingOrEmpty(parent.moduleReference) ? parent : undefined;
}
if (isNamedImports(parent) || isNamespaceImport(parent)) {
return isModuleSpecifierMissingOrEmpty(parent.parent.parent.moduleSpecifier) && (isNamespaceImport(parent) || parent.elements.length < 2) && !parent.parent.name
? parent.parent.parent
: undefined;
if (isModuleSpecifierMissingOrEmpty(parent.parent.parent.moduleSpecifier) && (isNamespaceImport(parent) || parent.elements.length < 2) && !parent.parent.name) {
// At `import { ... } |` or `import * as Foo |`, the only possible completion is `from`
return contextToken.kind === SyntaxKind.CloseBraceToken || contextToken.kind === SyntaxKind.Identifier
? SyntaxKind.FromKeyword
: parent.parent.parent;
}
return undefined;
}
if (isImportKeyword(contextToken) && isSourceFile(parent)) {
// A lone import keyword with nothing following it does not parse as a statement at all
Expand Down
26 changes: 26 additions & 0 deletions tests/cases/fourslash/importStatementCompletions1.ts
Expand Up @@ -73,3 +73,29 @@
}
});
});

// @Filename: /index13.ts
//// import {} /*13*/

// @Filename: /index14.ts
//// import {} f/*14*/

// @Filename: /index15.ts
//// import * as foo /*15*/

// @Filename: /index16.ts
//// import * as foo f/*16*/

[13, 14, 15, 16].forEach(marker => {
verify.completions({
marker: "" + marker,
exact: {
name: "from",
sortText: completion.SortText.GlobalsOrKeywords,
},
preferences: {
includeCompletionsForImportStatements: true,
includeInsertTextCompletions: true,
}
});
});