Narrow three more flow-sensitive assignment shapes - #1308
Open
apiology wants to merge 19 commits into
Open
Conversation
…literal type A parameter's typify always returned its declared @PARAM type once available, without ever consulting the types of its reassignments. Reassigning a parameter to the result of a call that narrows its type (e.g. a union normalized down to one member) was silently ignored, so later uses kept the stale declared type and got flagged against branches of the original union that could no longer occur. Track whether an assignment is guaranteed to have executed (definite) via a new Region#conditional flag, threaded through node processors for if/unless, while/until, when, rescue, block bodies, &&/||, and ||=. Pin::Parameter#typify now prefers the reassigned type over the declared type when the reassignment is definite, and continues to fall back to the declared type (as before) when it's only conditional, matching the existing union semantics for plain local variables. Fixes castwide#1250 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VHyn8dc8oSqcQJrXFgDWUo
`x = x.length` (or `index += 1` desugared to `index = index + 1`) resolved the RHS's reference to `x` against the type of the value being derived on that same line, instead of `x`'s prior type - `x.length` was resolving as `Integer#length` instead of `String#length`, since var_at_location/visible_at? treated any position from the start of the reassignment onward (including positions inside its own RHS) as already reflecting the new value. BaseVariable#visible_at? now excludes positions that fall strictly inside one of the pin's own assignment value nodes, so a self-referential RHS resolves against the variable's other assignments instead of the not-yet-computed value being derived. Reported against castwide#1282: castwide#1282 (comment)
Pin::Parameter#typify already preferred a definite reassignment's type over the declared @PARAM type, but plain local variables and instance variables kept unioning every assignment's type together instead, so `local = 5; local = 'hello'; local.upcase` (and the same pattern for an ivar reassigned within one method) still failed at strong: the combined pin's type came out as `Integer, String` instead of just `String`. BaseVariable#combine_assignments unconditionally unioned two pins' assignment nodes, and combine_with separately re-prepended the earlier pin's `assignment:` onto the merged list regardless. Make combine_assignments drop the earlier assignment(s) when the later pin's reassignment is definite (guaranteed to have executed) and in the same closure, and skip the redundant `assignment:` prepend in that case. Self-referential reassignments (`x = x.foo`, desugared `+=`, etc.) are excluded from the override: resolving their right-hand side needs the prior assignment(s) as a base case, so dropping them would leave nothing to resolve against. Un-pends three specs that were already asserting this behavior under 'sequential assignment support' and adds a spec for the reported local-variable case. The cross-method ivar case (assigned in `initialize`, reassigned in another method) is not addressed here - ivasgn_node.rb sets neither `presence:` nor `definite:`, so every ivar pin remains visible everywhere and `definite` defaults to true even inside conditionals. Addresses review feedback on castwide#1282: castwide#1282 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HKWGJjqfJuQFssuXEWLLMZ
…nsitive narrowing FlowSensitiveTyping#find_var used Array#find, returning the first local/ivar pin matching a variable name whose presence includes the query position. For `x = nil; x = 1; if x; ...`, both the original declaration and the reassignment have presences that include the `if` guard's position, so `find` always returned the stale `x = nil` pin instead of `x = 1`. That pin then got downcast and merged back into `locals` for narrowing, and because BaseVariable#override_assignments? (from the reassignment-override work) lets a later definite assignment supersede rather than union, the merge dropped the `x = 1` assignment and re-surfaced `nil` - regressing local variable inference to `undefined` at `y = x * 2`. find_var now picks the pin with the latest presence start among matches, and excludes any pin whose own assignment is still being evaluated at the query position (made BaseVariable#within_own_assignment? public so find_var can reuse the same check combine_with already relies on). This does not address the equivalent case for instance variables inside a conditional (e.g. `@x = nil; @x = 1; if @x; @x * 2; end`): ivar pins never get a `presence` range (ivasgn_node.rb doesn't set one, since an ivar stays visible across the whole class, so find_var's presence-based tie-break can't distinguish them, and the same stale-pin problem still surfaces via a separate path (Chain::InstanceVariable re-fetches raw ivar pins from the store rather than using FlowSensitiveTyping's narrowed list). That gap predates this fix and needs presence tracking for ivars to resolve; the regression reported in the PR comment was local-variable-only. Fixes castwide#1282 (review comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKhmGqQnKzRc89LEd8n7Ve EOF )
…arrowing infer_from_return_nodes filtered candidate locals to only those visible at the return node's own end position before resolving its type chain. A flow-sensitive downcast (e.g. narrowing a nilable parameter across the rhs of val.nil? || val < 5) has a presence range scoped to that sub-expression, which ends before the end of an enclosing expression like !(...). The pre-filter dropped the narrowed local outright, even though chain resolution already re-checks each local's presence at its own precise sub-node location. Pass the full local set instead and let that per-node check do the filtering. Fixes the regression reported at castwide#1282 (comment) Also drops two @sg-ignore comments that the fix's improved inference made unneeded (Cursor#end_of_word, SourceChainer#end_of_phrase). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A6t6f1rQ26s9o6sP7QUFxE
…y it A reassignment inside an if/while/until/block/rescue/&&/||/||= body was never eligible to override an earlier assignment's type, even at a use site later in the same branch that the reassignment provably dominates. Only presence-inclusion was checked, not whether the branch that skips the reassignment could also have reached the use site. Region now tracks the source range of the nearest enclosing conditional construct's body (conditional_boundary) instead of a bare boolean, and BaseVariable pins carry that range as conditional_override_boundary. When resolving a variable at a specific location, a non-definite pin still overrides an earlier one if the location falls inside its conditional_override_boundary - i.e. the same branch, after the reassignment - while remaining merely unioned with the earlier type for any use site outside that boundary (e.g. after the branch merges back). Fixes the case reported in castwide#1282 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YbhZvdCv7xdziXyKJiPuGk
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
Add a CompoundStatement parent chain and use it for reassignment override eligibility
Region#conditional was a separate boolean threaded alongside compound_statement, requiring every node processor to pass both in lockstep (e.g. block_node.rb: compound_statement: block_pin, conditional: true). Keeping two parallel values in sync at every call site is exactly the kind of duplication this refactor set out to remove, and it's the shape of bug that broke Block handling mid-refactor (definite briefly, incorrectly, derived from compound_statement.is_a?(Closure), which is true for Block despite a block body running zero or many times). conditional is now a constructor attribute on Pin::CompoundStatement itself, set once where each construct is built (Pin::Block.new(..., conditional: true), Pin::Method.new(...) defaulting false), so there's only one thing to get right per site instead of two. It can't be a class-level constant: the bare Pin::CompoundStatement class is used both for an if's own condition (never conditional) and for then/else/rhs/rescue bodies (always conditional) - same class, different instances, different answers - so it stays an instance attribute, same as closure:/compound_statement: already are. lvasgn_node.rb's definite computation becomes a single-hop read: `!region.compound_statement.conditional`, no separate Region field. Pin::CompoundStatement#combine_with merges the new attribute via `choose`, since two versions of the same construct should already agree on it. Verified: full suite (1638 examples, 0 failures), typecheck self-check diffed clean against the prior baseline (587 problems, unchanged), rubocop clean on touched files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YbhZvdCv7xdziXyKJiPuGk
The default-argument idiom - `tasks = ['a'] if tasks.nil?` followed by `tasks.each` - still reported `Unresolved call to each on Array<String>, nil`. PR castwide#1282 covered the dominance case (a use site inside the branch the reassignment dominates); here the use site is *after* the conditional, so what establishes the type on the path where the assignment did not run is the guard's condition, not dominance. At a merge point after an `if`, the incoming paths are (a) the clause ran and assigned a new value - already handled, that pin is unioned in - and (b) the clause did not run, leaving the original value, about which the condition tells us something. Path (b) was never asserted, so the original `Array<String>, nil` was unioned in unnarrowed. FlowSensitiveTyping#process_if now also asserts the opposite branch's condition facts over the rest of the enclosing compound statement, for the variables the clause definitely reassigns. Reusing #process_expression for that gets `&&`/`||`/`!` handling for free, including `and`'s deliberate refusal to propagate false-facts. The restriction to definitely-reassigned variables is what keeps this sound. Facts are filtered by variable name in #add_downcast_var, driven by a second FlowSensitiveTyping built over the same locals/ivars arrays with `restricted_names:` set. Without it, `xs = [] if xs.nil? || ys.nil?` would also narrow `ys` after the conditional, even though only `xs` was replaced. Likewise, only unconditional `lvasgn`/`ivasgn` in the clause count: an assignment nested in another conditional, or an `||=`, may leave the previous value in play. Guards that test something other than the variable (`tasks = ['a'] if flag`) and nil guards that don't reassign (`puts 'hi' if tasks.nil?`) keep nil in the type, as they must; specs cover both, plus the non-modifier `if`, `unless`, and else-clause forms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
The ignore added with the fix carried a one-off description. rules.rb keeps a tally of @sg-ignore texts grouped into buckets, so a novel string creates a bucket of one instead of joining an existing count. Reuse the established "Need to add nil check here" wording, matching this file's three sibling ignores on Range.from_node results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
A modifier-if guard stopped being applied once the variable it guards
had been reassigned:
got = lookup(name)
return got.length if got # asserts got is nil/false below here
got = lookup(name)
got.length if got # Unresolved call to length on nil, Boolean
The first guard's `return` leaves the method, so FlowSensitiveTyping
asserts the false branch's facts - `got` is `nil, false` - over the rest
of the compound statement, and that downcast pin's presence runs to the
end of the method. The second `got = lookup(name)` overwrites the value
the fact was about, but ApiMap#var_at_location still combined the stale
pin in: Pin::BaseVariable#combine_with already let a definite
reassignment supersede the earlier pin's *assignments*, yet unioned
intersection_return_type and exclude_return_type unconditionally. The
`nil, false` intersection survived and intersected the new value down to
nothing.
Narrowing recorded against a value expires when that value is definitely
overwritten, so when #override_assignments? says `other` supersedes us,
keep only `other`'s intersection/exclude types instead of unioning ours
in.
#references_name? then blocked the supersede in the shape this was
actually observed in, `lib/solargraph/workspace/gemspecs.rb`:
specish = all_gemspecs_from_bundle.find { |specish| specish.name == name }
return to_gem_specification specish if specish
The self-reference exclusion exists so `x = x.foo` keeps the assignment
its own right-hand side resolves against, but a block parameter of the
same name shadows the outer variable for the whole block - the mention
inside the body is the parameter, not the variable being assigned. The
walk now descends only into a shadowing block's receiver, which is still
evaluated outside the block.
Two @sg-ignore comments in gemspecs.rb are no longer needed and are
removed. Facts stay in force up to the reassignment, and a reassignment
that only runs in a nested branch still does not supersede; specs cover
both, plus a guard on an unrelated variable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
A reassignment made inside a branch was ignored by a use site later in
that same branch:
def clean(items) # @PARAM items [Array<String>, nil]
if items.nil?
items = fetch_items
items.reject! { |i| i.empty? } # Unresolved call to reject! on nil
end
end
Pin::Parameter#typify prefers a reassignment's inferred type over the
declared @PARAM type only when the reassigning pin is `definite`, and an
assignment inside an `if` body is not definite - it may never run.
#override_assignments? already handles that distinction for a specific
position via #definite_reaches?: the use site falls inside the
CompoundStatement the assignment was made in, so on every path that
reaches it the assignment ran. But that verdict only reached
#combine_assignments; the combined pin still carried
`definite: definite || other.definite`, which was false on both sides,
so #typify fell back to the declared type and kept nil in the union.
The combined pin is built for one resolved location, so when the
supersede check passes there, the result is definite at that location.
ApiMap#var_at_location is the only caller that passes a location, so
locationless combines are unaffected: without one, #override_assignments?
already requires `other.definite`.
A reassignment nested in a further conditional, and a use site earlier in
the branch than the reassignment, both still keep the original type;
specs cover each.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
The assignment-as-condition idiom asserted nothing about the variable it
assigns:
if (md = name.match(/\[(.*)\]/))
md[1].to_i # Unresolved call to []
else
0
end
Two things were missing. FlowSensitiveTyping#process_expression handled
:send, :and, :or and bare variable references, but not the one-child
:begin that parentheses produce, nor :lvasgn/:ivasgn - so the condition
was walked past without a fact being recorded. An assignment used as a
condition evaluates to the value assigned, so the branches say the same
thing about the variable as a bare reference would: not nil where the
condition held, `nil, false` where it did not.
Adding those handlers alone changed nothing, because IfNode#process ran
FlowSensitiveTyping *before* processing the condition node. The pin for
`md` is created by that condition, so #find_var had nothing to look up
and the facts were dropped. The FlowSensitiveTyping call now runs after
the condition is processed; the then/else clauses are still processed
after it, as before.
`if (md = ...) || fallback` stays unnarrowed without further work:
#process_or deliberately passes no true ranges down to its operands,
since either side alone may be what made the disjunction true. In the
else clause the variable is correctly narrowed to `nil, false` instead.
Four @sg-ignore comments in position.rb are no longer needed and are
removed.
WhileNode#process has the same FlowSensitiveTyping-before-condition
ordering, so `while (x = f.gets)` still misses this when `x` has no
earlier assignment; left alone here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
The integration branch renders a falsy-only receiver as `nil, false` where this branch renders `nil, Boolean`, so three exact-message assertions passed on each branch and failed on the merge. The property under test is that exactly one problem remains and its receiver is narrowed to the falsy types - not which of the two spellings the formatter picks - so match either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
The supersede-expiry rule was too broad. #override_assignments? is true
whenever `other`'s assignment is definite (or dominates the resolved
location) and does not reference us - including when `other` is another
flow-sensitive downcast of the *same* assignment. Those pins are not
competing values; they are separate facts about one value, and dropping
ours lost information:
a = lookup(name) # String, Integer, nil
a = 'd' if a.nil? || a.is_a?(Integer)
a # String, nil - nil survived
#process_or asserts the false branch of every operand, so the guard
produces one downcast excluding nil and another excluding Integer, both
derived from the `a = lookup(name)` pin. ApiMap#var_at_location folds
them in order; the second supersede replaced the first pin's exclusions
instead of adding to them, so only the last operand's fact reached the
use site.
Facts now expire only when `other`'s assignments are at different source
positions than ours. Position, not structural node equality: `AST::Node#==`
compares type and children, so two textually identical assignments on
different lines compare equal - and telling exactly those apart is what
the original fix is for (`got = lookup(name)` twice, with a guard between
them, is its regression spec).
Only the fact attributes use the narrower test. Assignment supersession
is unchanged: when the sites match, `combine_assignments` replacing our
assignments with an identical list was already a no-op.
Two operands hid this - one fact, nothing to drop - so it surfaced only
against a branch whose `==` handling contributes a second exclusion.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
The three-operand regression this follows was invisible to the existing suite: two-operand or-guards were covered, and at two operands there is only one flow-sensitive fact to fold, so nothing can be wrongly dropped. Add the four-operand case, and two negative controls that were verified by hand but never asserted. The controls matter more than the positive case. `¬(x || y)` implies every operand is false, so the guard's false path may narrow any variable it tests - but its true path only reassigns one. Nothing may be concluded about a second variable the guard merely mentions, nor about a variable the guard never tests. Without these, a future over-narrowing change would pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MEDQFCJ2M7gaYkUQVpiQzn
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.
Stacked on #1282 — please review that one first; these are three of its neighbours.
#1282 and its follow-on narrow a parameter after a definite reassignment, and after a nil-guarded default (
x = default if x.nil?) used past the conditional. Three adjacent shapes still failed. Each is reduced from a real suppression in a consuming codebase, and each is fixed in its own commit.1. Narrowing outlived a definite reassignment. After a guarded
returnplants a downcast, an unconditional reassignment superseded the earlier pin's assignments butcombine_withstill unionednarrowed_return_type/exclude_return_type, so the old value's falsy residue outlived the value it described —Unresolved call to length on nil, Boolean. Those two are now taken from the superseding pin alone. Second half:references_name?counted a same-named block parameter as a self-reference (find { |specish| specish.name == name }), blocking the supersede; the walk now descends only into a shadowing block's receiver, which is evaluated outside the block.2. A dominating reassignment wasn't treated as definite.
override_assignments?already resolved the location-specific case viadefinite_reaches?, but that verdict only reachedcombine_assignments— the combined pin keptdefinite: definite || other.definite, false on both sides, soPin::Parameter#typifyfell back to the declared@paramtype. Sound becauseApiMap#var_at_locationis the only caller passing alocation; without one,override_assignments?already requiresother.definite.3. A variable assigned inside an
ifcondition.if (md = name.match(/…/))—process_expressionhandled neither the one-child:beginthat parentheses produce nor:lvasgn/:ivasgn. Adding those handlers alone changed nothing, becauseIfNode#processranFlowSensitiveTypingbefore processing the condition, so the pin the condition creates didn't exist yet andfind_varreturned nil. The call now runs after the condition.Every fix ships with a negative control that must keep erroring, and all of them still do: a trailing
if other_thing; a use site between guard and reassignment; a reassignment only in a nested branch; a use before the assignment; and(md = …) || fallback, which stays unnarrowed becauseprocess_ordeliberately passes no true ranges to its operands.Effect on this repo's own suppressions: two
@sg-ignoremarkers removed fromworkspace/gemspecs.rband four fromposition.rb, all confirmed Unneeded at strong level rather than deleted on faith. Whole-repotypecheck --level strongproduces an identical sorted problem set to the base, every delta accounted for by line renumbering. Full suite green.One marker I restored rather than removed: dropping the
to_specone ingemspecs.rbsurfaced a liveUnresolved call to to_spec, so its "Unneeded" report was itself wrong — worth knowing that the Unneeded signal is not always reliable.Not addressed, flagged for a follow-up:
WhileNode#processhas the identical FST-before-condition ordering, sowhile (x = f.gets)still misses shape 3 whenxhas no earlier assignment.Authored by Claude (Anthropic's Claude Code) on behalf of @apiology.