Conversation
GemPins.combine only merged YARD and RBS pins for Pin::Method. A Pin::Namespace hit the same code path's early exit, so the RBS side of a class or module (e.g. its generics) was silently dropped, and the leftover RBS pin was excluded from the output entirely because its path already appeared in the YARD set. Route Namespace pins through the same combine_with-based merge as Method pins, matching Pin::Closure and Pin::Namespace generics handling. Namespace#combine_with is a new override, needed because combine_with builds a fresh pin from only the attrs it sets, and type/visibility/gates would otherwise silently reset to the Pin::Namespace constructor defaults. Closure#combine_with also gains generic_defaults, which had the identical drop bug. Verified against ActiveSupport::HashWithIndifferentAccess (real YARD class + RBS generic class declaration): source changes from :yardoc to :combined and generics from [] to [T, U].
Two comments this PR added ran well past the repo's per-method 1-3 line budget for restating context available elsewhere: - COMBINABLE_PIN_TYPES's docstring (5 lines) repeated what Pin::Namespace#combine_with already documents about which attributes it preserves; kept only the merge-vs-one-side-wins fact. - The comment above gem_pins_combine_spec.rb's yard_pin fixture (6 lines) narrated the Pin::Base#== mechanics in full; kept only the fact needed to explain the location: fixture attribute. No code changes; rubocop and the edited spec still pass.
GemPins.combine now merges Pin::Namespace pins in addition to Pin::Method pins, calling assert_same on :type. Nokogiri::XML::XPath was a class through nokogiri 1.11 but became a module at 1.12; ruby/gem_rbs_collection's nokogiri signature was never updated past 1.11, so every RBS-derived pin for later nokogiri versions disagrees with the real source on this attribute. Under SOLARGRAPH_ASSERTS=on this crashed both the standalone Typecheck job and Overcommit's Solargraph pre-commit hook. An RBS signature's class/module keyword is a hand-maintained annotation that can go stale independent of how accurate the rest of the signature is, while the YARD-derived pin comes directly from parsing the gem's actual class/module keyword. Namespace#combine_with now prefers whichever side has source == :yardoc when :type disagrees, and :combine_with_type joins the existing :combine_with_visibility exemption in assert_or_log so a real cross-source disagreement logs instead of raising.
undercover flagged lib/solargraph/pin/namespace.rb's new combine_type at 80% coverage: no spec exercised the branch where neither disagreeing pin is YARD-sourced.
assert_or_log's "not ready for prime time, ignored" exemption idiom (shared with :combine_with_visibility) says a case is a known, unsolved problem awaiting real handling. That doesn't describe combine_type: the RBS-vs-real-source disagreement is a decided, permanent trade-off (prefer the real-source pin), not a TODO. Routing it through assert_or_log also meant it produced zero output under SOLARGRAPH_ASSERTS=on, since assert_or_log's non-raising exempted path returns before ever reaching its own logger.info call. Log the disagreement directly instead, unconditionally, and drop :combine_with_type from assert_or_log's exemption list along with its now-inapplicable @todo.
warn overstated it: this is an expected, handled case, not an operational concern.
apiology
marked this pull request as ready for review
September 8, 2026 17:13
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.
GemPins.combine merges a gem's YARD and RBS pins for the same path, but only for Pin::Method - a Pin::Namespace hits the same early exit before RBS is checked, and the leftover RBS pin is then dropped entirely:
Routes Namespace pins through the same combine_with merge Method pins already use. Namespace needs its own combine_with, since the base implementation resets unset attributes (type, visibility, gates) to constructor defaults instead of carrying them over; Closure#combine_with gains generic_defaults, which had the same drop.
Merging Namespace pins this way surfaced a real cross-source disagreement: ruby/gem_rbs_collection's nokogiri signature still says
class Nokogiri::XML::XPath, unchanged since nokogiri 1.11, though nokogiri redeclared it a module at 1.12. combine_with now prefers the YARD-derived pin's :type when the two disagree, since it comes from parsing the real keyword rather than a signature that can go stale.Pin::Reference::Superclass has an analogous symptom but a different mechanism (both pins survive, ApiMap::Store#get_superclass just picks the first by insertion order) - left for separate follow-up.