Conversation
Flow-sensitive typing already narrows nil-checks on local/instance variables, but a nil-guard on `obj.attr` didn't narrow a later call to `obj.attr` in the same method body -- each call was treated as an independent, unnarrowed invocation. FlowSensitiveTyping now recognizes receivers that are a dotted chain of simple, argument-less calls rooted in a tracked local or instance variable (e.g. `pin.location`) and records nil-narrowing facts against a synthesized pin for that chain, the same way it already does for a plain variable. Chain::Call#resolve looks up those facts by threading a dotted "receiver path" through Chain#define, checked before falling back to ordinary method resolution. Also fixes a latent Pin::BaseVariable#equality_fields gap: downcast copies of the same pin (different presence/narrowed type) shared identical equality_fields, so they could collide as cache keys in Chain's inference cache and return a stale, wrongly-narrowed or wrongly-unnarrowed result depending on lookup order. Fixes castwide#1249 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGu6zb5faStTC754PxMUSA
apiology asked, on the receiver_path plumbing added for castwide#1249, whether the @sg-ignore on links.last.resolve was hiding a real bug rather than a false positive. It wasn't reachable (Chain's constructor pads an empty links array with UNDEFINED_CALL, so links is never empty, but suppressing it instead of expressing that invariant in the code was the wrong call. Extract links.last once and guard it for real. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGu6zb5faStTC754PxMUSA EOF )
Every "Need to add nil check here" ignore this PR had introduced is now either gone or replaced with a comment explaining why a real check is not needed: - Fixed the actual bug: type_name did not handle a :cbase root (the leading '::' in a fully-qualified constant like ::Integer), so `x.is_a?(::Foo)` guards never narrowed anywhere in this file -- parsing '::Foo' silently produced no type name at all. That is why the node.is_a?(::Parser::AST::Node) guard at the top of parse_receiver_chain was not narrowing node for the rest of the method. Fixing it made 7 of 9 ignores in that method unnecessary. - Added a real nil-check for the one Array#[range] slice that is legitimately nilable per its own type (children[2..].empty?). - The remaining two ignores (a node.children element, and Range.from_node(node).start) get explanatory comments instead of the generic placeholder -- both match an existing, already-accepted pattern elsewhere in this same file. Also added a regression spec for the type_name fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGu6zb5faStTC754PxMUSA
# Conflicts: # lib/solargraph/source/chain/call.rb
Cut three docstrings down to the 1-3 line review budget. Removed two invented-prose @sg-ignore comments after confirming (by deleting each and reconfirming no new error appears) that strong typecheck no longer needs them; replaced a third with the codebase's existing "Need to add nil check here" slug already used for the same Range.from_node pattern elsewhere in the file.
equality_fields includes presence alongside narrowed/exclude return type, but nothing exercised presence differing on otherwise-identical pins. Two LocalVariable pins sharing name/location but built with different presence ranges must stay distinct under eql?/hash, since flow-sensitive typing keys downcast copies on exactly this. Split from the 2026-08-04 integration branch's bundled undercover coverage commit 4aab8b2.
Chain::InstanceVariable#resolve gained a _receiver_path parameter with no @PARAM tag, which the repo's own strong typecheck and RuboCop's YARD/MismatchName cop both flagged once any doc comment was present. Document all four parameters, matching the convention already used by sibling Chain::Link subclasses (if.rb, variable.rb, etc). Pin::BaseVariable#equality_fields had 0% line coverage per undercover. Add a spec that downcasts a pin two different ways and asserts the resulting #equality_fields differ. Note: #equality_fields is not currently wired into any #==, #eql?, or #hash on Pin::Base or its subclasses (Pin::Base defines its own #== via #nearly?, and never includes the Equality mixin), so the existing "includes presence ... in #eql? and #hash" example in this file passes only because Object's identity-based #eql?/#hash always differ for distinct instances - not because of #equality_fields. The new spec calls #equality_fields directly via #send instead of relying on #eql?/#hash/#==.
FlowSensitiveTyping already narrows a plain-send call chain (pin.location) reused after a truthy guard, and separately narrows a safe-navigation call's own receiver (x&.foo) to non-nil. Neither recognized the other's node type: parse_receiver_chain and process_call_chain only matched :send, so a repeated pin&.location never got the same treatment as pin.location. Both now accept :csend alongside :send -- the two node types share an identical [receiver, method, *args] shape, so no other change is needed for the chain-word parsing itself. Also added process_csend (receiver-narrowing for x&.foo, recursing through x&.foo&.bar) and wired it into process_expression. Replaced the "Need to add nil check here" placeholder on the two new Range.from_node(node).start call sites with a real nil check, per the current house convention of not adding new instances of that generic marker. One combined regression spec (pin&.location used twice, narrowing the second occurrence) still fails at this commit -- Chain#nullable? statically treats any &. as always possibly short-circuiting, even when the receiver is already known non-nil, so it re-adds nil after the narrowing already stripped it. Fixed in the next commit.
Attempted to make Chain#nullable? binder-aware (only treat a `&.` link as introducing nil when the receiver's actual type in play is nilable, rather than unconditionally for any `&.` in the chain) so a narrowed receiver would stop maybe_nil from re-adding nil after a repeated safe-navigation call. That broke a pre-existing, deliberate spec: chain_spec.rb's "recognizes nil safe navigation" asserts `String.new&.strip` infers `String, nil` even though `String.new` itself is never nilable -- current behavior always treats a `&.` result as nilable, regardless of the receiver's actual type. Reverted the chain.rb change rather than override that on my own judgment. Converted the combined regression spec's now-blocked assertion to a pending example naming the cause, matching the file's existing pending-spec convention, so a future fix to Chain#nullable? will surface here as an unexpectedly-passing pending spec.
DocumentSymbol#process guards `pin.best_location&.filename`, then reuses the plain `pin.best_location.filename` chain a few lines down. FlowSensitiveTyping now recognizes that guard (a csend chain) and narrows the repeated composite call the same way it already did for plain-send chains, so the ignore above that line no longer suppresses anything real. Verified by removing exactly that line and confirming 0 problems on the file. The sibling `pin.best_location.range` call keeps its own ignore -- the guard only narrows the `.filename` chain, not `.best_location` on its own. Decremented the "Need to add nil check here" ledger count in rules.rb to match.
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.
Claude: This PR was written by Claude Code on behalf of @apiology.
Problem: A guard on
x&.foonarrowsxto non-nil, and a guard onpin.locationnarrows a later repeatedpin.location.filenamecall -- but neither recognized the other's node shape, so a guard written with safe navigation got none of the repeat-call narrowing.This builds on castwide/solargraph#1258, which added the plain-
.-chain version of this narrowing.Solution:
parse_receiver_chainandprocess_call_chainnow accept:csendalongside:send-- the two node types share an identical[receiver, method, *args]shape, so no other change was needed to let apin&.locationguard's fact reach a laterpin&.location.filename. The same fix cleared two pre-existingUnresolved call to name on RBS::Location, nilproblems in Solargraph's own source, plus one now-unneeded@sg-ignore.One case is left as a pending spec:
Chain#nullable?always adds nil back for any&.in a chain, even once narrowing has ruled it out, so a bare repeatedpin&.location(no further chaining) still showsLocation, nil. Fixing that would change behavior a pre-existing spec (String.new&.strip) deliberately asserts, so it's left open rather than changed unilaterally.Test plan: full
rspec(1656 examples, 0 failures, 68 pending) andsolargraph typecheck --level strong(531 problems vs 533 on this branch's base, no new problems).