Skip to content

Infer precise Hash{K => V} types from hash literals - #65

Closed
apiology wants to merge 18 commits into
masterfrom
fix-issue-1227
Closed

Infer precise Hash{K => V} types from hash literals#65
apiology wants to merge 18 commits into
masterfrom
fix-issue-1227

Conversation

@apiology

@apiology apiology commented Aug 21, 2026

Copy link
Copy Markdown
Owner

This PR was written by Claude Code on behalf of @apiology.

A Hash literal infers only the bare ::Hash — far less precise than an
Array literal, which already infers Array<T> from its elements.

# @return [Hash{String => Integer}]
def h
  { 'a' => 1 }   # infers ::Hash, not Hash{String => Integer}
end

That imprecision costs anything downstream that needs the Hash's real
key/value types to type-check — #fetch, #[], #each, and the
per-conjunct Hash-record dispatch in castwide#1231.

Solution

NodeChainer#hash_pairs chains each key/value pair of a hash literal,
and Chain::Hash#inferred_type infers Hash{K => V} from those pairs'
own inferred types, mirroring how Chain::Array already infers
Array<T>. The plugins.yml repoint tests this against
lekemula/solargraph-rspec#36, which expects the added precision.

Dropped the :allow_unmatched_interface addition to
Pin::Parameter#compatible_arg? that was bundled here to fix castwide#1227
castwide#1266 already fixes that properly via real
structural conformance, and castwide#1231's own Hash-record dispatch is the
more complete answer for the record-shaped case. The castwide#1227 regression
spec is now pending, citing castwide#1266.

Pin::Parameter#compatible_arg? rejected any argument against an
RBS-interface-typed parameter (e.g. Hash#fetch's _Key) unless the
argument's type was nominally included via CoreFills::INCLUDES. Since
Hash#fetch's non-block, non-default overload takes a _Key-typed
parameter, no overload of a plain h.fetch(k) call ever matched, and
Pin::Method#return_type fell back to unioning every overload's return
type together - including the unbound generic X from the two
overloads that require a default value or a block.

Add :allow_unmatched_interface to the rules passed to conforms_to? in
compatible_arg?, matching the leniency TypeChecker itself already
applies by default (see Rules#require_interfaces_resolved?) everywhere
except the :alpha level.

Fixes castwide#1227
Chain::Hash#resolve returned the bare, unparameterized ::Hash type
for every hash literal, so a chained fetch/[] off a hash literal
leaked generic<T>/generic<U> instead of resolving to a concrete
value type. Chain::Array already infers Array<T> from its elements
the same way; this brings Chain::Hash to parity by chaining each
pair's key and value node (NodeChainer#hash_pairs) and inferring
Hash{K=>V} from their actual types (Chain::Hash#inferred_type).

Stacked on castwide#65 (fix-issue-1227), which added the
_receiver_path param to Chain::Hash#resolve that this commit merges
with.

Follow-on to PR 65: this covers the generic<T>/generic<U> leak PR 65
doesn't, tracked as tool-limitation:pr-65-follow-on.
Comment thread lib/solargraph/pin/parameter.rb Outdated
Comment thread lib/solargraph/source/chain/hash.rb Outdated
Comment thread spec/type_checker/levels/strong_spec.rb
Cut the "without it" comparison from the allow_unmatched_interface
comment in Pin::Parameter#compatible_arg? - state the current
behavior only, per review. Drop the @type on @pairs in Chain::Hash
- confirmed unneeded, the @PARAM tag on initialize already gives
Solargraph what it needs.
apiology/solargraph-rspec:fix-stale-hash-literal-assertions updates
two convention_spec.rb assertions this PR's own hash-literal inference
fix made more precise. Not opened as a PR yet - too early for
castwide/solargraph-rspec until this PR itself is ready.
apiology added a commit that referenced this pull request Aug 26, 2026
…verloads

# Conflicts:
#	lib/solargraph/pin/parameter.rb
#	lib/solargraph/source/chain/hash.rb
apiology and others added 4 commits August 31, 2026 21:41
Compress three new docstrings/tags added by this PR down to the
1-3 line budget, keeping only the non-obvious why.
run_solargraph_rspec_specs cloned lekemula/solargraph-rspec's default
branch unpinned, so it tested this branch's Hash record inference
against expectations written before records were inferred:

  expected: "Hash"
       got: "Hash{String => String}"

That is better inference meeting an older spec, not a regression.
lekemula/solargraph-rspec#36 raises those expectations; clone its head
branch until it merges. The original clone line stays in place,
commented out, so reverting this is a one-line change.

Drop the commented-out checkout of fix-stale-hash-literal-assertions:
it names an earlier attempt at this same problem, predating pull/36,
and would otherwise leave two comment layers naming two branches.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TfUfWzAWx3NTkJ79AtGeA9
castwide#1266 already fixes the Hash#fetch generic leak
this addition targeted, via real structural conformance instead of a
blanket allow. Mark the regression spec pending on that PR and let
compatible_arg? go back to its narrower rule set.
@apiology apiology changed the title Fix Hash#fetch generic leak from unmatched RBS interface overloads Infer precise Hash{K => V} types from hash literals Sep 7, 2026
Hash#fetch only takes its key as the _Key interface (rather than the
generic K) as of RBS 4.1.0, so the leak this spec regresses against
does not reproduce on older RBS. Marking it unconditionally pending
made RSpec fail it on rbs 3.10.0 in CI, since a pending example that
passes is itself a failure. Gate the pending call on RBS::VERSION so
the spec only expects the failure where it actually occurs.
key_types and value_types start empty, so Solargraph has nothing to
infer their element type from - the same reason pairs already carries
a @type tag two lines above. Both feed straight into
UniqueType.new(key_types, subtypes:), which documents both as
Array<ComplexType>.
initialize already carries @PARAM pairs, which gives Solargraph
enough to infer @pairs's type on assignment - the local @type here
was redundant, and a @type on a local isn't part of the pre-authorized
carve-out for unasked annotation changes. Confirmed 0 typecheck
problems on the file without it.
Every other Chain subclass's resolve takes exactly (api_map, name_pin,
locals), matching Link#resolve and both call sites in chain.rb. This
was the only one carrying a fourth parameter, and nothing in the
method body used it.
Both cited castwide#1223 (still open), suppressing an argument-type mismatch
on the AST node's own children before hash_pairs existed. Solargraph
now resolves both calls cleanly without them; typecheck stays at the
same pre-existing 15 problems elsewhere in the file, none new.
response = { jsonrpc: '2.0', id: id } now infers Hash{Symbol =>
String, Integer} instead of bare ::Hash, so assigning result/error
into it needs the full value-type union stated explicitly.

Assigning result or error still fails even with that union declared:
each is itself a union containing nil, and conformance checking
rejects a nil-bearing union against a target union that also contains
nil, even though every member is present in the target. Verified
against castwide#1223 (stacked: #40) -
checked out that branch, reproduced the exact case standalone, 0
problems found. Not yet merged upstream, so sg-ignore until it lands.
result[:capabilities] returns V | nil per Hash#[]'s own RBS
signature, so every merge!/[]= off it was unresolved once result's
value type stopped being bare ::Hash. fetch(:capabilities) raises
instead of returning nil, matching the actual guarantee (the key was
just set two lines above) and clearing all 13 problems.
Position#to_hash returns { line: Integer, character: Integer }, not
the bare Hash it was declared as. Range#to_hash was declared
Hash{Symbol => Position}, but its literal calls start.to_hash and
ending.to_hash - it never returned a Position at either key. Both
tags now match what the methods actually return.
@Level = LEVELS[LEVELS.values.index(@rank)] indexed a Symbol-keyed
Hash with an Integer - always nil, confirmed empirically. Rewritten
to keep the input level directly instead of round-tripping through
rank; @rank now uses fetch to state the non-nil guarantee the
LEVELS.key?(level) branch already establishes.

Hash#fetch(key), called with exactly one argument and no block,
still leaks generic<X> from its other two overloads into the return
type - a distinct, minimal, reproducible bug from the Hash{K=>V}
precision work, confirmed fixed on castwide#1266's branch.
set_result's nil literal and normalize's Array(Integer, Integer)
element access both typecheck clean once castwide#1223
lands; until then Solargraph reports NilClass instead of nil and an
unnarrowed nil on tuple element access. Both pre-existing on master.
@apiology

apiology commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Claude: Reopened upstream as castwide#1352, same branch, so it targets the real project instead of the fork.

@apiology apiology closed this Sep 9, 2026
apiology added a commit that referenced this pull request Sep 9, 2026
The Hash#fetch generic leak now resolves cleanly once #65's per-pair
Hash{K=>V} inference lands alongside castwide#1231's intersection/record
dispatch - RSpec flagged the pending block as unexpectedly passing.
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.

Hash#fetch on Hash{Symbol => Class<X>} miscomputes generics when rbs >= 4.1.0

1 participant