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

Andarist · 2023-10-23T09:34:20Z

this is an expansion of the approached used by #54121

typescript-bot · 2023-10-23T09:34:29Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

src/compiler/checker.ts

Andarist · 2023-10-23T09:39:57Z

tests/cases/fourslash/typeErrorAfterStringCompletionsInNestedCall.ts

@@ -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"] });


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

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

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

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.

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:

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

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?

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.

Andarist · 2023-10-23T09:54:10Z

...cases/fourslash/stringLiteralCompletionsWithinInferredObjectWhenItsKeysAreUsedOutsideOfIt.ts

+//// });
+
+verify.completions({
+    marker: ["1", "2"],


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.

…n arguments

…s-include-source-inferences

…s-include-source-inferences # Conflicts: # src/compiler/utilities.ts

Andarist · 2024-04-28T16:11:51Z

@andrewbranch @sandersn is there anything blocking this from being merged? :)

sandersn

The method and results seem decent. I don't really like the code organization -- although there may be a reason for it that I'm missing.

sandersn · 2024-05-08T16:59:49Z

src/compiler/utilities.ts

+    const parent = walkUpParenthesizedExpressions(node.parent);
+    switch (parent.kind) {
+        case SyntaxKind.NewExpression:
+            return checker.getContextualType(parent as NewExpression, contextFlags);


using the checker like this implies that this utility should be in services. Does it also rely on some compiler internals? I looked and I don't see it.

sandersn · 2024-05-08T17:01:44Z

src/compiler/utilities.ts

+                : checker.getContextualType(node, contextFlags);
+        }
+        case SyntaxKind.CaseClause:
+            return checker.getTypeAtLocation((parent as CaseClause).parent.parent.expression);


getContextualTypeFromParent reads like an enhancement of checker.getContextualType. Are the new/case/binaryExpression additions not generally applicable? If not, what conditions are they relevant for? If so, should they be added to getContextualType in the checker?

It can't be moved to the checker. Those are specific to services and, for example, power renames in situations like:

type Foo = "a" | "b"; class C { p: Foo = "a"; m() { if (this.p === "a") {} } }

When u rename "a" to some other string that rename carries over to those other locations. But outside of situations like this the left/right operand isn't meant to contextually type the other one when equality comparator is involved.

Similarly, usually identifiers are not contextually-typed but this logic in services provides contextual typing for them.

src/compiler/checker.ts

sandersn · 2024-05-08T17:51:04Z

src/compiler/checker.ts

+        const seen = new Map<string, true>();
+
+        return [
+            ...getStringLiteralTypes(getContextualTypeFromParent(expression, checker, ContextFlags.None), seen),


passing checker into a function from inside the checker just seems quite strange. could getStringLiteralTypes be the part in the checker, and getContextualStringLiteralCompletionTypes and getContextualTypeFromParent be in services?

…s-include-source-inferences

Andarist · 2024-08-29T21:02:04Z

The method and results seem decent. I don't really like the code organization -- although there may be a reason for it that I'm missing.

@sandersn I addressed this by moving the logic closer to services. I agree that the previous shape of the code wasn't the best - thanks for pointing it out 👍

…s-include-source-inferences

jakebailey · 2024-10-31T19:27:03Z

src/services/stringCompletions.ts

@@ -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>();


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

Andarist · 2025-02-23T09:35:37Z

@jakebailey with 2 approvals, could this land now? :)

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 23, 2023

Andarist commented Oct 23, 2023

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

Andarist commented Oct 23, 2023

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

Andarist commented Oct 23, 2023

View reviewed changes

Include source node inferences in string literal completions deeper i…

183578b

…n arguments

Andarist force-pushed the fix/string-completions-include-source-inferences branch from bd9e711 to 183578b Compare October 23, 2023 09:55

sandersn requested review from andrewbranch and rbuckton November 2, 2023 21:55

sandersn assigned rbuckton Nov 2, 2023

Merge remote-tracking branch 'origin/main' into fix/string-completion…

fa9a5f5

…s-include-source-inferences

andrewbranch previously approved these changes Jan 2, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/string-completion…

1614651

…s-include-source-inferences # Conflicts: # src/compiler/utilities.ts

sandersn requested changes May 8, 2024

View reviewed changes

Andarist added 5 commits August 27, 2024 09:50

Merge remote-tracking branch 'origin/main' into fix/string-completion…

f98b92b

…s-include-source-inferences

move NewExpression case to the checker

6152848

exclude iffes from getContextualType

5daf0c0

move BinaryExpression case to the checker too

9422555

move logic back to services

39ddd44

Andarist force-pushed the fix/string-completions-include-source-inferences branch 2 times, most recently from 49fd25c to 39ddd44 Compare August 29, 2024 21:00

Andarist requested a review from sandersn August 29, 2024 21:02

Andarist added 2 commits August 29, 2024 23:03

Merge remote-tracking branch 'origin/main' into fix/string-completion…

dc19f47

…s-include-source-inferences

fmt

6adcd82

jakebailey approved these changes Oct 31, 2024

View reviewed changes

Andarist mentioned this pull request Feb 23, 2025

Unexpected Auto-Completion Behavior in Type Inference #61247

Closed

Andarist dismissed andrewbranch’s stale review via 6adcd82 March 31, 2025 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Andarist commented Oct 23, 2023 •

edited

Loading

typescript-bot commented Oct 23, 2023

Andarist Oct 23, 2023

Andarist Oct 23, 2023

andrewbranch May 1, 2024

andrewbranch May 1, 2024

Andarist May 2, 2024

andrewbranch May 2, 2024

Andarist May 2, 2024

Andarist Oct 23, 2023

Andarist commented Apr 28, 2024

sandersn left a comment

sandersn May 8, 2024

sandersn May 8, 2024

Andarist Aug 29, 2024

sandersn May 8, 2024

Andarist commented Aug 29, 2024

jakebailey Oct 31, 2024

Andarist commented Feb 23, 2025

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

Are you sure you want to change the base?

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

Conversation

Andarist commented Oct 23, 2023 • edited Loading

typescript-bot commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Apr 28, 2024

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Aug 29, 2024

Choose a reason for hiding this comment

Andarist commented Feb 23, 2025

Andarist commented Oct 23, 2023 •

edited

Loading