From 326c6ba3b9b9c7b0ce829018921b44e796cfe4b6 Mon Sep 17 00:00:00 2001 From: Nick Nassiri Date: Mon, 15 Jun 2026 23:12:15 -0700 Subject: [PATCH] Fix #653/#654: narrow variables across reassignment; widen all enclosing guards on escape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #653: after `x = v`, a variable read as its full declared type even when the RHS put it safely inside that type, so SharpTS rejected code tsc accepts (`x = "s"` on a `string | null` `x`, then `x.length`). CheckAssign now restores a tracked variable to the post-write flow-narrowed type (declared union filtered to the RHS-compatible members, via NarrowToDeclaredSlot) instead of unconditionally to the declared type — mirroring the property-write narrowing of #48. The narrowed binding lives in the current lexical scope and is discarded at its block's join, so it can't leak past a conditional. Two guards keep it sound: only function-local/parameter variables narrow (module vars aren't tracked in the declared-type stack — see #743), and a purely-nullish slot is not installed so a later access still raises "possibly null/undefined" (the bare-nullish access gap is #742). #654: WidenEnclosingNarrowing now widens EVERY enclosing scope that holds a narrowing of the reassigned variable, not just the nearest. When two guards narrowed the same variable and an escaping reassignment sat under the inner one, only the inner guard was widened, so a read at the outer level after the inner block kept the stale outer narrowing (a soundness hole — SharpTS accepted code tsc rejects). The walk stops at the variable's declaration (binding == declared type) so it never crosses into an outer, same-named shadowing variable whose own narrowing is still valid. Tests: new VariableAssignmentFlowNarrowingTests (#653, companion to the #48 property tests) and three #654 cases in NestedReassignmentNarrowingTests (verbatim repro, three-level nesting, and a shadowing non-regression guard). --- .../NestedReassignmentNarrowingTests.cs | 63 ++++++ .../VariableAssignmentFlowNarrowingTests.cs | 191 ++++++++++++++++++ TypeSystem/TypeChecker.Expressions.cs | 31 ++- TypeSystem/TypeChecker.cs | 67 +++++- 4 files changed, 338 insertions(+), 14 deletions(-) create mode 100644 SharpTS.Tests/TypeCheckerTests/NarrowingTests/VariableAssignmentFlowNarrowingTests.cs diff --git a/SharpTS.Tests/TypeCheckerTests/NarrowingTests/NestedReassignmentNarrowingTests.cs b/SharpTS.Tests/TypeCheckerTests/NarrowingTests/NestedReassignmentNarrowingTests.cs index 1a83bcf7..809ab9b6 100644 --- a/SharpTS.Tests/TypeCheckerTests/NarrowingTests/NestedReassignmentNarrowingTests.cs +++ b/SharpTS.Tests/TypeCheckerTests/NarrowingTests/NestedReassignmentNarrowingTests.cs @@ -127,4 +127,67 @@ function h(x: string | undefined, cond: boolean): number { """; Assert.ThrowsAny(() => TestHarness.RunInterpreted(source)); } + + // ---- #654: nested guards on the SAME variable must widen EVERY enclosing narrowing ---- + + [Fact] + public void IfGuard_NestedSameVarGuardReassignToExcluded_WidensOuterNarrowing() + { + // The verbatim #654 repro. Both guards narrow `x`; the inner reassignment to undefined must + // widen the OUTER guard's narrowing too, so the read after the inner block is rejected. + // Before the fix WidenEnclosingNarrowing only widened the nearest (inner) guard, leaving the + // outer narrowing stale — SharpTS accepted code tsc rejects. + var source = """ + function f(x: string | undefined): void { + if (x !== undefined) { + if (x !== "foo") { x = undefined; } + x.length; + } + } + """; + Assert.ThrowsAny(() => TestHarness.RunInterpreted(source)); + } + + [Fact] + public void IfGuard_ThreeLevelSameVarGuards_WidenAtEveryOuterLevel() + { + // Three guards deep on the same variable: an escaping reassignment at the deepest level must + // widen the narrowing at BOTH shallower levels. The read sits at the middle level here. + var source = """ + function f(x: string | undefined): number { + if (x !== undefined) { + if (x !== "a") { + if (x !== "b") { x = undefined; } + return x.length; + } + return x.length; + } + return 0; + } + """; + Assert.ThrowsAny(() => TestHarness.RunInterpreted(source)); + } + + [Fact] + public void IfGuard_ShadowingSameNameVar_DoesNotWidenOuterVariable() + { + // No regression from the widen-EVERY-scope change: an inner block that SHADOWS `x` with its + // own binding and reassigns it must not widen the OUTER `x`'s guard narrowing. The widening + // walk must stop at the inner declaration. Outer `x` stays string, so `x.length` is fine. + var source = """ + function f(x: string | undefined): number { + if (x !== undefined) { + { + let x: string | number = "y"; + if (typeof x === "string") { x = 42; } + void x; + } + return x.length; + } + return 0; + } + console.log(f("hello")); + """; + Assert.Equal("5\n", TestHarness.RunInterpreted(source)); + } } diff --git a/SharpTS.Tests/TypeCheckerTests/NarrowingTests/VariableAssignmentFlowNarrowingTests.cs b/SharpTS.Tests/TypeCheckerTests/NarrowingTests/VariableAssignmentFlowNarrowingTests.cs new file mode 100644 index 00000000..2b4c379b --- /dev/null +++ b/SharpTS.Tests/TypeCheckerTests/NarrowingTests/VariableAssignmentFlowNarrowingTests.cs @@ -0,0 +1,191 @@ +using SharpTS.Tests.Infrastructure; +using SharpTS.TypeSystem.Exceptions; +using Xunit; + +namespace SharpTS.Tests.TypeCheckerTests.NarrowingTests; + +/// +/// Tests for flow narrowing across VARIABLE assignments (issue #653) — the symmetric companion to +/// the property-write narrowing of #48 (see ). TypeScript's +/// CFA narrows a reference to the assigned value's type after a write, so x = "s" on a +/// string | null variable lets the subsequent x.length type-check. SharpTS previously +/// restored the full declared type after every variable assignment, rejecting code tsc accepts. +/// +/// The narrowed type is the declared union filtered to the members the RHS can be +/// (NarrowToDeclaredSlot), so literal members survive and the declared type is the fallback +/// when nothing narrows. Only function-local/parameter variables are narrowed — module-level +/// variables are not tracked in the declared-type stack, so they stay at the declared type. +/// +public class VariableAssignmentFlowNarrowingTests +{ + [Fact] + public void Reassign_WriteThenRead_NarrowsToRhsType() + { + // Primary reproduction from issue #653. + var source = """ + function f(x: string | null): number { + x = "hi"; + return x.length; + } + console.log(f(null)); + """; + + Assert.Equal("2\n", TestHarness.RunInterpreted(source)); + } + + [Fact] + public void Reassign_InGuardRecoveryPath_NarrowsToRhsType() + { + // Second #653 repro: reassigning inside the branch that recovers from `undefined`. + var source = """ + function f(x: string | undefined): number { + if (x === undefined) { + x = "recovered"; + return x.length; + } + return x.length; + } + console.log(f(undefined)); + console.log(f("hello")); + """; + + Assert.Equal("9\n5\n", TestHarness.RunInterpreted(source)); + } + + [Fact] + public void Reassign_CompatibleValueInsideGuard_KeepsNarrowing() + { + // Reassigning to a value the guard still admits keeps `x` narrowed to string, so `x.length` + // type-checks in the same branch (previously rejected by the restore-to-declared). + var source = """ + function f(x: string | null): number { + if (x !== null) { + x = "hi"; + return x.length; + } + return 0; + } + console.log(f("a")); + """; + + Assert.Equal("2\n", TestHarness.RunInterpreted(source)); + } + + [Fact] + public void Reassign_LiteralUnion_PreservesLiteralMember() + { + // The narrowed type is the surviving declared member, so a literal-union declared type keeps + // the assigned literal: `x = "a"` narrows `x` to `"a"`, assignable to an `"a"` return slot. + var source = """ + function f(x: "a" | "b" | null): "a" { + x = "a"; + return x; + } + console.log(f(null)); + """; + + Assert.Equal("a\n", TestHarness.RunInterpreted(source)); + } + + [Fact] + public void Reassign_DeclaredNonUnion_NoOp() + { + // When the declared type isn't a union there's nothing to narrow; still type-checks and runs. + var source = """ + function f(x: string): number { + x = "hello"; + return x.length; + } + console.log(f("world")); + """; + + Assert.Equal("5\n", TestHarness.RunInterpreted(source)); + } + + [Fact] + public void Reassign_AcrossUnionMembers_ChecksAgainstDeclaredNotNarrowed() + { + // After `x = "hi"` narrows `x` to string, a later `x = 42` must still be allowed: the + // assignment is checked against the DECLARED type, not the narrowed one. + var source = """ + function f(): string | number { + let x: string | number = "s"; + x = "hi"; + x = 42; + return x; + } + console.log(f()); + """; + + Assert.Equal("42\n", TestHarness.RunInterpreted(source)); + } + + [Fact] + public void Reassign_NullValue_StaysNullableAndRejectsNonNullSlot() + { + // Writing `null` keeps the variable nullable (a purely-nullish slot is deliberately NOT + // installed — see IsPurelyNullish — so a later access still raises "possibly null"). Returning + // it from a non-nullable `string` function is therefore still rejected. + var source = """ + function f(x: string | null): string { + x = null; + return x; + } + """; + + var ex = Assert.ThrowsAny(() => TestHarness.RunInterpreted(source)); + Assert.Contains("null", ex.Message); + } + + [Fact] + public void Reassign_NullThenAccess_StillRejected() + { + // Soundness: `x = null` followed by a property access must still error. Narrowing to a bare + // `null` would lose this (bare-nullish access isn't flagged yet), so the declared union is + // kept and `x.length` reports the nullish access. + var source = """ + function f(x: string | null): number { + x = null; + return x.length; + } + """; + + Assert.ThrowsAny(() => TestHarness.RunInterpreted(source)); + } + + [Fact] + public void Reassign_RhsSpansFullUnion_DoesNotNarrow() + { + // If the RHS can be the whole declared union, there is no narrowing benefit — the unguarded + // read still errors (matching the property analog WriteOfNullableRhs_DoesNotNarrow). + var source = """ + function getNullable(): string | null { return null; } + function f(x: string | null): number { + x = getNullable(); + return x.length; + } + """; + + var ex = Assert.ThrowsAny(() => TestHarness.RunInterpreted(source)); + Assert.Contains("null", ex.Message); + } + + [Fact] + public void Reassign_ConditionalEscapeToOtherMember_StaysSound() + { + // Soundness guard: a CONDITIONAL reassignment that escapes the guard narrowing to a different + // member must not let the post-branch read assume the assigned member. After the `if`, `x` is + // `string | number`, so `x.toFixed` (string has none) is correctly rejected — the narrowing + // must not persist past the join. + var source = """ + function f(x: string | undefined | number, cond: boolean): void { + if (typeof x === "string") { + if (cond) { x = 42; } + x.toFixed(2); + } + } + """; + + Assert.ThrowsAny(() => TestHarness.RunInterpreted(source)); + } +} diff --git a/TypeSystem/TypeChecker.Expressions.cs b/TypeSystem/TypeChecker.Expressions.cs index 525878ba..9591d236 100644 --- a/TypeSystem/TypeChecker.Expressions.cs +++ b/TypeSystem/TypeChecker.Expressions.cs @@ -1582,15 +1582,36 @@ private TypeInfo CheckAssign(Expr.Assign assign) // its descendants, AND widen any enclosing lexical narrowing the context stack can't reach. // `if`-guard variable narrowing lives in a child TypeEnvironment that a nested block's own // env shadows-then-discards, so without WidenEnclosingNarrowing a reassignment inside a - // nested branch leaves the outer guard's narrowing in place for later statements (#570). + // nested branch leaves the outer guard's narrowing in place for later statements (#570/#654). InvalidateNarrowingsFor(assignedPath); WidenEnclosingNarrowing(assign.Name.Lexeme, declaredType); } - // Also restore the declared type in the environment to undo any variable narrowing - // that was applied via TypeEnvironment.Define() in control flow statements. - // This ensures subsequent uses of the variable see the correct (un-narrowed) type. - _environment.Define(assign.Name.Lexeme, declaredType); + // Restore the environment binding, undoing any control-flow narrowing applied via + // TypeEnvironment.Define(). For a tracked (function-local/parameter) variable, restore it to + // the post-write flow-narrowed type (#653): subsequent reads see the declared type filtered to + // the members the RHS can be, mirroring the property-write narrowing of #48 (`o.x = "s"` + // narrows `o.x` to `string`). The narrowed binding lives in the current lexical scope and is + // discarded at its block's join, so it does not leak a too-narrow type past a conditional. + // + // Two guards keep this sound and non-regressive: + // * Only TRACKED (function-local/parameter) variables narrow. Module/top-level variables are + // not in the declared-type stack, so GetDeclaredType falls back to the environment for + // them — narrowing the binding would then corrupt the declared type a later assignment + // checks against. + // * A purely-nullish narrowed slot (`null`/`undefined`) is NOT installed; the variable keeps + // its declared type. Property access on a bare `null`/`undefined` type is not yet flagged + // (only on a union containing them — see CheckGetOnUnion), so narrowing `x = undefined` to + // `undefined` would silently drop the "possibly undefined" error that a later `x.length` + // must still raise (#570/#556). Keeping the declared union preserves that diagnostic. + var postAssignType = declaredType; + if (IsDeclaredTypeTracked(assign.Name.Lexeme) + && NarrowToDeclaredSlot(declaredType, valueType) is { } narrowedSlot + && !IsPurelyNullish(narrowedSlot)) + { + postAssignType = narrowedSlot; + } + _environment.Define(assign.Name.Lexeme, postAssignType); // tsc narrows a reference whose declared type is a bare type parameter by assignment: // the constraint is the narrowing domain, so after `x = y` the reference reads as the diff --git a/TypeSystem/TypeChecker.cs b/TypeSystem/TypeChecker.cs index 79cc9e66..3caa2e1b 100644 --- a/TypeSystem/TypeChecker.cs +++ b/TypeSystem/TypeChecker.cs @@ -273,25 +273,44 @@ private void InvalidatePropertyNarrowingsFor(Narrowing.NarrowingPath basePath) } /// - /// Resets any active lexical () narrowing of - /// in the ENCLOSING scope that holds it back to . if-guard + /// Resets active lexical () narrowings of in + /// the ENCLOSING scopes that hold them back to . if-guard /// variable narrowing is applied by redefining the variable in the guard's child environment; when /// a reassignment that escapes the narrowing happens inside a further-nested block, that block's /// environment is discarded at the join, so the outer guard narrowing would otherwise survive into - /// later statements (the #570 soundness gap). Widening the guard's environment closes that gap. + /// later statements (the #570 soundness gap). Widening the guards' environments closes that gap. /// The reassignment's own (current) scope is widened separately by the caller's - /// _environment.Define; this walks strictly OUTWARD to the nearest enclosing scope that - /// rebound the name, which for an outer guard is its narrowing override. + /// _environment.Define; this walks strictly OUTWARD. + /// + /// It widens EVERY enclosing scope that holds a narrowing of the variable, not just the nearest: + /// when two (or more) guards narrow the SAME variable and the escaping reassignment sits under the + /// inner one, widening only the inner guard left the outer guard's narrowing stale, so a read at + /// the outer level after the inner block still saw it (#654). The walk stops at the variable's + /// declaration — whose binding is the full declared type, since guards only ever install proper + /// subtypes — so it never crosses into an OUTER, same-named shadowing variable whose own narrowing + /// is still valid. + /// /// private void WidenEnclosingNarrowing(string name, TypeInfo declaredType) { for (TypeEnvironment? env = _environment.Enclosing; env != null; env = env.Enclosing) { - if (env.IsDefinedLocally(name)) - { - env.Define(name, declaredType); + if (!env.IsDefinedLocally(name)) + continue; + + var local = env.Get(name); + + // A binding that isn't assignable to the declared type belongs to a different + // (shadowing) variable of the same name — stop before touching it. + if (local != null && !IsCompatible(declaredType, local)) + return; + + env.Define(name, declaredType); + + // Reached the declaration (its binding is the full declared type): nothing further + // out narrows THIS variable, so stop rather than widen an outer shadowing variable. + if (local == null || TypeInfoEqualityComparer.Instance.Equals(local, declaredType)) return; - } } } @@ -339,6 +358,36 @@ private void RecordDeclaredType(string name, TypeInfo type) return _environment.Get(name); } + /// + /// Whether has a recorded declared type in the current function's + /// declared-type stack (rather than only living in the environment). Function locals and + /// parameters are tracked; module/top-level variables are NOT, so + /// falls back to the environment binding for them. Post-write variable narrowing (#653) replaces + /// the environment binding with the narrowed type, so it must only run for tracked variables — + /// otherwise a later assignment's would read the narrowed type as + /// the "declared" type and wrongly reject a valid reassignment to another union member. + /// + private bool IsDeclaredTypeTracked(string name) + { + foreach (var scope in _declaredVariableTypesStack) + if (scope.ContainsKey(name)) return true; + return false; + } + + /// + /// Whether is null, undefined, or a union composed only of + /// those. Property access on such a bare nullish type is not yet rejected by the checker (only a + /// union that ALSO has a real member is — see CheckGetOnUnion), so post-write variable + /// narrowing (#653) refuses to narrow a variable down to one, lest it drop a "possibly null/ + /// undefined" diagnostic a later access must still raise. + /// + private static bool IsPurelyNullish(TypeInfo type) => type switch + { + TypeInfo.Null or TypeInfo.Undefined => true, + TypeInfo.Union u => u.FlattenedTypes.All(IsPurelyNullish), + _ => false + }; + /// /// Invalidates property narrowings for an object when it's passed to a function /// that might mutate it. Only affects mutable property narrowings, not the object itself.