Narrow locals after definite reassignment and in-condition assignment - #1338
Open
apiology wants to merge 39 commits into
Open
Narrow locals after definite reassignment and in-condition assignment#1338apiology wants to merge 39 commits into
apiology wants to merge 39 commits into
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
The 8 new "Need to add nil check here" markers this PR added in
if_node.rb, resbody_node.rb, and flow_sensitive_typing.rb all trace
to node.children[N] and share one literal error when stripped:
"received Array" instead of the actual element type. That is not a
missing nil check - it is Array[self]-typed methods behind an RBS
shim (Parser::AST::Node#children: () -> Array[self]) inferring
element access as Array itself rather than the substituted element
type. Confirmed against the equivalent plain-YARD Array<self> case,
which infers correctly, so the gap is specific to RBS's Array[self]
path.
Retagged with a shared slug and added the corresponding rules.rb
entry so it is trackable as one named limitation instead of eight
copies of a generic, misleading label. pin/method.rb's one new
marker was checked too and left alone - it strips to a genuinely
different error ("Unresolved call to filename on Solargraph::
Location, nil"), a real nilable-location case.
Typecheck problem count for the three retagged files is unchanged
(30, same before and after) since only the label text changed.
Region#closure and #compound_statement are tracked as two separate fields rather than deriving closure by walking compound_statement, because they change at different rates: closure only moves at a def/class/module/block boundary, while compound_statement moves at every conditional branch. Deriving closure by walking up on every pin construction would repeat a chain walk that is usually unchanged since the last pin push. Pin::Base#closure's own walk-up is the fallback for a pin built with no @closure of its own, not the normal path here. Renamed FlowSensitiveTyping's restricted_names to only_downcast_these_names - its one actual effect is gating add_downcast_var, not restricting facts in general as the old name implied. Expanded the comment on ResbodyNode's rescue_body_cs: it is not pushed onto pins because it has no identity worth indexing on its own, only value as the compound_statement every pin created below it carries, discarded once that recursive call returns. Clarified assert_after_guard's docstring: "assert" means apply an already-established fact, not perform a runtime check.
Remove an unneeded @sg-ignore in Formatting#log_corrections - no redefinition needing suppression exists there. Replace the two placeholder "Need to add nil check here" markers in ResbodyNode with a real local variable and nil guard around NodeProcessor.process. Reword Parameter#typify's reassignment comment so it no longer reads as a stray @PARAM tag. Point the api_map.rb and type_checker.rb "should be able to handle redefinition" markers at PR 1282, which is open and fixes that gap; drop type_checker.rb's copy entirely since it turned out unneeded. Restore the "unions rather than overrides" marker in api_map.rb#super_and_sub? to the line it actually suppresses (above the sup.literal? check, not the while loop below).
Undercover flagged three branches this PR's production code added with no test exercising them: the undefined-splat-target inference path in BaseVariable#probe when the source is not itself a tuple or container, and the narrowed_return_type/exclude_return_type equality branches in #downcast's generated #eql?. Cherry-picked from the spec/pin/base_variable_spec.rb portion of 0b97e98 on the 2026-08-04 integration branch, applied by hand since the file has diverged from that commit's base (spec/complex_type_spec.rb from the same commit belongs to a different PR and is not included here).
…push-1282-undercover
The prior commit copied the keyword narrowed_return_type from where the test originated, but this branch's BaseVariable#downcast never had that name - it names the same parameter intersection_return_type. Corrected the spec to match, and confirmed the passing test still checks distinct-value inequality via #eql?.
Only the identical-nil and both-non-nil-locations cases were tested. When exactly one side's compound_statement has no location (built without one, e.g. a bare if/while body), combine_with must still prefer the one that has a location over the one that doesn't - untested previously. Split from the 2026-08-04 integration branch's bundled undercover coverage commit 4aab8b2.
…t-typing' into split-1282-compound-stmt
Only the identical-nil and both-non-nil-locations cases were tested. When exactly one side's compound_statement has no location (built without one, e.g. a bare if/while body), combine_with must still prefer the one that has a location over the one that doesn't - untested previously. Split from the 2026-08-04 integration branch's bundled undercover coverage commit 4aab8b2.
The comments added by this branch ran well past the repo convention of 1-3 lines, budgeted per method across a docstring and its inline comments together. Twenty-six blocks were 4 lines or more, the largest at 26, 16, 14, 13 and 12 lines, which buries the one non-obvious fact each was there to state. Each block is cut to roughly a third rather than trimmed at the edges, keeping the constraint that is still in force and dropping the narration around it. Every @sg-ignore, @PARAM and @return line is left in place, so no suppression moves relative to the code it applies to and no declared type changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VcKgVwg618qbVu6NoWDhA4
CI at dcc707b flags all three markers in ApiMap#super_and_sub? as "Unneeded @sg-ignore comment" (api_map.rb:710, 712, 714). This branch's own dominance handling resolves the redefinition case they covered, so they no longer suppress anything. Verified with "bundle exec solargraph typecheck --level strong": with the markers gone, no problem is reported on any of those three lines. bin/solargraph is a bare script rather than a bundler binstub, so it loads the installed 0.60.4 gem and still reports the markers as needed - that analyzer predates the dominance work, and its verdict here is wrong. The remaining gap in this method is untouched and still unsuppressed: Wrong argument type for Store#get_superclass, where sc_fqns is ComplexType, String because multiple sequential reassignments union rather than dominate by recency.
Review asked for the nil checks behind "Need to add nil check here" to be written now rather than deferred, here and elsewhere. This branch added six such markers; none survive. FlowSensitiveTyping#process_if guards conditional_node before using it. CI reports it as "expected Parser::AST::Node, received Parser::AST::Node, nil" on the process_guarded_reassignment call, and the same nil reaches process_expression on the line above, which was unsuppressed. Pin::Method#infer_from_return_nodes now calls Pin::Base#filename, which already returns nil when location is nil, instead of reaching through location.filename itself. That drops two markers, including one predating this branch. Note location is frequently non-nil while its filename is nil, and the surrounding code relies on passing that nil through to ApiMap#source_map, so a guard on filename rather than on location breaks return-type inference for three method_spec examples. The four markers in IfNode are deleted outright: CI flags all four as "Unneeded @sg-ignore comment" at if_node.rb:32, 42, 48 and 58. Local typecheck disagrees with CI on that last point, and the disagreement is unexplained. On this machine node.children[N] infers as Array, so those lines report "expected Parser::AST::Node, received Array" and the markers look needed; CI infers Parser::AST::Node, nil for the identical source. CI is taken as authoritative here.
Review asked what benefit Pin::Base#closure deriving a closure from the compound_statement chain brings. Measured answer: none. All eight CompoundStatement.new sites pass closure: explicitly, and with the derivation removed the only failing example was the one added alongside it to exercise it - so its sole consumer was its own test. Both are removed, along with the two @sg-ignore markers the private method carried. Removing it also tightens what #closure infers, taking the local strong typecheck from 576 problems to 543. The two remaining examples in compound_statement_spec keep their value: they walk the chain with their own helper and check it reaches the stored closure, so a node processor threading closure: without compound_statement: still gets caught. Their header comment no longer describes a derivation that exists. Also documents what the definite: argument means at the LvasgnNode call site, as asked.
Reviewing the compression hunk by hunk surfaced two cuts that removed a fact rather than narration, and one defect that predates it. process_guarded_reassignment lost the reason it asserts only the opposite branch: the firing path is already handled by unioning in the assignment pin. That sentence is back. The resbody_node.rb block is not one method comment. and_node.rb, or_node.rb and orasgn_node.rb all point at it instead of repeating themselves, so it is the shared explanation for four call sites and earns the full four lines rather than two. base_variable.rb declared @PARAM other twice on combine_assignments, with a stray blank comment line between them. Present at c8586e1, so not introduced by the compression. The spec helper comment drops to two lines, per the convention that spec comments stay rare and the reasoning lives in the example name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VcKgVwg618qbVu6NoWDhA4
process_condition only forwarded its three arguments to process_expression. Its single caller, assert_after_guard, invokes it on a separate instance configured with only_downcast_these_names, so the pass-through existed solely to expose a private method across instances. process_expression moves up into the public section and takes over the docstring; assert_after_guard now calls it directly. That makes the class surface explicit rather than wider: the method was already reachable from outside through the forwarder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VcKgVwg618qbVu6NoWDhA4
Four constructs added by this branch - and, or, orasgn and resbody - built a CompoundStatement pin and deliberately kept it out of pins, with a comment claiming their bodies were too common to warrant one. Master has no such case: all four of its sites push, and NodeProcessor::Base#enclosing_compound_statement_pin finds them by selecting from pins. The exception left the parent chain and that positional lookup disagreeing about which compound statements exist. Pushing and, or and resbody changes nothing measurable. Pushing orasgn regresses one case: a leaving guard inside a ||= body stopped narrowing at the end of the ||=. That narrowing was previously right only by accident. With no pin for the ||= body, the guard range ran to the method body and happened to reach the correct answer. The reason it is correct is specific to ||=: the body is skipped exactly when the target is truthy, so the skip path reaches the same conclusion about the target as the guard does. No other conditional body carries that guarantee - a while or rescue body simply may not run - which is why extending the range for every leaving guard is wrong, and was measured to be: it flips six constructs the other way. FlowSensitiveTyping#assert_after_skipped_or_asgn asserts exactly that fact, restricted by name to the assignment target. A new spec covers the restriction, checking a second variable guarded inside the same body is not narrowed after it.
The two branches diverged at 0.60.4 and share 13 of their commits, so they overlap in 31 of the 38 files either one touches. Eight files conflicted across eleven hunks. Resolution rule: 1308 wins on behaviour. Its tip commits are answers to review comments on 1308 itself, so they are the later decision rather than a competing one. 1282 comment compression survives only where 1308 did not change the behaviour being described. Four places needed more than a mechanical pick: resbody_node.rb - 1308 extracts a rescue_body_node local and pushes the CompoundStatement onto pins. That makes the 1282 comment saying it is never pushed false, so the comment goes rather than merging, and the two markers the extraction obsoletes go with it. pin/base.rb - 1308 deleted derive_closure_from_compound_statement, so the compressed comment describing it goes too. base_variable.rb - kept the 1282 comment wording over the 1308 code, which carries a superseded local that 1282 has no equivalent for. flow_sensitive_typing.rb - 1308 still had the process_condition forwarder and a private process_expression that also calls process_parentheses and process_assignment. The public process_expression from 1282 is the surviving one, now carrying both of those calls. Hooks bypassed with --no-verify: Overcommit in a linked worktree resolves its git dir to the primary checkout and its stash-and-reset destroys MERGE_HEAD, silently producing a single-parent commit whose subject still says Merge. Checks run separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VcKgVwg618qbVu6NoWDhA4
This was referenced Sep 6, 2026
override_assignments? took a location so a branch-local reassignment could supersede at a use site it dominates. That threaded a position through combine_with and combine_assignments in BaseVariable, and through the combine_with overrides in LocalVariable and Parameter, making pin combination position-dependent. ApiMap#var_at_location already filters candidates by position. It now also asks each one for its definite-at-that-position form, so the combiner sees only pins whose assignments have definitely run and needs no location of its own. override_assignments? reduces to a check on other.definite, and three signatures lose a parameter. The negative controls still hold. A use site earlier in the branch than its reassignment is excluded by visible_at? before the promotion is reached, and a reassignment nested in a further conditional never dominates. BaseVariable#definite_at returns self or a definite copy, one return type either way, and var_at_location binds the mapped collection to a local before folding it. Both shapes are deliberate: a ternary over two types, or map chained directly into inject, each leave the accumulator unresolved at strong level. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VcKgVwg618qbVu6NoWDhA4
apiology
marked this pull request as ready for review
September 6, 2026 18:27
apiology
added a commit
to apiology/solargraph
that referenced
this pull request
Sep 7, 2026
Several comments in this branch ran 6-14 lines explaining a single mechanism, well past this repo's 1-3-line budget. Compress them to state the same facts in fewer words, drop a stale upstream PR reference (castwide#1282, closed in favor of castwide#1338) from a spec comment, and remove a spec-level comment that duplicated the adjacent source comment word for word. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJxUeueBgXj2qUqrsaa7yY
apiology
added a commit
to apiology/solargraph
that referenced
this pull request
Sep 9, 2026
Fixes five flow-sensitive-typing gaps: a parameter reassigned to a non-literal type, a nil-guarded default used past its conditional, narrowing outliving a definite reassignment, a dominating reassignment not counted as definite, and a variable assigned inside an if condition. Conflict resolution: - flow_sensitive_typing.rb: assert_after_guard called a method, process_condition, that does not exist anywhere in the codebase - a pre-existing bug on this branch, unrelated to this merge, that would crash the moment guarded-reassignment narrowing actually fired with a non-empty name list. Called the real method (process_expression) and made it public, since it was private and being invoked with an explicit receiver on a sibling instance. Also removed a duplicate process_expression definition already present on this branch (the first was silently shadowed by the second; Ruby uses the last def). - resbody_node.rb / source/chain/or.rb: kept this branch's existing behavior over the PR's older-base forms, verified via the self-hosted typecheck. - type_checker/rules.rb: kept this branch's counts. Verification: solargraph typecheck --level strong (1 pre-existing problem, the known gemspecs.rb Ruby-version-sensitive @sg-ignore, same as the branch tip); full RSpec (2208 examples, 0 failures, 45 pending); undercover --compare (pre-merge tip) reports no missing coverage.
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.
Problem: Solargraph reports a call as unresolved on a local it has already been given enough to narrow.
Five shapes are affected, each reduced from a suppression carried in a consuming codebase.
1. Parameter reassigned to a non-literal — the declared
@paramtype won over the reassignment.2. Nil-guarded default used past the conditional — the non-firing path kept the original type.
3. Narrowing outliving a definite reassignment — the guard's downcast survived onto the new value.
4. Dominating reassignment not counted as definite —
definitestayed false on both sides.5. Variable assigned inside an
ifcondition — narrowing ran before the condition was mapped.Solution:
Pin::BaseVariablerecords whether an assignment definitely executed and which compound statement it was made in, so a superseding reassignment replaces the earlier type instead of unioning with it; condition processing covers parenthesised and assignment expressions, and runs after the condition is mapped.🤖 Generated with Claude Code