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.