add completion for promise context #32101
Conversation
There was a problem hiding this comment.
My suspicion is that most of the time when these completions appear, my desired fix is like this:
async function fetchThings(fetchOptions: RequestInfo): Thing[] {
const rawJson = (await fetch(fetchOptions)).json(); // accidentally a Promise<any>
return deserializeThingsResponse(rawJson);
}
// should become
async function fetchThings(fetchOptions: RequestInfo): Thing[] {
const rawJson = await (await fetch(fetchOptions)).json();
return deserializeThingsResponse(rawJson);
}If I have a variable declaration a) without a declared type, b) inside an async function, c) whose resolved type is a Promise, and d) I never successfully access any members of that Promise, then chances are, I meant to use await at the declaration site, not the use site.
Of course, that’s more semantic criteria to check. It also increases the likelihood that the codefix will take place offscreen. Let me see if I can try to find some data on how frequently we see parenthesized await expressions vs. as the initializer of a variable declaration.
|
Ok, so in our user tests (123,359 TS/JS/TSX/JSX files), I looked for the forms:
The results were:
This isn't a huge sample size (of files that actually contain |
|
That should be a quick fix and trigger by type mismatch. I‘m not sure but I think you cannot find too many property access is caused people always write the following code rather than parens and parens： async function fetchThings(fetchOptions: RequestInfo): Thing[] {
const resp = await fetch(fetchOptions)
const rawJson = await resp.json();
return deserializeThingsResponse(rawJson);
} |
|
Sure, maybe my example wasn’t very good. But I think there will generally be cases like declare function someAsyncThing(): Promise<{ foo: string }>;
async function whatever() {
const x = someAsyncThing(); // meant to await
x./*|*/
}and if we complete |
|
It's also a bit weird that we request autocomplete here and the leading code has been modified.
And I'm not sure that is acceptable what request find all reference in autocomplete? |
I’m working on something similar for the codefix scenario:
Over the next couple days I’ll be testing this out and seeing how the performance is. We don’t need to do quite a full find-all-references, and the scope should be limited to the source file at largest. I think we should get this merged, and if it seems reasonable to add the initializer await, I can make it pull in the same code I’m working on now. Have you addressed Sheetal’s feedback? |
Yeah, it doesn’t work for me either—I stepped through on the VS Code side and confirmed the response from TSServer looks good, and I couldn’t find where it was getting filtered out. It seems like it makes it all the way out of the typescript-language-features extension. Will likely need some help from the VS Code folks to figure this one out, but I don’t think it’s a problem for this PR. 👇 stupid MacBook trackpad clicked the button while I was typing 😑 |
src/services/completions.ts
Outdated
| @@ -1068,10 +1068,12 @@ namespace ts.Completions { | |||
| } | |||
|
|
|||
| function addSymbol (symbol: Symbol) { | |||
| if (insertAwait && !symbolToOriginInfoMap[getSymbolId(symbol)]) { | |||
| symbolToOriginInfoMap[getSymbolId(symbol)] = { kind: SymbolOriginInfoKind.Promise }; | |||
| if (preferences.includeCompletionsWithInsertText) { | |||
There was a problem hiding this comment.
Shouldn't this be something like if (!insertAwait || preferences.includeCompletionsWithInsertText)? Seems like addSymbol would now reject all completions if the preference is disabled 🙃
|
I still felt it's hard to get an await-missing expression when you are coding. |
|
Looks like just a lint error now. |
|
Hope I have not delay your release. |
src/services/completions.ts
Outdated
| } | ||
| function getCompletionEntryDisplayNameForSymbol( | ||
| symbol: Symbol, | ||
| target: ScriptTarget, | ||
| origin: SymbolOriginInfo | undefined, | ||
| kind: CompletionKind, | ||
| ): CompletionEntryDisplayNameForSymbol | undefined { | ||
| const needsConvertAwait = !!(origin && originIsPromise(origin)); |
There was a problem hiding this comment.
I don't think this needs to be passed around since this check can easily be performed as usage sites of needsConvertAwait
* add completion for promise context * check insert text inside add symbol helper * fix incorrect branch * avoid completions with includeCompletionsWithInsertText perferences * avoid useless parameter
Fixes #31450
This PR added a new
SymbolOriginInfofor Promise and create await oncreateCompletionEntryif the node is inAwaitContext.I'm not sure the current insertText behavior is totally correct, and as the result of
completionOfAwaitPromise3.ts, everything seems ok, but I cannot do it in my local vscodeunsupported:
asyncmodifier to container function