Add a CompoundStatement parent chain and use it for reassignment override eligibility - #57
Merged
apiology merged 2 commits intoAug 14, 2026
Conversation
Region now tracks compound_statement (the nearest enclosing CompoundStatement pin - an if/when/while/until/rescue/&&/||/||= body, a method/block body, or a namespace body), threaded through Region#update the same way closure already is. Every construct that creates a CompoundStatement-family pin, or previously only threaded conditional_boundary with no corresponding pin, now sets this pointer, giving every CompoundStatement pin a real link to its immediate parent instead of only the coarser closure chain (which already skips non-scope-forming branches like if-bodies). Pin::Base#closure becomes @closure || <derived by walking the compound_statement chain to the nearest ancestor that is_a?(Closure)>, kept strictly as a fallback behind the stored value - hand-built pins that pass closure: directly and have no derivable chain (send_node.rb's synthetic attr_reader/attr_writer pins, args_node.rb, etc.) are untouched. Every pin built through Region-threaded node processors still passes closure: explicitly today, so this is a no-behavior-change infra addition, verified by a new spec asserting the derived value agrees with the stored one across nested if/while/block structures. Pin::CompoundStatement also gains its own combine_with/ combine_compound_statement for incremental-reparse merging, mirroring BaseVariable#combine_closure's location-based tiebreak rather than reusing choose_pin_attr_with_same_name (unsuitable since bare CompoundStatement pins all share name == ''). BaseVariable also gains a compound_statement reader, threaded from lvasgn_node.rb, unused by any override logic yet - preparation for a follow-up that rewrites override_assignments?/definite_reaches? to walk this chain instead of comparing conditional_override_boundary Ranges, removing that duplicate bookkeeping. See the discussion on castwide#1282 for the fix this builds on and the design rationale for this follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YbhZvdCv7xdziXyKJiPuGk
BaseVariable#definite_reaches? no longer compares a query Location against a separately-stored conditional_override_boundary Range. Instead it checks whether the location falls within this pin's own compound_statement's location range - the CompoundStatement pin already carries that range, and since a nested CompoundStatement's location is always a subrange of its parent's, this single containment check already accounts for arbitrarily nested branches without needing to walk the chain further. This removes the duplicate bookkeeping the original PR 1282 fix introduced: Region#conditional_boundary (a Range) and BaseVariable#conditional_override_boundary are gone, along with the Range.from_node(...) computation every conditional-construct node processor performed to populate them - that range is now read directly off the compound_statement pin instead of being computed a second time. lvasgn_node.rb's `definite` computation goes back to a plain Region#conditional boolean rather than `conditional_boundary.nil?` (and was briefly, incorrectly, tried as `compound_statement.is_a? (Closure)` during this rewrite - reverted because a block's body pin IS a Closure, for variable-scoping purposes, despite running zero or many times, which is exactly the case `conditional_boundary`/`conditional` exists to distinguish). Every closure-creating node processor (def_node.rb, defs_node.rb, namespace_node.rb) now explicitly resets `conditional: false` for its body, since entering a fresh method/namespace scope always runs its body top-to-bottom regardless of how the closure itself was reached, unlike a block. Added: - A loop-ordering regression test confirming a reassignment inside a while body doesn't affect a reference textually before it. - combine_with specs for Pin::CompoundStatement covering the location-based tiebreak and the nil-vs-non-nil case. Verified: full suite (1638 examples, 0 failures), typecheck self-check diffed against the pre-fix baseline (587 problems vs. 591 baseline - net fewer, since deleting the Range.from_node calls also removed several instances of the pre-existing nilable-AST-child pattern already tolerated throughout these files). Combines what were originally staged as two follow-up PRs into one - see castwide#1282 for the base fix and design discussion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YbhZvdCv7xdziXyKJiPuGk
apiology
changed the title
apiology
marked this pull request as ready for review
August 14, 2026 18:36
apiology
merged commit Aug 14, 2026
02708aa
into
fix-1250-parameter-reassignment-typing
27 of 28 checks passed
3 tasks
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
1 participant
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.
Follow-up to castwide#1282, stacked on its head branch. Design and rationale discussed at castwide#1282 (comment) and follow-up conversation.
Summary
PR 1282 fixed a bug where a type-changing reassignment inside a conditional branch didn't override the earlier assignment's type even at a use site inside the same branch, right after the reassignment. That fix worked but duplicated structure the codebase already partially has:
Pin::CompoundStatement("a series of statements where if a later one executes, all earlier ones did too") andPin::Closure < CompoundStatement("a CompoundStatement that is also a scope"). This PR removes that duplication.Region#compound_statement: the nearest enclosingCompoundStatementpin, threaded throughRegion#updatethe same wayclosurealready is - every construct that creates aCompoundStatement-family pin (or previously had no corresponding pin at all -and/or/orasgn/rescue bodies) now sets this pointer.Pin::Base#closurebecomes@closure || <derived from the compound_statement chain>, strictly as a fallback behind the stored value - no behavior change, since every pin built through Region-threaded node processors still passesclosure:explicitly.BaseVariable#definite_reaches?no longer compares a queryLocationagainst a separately-computedconditional_override_boundaryRange. It now checks containment directly against thecompound_statementpin's ownlocation.range- the same range, read once instead of computed twice.Region#conditional_boundary,BaseVariable#conditional_override_boundary, and theRange.from_node(...)call in every conditional node processor are gone.Region#conditional(a plain boolean) replaces the oldconditional_boundary.nil?check for computingdefinite- kept separate from thecompound_statementchain because a block body pin is aClosure(for scoping) but still runs zero-or-many times, whichconditional/compound_statement.is_a?(Closure)would conflate if collapsed into one signal.closurematches the stored one across nested if/while/block structures (caught a real threading gap before it shipped), a loop-ordering regression (a while-body reassignment must not affect a reference textually before it), andcombine_withcoverage forPin::CompoundStatement.Test plan
bundle exec rspec spec/- 1638 examples, 0 failuresbundle exec solargraph typecheck --level strong- diffed against the pre-1282-follow-up baseline: 587 problems vs. 591 baseline (net fewer - deleting the old Range computation also removed several instances of the pre-existing nilable-AST-child pattern already tolerated throughout these files)bundle exec rubocop- clean on touched filesCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com