TypeScript
TypeScript copied to clipboard
microsoft
Include source node inferences in string literal completions
This adopts the proposed fix mentioned in https://github.com/microsoft/TypeScript/issues/49680#issuecomment-1199933521. 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
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)?
@typescript-bot user test tsserver @typescript-bot test tsserver top100 @typescript-bot pack this
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at ef50d4521d8d33332b2708ed07a8cafdffd93c6c. You can monitor the build here.
Update: The results are in!
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at ef50d4521d8d33332b2708ed07a8cafdffd93c6c. You can monitor the build here.
Update: The results are in!
Heya @jakebailey, I've started to run the tarball bundle task on this PR at ef50d4521d8d33332b2708ed07a8cafdffd93c6c. You can monitor the build here.
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]".;
@jakebailey Here are the results of running the user test suite comparing main and refs/pull/54121/merge:
Everything looks good!
@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/54121/merge:
Everything looks good!
I kinda already answered my own questions:
- 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 - I think that it's a valid concern that this PR here always performs the "double request" (unlike https://github.com/microsoft/TypeScript/pull/52875 https://github.com/microsoft/TypeScript/pull/53481 and https://github.com/microsoft/TypeScript/pull/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 https://github.com/microsoft/TypeScript/issues/53997 to the issues that this PR here fixes)
- 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 https://github.com/microsoft/TypeScript/issues/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.
- 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?)
- 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 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
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: https://github.com/microsoft/TypeScript/pull/54668
Is there anything left to do here? It's got conflicts, but it's approved by two people.
I think I’d rather not merge it two days before the stable release. I’ll merge once main is open to 5.4-bound code if Ron doesn’t first.
Er, 5.2 has already branched off, and the tree is open for 5.3 development; we're in the early stage of the cycle, unless you're worried about our ability to cherry pick to the release branch for 5.2 fixes being impacted?
Are we about to publish 5.2? I thought we were about to publish 5.3. Anyway, if we’ve already branched, we can go 😄
I’ll merge once main is open to 5.4-bound code if Ron doesn’t first
Wait, this is Ron’s PR. I was thinking this was @Andarist’s PR. I’m not going to merge Ron’s PR without Ron. Please ignore everything I’ve said on this thread 🤦
Wait, this is Ron’s PR. I was thinking this was @Andarist’s PR.
Well, it could have been my PR - if this PR would be a code review comment/request for change and not a competing PR to the one that I created earlier. 🤷♂️ I don't want to be snarky - it's just quite frustrating that an approved PR like this "missed the train" for the 5.2 release when it was already approved before 5.1 was even officially published.
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.
@rbuckton did u check the PR in which i resolved the conflicts ( https://github.com/microsoft/TypeScript/pull/54668 )? Iirc i already fixed it there but it has been a while so my memory around that is a little bit foggy
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
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 https://github.com/microsoft/TypeScript/pull/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).
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.
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 https://github.com/microsoft/TypeScript/pull/55552 .
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 https://github.com/microsoft/TypeScript/pull/55624 . It's best to see those changes in isolation though and this is the commit that tweaks the algorithm: https://github.com/Andarist/TypeScript/commit/26c1127bff6bdb213ee2fd6a7eba9f17bc4eebae#
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.
@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 https://github.com/Andarist/TypeScript/commit/26c1127bff6bdb213ee2fd6a7eba9f17bc4eebae 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?