Fix non-null discriminant narrowing consistency

[S.H Jeong (z0rimo)]

Fixes #62511

This fixes inconsistent discriminant narrowing between small and large nullable unions.

For example:

declare let value:
    | { type: "1" }
    | { type: "2" }
    | undefined;

if (value!.type === "1") {
    value.type;
}

The general discriminant-narrowing path preserves the nullable constituent in this case, while the optimized key-property path used by sufficiently large unions could return the matching constituent directly and drop undefined.

The optimized discriminant-property paths now decline the optimization when the current flow type is nullable under strictNullChecks, falling back to the existing general narrowing logic. This keeps the result consistent across union sizes without changing the optimization threshold or introducing special handling for particular expression shapes.

Optional chaining behavior is unchanged; value?.type === "1" continues to narrow value in the matching branch.

The change updates the existing narrowingUnionWithBang baseline and adds focused regression coverage for small and large unions, equality and inequality branches, switch narrowing, and optional chaining.

AI assistance was used during investigation and implementation. I reviewed the patch, traced the checker behavior, and verified the change with focused compiler tests and the repository's validation commands locally.

[S.H Jeong (z0rimo)]

@microsoft-github-policy-service agree

[Copilot]

🟡 Changes recommended

The implementation remains inconsistent for switch narrowing and can incorrectly narrow mutable values through aliased discriminants.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes inconsistent nullability during discriminant narrowing after non-null assertions.

Changes:

• Applies non-null facts before equality-based discriminant narrowing.
• Adds small/large union regression tests.
• Updates the affected type baseline.

File summaries

File	Description
tsc/internal/checker/flow.go	Normalizes nullable types before narrowing.
tsc/testdata/tests/cases/compiler/discriminatedUnionNonNullAccessNarrowing.ts	Adds regression coverage.
tsc/testdata/baselines/reference/compiler/narrowingUnionWithBang.types	Records corrected inferred types.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Combined non-null and optional-chain accesses incorrectly remove nullability from false and default branches.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

tsc/internal/checker/flow.go:505

• The mirrored right-hand form ("1" === value!?.type) has the same issue: optional chaining can complete with an undefined base, but this treats the base as non-null in both branches. Gate the eager non-null facts on the access not being an optional chain.

			if rightAccess == right && c.strictNullChecks && isNonNullAccess(rightAccess) && c.maybeTypeOfKind(t, TypeFlagsNullable) {

tsc/internal/checker/flow.go:1091

• For switch (value!?.type), the default path can be reached because value is nullish. This condition nevertheless removes nullability for every switch clause because an optional access whose receiver is a non-null expression also satisfies isNonNullAccess. Exclude optional chains here so the existing switch optional-chain containment logic decides which clauses imply presence.

			if access == expr && c.strictNullChecks && isNonNullAccess(access) && c.maybeTypeOfKind(t, TypeFlagsNullable) {

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Eager non-null facts can become stale after assignments in later operands or switch case expressions.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

tsc/internal/checker/flow.go:1093

• A switch discriminant is evaluated before its case expressions, and those expressions may reassign the matched reference. For example, switch (value!.type) { case (value = maybeUndefined, "1"): value.type; } can enter the case with value undefined, while this unconditional normalization drops that possibility. Account for writes from evaluated case labels before applying the eager fact (or restrict it to labels that cannot write the reference), and cover this ordering case in the regression test.

			if access == expr && c.strictNullChecks && !ast.IsOptionalChain(access) && isNonNullAccess(access) && c.maybeTypeOfKind(t, TypeFlagsNullable) {
				t = c.getTypeWithFacts(t, TypeFactsNEUndefinedOrNull)
			}

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Prefix assignments can invalidate dotted references while the new logic still applies an unsound non-null fact.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Mutation scanning misses assertion-wrapped assignments and incorrectly scans deferred function bodies.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

tsc/internal/checker/flow.go:1862

• Assignments whose target is wrapped in a type assertion are missed by this match. TypeScript permits (value as Small | undefined) = maybeUndefined; IsAssignmentTarget is true for the outer AsExpression, but isOrContainsMatchingReference does not unwrap that target. Consequently value!.type === ((value as Small | undefined) = maybeUndefined, "1") can remove undefined even though the true branch may run after assigning undefined. Please normalize assertion-wrapped assignment targets (including angle-bracket assertions) before matching and cover this form in the evaluation-order tests.
  tsc/internal/checker/flow.go:1865
• This recursively scans deferred function bodies, so merely creating a closure on the right-hand side suppresses the new narrowing. For example, if (value!.type === (() => { value = undefined; }, "1")) { value.type } reports value as possibly undefined even though the assignment is never evaluated. Please traverse only code evaluated as part of the operand/case expression (while still handling invoked functions such as IIFEs) and add a regression case.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Parenthesized assertions and synchronous assignments inside async IIFEs remain incorrectly handled.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tsc/internal/checker/flow.go:498

• Parenthesizing the assertion still prevents the fix: for if ((value!).type === "1") value.type, isNonNullAccess returns false because it only recognizes an immediate NonNullExpression (tsc/internal/checker/utilities.go:1092-1094). The optimized path is then skipped for the still-nullable type and the fallback preserves undefined, even though parentheses are semantically transparent. Normalize parentheses around the access base in isNonNullAccess and add equality/switch coverage for this spelling.

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Assignment-wrapped operands and loose equality can still propagate stale non-null facts.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

tsc/internal/checker/flow.go:507

• The right-hand candidate has the same assignment-wrapper hole: getReferenceCandidate can return the assignment LHS even though the assignment RHS runs after that access, and loose equality may perform coercion afterward. Thus "1" === (value!.type = (value = maybeUndefined, "1")) can incorrectly make value non-null in the branch. Only apply the fact when the original right operand is the direct access and no coercing operator remains.

			if rightAccess == right && c.strictNullChecks && !ast.IsOptionalChain(rightAccess) && isNonNullAccess(rightAccess) && c.maybeTypeOfKind(t, TypeFlagsNullable) {

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Computed discriminant keys can mutate the reference and make the new eager non-null facts stale.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

tsc/internal/checker/flow.go:1093

• The switch expression itself can still perform work after evaluating the non-null base. With switch (value![Keys.type]), an enum/const entity key is accepted as a discriminant, and a getter installed on Keys.type can set value to undefined while returning "type"; a matching case then enters with the stale non-null fact. The clause-expression check does not cover this evaluation. Require computed key evaluation in access to be side-effect-free (or only propagate for non-computed/literal-key accesses) before removing nullable constituents.

			if access == expr && c.strictNullChecks && !ast.IsOptionalChain(access) && isNonNullAccess(access) && switchClauseExpressionsAreSideEffectFree(data) && c.maybeTypeOfKind(t, TypeFlagsNullable) {

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The evaluation-order tests use small unions and therefore do not exercise the optimized path responsible for the stale-fact regression.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

tsc/testdata/tests/cases/compiler/discriminatedUnionNonNullAccessNarrowing.ts:204

• The switch evaluation-order regression is only exercised with Small, whose fallback narrowing was already conservative. Exercise this with Large so the test actually verifies that the optimized discriminant path no longer propagates the stale non-null fact through a mutating case expression.

declare let assignmentInCase: Small;

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Signed numeric and bigint discriminants remain incorrectly excluded from otherwise safe non-null narrowing.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tsc/internal/checker/flow.go:1865

• Signed numeric and bigint discriminants are incorrectly treated as side-effectful. For example, if (value!.type === -1) { value.type } and case -1 still retain undefined, because -1 is a PrefixUnaryExpression; with the new nullable guard, both paths fall back without applying the eager fact. Reuse the existing primitive-literal predicate, which includes side-effect-free signed literals, and add a signed-discriminant regression case.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation addresses union-size consistency and evaluation-order invalidation with comprehensive focused coverage.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced