Skip to content

Skip gem RBI check when sorbet/rbi/gems does not exist #2330

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.

Sign up for GitHub

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 2 commits into
base: main
Choose a base branch
from
Open

Skip gem RBI check when sorbet/rbi/gems does not exist #2330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
377adf6
Skip gem RBI check when gem RBI dir does not exist
alexcrocha Jun 20, 2025
5263661
Log when gem RBI check operations cannot proceed
alexcrocha Jun 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions lib/ruby_lsp/tapioca/addon.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,16 +209,42 @@ def file_updated?(change, path)
#: -> void
def run_gem_rbi_check
state = @global_state #: as !nil
gem_rbi_check = RunGemRbiCheck.new(state.workspace_path)
queue = @outgoing_queue #: as !nil

gem_rbi_dir = gem_rbi_directory(state.workspace_path)
unless File.exist?(gem_rbi_dir)
queue << Notification.window_log_message("Did not find gem RBI directory", type: Constant::MessageType::WARNING)
return
end

gem_rbi_check = RunGemRbiCheck.new(state.workspace_path, gem_rbi_dir)
gem_rbi_check.run

queue = @outgoing_queue #: as !nil
queue << Notification.window_log_message(gem_rbi_check.stdout) unless gem_rbi_check.stdout.empty?

unless gem_rbi_check.stderr.empty?
queue << Notification.window_log_message(gem_rbi_check.stderr, type: Constant::MessageType::WARNING)
end
end

#: (String) -> String
def gem_rbi_directory(workspace_path)
Copy link
Member

@paracycle paracycle Jun 23, 2025

Choose a reason for hiding this comment

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

I am not sure I understand why this method is needed? The CLI command should be transparently get the correct value of the gem rbi directory as part of the CLI command options. That needs to be passed down here, not read the config from scratch.

Copy link
Contributor Author

@alexcrocha alexcrocha Jun 25, 2025

Choose a reason for hiding this comment

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

This is triggered by the Ruby LSP through the activate method. At this point we haven't shelled out to the Tapioca gem command yet, and the config hasn't been parsed by CLI.

Before we shell out, we need to do some cleanup. When we detect deleted or lingering RBIs (through git commands) we either restore them or delete the new ones, source.

We can either duplicate the config processing here in the add-on, or move this cleanup to the Tapioca gem command to be triggered optionally in add-on mode.

Copy link
Member

@paracycle paracycle Jun 26, 2025

Choose a reason for hiding this comment

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

The gem RBI command does that kind of clean up automatically anyway, so I am not even sure what these git operations are trying to do.

I think doing this outside of the gem RBI command is asking for trouble. I don't think we should be doing any config parsing, or relying on constant values to assume anything about where RBI files live, or which ones should be cleaned up, etc.

All that logic already exists. Please tell me what's missing that all this is needed.

Copy link
Contributor Author

@alexcrocha alexcrocha Jun 27, 2025

Choose a reason for hiding this comment

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

The tapioca gem command only runs when we detect lockfile differences. However, consider this scenario:

  • Developer adds/modifies/removes a gem and regenerates RBIs
  • Developer reverts the Gemfile changes and runs bundle install
  • The lockfile returns to its committed state
  • LSP restarts due to lockfile change, but finds no diff against the committed version
  • tapioca gem <gems> doesn't run, leaving orphaned RBI files or missing previously deleted ones

This is where the git operations come in: they clean up orphaned RBIs and restore deleted ones. While I understand the concern about duplicating logic, the add-on and CLI run in separate processes, so we must read the configuration independently.

We did consider the alternative of running tapioca gem on every activation to simplify this. However, we decided against it due to the potential negative impact on developer experience. For instance, if a developer pulls main and the latest commit didn't include updated gem RBIs, the add-on would immediately generate gem RBIs for files they didn't touch, polluting their workspace. We felt it was better to avoid this.

Copy link
Member

@paracycle paracycle Jun 27, 2025

Choose a reason for hiding this comment

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

But the gem command can clean up stale RBI files and create RBI files that should exist as well. I still don't understand why this level of detection needs to happen outside of the normal Tapioca gem RBI generation. We should be able to find a way to make the gem RBI generation to be smarter about this.

I don't think we want to try to duplicate the logic of configuration files, etc, outside of that flow. That won't scale.

Copy link
Member

@paracycle paracycle Jun 27, 2025

Choose a reason for hiding this comment

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

The tapioca gem command only runs when we detect lockfile differences. However, consider this scenario:

  • Developer adds/modifies/removes a gem and regenerates RBIs
  • Developer reverts the Gemfile changes and runs bundle install
  • The lockfile returns to its committed state
  • LSP restarts due to lockfile change, but finds no diff against the committed version
  • tapioca gem <gems> doesn't run, leaving orphaned RBI files or missing previously deleted ones

This scenario is exactly why the git based detection is not a great idea in the first place, and not a good reason to add more complexity to this. There is no good way to synchronize the state of RBI files with the state of the lockfile changes.

config_file_path = File.join(workspace_path, ::Tapioca::TAPIOCA_CONFIG_FILE)

if File.exist?(config_file_path)
begin
config = YAML.load_file(config_file_path, fallback: {})
outdir = config.dig("gem", "outdir") || ::Tapioca::DEFAULT_GEM_DIR

return File.expand_path(outdir, workspace_path)
rescue Errno::ENOENT
queue = @outgoing_queue #: as !nil
queue << Notification.window_log_message("Invalid YAML or file read error", type: Constant::MessageType::WARNING)
end
end

File.expand_path(::Tapioca::DEFAULT_GEM_DIR, workspace_path)
end
end
end
end
27 changes: 14 additions & 13 deletions lib/ruby_lsp/tapioca/run_gem_rbi_check.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ class RunGemRbiCheck
#: Process::Status?
attr_reader :status

#: (String project_path) -> void
def initialize(project_path)
#: (String, ?String) -> void
def initialize(project_path, gem_rbi_dir = ::Tapioca::DEFAULT_GEM_DIR)
@project_path = project_path
@gem_rbi_dir = gem_rbi_dir
@stdout = "" #: String
@stderr = "" #: String
@status = nil #: Process::Status?
Expand Down Expand Up @@ -79,16 +80,16 @@ def generate_gem_rbis
#: (Array[String] gems) -> void
def execute_tapioca_gem_command(gems)
Bundler.with_unbundled_env do
cmd = ["bundle", "exec", "tapioca", "gem", "--lsp_addon"]

if @gem_rbi_dir != ::Tapioca::DEFAULT_GEM_DIR
cmd.push("--outdir", @gem_rbi_dir)
end

cmd.concat(gems)

stdout, stderr, status = Open3 #: as untyped
.capture3(
"bundle",
"exec",
"tapioca",
"gem",
"--lsp_addon",
*gems,
chdir: @project_path,
)
.capture3(*cmd, chdir: @project_path)

log_message(stdout) unless stdout.empty?
@stderr = stderr unless stderr.empty?
Expand All @@ -99,7 +100,7 @@ def execute_tapioca_gem_command(gems)
#: (Array[String] gems) -> void
def remove_rbis(gems)
files = Dir.glob(
"sorbet/rbi/gems/{#{gems.join(",")}}@*.rbi",
"#{@gem_rbi_dir}/{#{gems.join(",")}}@*.rbi",
base: @project_path,
)
delete_files(files, "Removed RBIs for")
Expand All @@ -117,7 +118,7 @@ def cleanup_orphaned_rbis
#: (*untyped flags) -> Array[String]
def git_ls_gem_rbis(*flags)
self #: as untyped # rubocop:disable Style/RedundantSelf
.execute_in_project_path("git", "ls-files", *flags, "sorbet/rbi/gems/")
.execute_in_project_path("git", "ls-files", *flags, "#{@gem_rbi_dir}/")
.lines
.map(&:strip)
end
Expand Down
20 changes: 20 additions & 0 deletions spec/tapioca/ruby_lsp/run_gem_rbi_check_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,26 @@ class RunGemRbiCheckSpec < SpecWithProject

assert_project_file_exist("sorbet/rbi/gems/[email protected]")
end

it "respects custom gem directory" do
FileUtils.mkdir_p("#{@project.absolute_path}/custom/gem/dir")

foo = mock_gem("foo", "0.0.1") do
write!("lib/foo.rb", FOO_RB)
end
@project.require_mock_gem(foo)
@project.bundle_install!

FileUtils.touch("#{@project.absolute_path}/custom/gem/dir/[email protected]")

check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path, "custom/gem/dir")
check.run

refute_project_file_exist("custom/gem/dir/[email protected]")
assert_project_file_exist("custom/gem/dir/[email protected]")

@project.remove!("custom")
end
end
end
end
Expand Down
Loading