Skip to content

Update a parameter's flow-sensitive type after reassignment to a non-literal type - #1282

Draft
apiology wants to merge 13 commits into
castwide:masterfrom
apiology:fix-1250-parameter-reassignment-typing
Draft

Update a parameter's flow-sensitive type after reassignment to a non-literal type#1282
apiology wants to merge 13 commits into
castwide:masterfrom
apiology:fix-1250-parameter-reassignment-typing

Conversation

@apiology

@apiology apiology commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Pin::Parameter#typify always returned the declared @param type once available, without ever consulting the types of the parameter's 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.

# @param position [Position, Array(Integer, Integer)]
def describe(position)
  position = PositionNormalizer.normalize(position)
  # `position` was still typed as `Position, Array(Integer, Integer)` here,
  # even though it was just reassigned to the return value of `normalize`,
  # which is declared `@return [Position]`.
  position.line
end

Fix

Added a definite flag to Pin::BaseVariable, set by node processors based on a new Region#conditional flag — true only when an assignment is guaranteed to have executed (not inside if/unless/while/until/when/rescue/block body/&&/||/||=). Pin::Parameter#typify now tries probe (which infers from assignments) first when definite, falling back to the declared type otherwise — matching the existing union semantics that plain local variables already had for conditional reassignment.

Default-value assignments (def foo(x = 1)) are marked non-definite since they only apply conditionally (when the caller omits the arg).

Net diff on real call sites

Two of the @sg-ignore flow sensitive typing should be able to handle redefinition comments named in #1250 are now unneeded and removed (lib/solargraph/range.rb#contain?/#include?, one in lib/solargraph/api_map.rb#super_and_sub?). A handful of newly-surfaced gaps from multiple sequential unconditional reassignments (which still union rather than dominate by recency) got matching @sg-ignores, consistent with the existing convention — net zero change in solargraph typecheck --level strong problem count (584 before and after).

Test plan

Fixes #1250

🤖 Generated with Claude Code

https://claude.ai/code/session_01VHyn8dc8oSqcQJrXFgDWUo


Follow-up: nil-guarded default narrowed after the conditional

The dominance case above is fixed, but a neighbouring shape still failed ��� a modifier-if guarded by the variable's own nilness, where the use site is after the conditional. There it is the guard's condition, not dominance, that establishes the type on the path where the assignment did not run:

# @param tasks [Array<String>, nil]
# @return [void]
def guarded_default(tasks)
  tasks = ['a'] if tasks.nil?
  tasks.each { |t| puts t }   # was: Unresolved call to each on Array<String>, nil
end

At the merge point, path (a) ��� clause ran, new value assigned ��� was already handled by unioning in the assignment pin. Path (b) ��� clause did not run, original value survives ��� was never narrowed by the guard's condition, so Array<String>, nil was unioned back in unchanged. process_if now also asserts the opposite branch's condition facts over the rest of the enclosing compound statement, restricted to the variables the clause definitely reassigns. That restriction is the soundness lever: facts are filtered by variable name, driven by a second FlowSensitiveTyping over the same locals/ivars with restricted_names: set. Reusing process_expression inherits &&/||/! handling, including and's refusal to propagate false-facts. Only unconditional lvasgn/ivasgn count as "definitely reassigns" ��� nested conditionals and ||= are excluded.

Fixed for the modifier if, non-modifier if, unless modifier, and else-clause forms. Negative controls, all still correctly reporting (nil is not eliminated unconditionally):

tasks = ['a'] if flag              -> Unresolved call to each on Array<String>, nil
puts 'hi' if tasks.nil?            -> Unresolved call to each on Array<String>, nil
xs = [] if xs.nil? || ys.nil?      -> ys.each still errors (xs.each narrows)
if tasks.nil?; tasks = [] if flag; end -> still errors
  • bundle exec rspec ��� 1646 examples, 0 failures, 57 pending
  • 8 new examples in spec/type_checker/levels/strong_spec.rb; the existing dominance spec at strong_spec.rb:942 still passes
  • bundle exec rubocop clean on touched files

…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
apiology added a commit to apiology/solargraph that referenced this pull request Aug 11, 2026
…eassignment to a non-literal type

Pin::Parameter#typify always returned the declared @PARAM type once
available, without ever consulting the types of the parameter's
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. Adds a
`definite` flag to Pin::BaseVariable, set by node processors based on
a new Region#conditional flag (true only when an assignment is
guaranteed to have executed, not inside if/unless/while/until/when/
rescue/block body/&&/||/||=). Pin::Parameter#typify now tries `probe`
(inferring from assignments) first when definite, falling back to the
declared type otherwise - matching the existing union semantics
plain local variables already had for conditional reassignment.
Default-value assignments are marked non-definite since they only
apply conditionally (when the caller omits the arg).

Fixes castwide#1250

Conflict in lib/solargraph/pin/base_variable.rb: incoming's branch,
based directly on castwide/master, still used the pre-rename
`intersection_return_type:` keyword; this branch already renamed it
to `narrowed_return_type:` in an earlier merge. Kept this branch's
name and added incoming's new `definite: true` parameter alongside
it. Also fixed a pre-existing, unrelated dead-name bug spotted in the
same file while resolving this: BaseVariable#equality_fields still
referenced the old `intersection_return_type` (undefined after the
rename, since attr_accessor only ever defined
`narrowed_return_type`) - every call to it would have raised
NoMethodError. Fixed to reference `narrowed_return_type`, matching
every other use in the file.

Verified: spec/type_checker, spec/pin, spec/parser,
spec/source_map/clip_spec.rb (782 examples, 0 failures, 25 pending),
and a broader safety net - spec/type_checker, spec/source,
spec/source_map/clip_spec.rb, spec/api_map_spec.rb,
spec/api_map_method_spec.rb, spec/pin, spec/parser (1018 examples, 1
failure, 33 pending). The 1 failure (spec/api_map_spec.rb:771) is the
same pre-existing order-dependent flake already confirmed unrelated
to this branch's work during the castwide#1278 merge
earlier in this session.
@apiology

Copy link
Copy Markdown
Contributor Author

🤖 Posted by Claude, not the account owner — acting on their behalf via their GitHub credentials.

Regression from this PR: reassigning a variable to a different type based on its own old value (x = x.length) applies the new type to the RHS's reference to the old value, on the same statement.

class Repro
  # @param x [String]
  # @return [void]
  def foo(x)
    x = x.length
  end
end
$ solargraph typecheck --level strong repro.rb
repro.rb:5: Unresolved call to length on Integer

x.length is String#length, evaluated against the old String-typed x — but it resolves against Integer, the type of the value being assigned on this same line. No @type tag needed.

Bisected: clean checkout at 368b3e0a (commit before this PR) — 0 problems. At b6eea57c (this PR merged) — reproduces every time.

`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)
The attr_reader carried the full explanation while initialize's
own @PARAM definite tag just said "[Boolean]" - move the
explanation onto the @PARAM tag it documents.
@apiology
apiology marked this pull request as ready for review August 12, 2026 01:04
apiology added a commit to apiology/solargraph that referenced this pull request Aug 12, 2026
apiology added a commit to apiology/solargraph that referenced this pull request Aug 12, 2026
Re-enables `solargraph typecheck --level strong` as an enforced CI
gate - removes `continue-on-error: true` from
.github/workflows/typecheck.yml. Most of the diff is @sg-ignore
comments documenting type gaps strong mode can't resolve on its own
(flow-sensitive-typing limits, the nil-vs-NilClass representation
mismatch, guard-then-fetch patterns that don't narrow, RBS overload/
type-alias gaps). A handful of real fixes are included (missing/wrong
@return/@PARAM tags).

This branch's own lib/ tree has diverged substantially from castwide#1240's
target (39+ merged PRs' worth of independent work), so merging this
required a full annotation sweep on top of the mechanical merge to
actually make the newly-hard CI gate pass - see below.

Conflicts (20 files) fell into two categories:

1. Genuine competing logic, where incoming's branch (based directly on
   castwide/master) predated work already merged into this branch.
   Kept this branch's side throughout: doc_map.rb's entire in-memory
   pin-cache architecture (superseded by the PinCache instance-based
   rewrite from castwide#1252, same pattern already identified during the
   castwide#1239 investigation earlier this session), rbs_translator.rb's
   compound-type-as-ComplexType-graph architecture (from castwide#1281,
   predates incoming's tag-string type_to_tag reintroduction),
   flow-sensitive-typing/node_chainer additions (rhs_never_returns
   tracking from castwide#1259), base_variable.rb's definite/narrowed_return_type
   naming (from castwide#1282), node_methods.rb's ENSURE handling (from castwide#1285),
   chain.rb/call.rb's receiver_path threading, and
   workspace.rb/pin_cache.rb duplicate method definitions incoming
   reintroduced that already exist elsewhere in this branch's own
   `class << self` blocks.
2. Pure annotation differences (add/adjust an @sg-ignore comment) where
   kept whichever side matched this branch's actual code structure.

2. Annotation sweep: after resolving conflicts, this branch's strong
   typecheck still reported 289 problems (down from 547 pre-merge,
   since castwide#1240's own annotations covered about half). 194 were
   "Unneeded @sg-ignore comment" (incoming's own ignore comments,
   correct on castwide#1240's target tree, landing on lines this branch's
   independent fixes already resolve) - removed mechanically by
   scanning upward from each flagged line for its comment. The
   remaining 95 were genuine new gaps on this branch's own code paths
   (mostly not exercised by castwide#1240's target tree at all) - added one
   @sg-ignore per flagged line, matching the established
   message-as-comment convention used throughout this codebase
   (@sg-ignore matches by string presence, not exact message, so one
   comment per line suffices even where a line has multiple flagged
   sub-expressions). Spot-checked the ones that looked most like real
   bugs rather than static-analysis gaps (BigDecimal-typed values in
   Integer-declared contexts, an Array#push type mismatch) against
   already-documented, already-tracked false-positive patterns in this
   codebase (the known BigDecimal-contamination artifact from earlier
   PR work, and a known is_a?-narrowing gap) - none were new bugs.
   Also fixed one new Style/Next rubocop offense the sweep introduced.

Verified: full local `bundle exec rspec` (1826 examples, 0 failures,
51 pending - the only local-environment-dependent example,
'ignores undefined method calls from external sources', a
pre-existing order-dependent kramdown-parser-gfm gem-cache flake
already confirmed unrelated to this session's work, passed in this
run), `solargraph typecheck --level strong` (0 problems, confirming
the now-hard-gated CI job will pass), and `rubocop lib/` (13 offenses,
matching this branch's pre-existing baseline exactly - none newly
introduced by this merge).
@apiology

Copy link
Copy Markdown
Contributor Author

🤖 Filed by Claude, not Vince — acting on his behalf via his GitHub credentials.

This fixes reassignment-type tracking for parameters, but the same gap remains for plain local variables and instance variables. The definite flag this PR adds to Pin::BaseVariable, and the region-conditional plumbing that sets it, already runs through lvasgn_node.rb and ivar assignment — so the infrastructure covers locals and ivars. What's missing is the consuming side: definite's override-over-union behavior is only wired into Pin::Parameter#typify; Pin::LocalVariable and Pin::InstanceVariable have no typify override at all, so they fall back to the un-narrowed default regardless of definite.

# @return [void]
def run
  local = 5
  local = 'hello'
  local.upcase
  nil
end
$ bundle exec solargraph typecheck --level strong repro.rb
repro.rb:5: Unresolved call to upcase

Swapping local for an @ivar (assigned in initialize, reassigned in the method) reproduces the identical error.

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
@apiology

Copy link
Copy Markdown
Contributor Author

🤖 Filed by Claude, not Vince — acting on his behalf via his GitHub credentials.

Pushed ce7332967: BaseVariable#combine_assignments now drops an earlier assignment in favor of a later same-closure definite reassignment instead of unioning them (with a guard for self-referential reassignments like x = x.foo, since resolving those needs the prior assignment as a base case). This fixes locals and same-method ivar reassignment; it does not reach the cross-method ivar case (initialize + another method), since ivasgn_node.rb never sets presence:/definite: on ivar pins - a larger, separate change. Three specs that were already asserting this behavior under 'sequential assignment support' are un-pended, plus a new spec; full suite is green (1629 examples, 0 failures) with no new self-typecheck problems.

# @return [void]
def run
  local = 5
  local = 'hello'
  local.upcase  # was: Unresolved call to upcase on Integer, String
  nil
end

@apiology

Copy link
Copy Markdown
Contributor Author

Claude: This regresses local/instance variable inference to undefined. Reduced to one mechanism (an earlier draft of this comment described what looked like two separate patterns - a while-loop case and a begin/ensure case - but neither the loop, the begin block, nor ensure turned out to be load-bearing; both collapse to this):

def m
  x = nil
  x = 1
  if x
    y = x * 2 # infers `Integer` on the parent commit, `undefined` here
  end
end

While resolving x's visibility at the if x guard, combine_assignments/override_assignments? get invoked twice: once correctly unioning x's two assignments (nil and 1), then a second time re-merging the same original x = nil declaration pin against the already-merged result. That second call is definite: true, same closure, and doesn't reference x by name, so override_assignments? treats it as a genuine supersede and replaces the accumulated [nil, 1] with just [nil] again - dropping the branch that actually executes.

…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
)
apiology added a commit to apiology/solargraph that referenced this pull request Aug 13, 2026
…g a stale pin during flow-sensitive narrowing

FlowSensitiveTyping#find_var used Array#find, returning the first local/ivar
pin matching a variable name whose presence includes the query position.
For a variable reassigned then read inside a subsequent guard, both the
original declaration and the reassignment have presences that include the
guard's position, so find always returned the stale original pin instead
of the reassignment. That pin then got downcast and merged back into
locals for narrowing, and because BaseVariable#override_assignments? (from
the earlier reassignment-override work) lets a later definite assignment
supersede rather than union, the merge dropped the reassignment and
re-surfaced the original declared type - regressing local variable
inference to `undefined`.

find_var now picks the pin with the latest presence start among matches,
excluding any pin whose own assignment is still being evaluated at the
query position.

Does not address the equivalent case for instance variables inside a
conditional - ivar pins never get a presence range, so find_var's
presence-based tie-break can't distinguish them. Pre-existing, separate
gap, acknowledged by the source commit.

Removes 4 @sg-ignore comments this fix made unneeded (2 in
flow_sensitive_typing.rb, 2 in base_variable.rb) and updates 3 spec
expectations for literal-type inference (already merged into this branch
via castwide#1223) that the source commit's tests didn't account for - strong
typecheck problem count on this branch drops from 133 to 4 (all 4 are
pre-existing/environment-specific: 3 Ruby-version-dependent Vernier
constants, 1 unrelated pre-existing item).
@apiology

Copy link
Copy Markdown
Contributor Author

Claude: A common pattern is guarding a nilable value and then, in the same ||-combined boolean expression, comparing that same variable — then coercing the whole thing to a real boolean with !! for a predicate method's declared Boolean return. That combination stopped resolving on this branch: the method's return type can no longer be inferred, even though Boolean is exactly what the expression evaluates to.

# typecheck --level strong

# @param val [Integer, nil]
# @return [Boolean]
def check?(val)
  !!(val.nil? || val < 5)
end

# #check? return type could not be inferred

Reduced from real code: removing !!, removing the ||, using a non-nilable param, or guarding/comparing two different variables instead of the same one each independently clear the error — only the full combination (nilable var, guarded and compared via ||, then !!-coerced) triggers it.

…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
apiology added a commit to apiology/solargraph that referenced this pull request Aug 14, 2026
…d or-expressions with flow narrowing

Brings in a3ec7ba, the only new commit on apiology/fix-1250-parameter-reassignment-typing
since this PR's prior merges (12f0a15). Resolved conflicts by keeping HEAD's
already-merged definite-reassignment-override and equality_fields logic, and
dropping the incoming branch's stale sg-ignore comments that predate refactors
already on this branch (find_var's pins.select rewrite, references_name?).
@apiology

Copy link
Copy Markdown
Contributor Author

Claude: Written by Claude and posted using @apiology's GitHub credentials.

Reassignment to a non-literal type resolves correctly on this branch when it happens unconditionally. It still fails when the reassignment is inside a conditional — definite looks like it's judged for the whole closure, so a branch's own assignment doesn't take precedence over an earlier one from the enclosing scope, even at a use site on the next line.

# @param str [String]
# @param num [Integer]
# @return [void]
def unconditional_reassign(str, num)
  local = num
  local = str
  local.upcase
end

# @param str [String]
# @param num [Integer]
# @param flag [Boolean]
# @return [void]
def conditional_reassign(str, num, flag)
  local = num
  if flag
    local = str
    local.upcase
  end
end
$ solargraph typecheck --level strong repro.rb
repro.rb:20: Unresolved call to upcase
1 problem found.

Same failure with literal values in place of the parameters, and a first assignment or a same-type reassignment inside a conditional is fine — so it's specific to a type-changing reassignment inside a branch, which is where the override would need to drop the outer assignment. Verified on apiology/solargraph@0653590d4, which contains this PR's head a3ec7bab0.

apiology and others added 2 commits August 14, 2026 11:57
…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
apiology and others added 3 commits August 14, 2026 14:35
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
@apiology
apiology marked this pull request as draft August 14, 2026 22:23
apiology added a commit to apiology/solargraph that referenced this pull request Aug 14, 2026
… the use site is dominated by it

# Conflicts:
#	lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb
@apiology

Copy link
Copy Markdown
Contributor Author

The dominance case from my earlier comment is fixed on this branch. A neighbouring shape still fails: a modifier-if guarded by the variable's own nilness, where the use site is after the conditional rather than inside it. Here it is the guard's condition, not dominance, that establishes the type on the path where the assignment did not run.

# Fixed here -- use site inside the branch, dominated by the assignment
# @param str [String]
# @param num [Integer]
# @param flag [Boolean]
# @return [void]
def conditional_reassign(str, num, flag)
  local = num
  if flag
    local = str
    local.upcase
  end
end

# Still failing -- use site after the conditional
# @param tasks [Array<String>, nil]
# @return [void]
def guarded_default(tasks)
  tasks = ['a'] if tasks.nil?
  tasks.each { |t| puts t }
end
$ solargraph typecheck --level strong probe.rb
probe.rb:22: Unresolved call to each on Array<String>, nil
1 problem found.

Verified on apiology/solargraph@bd9fb82cb, which contains this PR's head 9fe7637c1 (behind_by=0). This is the standard Ruby default-argument idiom and accounts for 7 ignore markers in one downstream codebase.

Written by Claude and posted using @apiology's GitHub credentials.

apiology and others added 2 commits August 15, 2026 18:52
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
apiology added a commit to apiology/solargraph that referenced this pull request Aug 16, 2026
…faults

Conflict resolution in flow_sensitive_typing.rb:

- #initialize takes both this branch's `closure` positional arg and castwide#1282's
  `restricted_names:` keyword; the internal FlowSensitiveTyping.new in
  assert_after_guard now passes `closure` through.
- attr_reader lists both :closure and :restricted_names.
- Dropped castwide#1282's local always_leaves_compound_statement?. This branch already
  gets a richer version from Parser::NodeMethods that recurses into :begin for
  multi-statement clauses and treats raise/fail sends as leaving; a definition
  in the class shadows the included module, which broke the four
  "raise if()" nil-refinement specs. castwide#1282 wrote its simple copy before
  NodeMethods had one.

Full suite: 1899 examples, 0 failures, 47 pending.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flow-sensitive typing doesn't update a variable's type after reassignment to a different (non-literal) type

1 participant