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.

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

refactor: improve string export name completions #58818

Merged
merged 2 commits into from
Aug 26, 2024
+38 −15
Merged

refactor: improve string export name completions #58818

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cecee3
refactor: improve string export name completions
Jack-Works Jun 10, 2024
75fe5fd
chore: apply review
Jack-Works Aug 24, 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
24 changes: 21 additions & 3 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ import {
rangeContainsPosition,
rangeContainsPositionExclusive,
rangeIsOnSingleLine,
scanner,
ScriptElementKind,
ScriptElementKindModifier,
ScriptTarget,
Expand Down Expand Up @@ -1842,9 +1843,16 @@ function createCompletionEntry(
if (parentNamedImportOrExport) {
const languageVersion = getEmitScriptTarget(host.getCompilationSettings());
if (!isIdentifierText(name, languageVersion)) {
insertText = JSON.stringify(name);
insertText = quotePropertyName(sourceFile, preferences, name);

if (parentNamedImportOrExport.kind === SyntaxKind.NamedImports) {
insertText += " as " + generateIdentifierForArbitraryString(name, languageVersion);
// check if it is import { ^here as name } from '...'
// we have to access the scanner here to check if it is { ^here as name } or { ^here, as, name }.
scanner.setText(sourceFile.text);
scanner.resetTokenState(position);
if (!(scanner.scan() === SyntaxKind.AsKeyword && scanner.scan() === SyntaxKind.Identifier)) {
insertText += " as " + generateIdentifierForArbitraryString(name, languageVersion);
}
}
}
else if (parentNamedImportOrExport.kind === SyntaxKind.NamedImports) {
Expand All @@ -1855,6 +1863,13 @@ function createCompletionEntry(
}
}

// TODO: the output code is still invalid (QualifiedName current cannot have form in namespace["invalid text name"] and but is no other way to access it)
// generating namespace["invalid text name"] with a type error is better than generating 'namespace.invalid text name' and have a syntax error.
if (needsConvertPropertyAccess && contextToken?.kind === SyntaxKind.DotToken && contextToken.parent.kind === SyntaxKind.QualifiedName) {
replacementSpan = createTextSpanFromNode(contextToken, sourceFile);
insertText = `[${quotePropertyName(sourceFile, preferences, name)}]`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth doing this if it's guaranteed to be wrong, compared to just not offering this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I hope QualifiedName can contain ["stringLiteral"] in the future, since now a type name may not be an identifier.

Copy link
Member

Choose a reason for hiding this comment

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

What does the error message say when you land in this position? IMO it’s very annoying and confusing for completions to be invalid. It might be ok in this edge case if the diagnostic explains what’s going on and how to fix it, but if it’s just Module 'foo' has no export "non identifier", then I think we need to do better somehow, and I’d lean toward not offering the completion in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code:

type z = x.

Before this PR:

type z = x.a b
// Namespace '...' has no exported member 'a'.
// ';' expected.
// Cannot find name 'b'.

After this PR:

type z = x['a b']
// Cannot use namespace 'x' as a type.

I think the error message becomes better

Copy link
Member

Choose a reason for hiding this comment

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

But is that better than not offering the broken completion at all? I’m not sure.


// TODO(drosen): Right now we just permit *all* semantic meanings when calling
// 'getSymbolKind' which is permissible given that it is backwards compatible; but
// really we should consider passing the meaning for the node so that we don't report
Expand Down Expand Up @@ -4835,7 +4850,10 @@ function getCompletionData(
return !isFromObjectTypeDeclaration(contextToken);

case SyntaxKind.Identifier: {
if (containingNodeKind === SyntaxKind.ImportSpecifier &&
if ((
containingNodeKind === SyntaxKind.ImportSpecifier ||
containingNodeKind === SyntaxKind.ExportSpecifier
) &&
contextToken === (parent as ImportSpecifier).name &&
(contextToken as Identifier).text === "type"
) {
Expand Down
38 changes: 26 additions & 12 deletions tests/cases/fourslash/completionsImportOrExportSpecifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,20 @@
//// export let foo = 1;
//// let someValue = 2;
//// let someType = 3;
//// type someType2 = 4;
//// export {
//// someValue as "__some value",
//// someType as "__some type",
//// type someType2 as "__some type2",
//// };

// @Filename: values.ts
//// import { /*valueImport0*/ } from "./exports";
//// import { /*valueImport1*/ as valueImport1 } from "./exports";
//// import { foo as /*valueImport2*/ } from "./exports";
//// import { foo, /*valueImport3*/ as valueImport3 } from "./exports";
//// import * as _a from "./exports";
//// _a./*namespaceImport1*/;
////
//// export { /*valueExport0*/ } from "./exports";
//// export { /*valueExport1*/ as valueExport1 } from "./exports";
Expand All @@ -25,34 +29,44 @@
//// import { type /*typeImport1*/ as typeImport1 } from "./exports";
//// import { type foo as /*typeImport2*/ } from "./exports";
//// import { type foo, type /*typeImport3*/ as typeImport3 } from "./exports";
//// import * as _a from "./exports";
//// type _b = _a./*namespaceImport2*/;
////
//// export { type /*typeExport0*/ } from "./exports";
//// export { type /*typeExport1*/ as typeExport1 } from "./exports";
//// export { type foo as /*typeExport2*/ } from "./exports";
//// export { type foo, type /*typeExport3*/ } from "./exports";

const __some_type = { name: "__some type", insertText: '"__some type"' }
const __some_type2 = { name: "__some type2", insertText: '"__some type2"' }
const __some_value = { name: "__some value", insertText: '"__some value"' }
const __some_type_as = { name: "__some type", insertText: '"__some type" as __some_type' }
const __some_type_squared = { name: "__some type", insertText: '["__some type"]' }
const __some_type2_as = { name: "__some type2", insertText: '"__some type2" as __some_type2' }
const __some_type2_squared = { name: "__some type2", insertText: '["__some type2"]' }
const __some_value_as = { name: "__some value", insertText: '"__some value" as __some_value' }
const __some_value_squared = { name: "__some value", insertText: '["__some value"]' }
const typeKeyword = { name: "type", sortText: completion.SortText.GlobalsOrKeywords }

verify.completions({ marker: "valueImport0", exact: [__some_type_as, __some_value_as, "foo", typeKeyword] });
verify.completions({ marker: "valueImport1", exact: [__some_type_as, __some_value_as, "foo", typeKeyword] });
verify.completions({ marker: "valueImport0", exact: [__some_type_as, __some_type2_as, __some_value_as, "foo", typeKeyword] });
verify.completions({ marker: "valueImport1", exact: [__some_type, __some_type2, __some_value, "foo", typeKeyword] });
verify.completions({ marker: "valueImport2", exact: [] });
verify.completions({ marker: "valueImport3", exact: [__some_type_as, __some_value_as, typeKeyword] });
verify.completions({ marker: "valueImport3", exact: [__some_type, __some_type2, __some_value, typeKeyword] });
// see https://github.com/microsoft/TypeScript/issues/58815
verify.completions({ marker: 'namespaceImport1', exact: [/* __some_type_squared, __some_value_squared, */ 'foo'] })

verify.completions({ marker: "valueExport0", exact: [__some_type, __some_value, "foo", typeKeyword] });
verify.completions({ marker: "valueExport1", exact: [__some_type, __some_value, "foo", typeKeyword] });
verify.completions({ marker: "valueExport0", exact: [__some_type, __some_type2, __some_value, "foo", typeKeyword] });
verify.completions({ marker: "valueExport1", exact: [__some_type, __some_type2, __some_value, "foo", typeKeyword] });
verify.completions({ marker: "valueExport2", exact: [] });
verify.completions({ marker: "valueExport3", exact: [__some_type, __some_value, typeKeyword] });
verify.completions({ marker: "valueExport3", exact: [__some_type, __some_type2, __some_value, typeKeyword] });

verify.completions({ marker: "typeImport0", exact: [__some_type_as, __some_value_as, "foo"] });
verify.completions({ marker: "typeImport1", exact: [__some_type_as, __some_value_as, "foo"] });
verify.completions({ marker: "typeImport0", exact: [__some_type_as, __some_type2_as, __some_value_as, "foo"] });
verify.completions({ marker: "typeImport1", exact: [__some_type, __some_type2, __some_value, "foo"] });
verify.completions({ marker: "typeImport2", exact: [] });
verify.completions({ marker: "typeImport3", exact: [__some_type_as, __some_value_as] });
verify.completions({ marker: "typeImport3", exact: [__some_type, __some_type2, __some_value] });
verify.completions({ marker: 'namespaceImport2', exact: [__some_type2_squared] })

verify.completions({ marker: "typeExport0", exact: [] });
verify.completions({ marker: "typeExport1", exact: [__some_type, __some_value, "foo"] });
verify.completions({ marker: "typeExport0", exact: [__some_type, __some_type2, __some_value, "foo"] });
verify.completions({ marker: "typeExport1", exact: [__some_type, __some_type2, __some_value, "foo"] });
verify.completions({ marker: "typeExport2", exact: [] });
verify.completions({ marker: "typeExport3", exact: [] });
verify.completions({ marker: "typeExport3", exact: [__some_type, __some_type2, __some_value] });