checker.ts :: checkModuleDeclaration(): Redundant type guard erroneously narrows node type to 'never'

[rafasofizada]

Bug Report

I have a ready, very easy fix for this bug, so if you indeed recognize this as a bug, please assign it to me, and I'll submit a PR.

🔎 Search Terms

checkModuleDeclaration node type never
checker.ts
isExternalModuleAugmentation
Property 'parent' does not exist on type 'never' (ts2339)

🕗 Version & Regression Information

This is a logical bug in checker.ts that should arise in pretty much any TypeScript version that supports type guards/predicaments. However, since isAmbientModule()'s result is assigned to a variable, and control flow of aliased conditional expressions is introduced only in v4.4.0, this issue will only appear in v4.4.0+ (or any other version with pull request #44730)

Judging by related commits that I've found by using git blame -L37720,37722 src/compiler/checker.ts and
git log -L37720,37722:src/compiler/checker.ts, the bug was introduced somewhere around v2.8-rc:

• 4a963a2 – Dec 22, 2015, seems to be v1.7.5 or v1.8.0-beta – Added the isExternalModuleAugmentation() check (which will become redundant in the next commit)
• 5e593ac – Mar 6, 2018, seems to be v2.8-rc – Added a type predicate to isExternalModuleAugmentation(), which created a redundancy with parent isAmbientModule check.

⏯ Playground Link

Close recreation of the issue

💻 Code

checker.ts : line 37664

function checkModuleDeclaration(node: ModuleDeclaration) {
    // ...

    // Control flow analysis of aliased conditional expressions 
    // (e.g. type guard result saved in a variable) are introduced
    // only in v4.4.0 https://github.com/microsoft/TypeScript/pull/44730
    const isAmbientExternalModule = isAmbientModule(node);

   // ...

    if (isAmbientExternalModule) {
        // Before v4.4.0, node here is of type 'TNode'
        // After v4.4.0, node here is of type 'AmbientModuleDeclaration'
        if (isExternalModuleAugmentation(node)) {
            // isExternalModuleAugmentation() contains a redundant isAmbientModule() check
        }
        else if (isGlobalSourceFile(node.parent)) {
        //                          ^^^^^^^^^^^^
        // Here, tsc v4.5.0-dev.20210824 (and all others) give an error:
        // Property 'parent' does not exist on type 'never'.ts(2339)
        // Because of the redundant check in if (isExternalModuleAugmentation(node)) ...,
        // control flows deduces node's type to be 'never'.

        // ...
        } else {
            // ...
        }
    }
    // ...
}

🙁 Actual behavior

Covered in the code examples/playground.

🙂 Expected behavior

Covered in the code examples/playground.

[andrewbranch]

I think your sources are out of date; this was fixed as soon as it became a problem. It now reads

const isAmbientExternalModule: boolean = isAmbientModule(node);

The compiler compiles itself in CI, so we automatically catch things like this as we go.

[rafasofizada]

EDIT: Okay, it seems like just adding a type annotation solves this issue. But how? isAmbientExternalModule should be inferred as a boolean anyway, because it is a result of a type guard. Adding a boolean type annotation shouldn't change the fact that isAmbientModule() and isExternalModuleAugmentation() intersect and node is inferred as never in subsequent conditional statements. I still think the issue should be reopened, because the real solution to this is to replace isExternalModuleAugmentation() with isModuleAugmentationExternal().

@andrewbranch that doesn't really change anything about the issue, does it? The issue is not in the type definition of isAmbientExternalModule, it is in the intersection of isAmbientModule() and isExternalModuleAugmentation(), defined in utilities.ts.

The line if (isExternalModuleAugmentation(node)) should be if (isModuleAugmentationExternal(node)) instead, as isExternalModuleAugmentation() already includes isAmbientModule() check inside of it.

// utilities.ts:773
export function isExternalModuleAugmentation(node: Node): node is AmbientModuleDeclaration {
  return isAmbientModule(node) && isModuleAugmentationExternal(node);
}

I think the issue should be reopened. The bug is still there.