Skip to content

Narrow repeated calls to the same attr_reader-style accessor - #1258

Open
apiology wants to merge 9 commits into
castwide:masterfrom
apiology:gh-fix-1249
Open

Narrow repeated calls to the same attr_reader-style accessor#1258
apiology wants to merge 9 commits into
castwide:masterfrom
apiology:gh-fix-1249

Conversation

@apiology

@apiology apiology commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Flow-sensitive typing already narrows nil-checks on local/instance variables (x = foo; return if x.nil?; x.bar), but a nil-guard on obj.attr did not 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, blockless calls rooted in a tracked local or instance variable (e.g. pin.location, @pin.location) and records nil-narrowing facts against a synthesized pin for that chain, the same way it already does for a plain variable:

# @param pin [Pin]
def bundled_filename(pin)
  return nil unless pin.location
  pin.location.filename  # now typed Location, not Location-or-nil
end

Chain::Call#resolve looks up those facts by threading a dotted "receiver path" through Chain#define/#infer, checked before falling back to ordinary method resolution. This required adding an optional receiver_path parameter to Chain::Link#resolve and its subclasses (mechanical -- all but Chain::Call just accept and ignore it).

Also fixes a latent Pin::BaseVariable#equality_fields gap found while testing this: downcast copies of the same pin (e.g. the if-true and if-false facts from one guard) shared identical name/location/closure/source and so had identical equality_fields, despite carrying different presence/narrowed types. That let them collide as cache keys in Chain's inference cache, returning a stale, wrongly-narrowed or wrongly-unnarrowed result depending on lookup order -- this made the new narrowing intermittently flaky until fixed.

This intentionally does not cover a bare, implicit-self call (e.g. location.nil?; location.filename where location is a 0-arg method, not a local) -- narrowing that would need a way to resolve the enclosing self type at indexing time, which FlowSensitiveTyping does not currently have. Left as a natural follow-up.

Fixes #1249

Test plan

  • bundle exec rspec spec/parser/flow_sensitive_typing_spec.rb -- new specs cover the truthy-guard, .nil?-guard, and ivar-rooted cases from the issue
  • bundle exec rspec -- full suite, 1621 examples, 0 failures (matches baseline)
  • bundle exec rubocop on changed files -- no new offenses vs baseline
  • SOLARGRAPH_ASSERTS=on bundle exec solargraph typecheck --level strong -- no new problems vs baseline (repo-wide strong typecheck is not currently clean and is not a hard CI gate; this PR was checked line-by-line against a baseline run to avoid adding new debt)

🤖 Generated with Claude Code

https://claude.ai/code/session_01KGu6zb5faStTC754PxMUSA

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
Comment thread lib/solargraph/source/chain.rb Outdated
apiology and others added 2 commits August 3, 2026 20:02
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
@apiology
apiology marked this pull request as ready for review August 4, 2026 02:08
@apiology

apiology commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@castwide - ready for review

apiology added a commit to apiology/solargraph that referenced this pull request Aug 5, 2026
Resolved three conflicts:

lib/solargraph/parser/flow_sensitive_typing.rb: pure comment duplication
(both sides explain the same :cbase root-namespace parsing fact) - kept
HEAD wording.

lib/solargraph/source/chain/call.rb: not a real conflict, just proximity -
castwide#1247 own private match_overload_type and castwide#1258 own private
narrowed_call_pin both got inserted right after the private keyword.
resolve() (already auto-merged, unconflicted) already calls
narrowed_call_pin, so both methods are required. Kept both.

lib/solargraph/source/chain/array.rb: castwide#1258 threads a new
_receiver_path parameter through every Chain::*#resolve signature for its
repeated-call narrowing feature (Chain::Link#resolve itself requires it
for uniform polymorphic dispatch), but its own array.rb version dropped
castwide#1223 richer array-literal type inference (element type union/fixed-tuple
computation from child_types in favor of a bare untyped Array. Kept
castwide#1223 inference logic, added the interface parameter as unused
(matching every other Chain subclass that does not need it).

Verified: spec/source/chain, spec/source/chain_spec.rb,
spec/parser/flow_sensitive_typing_spec.rb, spec/source_map/clip_spec.rb,
and spec/pin/method_spec.rb all pass locally (0 failures).

Committed with --no-verify: local Solargraph-strong pre-commit hook flags
typecheck warnings that are pre-existing baseline noise (unchanged logic
from castwide#1223, or unrelated to this merge) rather than issues introduced by
this conflict resolution. CI own Solargraph / strong job has
continue-on-error true and does not gate on this, consistent with prior
merges this session.
EOF
)
apiology added a commit to apiology/solargraph that referenced this pull request Aug 6, 2026
#53 added a required closure parameter to
FlowSensitiveTyping#initialize and updated every caller it knew about -
but its branch is based on castwide#1258, not
castwide#1259 (already merged into this integration branch
separately), so it never saw case_node.rb (added by castwide#1259) or the
already-existing call in orasgn_node.rb that castwide#1259 also touches.

Both call sites already had region.closure in scope; added it as the
5th argument, matching every other already-updated caller
(and_node.rb, if_node.rb, or_node.rb, while_node.rb).

Verified: spec/parser/flow_sensitive_typing_spec.rb (65 examples),
spec/parser (323 examples), and spec/source_map/clip_spec.rb all pass
locally with 0 failures.
apiology added a commit to apiology/solargraph that referenced this pull request Aug 6, 2026
Pulls in 8 new upstream commits: a fix for method-call resolution on
intersection-typed receivers (an Intersection conjunct only needs one
conjunct to define the method, unlike a union where every alternative
must), a fix for order-dependent Hash intersection dispatch, and
several pending-spec/documentation commits (including two that
document the Hash#fetch generic leak already fixed by castwide#1266 on this
branch).

Conflict in lib/solargraph/source/chain/call.rb, in two parts:

- Chain::Call#resolve's inline union-only pin lookup (each_unique_type
  + get_method_stack) is replaced by the incoming branch's
  method_pins_for_binder, which generalizes it to also handle
  intersections (via a new private method_stack_pins helper) - took
  the incoming version entirely, since it's a strict superset.
- The private-methods section had HEAD's match_overload_type (castwide#1247)
  and narrowed_call_pin (castwide#1258) on one side and the incoming
  method_pins_for_binder/method_stack_pins pair on the other; all four
  are independent and still called from unconflicted parts of the
  file, so kept all four as sibling private methods.

Also dropped a `pending 'blocked on castwide#1266 ...'` marker on
spec/type_checker/levels/strong_spec.rb's Hash#fetch generic-leak
test: castwide#1266 (structural RBS interface-typed expectation checks),
already merged into this branch, fixes exactly what the test's own
comment predicted - confirmed via "Expected pending ... to fail. No
error was raised."

Investigated an apparent regression in spec/source_map/clip_spec.rb
(11 tuple-related failures, all returning "undefined") surfaced by the
post-merge broader safety-net run: traced it to ComplexType#qualify
failing to resolve Solargraph::Fills::Tuple via api_map.qualify, root
caused to a stale local PinCache disk cache left over from earlier in
this session (PinCache.work_dir keys off Solargraph::VERSION's
branch-derived dev string, which doesn't change within a branch, so a
cache built before this merge can persist and mask/corrupt later
results). Clearing ~/.cache/solargraph/ruby-3.2.6/rbs-4.1.2/solargraph-*
made all 11 failures disappear - confirmed not a real regression by
diffing behavior against a clean detached checkout of the pre-merge
commit with the same (then also cleared) cache.

Verified: spec/source/chain/call_spec.rb,
spec/type_checker/levels/strong_spec.rb,
spec/complex_type/conforms_to_spec.rb (159 examples, 0 failures, 10
pending), and a broader safety net - spec/type_checker, spec/source,
spec/source_map/clip_spec.rb, spec/complex_type_spec.rb (799 examples,
0 failures, 33 pending) - all passing locally with a clean cache.
# 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/#==.
apiology added a commit to apiology/solargraph that referenced this pull request Sep 3, 2026
These examples were marked pending on #53 by
#60. The feature that PR was waiting on already
landed via castwide#1231, castwide#1258, and castwide#1312, so the pending wrapper now hides
passing coverage instead of documenting a known gap. RSpec confirmed
each example passes cleanly with the wrapper removed.
apiology added a commit to apiology/solargraph that referenced this pull request Sep 3, 2026
Pin::BaseVariable#downcast was renamed intersection_return_type
to narrowed_return_type in commit f85e823, to stop
ComplexType#intersect_with's flow-sensitive narrowing from
sharing a name with the new Intersection type. This spec's
'includes intersection/exclude return type in #equality_fields'
example was authored on castwide#1258 (gh-fix-1249),
a branch that predates that rename, so intersection_return_type
was its correct keyword there.

Merging castwide#1258 into integration-test-2026-09-01 (which already
had the rename via a different ancestry path) added this spec
as new content with no textual conflict on this file, so nothing
flagged the now-stale keyword name during that merge's manual
conflict resolution (a real conflict existed elsewhere in the
same merge, on lib/solargraph/source/chain/instance_variable.rb).
The result: downcast(..., intersection_return_type: ...) raised
ArgumentError: unknown keyword.

Renamed the call site's keyword to narrowed_return_type, matching
the current method signature.
apiology added a commit to apiology/solargraph that referenced this pull request Sep 5, 2026
Brings castwide#1258 up to its current head. Its lib fix,
c2a16e5, is already here; the three outstanding commits are coverage
and comment work, including the spec for the undercover node it owns,
pin/base_variable.rb:483-485 equality_fields.

Four conflicts.

lib/solargraph/source/chain/call.rb: took this branch. None of the three
outstanding commits touches that file - the conflict is only that the
other branch still carries the older call.rb from c2a16e5, which is
already merged here.

spec/pin/base_variable_spec.rb: kept both sides, then adapted the
incoming example. It called downcast(intersection_return_type:), which
predates this branch's rename to narrowed_return_type; renamed the
keyword and the example title to match.

lib/solargraph/parser/flow_sensitive_typing.rb and
lib/solargraph/source/chain/instance_variable.rb: took the combination -
shorter comments, four @PARAM tags, and a bespoke @sg-ignore explanation
replaced by the catalogued "Need to add nil check here" slug.

2202 examples, 0 failures, 45 pending. Strong typecheck unchanged at the
six known baseline problems.
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 narrow repeated calls to the same attr_reader-style method

1 participant