Fix #653/#654: variable reassignment flow narrowing + widen all enclosing guards on escape

SharpTS.Tests/TypeCheckerTests/NarrowingTests/NestedReassignmentNarrowingTests.cs

@@ -127,4 +127,67 @@ function h(x: string | undefined, cond: boolean): number {
             """;
         Assert.ThrowsAny<TypeCheckException>(() => 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<TypeCheckException>(() => 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<TypeCheckException>(() => 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));
+    }
 }

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;
+
+/// <summary>
+/// Tests for flow narrowing across VARIABLE assignments (issue #653) — the symmetric companion to
+/// the property-write narrowing of #48 (see <see cref="AssignmentFlowNarrowingTests"/>). TypeScript's
+/// CFA narrows a reference to the assigned value's type after a write, so <c>x = "s"</c> on a
+/// <c>string | null</c> variable lets the subsequent <c>x.length</c> type-check. SharpTS previously
+/// restored the full declared type after every variable assignment, rejecting code <c>tsc</c> accepts.
+///
+/// The narrowed type is the declared union filtered to the members the RHS can be
+/// (<c>NarrowToDeclaredSlot</c>), 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.
+/// </summary>
+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<TypeCheckException>(() => 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<TypeCheckException>(() => 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<TypeCheckException>(() => 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<TypeCheckException>(() => TestHarness.RunInterpreted(source));
+    }
+}

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

TypeSystem/TypeChecker.cs

@@ -273,25 +273,44 @@ private void InvalidatePropertyNarrowingsFor(Narrowing.NarrowingPath basePath)
     }
 
     /// <summary>
-    /// Resets any active lexical (<see cref="TypeEnvironment"/>) narrowing of <paramref name="name"/>
-    /// in the ENCLOSING scope that holds it back to <paramref name="declaredType"/>. <c>if</c>-guard
+    /// Resets active lexical (<see cref="TypeEnvironment"/>) narrowings of <paramref name="name"/> in
+    /// the ENCLOSING scopes that hold them back to <paramref name="declaredType"/>. <c>if</c>-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
-    /// <c>_environment.Define</c>; this walks strictly OUTWARD to the nearest enclosing scope that
-    /// rebound the name, which for an outer guard is its narrowing override.
+    /// <c>_environment.Define</c>; this walks strictly OUTWARD.
+    /// <para>
+    /// 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.
+    /// </para>
     /// </summary>
     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);
     }
 
+    /// <summary>
+    /// Whether <paramref name="name"/> 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 <see cref="GetDeclaredType"/>
+    /// 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 <see cref="GetDeclaredType"/> would read the narrowed type as
+    /// the "declared" type and wrongly reject a valid reassignment to another union member.
+    /// </summary>
+    private bool IsDeclaredTypeTracked(string name)
+    {
+        foreach (var scope in _declaredVariableTypesStack)
+            if (scope.ContainsKey(name)) return true;
+        return false;
+    }
+
+    /// <summary>
+    /// Whether <paramref name="type"/> is <c>null</c>, <c>undefined</c>, 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 <c>CheckGetOnUnion</c>), 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.
+    /// </summary>
+    private static bool IsPurelyNullish(TypeInfo type) => type switch
+    {
+        TypeInfo.Null or TypeInfo.Undefined => true,
+        TypeInfo.Union u => u.FlattenedTypes.All(IsPurelyNullish),
+        _ => false
+    };
+
     /// <summary>
     /// 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.