Inverted Promise should warn like it does without inverting

[soffes]

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:

• This wouldn't be a breaking change in existing TypeScript/JavaScript code
• This wouldn't change the runtime behavior of existing JavaScript code
• This could be implemented without emitting different JS based on the types of the expressions
• This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
• This feature would agree with the rest of TypeScript's Design Goals.

⭐ 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]

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]

First case looks similar to #44840

[andrewbranch]

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]

...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]

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]

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

I started trying to implement a solution here: 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 :)

[gabritto]

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

I started trying to implement a solution here: 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 :)

@tjenkinson It looks ok at a first glance.
I noticed one thing missing.
When the thing that is always truthy has a function type, right now we only error if the thing is not used in the body of the condition. Example:

{
    let fun!: () => void;

    if (fun) { // Error
        let x = 1;
    }
}

{
    let fun!: () => void;

    if (fun) { // No error, because `fun` could actually be undefined and this is a valid check
        let x = fun();
    }
}

For the negative case, the equivalent would be this:

{
    let fun!: () => void;

    if (!fun) { // Should be error, probably a bug
        // ...
    }
}

{
    let fun!: () => void;

    if (!fun) { // Should not be error, `fun` could actually be undefined and this check is valid
        // ...
    }
    else {
        let x = fun();
    }
}

so your implementation would have to account for that.

It would also be good to have more tests, for the other possible error and non-error cases for instance (e.g. unawaited promises), if you want to move forward with this.

[tjenkinson]

thanks @gabritto

I'm wondering what the reasoning is behind ignoring when the variable is referenced in the block? Is it because there are a lot of cases in the wild where the types are wrong and therefore logic is guarded in if statements just in case?

If that's the case I wonder if the same really applies when checking the inverse?

If it does I guess we'd also need to allow code like

let fun!: () => void;

if (!fun) {
    throw new Error('`fun` missing');
}

fun();

which looks more complicated given the call is not part of the if/else?

Wondering if when using ! we should only check the promise case?

I.e.

let isEnabled!: () => Promise<boolean>;

// always error
if (!isEnabled()) {}

// always ok
if (!isEnabled) {}

WDYT? Updated the branch for that

[gabritto]

I'm wondering what the reasoning is behind ignoring when the variable is referenced in the block? Is it because there are a lot of cases in the wild where the types are wrong and therefore logic is guarded in if statements just in case?

Yes.

If that's the case I wonder if the same really applies when checking the inverse?

We can start looking into that by creating a PR with the original version of your code (not doing anything special for promises or the negation case), and then I can run the extended tests on it to see if we need any special casing or not.

[tjenkinson]

@gabritto sounds great opened #60068 and it's back to checking everything