Conversation
Combining two method pins that describe the same arity with different
parameter types should keep one signature per parameter type, so a call
with a Float argument still selects the Float return. Today the two
collapse into a single signature whose parameter type is whichever side
came first and whose return type is the union of both:
(Integer) -> Integer combined with (Float) -> Float
=> (Integer) -> Integer, Float
Pin::Method#combine_same_type_arity_signatures already means to prevent
this: it merges two signatures only when the merge leaves type_arity
unchanged, precisely so parameter types can go on choosing the return
type. That check cannot fire. Pin::Parameter#combine_with returns self
unless the other parameter shares its closure, and a parameter pin's
closure is its enclosing signature pin, so parameters from two
separately-parsed method pins never combine. Their types never widen,
type_arity never changes, and the guard sees a merge that looks free.
The example asserts the desired behavior and is marked pending until
parameter types can widen across method pins.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
Combining two method pins collapsed signatures describing different
parameter types into one, so a call could no longer use its argument
types to pick a return type:
(Integer) -> Integer combine_with (Float) -> Float
=> (Integer) -> Integer, Float
combine_same_type_arity_signatures meant to prevent exactly that: it
merged only when combining left type_arity unchanged, type_arity being
the parameter decl markers plus a count of each parameter type's union
members. That count cannot change on this path. Parameter#combine_with
returns self unless the other parameter shares its closure, and a
parameter pin's closure is its enclosing signature pin, which differs
between two separately parsed methods. The combined signature therefore
always carried the left side's parameter types, type_arity always
matched, and the guard always merged.
type_arity was doing two jobs at once. Bucketing now keys on arity, the
parameter shape by itself, and the merge test is a direct comparison of
the two input signatures: matching generics, matching block, and every
parameter type equal by rooted_tags. Comparing the inputs rather than
the combined result is what lets the test fire at all. Return types
stay out of it, since widening one is the point of merging.
full_type_arity, type_arity and type_arity_decl are left in place; no
caller remains outside full_type_arity, which itself has none.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
Merging on matching parameter types keeps the Integer and Float signatures apart, so the example passes and RSpec fails it as a pending test that succeeded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
Integer#+ declares four RBS signatures differing only in parameter type. Merging them into one would leave a single return type and lose the distinction the declarations exist to make. The spec drives a real RBS-backed ApiMap and infers at four call sites, asserting Integer, Float, Rational and Complex in turn - one per signature. Nothing else in the suite covers dispatch end to end; the existing coverage checks the signature count on the pin, which stays correct even if inference collapses downstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
The existing end-to-end spec drives Integer#+, which is core RBS. Pin::Method#combine_with runs only from GemPins.combine_method_pins, so core method pins never reach combine_signatures at all and that spec passes with or without the parameter-type guard. Reproduce the merge where it actually happens: two Pin::Method pins for one method, combined the way a gem yardoc and its RBS collection entry are, with same-arity signatures differing only in parameter type. Master collapses them into one signature and Clip#infer returns the union of both return types; the guard keeps them apart so the Float argument still selects String. The same collapse is observable on the date gem today, where Date#upto(Date.new) infers Enumerator<Date, Date>, Date instead of Enumerator<Date, Date>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
combine_same_arity_signatures merges two same-arity signatures only when Signature#same_parameter_types? agrees, and that compares param.return_type.rooted_tags. YARD pin types are still unqualified when GemPins.combine runs while RBS types are rooted, so a YARD (String) -> Boolean and an RBS (::String) -> ::Boolean compare unequal and both survive, where they name the same types and should collapse to one. Measured over the 73 gems carrying both a YARD and an RBS-collection cache: 18059 method paths appear in both, 2818 reach the by-arity merge, and 867 produce different signature sets between this branch and master. Over 800 of those 867 are this rooted-versus-unrooted spelling difference rather than a real type difference, and nothing in the suite catches it. The example asserts the collapse to a single signature and stays agnostic on which spelling survives: combine_return_type picks the unrooted side today, while a qualify-before-compare fix would keep the rooted one, and pinning either would leave the example pending forever under the other. It fails on the signature count, so RSpec reports it pending rather than passing unexpectedly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
combine_same_arity_signatures merges two same-arity signatures only when Signature#same_parameter_types? agrees, and that compares param.return_type.rooted_tags. YARD pin types are unqualified when GemPins.combine runs while RBS types are rooted, so a YARD (Integer) -> String and an RBS (::Integer) -> ::String compare unequal and both survive, where they name one type and one overload. The previous example asserted signatures.length on the combined pin. That is the point of explosion rather than the consequence, and it would keep reporting pending even if the consequence were fixed some other way. Replace it with the symptom a user meets: typing Widget.new.scan( puts two entries labelled scan(count) in the parameter-hints popup, because TextDocument::SignatureHelp feeds Clip#signify results through Pin::Method#signature_help, which emits one entry per signature. The example asserts a single entry and names no type, so a qualify-before-compare fix and an unroot-before-compare fix both satisfy it. Confirmed against real gem data. Loading the cached YARD and RBS pins for the 62 gems in this namespace and running GemPins.combine leaves 88 method paths whose surviving signatures become identical once the leading :: is stripped from every parameter type. AST::Node#to_sexp is the clearest: (::Integer) -> ::String alongside (Integer) -> String, whose Pin::Method#detail then renders (*) => String instead of (count) => String in hover and in the completion list. Three other candidate symptoms show no difference and are not asserted. Clip#infer returns ::String either way, since the union is qualified before it reaches the caller. Strong typecheck reports no problems either way. Both signify and define collapse to one signature once a real argument is present, because overload selection picks the matching one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
999d703 pins this defect at Clip#signify, one layer below the protocol. Add a second pending example driving the same input through TextDocument::SignatureHelp, so it fails where an editor actually reads the result: message.result[:signatures] carries two entries labelled scan(count) where one describes the overload. Getting a combined pin into a Host-driven spec needs a route that survives Library#sync_catalog, which rebuilds the ApiMap from the bench on every request. Injecting pins with ApiMap#index, as the clip_spec example does, is wiped by that rebuild. A Convention supplying the pins in its Environ is carried through it, and spec/source_map_spec.rb already registers one that way. The unregister call sits in an ensure block rather than at the end of the example. A pending example stops at the raised expectation, so a trailing unregister would never run and Widget#scan would be injected into every source map mapped later in the same process. Verified against a control: two pins both spelling the parameter ::Integer collapse to a single scan(count) entry through this same harness, so the duplication comes from the rooted-unrooted comparison rather than from the Convention route. The assertion names only the label, so a qualify-before-compare fix and an unroot-before-compare fix both satisfy it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
Pin::Method#combine_same_arity_signatures merges two signatures only
when Callable#same_parameter_types? says their parameter types agree.
That comparison keyed on ComplexType#rooted_tags, but the two pins
GemPins.combine hands it come from different sources: YARD types are
still unqualified, RBS types are rooted. So (String) and (::String) -
one type, two spellings - compared unequal and both signatures survived.
AST::Node#to_sexp came out of combine with two, where master had one:
signatures=2 detail="(*) => String"
params=["::Integer"] return="::String"
params=["Integer"] return="String"
88 method paths across this workspace's cached gems show that shape.
Key on ComplexType#tags instead. It renders every union member and
every subtype unrooted, so Array<::String> and Array<String> both come
out "Array<String>", while ::Foo::Bar still renders "Foo::Bar" and stays
distinct from "Bar". Qualifying the YARD types up front was ruled out:
at both pin-creation and combine time the map lacks dependency-gem
namespaces, and a name like Parser::Source::Range mis-resolves to core
::Range rather than failing.
The two examples that pinned the gap are unpended here rather than in a
follow-up commit, so that no commit reports them as passing
unexpectedly.
Pin::Parameter#type_arity_decl, Pin::Callable#type_arity and Pin::Callable#full_type_arity existed to decide whether combining two signatures would lose the mapping from specific parameter types to specific return types. That guard now runs through same_parameter_types?, which compares parameter type tags directly, so the three became unreachable except from each other. Pin::Parameter#arity_decl stays: Pin::Callable#arity still uses it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNEbPE8fjo8Ub7EXsBkbJJ
GemPins.combine resolved the RBS counterpart of each YARD pin by building an ApiMap over the gem's RBS pins and looking the path up in it. ApiMap also loads Ruby core, so for a method the gem adds to a core class the lookup returned core's pin rather than the gem's. The wrong pin was then combined, and because the path had already been recorded as seen in YARD, the gem's own pin was excluded from the rbs-only remainder too, so its signature was dropped entirely. bigdecimal declares `def +: (BigDecimal) -> BigDecimal`; the pin exists in its RbsMap and appeared nowhere afterwards. Index the gem's own method pins by path instead. The lookup only ever wanted a pin from the array it was handed.
Pins are combined per gem when its cache is built, so core and a gem that reopens a core class never meet: each is cached on its own and both reach the api map as separate pins for one path. A caller taking the first of them sees only that source's signatures. Recovering bigdecimal's Integer#+ signature is not enough on its own for that reason - it lands on a different pin from core's four, and whichever sorts first wins. Group by path and combine when resolving aliases, which is the point every pin for a path has been assembled. This restores at lookup what castwide#1195 moved to cache-build time. That change was deliberate, and the cost it avoided is real; the narrower case it did not anticipate is two sources for one path that are cached separately and so are never combined at build time at all.
Asserts what the fix restores: with bigdecimal required, Integer#+ offers a signature taking a BigDecimal alongside core's four. Reverting either commit fails it - the index fix alone leaves only the BigDecimal signature, and neither alone gives five. The assertion is made against ApiMap#get_method_stack rather than DocMap#pins. The gem's pin carries its own signature either way; the loss only shows one layer up, where core's pin for the same path meets it. Three earlier attempts asserted against the doc map and passed with the fix reverted, guarding nothing. The gem's combined cache entry is cleared first, because GemPins.combine runs only when that entry is absent and a stale one would leave the example reporting on cached output.
The shipped example asserted only that the merged Integer#+ pin offers
bigdecimal's BigDecimal signature. That passes with either half of the
fix applied on its own, so neither half was covered, and nothing checked
the behaviour a user sees.
Four examples now split the two halves and reach the type checker:
- offers the gem's signature alongside core's - fails only when both
halves are reverted
- still offers core's own signatures - fails when the
resolve_method_aliases half is reverted, where the lookup returns
the gem's pin alone
- caches the gem's signature by itself, without core's - fails when
the GemPins.combine half is reverted, where core's four overloads
are baked into the gem's cached pin
- accepts an argument of the type the gem declares - typechecks a
one-file project at strong level; without both halves it reports
four Wrong argument type problems on 1 + big
A unit example covers combine_method_pins_by_path directly: same-path
method pins merge into one, other pins pass through untouched.
The context's requires list is now empty. The outer before block ran
DocMap#cache_all! for bigdecimal ahead of the uncache, leaving a
memoized copy of the previous run's combined pins that the uncache
could not reach, so a run after a lib change read stale output.
The RSpec, Linting, Typecheck and Plugin workflows did not start for 2e21416; only CodeQL did, and the PR reported mergeable state UNKNOWN across repeated polls. This empty commit tests whether a fresh push gets them scheduled.
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.
This PR was written by Claude Code on behalf of @apiology.
Problem: A gem's RBS declaration for a method on a class Ruby core already defines is silently dropped, so calls it documents are reported as type errors.
bigdecimalshipsdef +: (BigDecimal) -> BigDecimal, and in a one-file project that requires it:Measured across the 133 gems in this repo's bundle, plus activesupport and its
gem_rbs_collectionRBS:<<closereadwrite…find_by_nameeach_slice…+-*/on eachcapture2capture2ecapture3popen2popen3+-ActiveSupport is small here because only 2 of the 341 methods it declares on core classes share a path with a core method; the rest are new methods and resolve correctly.
Solution:
GemPins.combineresolved the gem's RBS pin throughApiMap.new(pins: rbs_pins), which also loads Ruby core — so it found core's pin, combined that, and discarded the gem's. It now indexes the gem's own pins by path. Core and a gem reopening a core class are also cached separately and meet nowhere, soresolve_method_aliasescombines by path at lookup.Stacked on #1356, which keeps the recovered signatures distinct instead of merging them into one polluted union. Until that merges, the diff here also shows its commits.