Support RBS signatures in check-shims command #2482
+94
−1
Conversation
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
The check-shims command now recognizes RBS comment syntax (#:) in
addition to Sorbet's sig blocks when determining if a shim is adding
type information versus duplicating existing definitions.
This ensures shims using RBS syntax like `#: () -> void` are treated
the same as shims using `sig { void }` - they won't be flagged as
duplicates if they're adding type information to an untyped method.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
amomchilov
approved these changes
Jan 7, 2026
|
|
||
| # Another prop has the same RBS comment, we have a duplicate | ||
| other_rbs_comments = extract_rbs_comments(node) | ||
| return true if shim_rbs_comments.any? { |rbs| other_rbs_comments.include?(rbs) } |
Contributor
There was a problem hiding this comment.
Suggested change
| return true if shim_rbs_comments.any? { |rbs| other_rbs_comments.include?(rbs) } | |
| return true if shim_rbs_comments.intersect?(other_rbs_comments) |
Contributor
Author
There was a problem hiding this comment.
Unfortunately intersect? won't work here because RBI::RBSComment implements == but not eql?/hash:
c1 = RBI::RBSComment.new('() -> void')
c2 = RBI::RBSComment.new('() -> void')
c1 == c2 # => true
[c1].include?(c2) # => true (uses ==)
[c1].intersect?([c2]) # => false (uses eql?/hash)I've added a comment to explain this.
Contributor
There was a problem hiding this comment.
@sambostock Not blocking this PR, but could we make RBI::RBSComment implement .eql? or is there something preventing that?
RBI::RBSComment implements == but not eql?/hash, so intersect? (which uses Set-like comparisons) won't work correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
egiurleo
approved these changes
Jan 20, 2026
Contributor
|
@sambostock I suspect you'll need to rebase to get CI fixes. I'll investigate further if that doesn't fix things. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Motivation
The
check-shimscommand detects duplicate definitions between shim files and generated RBIs. It correctly ignores duplicates when the shim adds asigblock (since the shim is adding type information, not just duplicating). However, it doesn't recognize RBS comment syntax (#:) as a signature, so shims using RBS syntax are incorrectly flagged as duplicates.With Sorbet's RBS support, users can now write type signatures using RBS comments instead of
sigblocks. Thecheck-shimscommand should treat these equivalently.Implementation
Modified
has_duplicated_methods_and_attrs?inRBIFilesHelperto check forRBI::RBSCommentinstances in a node'scommentsarray, in addition to checking thesigsarray. The logic mirrors the existing signature handling:Added a small helper method
extract_rbs_commentsto extract RBS comments from a node.Tests
Added 3 new tests mirroring the existing
sigblock tests:ignores duplicates that have an RBS signatureignores duplicates that have a different RBS signaturedetects duplicates that have the same RBS signature