Skip to content

Solargraph drops the types gems add to core classes - #100

Draft
apiology wants to merge 5 commits into
merge-signatures-on-parameter-typesfrom
fix-gem-rbs-pin-lookup
Draft

Solargraph drops the types gems add to core classes#100
apiology wants to merge 5 commits into
merge-signatures-on-parameter-typesfrom
fix-gem-rbs-pin-lookup

Conversation

@apiology

@apiology apiology commented Sep 9, 2026

Copy link
Copy Markdown
Owner

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. bigdecimal ships def +: (BigDecimal) -> BigDecimal, and in a one-file project that requires it:

1 + BigDecimal('2')
# Wrong argument type for Integer#+: arg_0 expected Integer, received BigDecimal
# ...and the same for Float, Rational and Complex

Measured across the 133 gems in this repo's bundle, plus activesupport and its gem_rbs_collection RBS:

gem class methods lost
stringio, cgi, csv, open-uri StringIO 67 each — << close read write
rbs Bundler::*, Gem::Specification, Enumerable 17 — find_by_name each_slice
bigdecimal Integer, Float, Rational, Complex 16 — + - * / on each
open3 Open3 5 — capture2 capture2e capture3 popen2 popen3
activesupport Time 2 — + -

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.combine resolved the gem's RBS pin through ApiMap.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, so resolve_method_aliases combines by path at lookup.

Stacked on #95, which keeps the recovered signatures distinct instead of merging them into one polluted union.

@apiology
apiology added this pull request to stack #101 September 9, 2026 13:33
@apiology apiology changed the title Adding a BigDecimal to an Integer is reported as a type error Solargraph drops the types gems add to core classes Sep 9, 2026
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.
@apiology
apiology force-pushed the fix-gem-rbs-pin-lookup branch from cb32d9e to 2b63a29 Compare September 9, 2026 13:38
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.
@apiology
apiology removed this pull request from stack #101 September 10, 2026 03:02
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.
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