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
apiology
marked this pull request as draft
August 31, 2026 21:40
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).
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.
apiology
added a commit
to apiology/solargraph
that referenced
this pull request
Sep 5, 2026
Brings castwide#1308 up to its current head. Its narrowing work is already here through 11e3386; the two outstanding commits are dcc707b, covering the nil-location tiebreak in CompoundStatement#combine_with - the undercover node this PR owns - and 6b12401, a marker-placement fix. Two conflicts, both taken from that branch, since 6b12401 exists to correct exactly those lines: api_map.rb and resbody_node.rb. The latter also extracts a local and adds a real nil guard around NodeProcessor.process, which is why it no longer needs a marker there. 6b12401's marker moves then had to be partly undone here, because the combined tree narrows differently from that branch alone: api_map.rb Its three markers all reported Unneeded, while store.get_superclass(sc_fqns) two lines below went unguarded - sc_fqns inherits ComplexType, String across the simplify_literals reassignment. Removed all three and put the "unions rather than overrides" marker back on the while line, where it had been before. formatting.rb Restored a marker it deleted as unneeded, using the catalogued "Need to add nil check here" slug. 2203 examples, 0 failures, 45 pending. Strong typecheck back to the six known baseline problems.
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.
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.
8 tasks
Contributor
Author
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.