fix: reapply tracer circular dependency fix without breaking if-block narrowing#3436
Open
RomanSpector wants to merge 2 commits into
Open
fix: reapply tracer circular dependency fix without breaking if-block narrowing#3436RomanSpector wants to merge 2 commits into
RomanSpector wants to merge 2 commits into
Conversation
… in tracer" This reverts commit e041833. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The _compilingAssigns guard (3.18.1) prevented stale incomplete types from propagating when calcNode re-enters an assign that is still being compiled, but the skipped lookIntoBlock left nodes[subBlock] unset: the if-handler then merged the branch entry node instead of the assignment result, and every node cached until the outer compile finished kept that poisoned narrowing (e.g. reassignment in an if-block was lost for all code after the block once a position inside the branch had been compiled first). This is why the fix was reverted in 3.18.2. Track in-flight assigns per coroutine (so an abandoned coroutine cannot leak the flag) and count re-entrant hits; when the outer calcNode finishes compiling and a re-entrant hit happened meanwhile, drop the tracer node/mark caches so later lookups recompute with the final type. Adds dual-query regression tests that compile a position inside the branch before a position after the block, matching the compile order of a real editor session that single-query tests cannot reproduce. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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.
Summary
3.18.2 reverted the tracer fix from #3392 ("Fix type inference for and/or idioms and circular dependency in tracer", 46d0faf) because the
_compilingAssignsguard broke type narrowing in if-blocks that reassign the variable. This PR reapplies that fix and resolves the regression that caused the revert, restoring the fixes for #2236, #2374 and #2494 (all three are broken again on current master).Root cause of the regression that triggered the revert
The guard fired when
calcNodere-entered an assign that was still being compiled — this happens when the assigned value's type depends on the variable itself (e.g.x = x .. 's'). On the re-entrant path it skippedlookIntoBlock, sonodes[subBlock]was left unset; theifhandler then fell back to the branch entry node, dropping the assignment's effect, and the outer walk cached that poisoned narrowing for every position after the block.The bug was invisible to the test suite: it only manifests when a position inside the branch is compiled before a position after the block — the normal compile order of an editor session, but something single-query-per-file tests never do. That is why CI stayed green while the editor misbehaved:
Fix
calcNodenow tracks in-flight assigns per coroutine (weak-keyed bycoroutine.running(), so an abandoned coroutine can no longer permanently leak the in-flight flag) and counts re-entrant hits. When the outer call finishes compiling and a re-entrant hit happened meanwhile, the tracer drops itsnodes/markcaches, so every later lookup recomputes with the final type instead of the poisoned intermediate state. Purges are rare (only self-referential assignments reachable from control-flow handlers) and terminate becausevm.compileNoderesults are cached, so a source can trigger at most one re-entrant compile.Tests
test/type_inference/reassign_narrow.luawith a dual-query harness that queries a position inside the branch before a position after the block, reproducing the editor compile order:unknowninside the branch andnilafter it (the reverted Type is "lost" of variable within For Loop #2374/Can not infer basic assignments in nested loop. #2494 behavior);integer?instead ofstring?) — the regression described in the 3.18.2 changelog.bin/lua-language-server.exe test.lua).Closes #2236, closes #2374, closes #2494
🤖 Generated with Claude Code