checker.ts: explicit boolean type annotation overrides type guard, silencing an error

[rafasofizada]

Bug Report

I wouldn't call this a bug, but rather a piece of code inconsistent with "best practices" and prone to cause more confusion in future.

This is a spin-off from issue #45572. I have investigated the line mentioned in Andrew Branch (@andrewbranch) 's comment, and answered my own question in the subsequent reply.

If you indeed recognize it as a bug, please read the "Solution" section and assign the bug to me. Thanks for working on and maintaining such a wonderful project!

🔎 Search Terms

Control flow analysis of aliased conditional expressions
Type guard result saved to variable

🕗 Version & Regression Information

This was introduced in v4.4.0-beta, by pull request #44730 "Control flow analysis of aliased conditional expressions", commit 08b6e2e.

"inlining CFA" through an explicit type annotation:

@@ -38340,9 +38340,9 @@
function checkModuleDeclaration(node: ModuleDeclaration) {
    ...
-   const isAmbientExternalModule = isAmbientModule(node);
+   const isAmbientExternalModule: boolean = isAmbientModule(node);

⏯ Playground Links & 💻 Code

// utilities.ts

export function isAmbientModule(node: Node): node is AmbientModuleDeclaration {
    return isModuleDeclaration(node) && (node.name.kind === SyntaxKind.StringLiteral || isGlobalScopeAugmentation(node));
}

export function isExternalModuleAugmentation(node: Node): node is AmbientModuleDeclaration {
    return isAmbientModule(node) && isModuleAugmentationExternal(node);
}          ^^^^^^^^^^^^^^^^^^^^^

Playground link illustrating the "Before" code example
Before #44730 (v4.3.5):

function checkModuleDeclaration(node: ModuleDeclaration) {
    // ...
    
    // Type guard result gets assigned to a variable.
    const isAmbientExternalModule = isAmbientModule(node);

    // ...

    // Control flow analysis would've narrowed `node` to AmbientModuleDeclaration 
    if (isAmbientExternalModule) {
        // ... so, here, `node` doesn't get narrowed to AmbientModuleDeclaration — it stays just a ModuleDeclaration, 
        // as annotated in the function signature

        if (isExternalModuleAugmentation(node)) {

            // Now, because the type guard is inside the conditional statement, `node` will get narrowed to AmbientModuleDeclaration.

        }
        // However, outside the if statement, `node` is still ModuleDeclaration. No errors.
        // If CFA recognized that isAmbientExternalModule narrows `node` to AmbientModuleDeclaration,
        // the redundancy in isExternalModuleAugmentation() type guard (which contains isAmbientModule() in itself)
        // would cause `node` to be narrowed down to `never`.
        else if ( /* `node` is `ModuleDeclaration` here... */ ) { 
       
        }
        else {
            // ...and here.
        }
    }

    // ...
}

Playground link illustrating the "After, current" code example
After (with explicit type annotation, e.g. current version, WITHOUT error) #44730 (v4.4-beta):

function checkModuleDeclaration(node: ModuleDeclaration) {
    // ...
    
    // Type guard result gets assigned to a variable. CFA would've narrow down `node`
    // in conditional statements below, IF NOT for the `boolean` explicit annotation. Because of it, CFA no longer treats isAmbientExternalModule as a type guard result. No type narrowing will happen below, so further this example
    // is identical to the "Before" example above.
    const isAmbientExternalModule: boolean = isAmbientModule(node);
    
    // ...
}

Playground link illustrating the "After, current" code example
After #44730 (edited out the type annotation, GIVES ERROR) (v4.4-beta):

function checkModuleDeclaration(node: ModuleDeclaration) {
    // ...
    
    // Type guard result gets assigned to a variable. Now, there's no explicit type annotation, 'boolean' doesn't override
    // the type guard's result (predicament?)
    const isAmbientExternalModule = isAmbientModule(node);

    // ...

    // Control flow analysis NARROWS `node` to AmbientModuleDeclaration 
    if (isAmbientExternalModule) {
        // And here, because both isAmbientExternalModule and isExternalModuleAugmentation assert that `node` is AmbientModuleDeclaration ...
        if (isExternalModuleAugmentation(node)) {
            // `node` is `AmbientModuleDeclaration` here 
        }
        // ... CFA will exhaust all possible types for `node` after the first if() statement 
        // (what else can AmbientModuleDeclaration be if not an AmbientModuleDeclaration?)
        // so `node` will be `never`.
        else if ( /* `node` is `never` here... */ ) { 
       
        }
        else {
            // ...and here.
        }
    }

    // ...
}

✅ Solution

Playground link the proposed solution based on prev. examples

src/compiler/checker.ts

function checkModuleDeclaration(node: ModuleDeclaration) {
    ...
    // Removing explicit boolean type annotation will allow CFA to narrow `node` in subsequent conditional statements.
-   const isAmbientExternalModule: boolean = isAmbientModule(node);
+   const isAmbientExternalModule = isAmbientModule(node);

    ...
    
    if (isAmbientExternalModule) {
         // Removing the second isAmbientModule() check (as a part of isExternalModuleAugmentation()) will prevent
         // CFA from narrowing `node` to `never`.
-       if (isExternalModuleAugmentation(node)) {
+       if (isModuleAugmentationExternal(node)) {
        ...

[fatcerberus]

By design, per #44730:

Narrowing through indirect references occurs only when the conditional expression or discriminant property access is declared in a const variable declaration with no type annotation, and the reference being narrowed is a const variable, a readonly property, or a parameter for which there are no assignments in the function body.

Emphasis mine.

[rafasofizada]

Bruce Pascoe (@fatcerberus) thank you for the reference! I missed that somehow.