Skip to content

Include source node inferences in string literal completions deeper in arguments #56182

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+169 −9
Open

Include source node inferences in string literal completions deeper in arguments #56182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
183578b
Include source node inferences in string literal completions deeper i…
Andarist Oct 23, 2023
fa9a5f5
Merge remote-tracking branch 'origin/main' into fix/string-completion…
Andarist Dec 26, 2023
1614651
Merge remote-tracking branch 'origin/main' into fix/string-completion…
Andarist Apr 28, 2024
f98b92b
Merge remote-tracking branch 'origin/main' into fix/string-completion…
Andarist Aug 27, 2024
6152848
move `NewExpression` case to the checker
Andarist Aug 29, 2024
5daf0c0
exclude iffes from `getContextualType`
Andarist Aug 29, 2024
9422555
move `BinaryExpression` case to the checker too
Andarist Aug 29, 2024
39ddd44
move logic back to services
Andarist Aug 29, 2024
dc19f47
Merge remote-tracking branch 'origin/main' into fix/string-completion…
Andarist Aug 29, 2024
6adcd82
fmt
Andarist Aug 29, 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
4 changes: 2 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22056,7 +22056,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const [sourceType, targetType] = getTypeNamesForErrorDisplay(source, target);
let generalizedSource = source;
let generalizedSourceType = sourceType;

// Don't generalize on 'never' - we really want the original type
// to be displayed for use-cases like 'assertNever'.
if (!(target.flags & TypeFlags.Never) && isLiteralType(source) && !typeCouldHaveTopLevelSingletonTypes(target)) {
Expand Down Expand Up @@ -32130,7 +32130,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return getContextualTypeForAwaitOperand(parent as AwaitExpression, contextFlags);
case SyntaxKind.CallExpression:
case SyntaxKind.NewExpression:
return getContextualTypeForArgument(parent as CallExpression | NewExpression | Decorator, node);
return getContextualTypeForArgument(parent as CallExpression | NewExpression, node);
case SyntaxKind.Decorator:
return getContextualTypeForDecorator(parent as Decorator);
case SyntaxKind.TypeAssertionExpression:
Expand Down
22 changes: 16 additions & 6 deletions src/services/stringCompletions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
CompletionEntry,
CompletionEntryDetails,
CompletionInfo,
concatenate,
contains,
containsPath,
ContextFlags,
Expand Down Expand Up @@ -83,6 +84,7 @@ import {
isApplicableVersionedTypesKey,
isArray,
isCallExpression,
isCallLikeExpression,
isIdentifier,
isIdentifierText,
isImportCall,
Expand Down Expand Up @@ -411,7 +413,15 @@ function getStringLiteralCompletionEntries(sourceFile: SourceFile, node: StringL
// });
return stringLiteralCompletionsForObjectLiteral(typeChecker, parent.parent);
}
return fromContextualType() || fromContextualType(ContextFlags.None);
if (findAncestor(parent.parent, isCallLikeExpression)) {
const uniques = new Map<string, true>();
Copy link
Member

Choose a reason for hiding this comment

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

Nothing you did (this is preexisting), but it sure does make me sad to see Map<T, true> in 2024 😄

const stringLiteralTypes = concatenate(
getStringLiteralTypes(typeChecker.getContextualType(node, ContextFlags.None), uniques),
getStringLiteralTypes(typeChecker.getContextualType(node, ContextFlags.Completions), uniques),
);
return toStringLiteralCompletionsFromTypes(stringLiteralTypes);
}
return fromContextualType(ContextFlags.None);

case SyntaxKind.ElementAccessExpression: {
const { expression, argumentExpression } = parent as ElementAccessExpression;
Expand Down Expand Up @@ -521,14 +531,14 @@ function getStringLiteralCompletionEntries(sourceFile: SourceFile, node: StringL
function fromContextualType(contextFlags: ContextFlags = ContextFlags.Completions): StringLiteralCompletionsFromTypes | undefined {
// Get completion for string literal from string literal type
// i.e. var x: "hi" | "hello" = "/*completion position*/"
const types = getStringLiteralTypes(getContextualTypeFromParent(node, typeChecker, contextFlags));
if (!types.length) {
return;
}
return { kind: StringLiteralCompletionKind.Types, types, isNewIdentifier: false };
return toStringLiteralCompletionsFromTypes(getStringLiteralTypes(getContextualTypeFromParent(node, typeChecker, contextFlags)));
}
}

function toStringLiteralCompletionsFromTypes(types: readonly StringLiteralType[]): StringLiteralCompletionsFromTypes | undefined {
return types.length ? { kind: StringLiteralCompletionKind.Types, types, isNewIdentifier: false } : undefined;
}

function walkUpParentheses(node: Node) {
switch (node.kind) {
case SyntaxKind.ParenthesizedType:
Expand Down
35 changes: 35 additions & 0 deletions tests/baselines/reference/generatorTypeCheck64.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//// [tests/cases/conformance/es6/yieldExpressions/generatorTypeCheck64.ts] ////

=== generatorTypeCheck64.ts ===
function* g3(): Generator<Generator<(x: string) => number>> {
>g3 : Symbol(g3, Decl(generatorTypeCheck64.ts, 0, 0))
>Generator : Symbol(Generator, Decl(lib.es2015.generator.d.ts, --, --))
>Generator : Symbol(Generator, Decl(lib.es2015.generator.d.ts, --, --))
>x : Symbol(x, Decl(generatorTypeCheck64.ts, 0, 37))

yield function* () {
yield x => x.length;
>x : Symbol(x, Decl(generatorTypeCheck64.ts, 2, 13))
>x.length : Symbol(String.length, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(generatorTypeCheck64.ts, 2, 13))
>length : Symbol(String.length, Decl(lib.es5.d.ts, --, --))

} ()
}

function* g4(): Iterator<Iterable<(x: string) => number>> {
>g4 : Symbol(g4, Decl(generatorTypeCheck64.ts, 4, 1))
>Iterator : Symbol(Iterator, Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.esnext.iterator.d.ts, --, --))
>Iterable : Symbol(Iterable, Decl(lib.es2015.iterable.d.ts, --, --))
>x : Symbol(x, Decl(generatorTypeCheck64.ts, 6, 35))

yield (function* () {
yield (x) => x.length;
>x : Symbol(x, Decl(generatorTypeCheck64.ts, 8, 11))
>x.length : Symbol(String.length, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(generatorTypeCheck64.ts, 8, 11))
>length : Symbol(String.length, Decl(lib.es5.d.ts, --, --))

})();
}

67 changes: 67 additions & 0 deletions tests/baselines/reference/generatorTypeCheck64.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
//// [tests/cases/conformance/es6/yieldExpressions/generatorTypeCheck64.ts] ////

=== Performance Stats ===
Type Count: 1,000
Instantiation count: 2,500

=== generatorTypeCheck64.ts ===
function* g3(): Generator<Generator<(x: string) => number>> {
>g3 : () => Generator<Generator<(x: string) => number>>
> : ^^^^^^
>x : string
> : ^^^^^^

yield function* () {
>yield function* () { yield x => x.length; } () : any
>function* () { yield x => x.length; } () : Generator<(x: string) => number, void, any>
> : ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>function* () { yield x => x.length; } : () => Generator<(x: string) => number, void, any>
> : ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

yield x => x.length;
>yield x => x.length : any
>x => x.length : (x: string) => number
> : ^ ^^^^^^^^^^^^^^^^^^^
>x : string
> : ^^^^^^
>x.length : number
> : ^^^^^^
>x : string
> : ^^^^^^
>length : number
> : ^^^^^^

} ()
}

function* g4(): Iterator<Iterable<(x: string) => number>> {
>g4 : () => Iterator<Iterable<(x: string) => number>>
> : ^^^^^^
>x : string
> : ^^^^^^

yield (function* () {
>yield (function* () { yield (x) => x.length; })() : any
>(function* () { yield (x) => x.length; })() : Generator<(x: string) => number, void, any>
> : ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>(function* () { yield (x) => x.length; }) : () => Generator<(x: string) => number, void, any>
> : ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>function* () { yield (x) => x.length; } : () => Generator<(x: string) => number, void, any>
> : ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

yield (x) => x.length;
>yield (x) => x.length : any
>(x) => x.length : (x: string) => number
> : ^ ^^^^^^^^^^^^^^^^^^^
>x : string
> : ^^^^^^
>x.length : number
> : ^^^^^^
>x : string
> : ^^^^^^
>length : number
> : ^^^^^^

})();
}

15 changes: 15 additions & 0 deletions tests/cases/conformance/es6/yieldExpressions/generatorTypeCheck64.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// @strict: true
// @target: esnext
// @noEmit: true

function* g3(): Generator<Generator<(x: string) => number>> {
yield function* () {
yield x => x.length;
} ()
}

function* g4(): Iterator<Iterable<(x: string) => number>> {
yield (function* () {
yield (x) => x.length;
})();
}
33 changes: 33 additions & 0 deletions ...es/fourslash/stringLiteralCompletionsWithinInferredObjectWhenItsKeysAreUsedOutsideOfIt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/// <reference path="fourslash.ts" />

// @strict: true

//// declare function createMachine<T>(config: {
//// initial: keyof T;
//// states: {
//// [K in keyof T]: {
//// on?: Record<string, keyof T>;
//// };
//// };
//// }): void;
////
//// createMachine({
//// initial: "a",
//// states: {
//// a: {
//// on: {
//// NEXT: "/*1*/",
//// },
//// },
//// b: {
//// on: {
//// NEXT: "/*2*/",
//// },
//// },
//// },
//// });

verify.completions({
marker: ["1", "2"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't work for marker 1 because the completions type took priority over the regular contextual type. With inference blocked and all the T was inferred here as { a: any } based on the initial: keyof T. That result "locked" the string literal completions to that blocked result whereas the intended/better completions can be returned here based on the contextual type (when the inferred type of T is { a: unknown; b: unknown; }).

Note that this PR doesn't improve the very same situation for regular (non-string) completions - when we have a node with ThisNodeHasError flag. Improvements to that can be explored separately.

exact: ["a", "b"]
})
2 changes: 1 addition & 1 deletion tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

goTo.marker("1");
edit.insert(`x`)
verify.completions({ exact: ["MORNING", "LUNCH_TIME", "ALOHA"] });
verify.completions({ exact: ["ALOHAx", "MORNING", "LUNCH_TIME", "ALOHA"] });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ALOHAx is not a "valid" completion here but the IDE won't list it anyway since it's the literal value of what is the current value of the string literal node for which the completion is requested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, actually... it doesn't show it when typing (the completions popup goes away when my input goes outside of the valid values) but it does show up when requested explicitly with cmd+space.

I still feel like this is an improvement though as the invalid value can be somewhat easily ignored by the user. Maybe this is something that could be improved on the VS Code side though unless this is the intended behavior here

Copy link
Member

Choose a reason for hiding this comment

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

This kind of makes sense since it is actively changing the inference to TEvent in raise, but the experience is not really desirable. I think VS Code showing you completion items that fully match your current token text is intentional and desirable in general, though. The reason this completion entry is bad is that the token being completed is "self-fulfilling," i.e. that text/type doesn't appear anywhere else. I fixed property assignments like that a while ago, so maybe a similar strategy could be used for string literals. I don't think it's a blocker for this PR, but I think it's worth investigating. #35709

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, on second thought, if you change the token text to a substring of one of the valid options, like ALO/**/, it's going to be really annoying if the visible completions in VS Code are ALO first and ALOHA second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, on second thought, if you change the token text to a substring of one of the valid options, like ALO/**/, it's going to be really annoying if the visible completions in VS Code are ALO first and ALOHA second.

This is not a new problem so I think it shouldn't be a blocker here (TS playground):

type GreetingEvent =
  | "MORNING"
  | "LUNCH_TIME"
  | "ALOHA";

interface RaiseActionObject<TEvent> {
  type: "raise";
  event: TEvent;
}

declare function raise<TEvent>(
  ev: TEvent
): RaiseActionObject<TEvent>;

declare function createMachine<TEvent>(config: {
  actions: RaiseActionObject<TEvent>;
}): void;

createMachine<GreetingEvent>({
  actions: raise("ALO"),
});

We can get those completions here:
completions popup showing both ALO and ALOHA

All this PR does is reusing the existing algorithm in a new code path.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I see that in the playground, but the diff in the fourslash test makes it look like a new problem, doesn’t it? What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cases in the TS playground above and in the added fourslash test are different (and yet the same :P).

Strings used directly at argument positions and somewhere deeper within arguments (like as property values) hit 2 different codepaths. So what we observe on the TS playground above is a result of that other codepath and what this PR does is to port some logic from that other codepath to the other place. So now this fourslash test starts to behave like the TS playground above.

verify.getSemanticDiagnostics([{
code: 2322,
message: `Type 'RaiseActionObject<{ type: "ALOHAx"; }>' is not assignable to type 'RaiseActionObject<GreetingEvent>'.\n Type '{ type: "ALOHAx"; }' is not assignable to type 'GreetingEvent'.\n Type '{ type: "ALOHAx"; }' is not assignable to type '{ type: "ALOHA"; }'.\n Types of property 'type' are incompatible.\n Type '"ALOHAx"' is not assignable to type '"ALOHA"'.`,
Expand Down