microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.27k stars 12.39k forks source link

Inverted `Promise` should warn like it does without inverting #46140

Open soffes opened 2 years ago

soffes commented 2 years ago

Suggestion

🔍 Search Terms

This condition will always return true since this 'Promise<string | undefined>' is always defined.(2801) Did you forget to use 'await'?

✅ Viability Checklist

My suggestion meets these guidelines:

⭐ Suggestion

When checking the inverted truthyness if something returned from an async function without calling await, it should warn you you forgot await. This works without inverting.

📃 Motivating Example

async function foo(): Promise<string | undefined> {
    return
}

const value = foo()
if (!value) { // <-- fails silently
    // ...
}

const awaitedValue = await foo()
if (awaitedValue) { // <-- Emits ts2801
    // ...
}

Playground

💻 Use Cases

In my use, I had a function that returned a User | undefined and I was doing something if there wasn't a user returned. Later, I made the function async and forgot to update this since it was failing silently.

andrewbranch commented 2 years ago

This should probably be predicated on the possibility that the awaited type of the Promise is falsy. I think we could look at this for uncalled functions too, since that shares most of the same code path.

IllusionMH commented 2 years ago

First case looks similar to #44840

andrewbranch commented 2 years ago

Yeah, there are several issues asking us to be more aggressive about either recognizing code as unreachable or issuing errors when we already do (like here—value is successfully narrowed to never inside if (!value), but the user has no way of noticing that because they think they just detected it to be undefined, so of course they’re not going to try to use it). But I like this issue because the scope is very narrow, and seems to sit well within the unawaited Promise / uncalled function checks we already have.

Also related is #45267

fatcerberus commented 2 years ago

...but the user has no way of noticing that because they think they just detected it to be undefined, so of course they’re not going to try to use it

One potential issue: narrowing to never is not guaranteed to produce an error even if the value is used, since never is assignable to all other types.

Miodec commented 2 years ago

Any update? Just ran into this issue. Tried 4.6.4 and its still a problem.

The bug I found today:

...
  if (!isNameAvailable(name)) {
    throw new MonkeyError(409, "Username already taken", name);
  }
...

isNameAvailable returns a promise, and so the throw line will never execute.

Removing the inversion highlights this error correctly:

if (isNameAvailable(name)) {
This condition will always return true since this 'Promise<boolean>' is always defined.ts(2801)
user.ts(54, 7): Did you forget to use 'await'?

Another interesting fact is that using === also highlights this error correctly:

if (!isNameAvailable(name) === true) {
This condition will always return 'false' since the types 'false' and 'true' have no overlap.ts(2367)

But, as mentioned, the first code block is silently always false, causing headaches 😄

tjenkinson commented 6 days ago

Hey! We had a bug that would also have been caught by this.

I started trying to implement a solution here: https://github.com/microsoft/TypeScript/compare/main...tjenkinson:TypeScript:support-condition-always-true-in-prefix-unary-expression

Let me know if it looks like the right approach and I'll make it a PR and add some tests :)