-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Improve error messages for potentially missing 'await' #32239
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
Improve error messages for potentially missing 'await' #32239
Conversation
@typescript-bot perf test this please and thank you |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 63dd5bd. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@andrewbranch Here they are:Comparison Report - master..32239
System
Hosts
Scenarios
|
1722bd2
to
6450199
Compare
@typescript-bot test this |
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 6450199. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @andrewbranch, I've started to run the extended test suite on this PR at 6450199. You can monitor the build here. It should now contribute to this PR's status checks. |
Actually, the logic I had was
and I’m updating it to
I think there were a bunch of cases in RWC where |
…type is not promise-like
@typescript-bot test this again please |
Heya @andrewbranch, I've started to run the extended test suite on this PR at bdd8a3e. You can monitor the build here. It should now contribute to this PR's status checks. |
RWC had an error I don't totally understand; the build says "Error: TF401179: An active pull request for the source and target branch already exists" among other things, but the baseline diff went back to no changes. @typescript-bot test this? |
Heya @andrewbranch, I've started to run the extended test suite on this PR at bdd8a3e. You can monitor the build here. It should now contribute to this PR's status checks. |
@weswigham halp, what does this mean? I thought the existing baseline diff PR should be updated 🤔 |
Yeah, just means the diff with master is nil, but master is currently failing. |
This is a partial fix for #30646. The rest of the fix will be enabling some quick fixes triggered by these errors where appropriate (some cases are already covered in #32101).
The conformance test file I added shows one or more instances of every error I added. To summarize them here (operators shown in examples aren’t exhaustive, but the particular types of errors enumerated are):
x++
whenx
is a Promise ofbigint
ornumber
a | b
whena
orb
is a Promise ofbigint
ornumber
a > b
whenawait a > await b
would check successfullya === b
whenawait a === await b
would check successfullyf(x)
when the parameter type inf
is not a Promise andf(await x)
would check successfullyfor (const _ of x)
whenx
is a Promisefor await (const _ of x)
whenx
is a Promisex()
whenx
has no call signatures butawait x
doesnew C()
whenC
has no construct signatures butawait C
doesAlready existed:
x.foo
whenx
is a Promise that resolves to something with afoo
propertyNote: I followed suit with the existing property access error and did not predicate these error messages on being in an
async
context. I think this makes sense, because often times I begin writing a function as non-asyncfunction f()
, and only when I get to the first time I need to await do I go back and addasync
to my function declaration. (I even think enabling a quick fix in a non-async function is ok, as another quick fix to add 'async' will appear directly after. I think having those as two separate steps, but readily available, is a pretty good UX—it's a middle ground where we make extra sure you’re really ok with changing your function to async, but we still fix up your code with very few keystrokes. But, we can dig into that in a future PR.)/cc @bterlson for review as well, if you like.