Include source node inferences in string literal completions

[rbuckton]

This adopts the proposed fix mentioned in #49680 (comment). This is a different approach than the one taken by @Andarist in #52997, as that PR attempted to pre-filter results.

Fixes #49680
Supersedes #52997

[jakebailey]

Code LGTM though I'll leave it to Andrew as to whether or not this is the better approach (I assume it is)

[Andarist]

A couple of my merged PRs related to completions are using a similar "double request" technique. One notable difference is that this PR here always performs the double request whereas those other PRs use the second request as a fallback - only if the first one didn't return anything useful.

Out of curiosity, I wonder:

• if this particular test case would work if "the second request" would be performed conditionally (in other words - is "the first request" returning anything here?)
• can we think of a good reason for this completion type here to always use results from both requests if other completion requests perform 2 requests conditionally?
• how does this PR compare with this PR of mine that implements the "double request technique" around the same getStringLiteralCompletionsFromSignature (that is transitively touched here)?

[jakebailey]

@typescript-bot user test tsserver
@typescript-bot test tsserver top100
@typescript-bot pack this

[typescript-bot]

Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at ef50d45. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at ef50d45. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @jakebailey, I've started to run the tarball bundle task on this PR at ef50d45. You can monitor the build here.

[typescript-bot]

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/153799/artifacts?artifactName=tgz&fileId=D9DBEDB481AEE9C8CA59301474984667DCA709E5AF3258C9390950189FBB5CAB02&fileName=/typescript-5.1.0-insiders.20230503.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

[typescript-bot]

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/54121/merge:

Everything looks good!

[typescript-bot]

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/54121/merge:

Everything looks good!

[Andarist]

I kinda already answered my own questions:

1. the first request returns 'a' | 'b' (although those don't end up being actually suggested to the user since they don't match the string in the argument) - so to satisfy this test case we kinda need the second request since it's that request that returns 'b.c' here
2. I think that it's a valid concern that this PR here always performs the "double request" (unlike Retry querying string completions from the inferred type if the original completions request doesn't return anything #52875 Retry string completions from the inferred type by default #53481 and Improve contextual completions #53554 ). I'm not saying that it's incorrect - it's just inconsistent and might lead to different results based on the position associated with the request. This is actually shown in the expected results here (I created a PR with extra tests that targets this PR here, you can also add Validated completions available for string props are not available for shallow strings #53997 to the issues that this PR here fixes)
3. this is basically answered with 1. I think that my PR might have some nice refactors in it - but that's orthogonal to the issue fixed at hand. My PR doesn't fix the OP's case from with infer in template string, suggestion works in 4.6, but not work in 4.7 #49680 since it's doing the second request conditionally.

I think that there is a way to avoid the second request here by smartly providing any to conditional types (and thus probing both branches of the conditional type). Perhaps that wouldn't be ideal either so maybe "double request at all times" is the way to go here - but I feel like it's not perfect that it only happens here and not in other places.

[Andarist]

nit: other "double requests" are performed outside of the checker - it's the caller's responsibility to "probe" both type of contextual types (with and without inference blocking). Is this a problem? Perhaps that other code should be moved to the checker?

[rbuckton]

• if this particular test case would work if "the second request" would be performed conditionally (in other words - is "the first request" returning anything here?)

1. the first request returns 'a' | 'b' (although those don't end up being actually suggested to the user since they don't match the string in the argument) - so to satisfy this test case we kinda need the second request since it's that request that returns 'b.c' here

Yes, the first request is returning candidates that result in "a" | "b". It's not that they aren't suggested to the user, they are in the suggestions we provide to VS Code. However, it is the editor that is supposed to make the decision whether to filter out suggestions based on what has been typed, not the language service.

• can we think of a good reason for this completion type here to always use results from both requests if other completion requests perform 2 requests conditionally?

This is the simplest solution to the problem. In essence, we want completions that take into account what the user has already typed, but not to the extent that we filter out valid completions such as those that #48410 addressed. We are also considering a more comprehensive solution long term, but that may require more in-depth change to inference and needs further time for consideration and discussion.

[andrewbranch]

Let’s pull in the extra test cases from @Andarist and then I’m good with this.

[Andarist]

@andrewbranch perhaps it would be good to pull this into 5.1? I have some further improvements in mind for this, my tests might very well just get merged as part of those - I'm just waiting for this one to land to start working on a follow-up

[andrewbranch]

5.2*

It’s @rbuckton’s PR to merge whenever he’s ready.

[Andarist]

Since this PR has conflicts and I was working on the code that created them, I resolved them cause I knew exactly how they should be resolved - but since I can't push to this branch I prepared a PR for you here: #54668

[Andarist]

@rbuckton friendly 🏓

[jakebailey]

Is there anything left to do here? It's got conflicts, but it's approved by two people.

[rbuckton]

I'm updating the PR, but it may have been broken by some of the changes in #53996 as it's no longer giving the expected results. I'm working out what the issue is and will post an update once that's been resolved.

[Andarist]

@rbuckton did u check the PR in which i resolved the conflicts ( #54668 )? Iirc i already fixed it there but it has been a while so my memory around that is a little bit foggy

[rbuckton]

This issue appears to be that we no longer skip type argument inference from arguments when checking for string literal completions, and end up inferring never from an argument instead of string from the constraint.

@rbuckton did u check the PR in which i resolved the conflicts ( #54668 )? Iirc i already fixed it there but it has been a while so my memory around that is a little bit foggy

That PR lists 5000+ file changes and is too large for GitHub to render in the UI, so I'm not sure what changes you may have made. At least, that was an immediate difference that I noticed between when this PR was drafted and main. The change is here: https://github.com/microsoft/TypeScript/pull/53996/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8L32511-R32515

[Andarist]

That PR lists 5000+ file changes and is too large for GitHub to render in the UI, so I'm not sure what changes you may have made.

We can compare that branch directly against main (diff) and manually~/visually compare it with the diff visible here. I only focused on test cases and it seems actually they expected results there are slightly different than here.

The only difference is in this expectation. This is basically the same test case that was introduced in #53996 and it could be removed from this PR to avoid duplication.

I understand that perhaps the current expactation from this PR might be considered better but it isn't noticeable for the majority of end users. So maybe it's worth landing this PR without fixing it (as the PR's focus was to fix a different case anyway) and to open an issue about it? The expected results are at least consistent right now between both markers (see here) and I think that the ideal result is that they always should be the same. The only reason why they are different here is that the code path used for object property values completions is quite different from the code path used for string completions directly at argument positions. It's not because they should behave differently - they just happen to behave differently (maybe because it's a design limitation since it might be more complex to do what argument completions do within objects but still).

[rbuckton]

I had to make other changes than what you've illustrated to get the expectations from the tests I added to pass, reverting a number of places where CheckMode.IsForStringLiteralArgumentCompletions was dropped in checker, but I have something that's working now.

[rbuckton]

Or not, it looks like it's still not quite correct.

[Andarist]

this file can be removed since this test already got its way to the main branch:
https://github.com/microsoft/TypeScript/blob/main/tests/cases/fourslash/stringCompletionsFromGenericConditionalTypesUsingTemplateLiteralTypes.ts

[Andarist]

That's interesting! It turns out that for this one test the wildcardType used in place of a blocked string performed better. I had to replace it with blockedStringType just 4 days ago (and my merge didn't include this change), see #55552 .

[Andarist]

I had to make other changes than what you've illustrated to get the expectations from the tests I added to pass, reverting a number of places where CheckMode.IsForStringLiteralArgumentCompletions was dropped in checker, but I have something that's working now.

I managed to get it working again without those reverts. The proposed change also brings back your expected results for the
stringCompletionsFromGenericConditionalTypesUsingTemplateLiteralTypes test case. The changes can be found on my Andarist:get-rid-off-for-string-literal-arg-completions branch which is stacked on top of the simplification from #55624 . It's best to see those changes in isolation though and this is the commit that tweaks the algorithm: Andarist@26c1127

That change is somewhat significant and perhaps bikesheddable so it might be best to leave it for a follow-up PR that I can open once this one lands.

[rbuckton]

@Andarist:

That change is somewhat significant and perhaps bikesheddable so it might be best to leave it for a follow-up PR that I can open once this one lands.

The change in Andarist@26c1127 doesn't seem too complex, but I agree we can tackle it in a follow-on PR.

@andrewbranch can you take a quick look at this again before I merge?

[rbuckton]

@andrewbranch, @jakebailey can one of you take a look over the latest changes?

[rbuckton]

I'll fix the merge conflict with checker shortly

[jakebailey]

Still seems good to me.