Closed
Description
π Search Terms
- missing await in conditional
- await
- conditional
π Version & Regression Information
Since 4.3.0-dev
β― Playground Link
Playground link with relevant code
π» Code
declare const cache: Record<string, Promise<string>>
declare const fetchThing: (id: string) => Promise<string>
const getCachedThing1 = (id: string) => {
let thing = cache[id]
return !thing // not an error, if the error in getCachedThing3 is valid then why this isn't an error?
? (cache[id] = fetchThing(id))
: thing
}
const getCachedThing2 = (id: string) => {
let thing = cache[id]
return thing ?? (cache[id] = fetchThing(id)) // not an error
}
const getCachedThing3 = (id: string) => {
let thing = cache[id]
return thing // error (2801), but it should't be, getCachedThing3 isn't async function
? thing
: (cache[id] = fetchThing(id))
}
After applying quick fix to getChachedThing3
error:
const getCachedThing3 = (id: string) => {
let thing = cache[id]
return await thing // error (2801), but it should't be, getCachedThing3 isn't async function
// error (2801), but it should't be, getCachedThing3 isn't async function
? thing
: (cache[id] = fetchThing(id))
}
Quick fixed code is invalid (await
is added to non-async function). Also, comment gets duplicated.
π Actual behavior
- Missing await in conditional errors are inconsistent
- Missing await in conditional errors get reported in non-async functions
- Quick fixing the error in non-async function produces invalid results
π Expected behavior
- Missing await in conditional errors to be consistent
- Missing await in conditional errors to NOT be reported in non-async functions
- No quick fix suggestion in non-async function
As a sidenote, I think the "missing await in conditional error" feature should be under a flag
- It is wildly inconsistent
- This is a job for linters
- Objects are thruthy, so let them be thruthy :)
- This feature breaks existing code
Activity
andrewbranch commentedon Apr 5, 2021
This example is unfortunate because the type of
thing
comes from reading from an index signature, which by default we treat as always-defined but in reality must usually be checked forundefined
. The error goes away and everything works as expected if you use--noUncheckedIndexedAccess
, or simply write the type annotation forthing
:EDIT: As pointed out in #43565, I screwed this example up βοΈ. What I meant to write was
getCachedThing1
is not an error because we just donβt consider nested expressions for this missing await check. In that function, we donβt see a Promise being tested for truthiness, we see a boolean being tested for truthiness, which is always fine.getCachedThing2
is not an error because we donβt do definitely-truthy checks on operands of logical binary expressions. We generally want to allow you to code more defensively than your types suggest you need to.getCachedThing3
matches the pattern of something we think looks suspicious, again primarily because the type ofthing
should includeundefined
but doesnβt. I think we should be doing the same thing here as we do for uncalled function checks, which is to silence the error if the expression being checked for truthiness is accessed in the true side of the conditional body. I donβt see that discussed at #39175, but it would make sense to me for those to work the same.The fact that the
await
is added to a non-async function was an intentional decision, because the error on the addedawait
gives you access to a subsequent codefix to make the function async. We were concerned about cases where you're editing a large function and theasync
keyword would get added way off-screen, so we wanted to make that choice more obvious to the user. But that's a decision we could revisit.The duplicated comment is a bug.
bagdonas commentedon Apr 6, 2021
Thank you for very detailed explanation. The reason I found these behaviours "sketchy" to the point of filling out an issue is because from the perspective of someone who doesn't know these very specific details (and a sample of 1 such someone) the feature seems broken. I was expecting it to be more generic. Now that I went back and read 4.3 beta announcement again, everything checks out. It works just like advertised.
It still seems super weird that the error is triggered by one specific pattern leaving all other patterns that produce conceptually identical results unchecked. It also feels weird that typechecker is checking for such a pattern (feels like it disregards truthiness of objects). Lots of feelings, I know... π
andrewbranch commentedon Apr 6, 2021
How do you mean? The general idea is that doing
if (somePromise)
looks like a mistake precisely because we believesomePromise
is an object and objects are always truthy, and therefore should never need to be checked for truthiness. And we issue the error on Promises specifically because thereβs a plausible assumption that what you meant to do wasawait
it. We donβt give the same error for random objects likeif (someNonPromiseObject)
because even though that also looks to be always truthy, we have no guesses about what you meant to do instead, so we just let you do it, assuming you may have a good reason we donβt know about.bagdonas commentedon Apr 6, 2021
Promise is just an object, special handling for something that is truthy in a ternary/if feels weird. Especially when use of such value in a condition operand results in typechecker error.
As twisted as my logic is, exactly for this reason. I'm so accustomed to never getting errors when checking values for their truthiness that getting one with this new feature feels weird. Again, just a feeling.