Skip to content

Bump typing gems and implement required changes #3610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Jun 17, 2025

Motivation

Bump Sorbet to get the latest RBS support

Implementation

  • Since Sorbet now supports type aliases in RBS, we can remove the shims
  • Abstract methods now are required to raise a NotImplementedError
    • However, because the RuboCop addon does NOT implement the version method yet, we need to rescue NotImplementedError in the server or it never starts.

I think raising runtime error for abstract methods imposes a great risk for Ruby LSP since we don't have control on how addons are implemented. Granted that we were lucky that addon.version could previously be nil without any trouble, but now that we're even more vulnerable to addon implementation breaking Ruby LSP.

(I know it'd have been the case if we still use RBI for abstract methods)

Automated Tests

Manual Tests

Copy link
Member Author

st0012 commented Jun 17, 2025


How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@st0012 st0012 force-pushed the bump-typing-gems branch from c60c6ab to 0f0b9bf Compare June 17, 2025 12:27
- Since Sorbet now supports type aliases in RBS, we can remove the shims
- Abstract methods now are required to raise a NotImplementedError
  - However, because the RuboCop addon does NOT implement the `version`
    method yet, we need to rescue NotImplementedError in the server or
    it never starts.
@st0012 st0012 force-pushed the bump-typing-gems branch from 0f0b9bf to 83436bd Compare June 17, 2025 12:31
@st0012 st0012 changed the title Bump Tapioca, Spoom, and Sorbet Bump typing gems and implement required changes Jun 17, 2025
@st0012 st0012 added chore Chore task server This pull request should be included in the server gem's release notes labels Jun 17, 2025
@st0012 st0012 marked this pull request as ready for review June 17, 2025 12:36
@st0012 st0012 requested a review from a team as a code owner June 17, 2025 12:36
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about defining a custom error class for AbstractMethodError so that we don't have to copy the message around everywhere?

version = begin
addon.version
rescue NotImplementedError
"[unknown version]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"[unknown version]"
nil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore task server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants