Iterate antecedent pass so self-assignment in a loop correctly narrows - #64295
Open
milkcask (milkcask) wants to merge 2 commits into
Open
milkcask (milkcask) wants to merge 2 commits into
milkcask (milkcask) wants to merge 2 commits into
Conversation
A union-typed variable reassigned in a loop from an expression that reads it, such as `cur = cur.next`, now narrows to every constituent the loop can reach. Previously the loop body saw one step less than it should: for a chain of three classes `cur` was typed as the first two, so a property present only on the third was rejected and a property missing from it was accepted. Longer chains lost exactly the same step. The antecedent pass over a loop label is one step of an approximation toward the correct type, and, since 4.3.5, it was only ever run once. To type the assignment it asks the expression checker for the assigned type, which starts a nested flow analysis of the same variable. That analysis reaches the loop label still on the flow loop stack, takes the partial union gathered so far, and returns it with its incomplete marker dropped at the analysis boundary. The label unions that in and caches the result as final, so the number of narrowing steps was the depth of that recursion rather than the number needed to converge. Add a `reentered` field to `FlowLoopInfo`, the flow loop stack entry. It is set when the checker computes the flow type of a variable while that variable's own assignment is being evaluated, as in `currentParent = currentParent.parent`. The antecedent pass is then wrapped in an outer loop: push the loop label flow node onto the flow loop stack, run the pass, and if the `reentered` flag was set and the resulting union grew, run the pass again, as a fixed-point iteration, until nothing is added. At that point the approximation is exact. Only union declared types iterate, since only they narrow by assigned type; re-seeding an evolving array would degrade it to `any[]`. Passes are bounded by the union's size, falling back to the declared type. Add a test with chains of 3, 4 and 50 levels, a discriminant narrowing that used to become `never`, a property access that must error, and an evolving array that must keep its element type. Fixes microsoft#59715
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Nested loop cache entries can remain stale across fixed-point retries and preserve incorrect narrowing.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes loop self-assignment narrowing by iterating flow analysis toward a fixed point.
Changes:
- Tracks re-entry into partially analyzed loop flow.
- Adds convergence logic and regression coverage for varied chain depths and evolving arrays.
File summaries
| File | Description |
|---|---|
tsc/internal/checker/checker.go |
Adds loop re-entry state. |
tsc/internal/checker/flow.go |
Implements iterative antecedent analysis. |
tsc/testdata/tests/cases/conformance/controlFlow/controlFlowLoopSelfReferentialAssignment.ts |
Adds regression scenarios. |
tsc/testdata/baselines/reference/conformance/controlFlowLoopSelfReferentialAssignment.types |
Records inferred types. |
tsc/testdata/baselines/reference/conformance/controlFlowLoopSelfReferentialAssignment.symbols |
Records resolved symbols. |
tsc/testdata/baselines/reference/conformance/controlFlowLoopSelfReferentialAssignment.errors.txt |
Records the expected soundness error. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cache entries from partial passes remain stale on the restart bailout and convergence fallback paths.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
tsc/internal/checker/flow.go:1431
- The convergence fallback replaces this junction's result with the declared type but leaves inner-loop cache entries and shared-flow results from the final partial pass intact. Those entries were computed from the pre-fallback approximation and can later return narrower stale types. Apply the same per-pass invalidation before breaking on the bound.
if pass >= maxPasses {
result = f.declaredType
break
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Add inner loop test.
milkcask (milkcask)
force-pushed
the
fix/59715-loop-fixpoint
branch
from
September 16, 2026 06:28
bdab1a0 to
cb298e9
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The change modifies recursive compiler flow analysis and cache behavior, warranting final human validation despite comprehensive tests.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #59715, see full discussion there.
A union-typed variable reassigned in a loop from an expression that reads it, such as
cur = cur.next, now narrows to every constituent the loop can reach. Previously the loop body saw one step less than it should: for a chain of three classescurwas typed as the first two, so a property present only on the third was rejected and a property missing from it was accepted. Longer chains lost exactly the same step.The antecedent pass over a loop label is one step of an approximation toward the correct type, and, since 4.3.5, it was only ever run once. To type the assignment it asks the expression checker for the assigned type, which starts a nested flow analysis of the same variable. That analysis reaches the loop label still on the flow loop stack, takes the partial union gathered so far, and returns it with its incomplete marker dropped at the analysis boundary. The label unions that in and caches the result as final, so the number of narrowing steps was the depth of that recursion rather than the number needed to converge.
Add a
reenteredfield toFlowLoopInfo, the flow loop stack entry. It is set when the checker computes the flow type of a variable while that variable's own assignment is being evaluated, as incurrentParent = currentParent.parent. The antecedent pass is then wrapped in an outer loop: push the loop label flow node onto the flow loop stack, run the pass, and if thereenteredflag was set and the resulting union grew, run the pass again, as a fixed-point iteration, until nothing is added. At that point the approximation is exact. Only union declared types iterate, since only they narrow by assigned type; re-seeding an evolving array would degrade it toany[]. Passes are bounded by the union's size, falling back to the declared type.Add a test with chains of 3, 4 and 50 levels, a discriminant narrowing that used to become
never, a property access that must error, and an evolving array that must keep its element type.A reference fix for the old js checker is at milkcask@ad43e66. Both checkers give identical numbers when run over the
stradacompiler sources: 10,782 loop labels, 445 with a second pass, none with a third, no result changed, and check time within noise ofmain.Assisted by Copilot and Claude Code. They really struggled to produce a human readable write-up, so I have to write it, and stand ready to defend it.