Narrowing to never with in in non-union contexts with feature-tested properties

[RyanCavanaugh]

Bug Report

🔎 Search Terms

never in narrow

🕗 Version & Regression Information

• This changed between versions 4.7 and 4.8 versions 4.8 and 4.9

⏯ Playground Link

Playground link with relevant code

💻 Code

export function generateUID() {
    if ('randomUUID' in crypto) {
        return crypto.randomUUID();
    }

    // Error here, `crypto` is `never`
    return crypto.getRandomValues(new Uint8Array(16));
}

🙁 Actual behavior

Error at indicated line - crypto is of type never

🙂 Expected behavior

Shouldn't be an error?

Discussion

randomUUID is a method available in secure contexts only. In this sense it's "optional" in that it may or may not be present, but in another sense it's not optional because any piece of code might willingly take a hard dependency on only running in contexts where this is supported. For this code snippet, which polyfills its own behavior when randomUUID isn't present, the bottom codepath is definitely reachable.

[DanielRosenwasser]

It seems bad given that you can do a check for

if (crypto.randomUUID) {
}
else {
  // not never here, right?
}

without getting never in the else branch.

(playground link)

[fatcerberus]

Somebody pick up that phone because I called it
#50954 (comment)

[fatcerberus]

On an unrelated note why does everything need a secure context now? You can't even use ES modules without a secure context, which makes no sense. It makes it a pain to test things locally.

[ahejlsberg]

This is working as intended.

First, narrowing was quite inconsistent in 4.8 and earlier:

function test1(crypto: Crypto) {
    if ('randomUUID' in crypto) {
        crypto;  // Crypto
    }
    else {
        crypto;  // Crypto
    }
}

function test2(crypto: Crypto | undefined) {
    if (!crypto) {
        return;
    }
    if ('randomUUID' in crypto) {
        crypto;  // Crypto
    }
    else {
        crypto;  // never
    }
}

The difference was based on whether the declared type is a singleton or a union, but that obviously leads to inconsistent and surprising behavior. It is simply not feasible to have narrowing behavior depend on singleton vs. union in this manner.

The issue really is that the declaration of Crypto is wrong:

/** Basic cryptography features available in the current context. It allows access to a cryptographically strong random number generator and to cryptographic primitives. */
interface Crypto {
    /** Available only in secure contexts. */
    readonly subtle: SubtleCrypto;
    getRandomValues<T extends ArrayBufferView | null>(array: T): T;
    /** Available only in secure contexts. */
    randomUUID(): string;
}

If randomUUID is only present sometimes, then it should be declared as randomUUID?(): string;. That'll then require usage to be preceded by a non-null check, an in check, or to include a ! assertion.

[fatcerberus]

If randomUUID is only present sometimes, then it should be declared as …

Fair, but suppose rather than “this only exists in secure contexts”, the reason you’re doing the check is instead “this only exists in newer browsers than the oldest one I support”. You can’t just declare all the functions in the DOM as optional…

[ahejlsberg]

Right, but ultimately the checker has to trust the types, and it has to do so in a consistent and predictable manner. I think the 4.9 behavior is the best we can do given the current declaration Crypto type. Of course if you don't want any narrowing to take place you can cast the string literal to type string such that the checker doesn't "see" the effects:

if ('randomUUID' as string in crypto) {
    crypto.randonUUID();  // Ok
}
else {
    crypto.randomUUID();  // Ok
}

[typescript-bot]

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.