Skip to content

Narrow bare, implicit-self attr_reader-style accessor calls - #53

Open
apiology wants to merge 11 commits into
gh-fix-1249from
self-rooted-accessor-narrowing
Open

apiology wants to merge 11 commits into
gh-fix-1249from
self-rooted-accessor-narrowing

Conversation

@apiology

@apiology apiology commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Summary

A nil-guard on a bare, implicit-self call (e.g. return nil if steps.nil?, where steps is an argless method like an attr_reader) left a later call to the same accessor (e.g. steps.empty?) unnarrowed — reported at castwide#1258 (comment).

Root cause: FlowSensitiveTyping#chain_pin's length-1 branch always went through find_var, which only matches tracked local/instance-variable pins. A bare steps reference is a :send node (method call), not :lvar, so lookup silently found nothing.

class Repro
  # @return [Array<Hash>, nil]
  attr_reader :steps

  def identify
    return nil if steps.nil? # or: return nil unless steps
    steps.empty? # was: Unresolved call to empty? on Array<Hash>, nil
  end
end

chain_pin now distinguishes a length-1 chain word rooted in a real :lvar/:ivar node from one rooted in a :send node (a 0-arg self-call), and synthesizes a pin against the enclosing closure (now threaded into FlowSensitiveTyping from each node processor's region.closure) instead of a variable's. process_call_chain's bare-truthy-guard handling is extended from chain length >= 2 to >= 1 for the same reason.

Based on gh-fix-1249 (castwide/solargraph#1258) since it depends on that PR's chain-narrowing infrastructure. Once castwide/solargraph#1258 merges, this should be retargeted at castwide/solargraph:master (or opened fresh there).

Test plan

  • Added 2 regression specs (bare .nil? guard, bare truthy guard) to spec/parser/flow_sensitive_typing_spec.rb
  • bundle exec rspec -- 1629 examples, 0 failures
  • bundle exec rubocop on changed files -- no offenses
  • bundle exec solargraph typecheck --level strong on changed files -- 28 pre-existing problems on gh-fix-1249 unmodified; this diff adds none (verified line-by-line)
  • Manually reproduced the original comment's repro and confirmed it now type-checks clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01PMbuVPLPj8CjHG8EkEQjrh

A nil-guard on a bare call (e.g. 'return nil if steps.nil?', where
steps is an argless method like an attr_reader) previously left a
later call to the same accessor (e.g. 'steps.empty?') unnarrowed --
FlowSensitiveTyping's chain-narrowing (added for explicit-receiver
chains like 'pin.location') only resolved a single-word chain via
find_var, which looks up tracked local/instance variables and can
never match a method call.

chain_pin now recognizes when a length-1 chain word actually came
from a :send node (a method call, since the parser only emits :lvar
for names already assigned as locals in scope) rather than an :lvar
node, and synthesizes a pin rooted at the enclosing closure instead
of a variable's. FlowSensitiveTyping now takes that closure as a
constructor argument from each node processor's `region.closure`.

process_call_chain's bare-truthy-check handling is extended from
chain_words.length >= 2 to length >= 1 for the same reason, so
'return nil unless steps' narrows the same way 'return nil if
steps.nil?' does.

SKIP=Solargraph: this branch (castwide#1258, unmerged)
already has 28 pre-existing `solargraph typecheck --level strong`
problems in these files before this commit; this change adds none
(verified line-by-line against the pre-existing baseline).

Fixes castwide#1258 (comment)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PMbuVPLPj8CjHG8EkEQjrh
apiology added a commit that referenced this pull request Aug 6, 2026
apiology added a commit 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
apiology marked this pull request as ready for review August 6, 2026 02:15
@apiology

apiology commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Claude: drafted while auditing @sg-ignore suppressions in a downstream project, reviewing before posting.

This fix narrows a bare accessor when the guarded expression is called again directly, but the narrowing doesn't propagate through an assignment into a fresh local.

Reproduction

# typed: true
# frozen_string_literal: true

class Repro
  # @return [Array<Hash>, nil]
  attr_reader :steps

  # @return [Boolean]
  def works
    return false if steps.nil?

    steps.empty?  # 0 problems - narrows correctly
  end

  # @return [Boolean]
  def still_broken
    return false if steps.nil?

    local = steps
    local.empty?  # Unresolved call to empty?
  end
end

Confirmed against the current fork tip (this PR's commit is already an ancestor).

…nment

#53 (comment) reported that narrowing a bare,
implicit-self accessor doesn't propagate through an assignment into a
fresh local (e.g. local = steps; local.empty? left unresolved).

Verified against this branch's HEAD (bbaf7f9) that the reported
repro already resolves correctly: solargraph typecheck --level
strong reports 0 problems, and Chain#infer for local.empty?
resolves local to ::Array<::Hash>. BaseVariable#probe re-infers a
local's assignment expression at its own source position via
Chain#infer, and that position already falls inside the narrowed
presence range recorded by FlowSensitiveTyping for the bare steps
call, so the fact carries through without any code change needed.

This adds the missing spec coverage for that path so a future
regression here is caught.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RiL3AMsRKcUzfrTHTsVzxv
@apiology

apiology commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Reopening to nudge CI ��� push at 2026-08-06T19:01:06Z never triggered a synchronize event (no check-runs created for commit 70992a5 after 20+ minutes, while other branches in this repo triggered CI within seconds).

@apiology apiology closed this Aug 6, 2026
@apiology apiology reopened this Aug 6, 2026
@apiology

apiology commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Claude: investigated this report.

Doesn't reproduce at this branch's current HEAD, commit bbaf7f9.

Checked two ways:

  1. solargraph typecheck --level strong on the exact repro from this comment (both works and still_broken) reports 0 problems, including a fresh (non-cached) run.
  2. Direct ApiMap#clip_at probe on local.empty? infers local as ::Array<::Hash>.

Root cause it isn't hitting: BaseVariable#probe re-infers a local's assignment expression (Chain#infer) at the assignment's own source position, not at the point of use. local = steps sits inside the presence range FlowSensitiveTyping already recorded for the bare steps call from the .nil? guard, so the narrowing fact is already in scope when local's type gets computed — no propagation-through-assignment logic is needed for this shape.

Added a spec (narrows a bare, implicit-self attr_reader-style accessor assigned into a fresh local variable) matching this comment's repro to spec/parser/flow_sensitive_typing_spec.rb and pushed it to this branch: 70992a5. bundle exec rspec spec/parser/flow_sensitive_typing_spec.rb -- 52 examples, 0 failures, run locally.

GitHub Actions hasn't picked up this commit yet (unrelated platform outage), so CI hasn't confirmed this independently yet.

If you were seeing this fail in the downstream project outside this minimal repro, a fuller reproduction (e.g. the local reassigned conditionally, or read across a block boundary) would help narrow down what's different.

apiology added a commit that referenced this pull request Aug 6, 2026
…only inference

Pulls in two new upstream commits on top of the already-merged castwide#1259
base:

- Recognize multi-statement raise/fail branches in flow-sensitive
  typing: always_leaves_compound_statement? now recurses into :begin
  nodes' last child, so a clause like `msg = "bad"; raise msg` is
  still recognized as unconditionally leaving, not just a single bare
  `raise`/`fail` send.
- Infer only the lhs type for `x || raise(...)` and `x ||= raise(...)`:
  new logic in node_chainer.rb and Chain::Or#resolve treats a
  never-returning rhs the same way as the flow-sensitive-typing
  narrowing already does, so the combined type is just the lhs's
  non-nil type instead of a union with the (unreachable) rhs.

Conflict in lib/solargraph/parser/flow_sensitive_typing.rb: the
upstream commits move always_leaves_compound_statement? out of
FlowSensitiveTyping and into the shared
Solargraph::Parser::ParserGem::NodeMethods module (aliased as
Solargraph::Parser::NodeMethods, already included by
FlowSensitiveTyping), so it can be reused from node_chainer.rb, and
extend it with :begin-node support. Removed the now-duplicate local
copy in favor of the shared one; kept this branch's own :closure
attr_reader addition (from #53, needed by every
other FlowSensitiveTyping.new caller already updated on this branch).

Also dropped two now-superfluous `@sg-ignore Need to add nil check
here` comments in node_chainer.rb (above the
always_leaves_compound_statement?(or_asgn_rhs_node) and
always_leaves_compound_statement?(or_rhs_node) calls): on the PR's own
source branch, NodeChainer.chain's node arguments are inferred as
`Array, nil` throughout that branch (a broad, unrelated pre-existing
mismatch visible across dozens of lines when typechecked standalone),
so the @sg-ignore was suppressing a real mismatch there. On this
integration branch those same node variables are already correctly
typed `Parser::AST::Node, nil` (fixed by an earlier-merged PR), and
always_leaves_compound_statement?'s own param is already declared
nilable, so passing them needs no suppression - the local Solargraph
typecheck hook correctly flagged both comments as unneeded.

Verified: spec/parser/flow_sensitive_typing_spec.rb,
spec/parser/node_methods_spec.rb, spec/source/chain/or_spec.rb,
spec/parser/node_chainer_spec.rb (134 examples, 0 failures, 4
pending), and a broader safety net — spec/parser, spec/source,
spec/source_map/clip_spec.rb (475 examples, 0 failures, 14 pending) —
all passing locally.
@apiology
apiology marked this pull request as draft August 18, 2026 02:51
The two apiology/plate-spinner suppressions slugged pr-53-follow-on both
have the same shape: a bare, implicit-self accessor is nil-guarded by an
early return, the next line assigns it to a local carrying an explicit
`# @type` tag, and the TypeChecker reported "Declared type Array<Object>
does not match inferred type Array<Object>, nil for variable steps_list".

Existing coverage for this branch checks Clip#infer on a later reference
to the local. The declared-vs-inferred check that produced those
suppressions runs through Pin::BaseVariable#probe instead and had no
spec, so these add it for both guard forms in that file - `return nil if
steps.nil?` and `return ['', nil] if substeps.nil?`.

Both pass on this branch as-is; no lib change accompanies them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CXmnT5gSB1PheL9UbiGEVA
apiology added a commit that referenced this pull request Aug 22, 2026
…26-08-04

# Conflicts:
#	spec/type_checker/levels/strong_spec.rb
apiology added a commit that referenced this pull request Aug 22, 2026
…ranch 2026-08-04"

This reverts commit 1d26721, reversing
changes made to 4415fcf.
@apiology

Copy link
Copy Markdown
Owner Author

Claude: CI triggering issue has since resolved - all checks now run on this branch (confirmed via gh pr checks 53). One check is currently failing though: run_solargraph_rspec_specs (a downstream solargraph-rspec compatibility run), 3 failures in Solargraph::Rspec::Convention type inference specs, unrelated to this PR's narrow-through-assignment fix. Filed separately as a decision item rather than pulled into this comment thread.

apiology added a commit that referenced this pull request Sep 2, 2026
This branch independently introduced the same self-exclusion logic
in Pin::BaseVariable#return_types_from_node that commit 2824fc2
fixed elsewhere: comparing candidate assignments with Array#include?
(structural Parser::AST::Node#==) instead of by identity, which can
wrongly exclude an unrelated but textually-identical guard call from
narrowing candidates. Ported the same fix here.

The 4 new regression specs are marked pending on
#53 - the capability they
exercise (narrowing a bare, implicit-self attr_reader-style accessor
call at all) is introduced by that PR and is not present on this
branch yet. RSpec will force removal of the pending markers ("Pending
test passed unexpectedly") once both #53 and this
PR have merged and that capability is available here.
apiology added a commit 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 and others added 6 commits September 3, 2026 21:38
Brings PR 1258's latest three commits (comment trimming, BaseVariable
eql?/hash coverage, resolve-param and equality_fields coverage) onto the
bare implicit-self narrowing work stacked on top of it.

Two conflicts, both docstring-only, in FlowSensitiveTyping; no code hunk
conflicted. gh-fix-1249 compressed the #chain_pin and #process_call_chain
docstrings to the 1-3 line review budget while this branch had extended
both to describe the new bare 0-arg self-call case. Each resolution keeps
the compressed wording and adds one clause naming that case, so both
intents survive.

The rest of gh-fix-1249's trimming (the #parse_receiver_chain docstring,
two removed prose ignore markers, and the shorter Range.from_node marker
slug) auto-merged; every code change from this branch, including
#self_call_pin, the closure constructor argument, and the length >= 1
guard in #process_call_chain, is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TfUfWzAWx3NTkJ79AtGeA9
The new docstrings and inline comments for bare, implicit-self
accessor narrowing ran 4-7 lines each, well past this repo's
budget of a few lines per method. Compress them to state the
same facts without re-explaining alternatives already visible
in the code.
The two new @type-cast specs proved non-nilness only by calling
generic Enumerable methods on the assigned local, which doesn't
check the declared element type. Pass the local into a method
whose parameter is typed exactly Array<Hash> instead, so a wrong
or still-nilable inferred type fails the call directly.
The word wasn't otherwise defined anywhere in this scope, so it
read as a reference rather than an illustration. Add "e.g." to
say so explicitly.
Flow-sensitive narrowing already removes nil from the local
before it reaches consume(); the explicit @type annotation
changed nothing. Removed it and renamed both examples off
"@type" to describe what they actually check.
chain_words.length == 1 already guarantees a first element, but
Solargraph can't prove that from a length check alone. Binding
it to a local and returning early on nil lets the checker verify
it directly, instead of trusting an unenforced caller invariant.
@apiology
apiology marked this pull request as ready for review September 8, 2026 18:31
lib/solargraph/parser/flow_sensitive_typing.rb's chain_pin method has
an @sg-ignore reading "chain_words is never empty - callers already
checked," describing a check that can't actually fail, not the real
reason the ignore is needed. The call site - find_var(chain_words.first,
...) inside chain_pin - is reached from a receiver chain rooted in a
block parameter, the exact shape this PR's own fix addresses for a
different call site in this same file.

Relabels the marker to cite this PR's URL instead of the stale reason
text.

Comment-text only. solargraph typecheck --level strong reports the
same 12 pre-existing problems before and after; rubocop clean.
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.

1 participant