Skip to content
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.

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

Add quick fix to add missing 'await' #32356

Merged

Conversation

@andrewbranch
Copy link
Member

andrewbranch commented Jul 11, 2019

Fixes #30646

Most of this PR is not terribly interesting, so let me point out the interesting parts:

  1. I’m currently not limiting the quick fix to async functions, based on a couple things: a) our pre-existing usage of the message “Did you forget to use 'await'?” was not predicated on being in an async function, and b), if you add await inside a non-async function, we’ll immediately surface a subsequent code fix to add async to the function, which I predict will be a common and convenient sequence. So right now, I’ve limited the quick fix to appearing when the await would go inside a function/arrow/method body (something that can take async).
  2. When problem-Promise is a variable who has a declaration with an initializer in the same file that is not exported, and that variable is not yet used anywhere else, and the variable does not have a declared type, we offer two code fixes: the first in the list is to add await to the initializer of the variable at the declaration site. The second is the standard insertion of await at the use site, as is available everywhere else. When applying “Fix all expressions possibly missing 'await',” the initializer fix is preferred if available.

add-await

@andrewbranch andrewbranch marked this pull request as ready for review Jul 11, 2019
andrewbranch added 12 commits Jul 9, 2019
… related info
@andrewbranch andrewbranch force-pushed the andrewbranch:enhancement/add-missing-await branch from 87e017e to 355469e Jul 11, 2019
Copy link
Member

sandersn left a comment

Code looks good, but it looks like this fix shows for any incorrect addition, like number + HTMLElement, and then proceeds to do nothing when invoked. Did you think about making specific errors for + et al when one of the sides is a Promise? That would make the codefix suggestion quite a bit more accurate. What do you think?

src/harness/fourslash.ts Outdated Show resolved Hide resolved
const errorCodes = [
Diagnostics.An_arithmetic_operand_must_be_of_type_any_number_bigint_or_an_enum_type.code,
Diagnostics.The_left_hand_side_of_an_arithmetic_operation_must_be_of_type_any_number_bigint_or_an_enum_type.code,
Diagnostics.The_right_hand_side_of_an_arithmetic_operation_must_be_of_type_any_number_bigint_or_an_enum_type.code,

This comment has been minimized.

Copy link
@sandersn

sandersn Jul 11, 2019

Member

This will show for any old type combination, like number + Event, right? What happens in that case? Does it just fail to do anything when you try to "add missing await"?

This comment has been minimized.

Copy link
@andrewbranch

andrewbranch Jul 11, 2019

Author Member

It's supposed to be filtered out further in getCodeActions (it doesn’t show anything if you return undefined / empty array in there). This is back to the tradeoffs we were discussing about using related info as a “bonus error message” vs. copying and pasting all these into new, unique messages. I wrote this down as a larger LS design/API change to consider at some later point.

@andrewbranch

This comment has been minimized.

Copy link
Member Author

andrewbranch commented Jul 11, 2019

Code looks good, but it looks like this fix shows for any incorrect addition, like number + HTMLElement, and then proceeds to do nothing when invoked. Did you think about making specific errors for + et al when one of the sides is a Promise? That would make the codefix suggestion quite a bit more accurate. What do you think?

Hum, yes, I did think about it, but it should be handled by isMissingAwaitError. Something is amiss. (Related, I had a discussion with Wes in Teams about how VS Code doesn’t request code fixes for related info error codes, which would make this much cleaner. I’m thinking about PRing them to suggest a change there.)

andrewbranch and others added 2 commits Jul 11, 2019
Co-Authored-By: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
…s a Promise
@andrewbranch

This comment has been minimized.

Copy link
Member Author

andrewbranch commented Jul 11, 2019

but it looks like this fix shows for any incorrect addition, like number + HTMLElement

Ah, I thought you were saying you observed this happening, but I think you were just looking at the error codes? I believe it’s working correctly, but I just pushed a test for that negative case specifically. It’s a good thing to assert in the tests.

I gave some more context about what’s happening and why I took this approach in the inline comments, but TL;DR, the error codes are necessary but not sufficient to surface a quick fix. Once getCodeActions is invoked with a possible candidate, it correlates the error with the related message Did you forget to use 'await'? that the checker added via semantic knowledge that there’s a strong chance await is appropriate (#32239). If it doesn’t find that, it bails, and a code fix doesn’t show.

@sandersn

This comment has been minimized.

Copy link
Member

sandersn commented Jul 11, 2019

Nope, I didn't run it, just read the code. I think I was thinking of the refactor API instead of the codefix API? IIRC the refactor API is the one with two calls, and the first one has to be fast because it runs a lot.

@andrewbranch andrewbranch merged commit 71bec5b into microsoft:master Jul 12, 2019
8 checks passed
8 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
node10 Build #36108 succeeded
Details
node10 (Test) Test succeeded
Details
node12 Build #36106 succeeded
Details
node12 (Test) Test succeeded
Details
node8 Build #36107 succeeded
Details
node8 (Test) Test succeeded
Details
@andrewbranch andrewbranch deleted the andrewbranch:enhancement/add-missing-await branch Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.