Skip to content
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

[Tapioca Add-on] Generate DSL RBIs in add-on mode #2093

Merged
merged 3 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
66 changes: 64 additions & 2 deletions lib/ruby_lsp/tapioca/addon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
return
end

require "zlib"

module RubyLsp
module Tapioca
class Addon < ::RubyLsp::Addon
Expand All @@ -23,24 +25,33 @@ def initialize

@global_state = T.let(nil, T.nilable(RubyLsp::GlobalState))
@rails_runner_client = T.let(nil, T.nilable(RubyLsp::Rails::RunnerClient))
@index = T.let(nil, T.nilable(RubyIndexer::Index))
@file_checksums = T.let({}, T::Hash[String, String])
@outgoing_queue = T.let(nil, T.nilable(Thread::Queue))
end

sig { override.params(global_state: RubyLsp::GlobalState, outgoing_queue: Thread::Queue).void }
def activate(global_state, outgoing_queue)
@global_state = global_state
return unless @global_state.enabled_feature?(:tapiocaAddon)

@index = @global_state.index
@outgoing_queue = outgoing_queue
Thread.new do
# Get a handle to the Rails add-on's runtime client. The call to `rails_runner_client` will block this thread
# until the server has finished booting, but it will not block the main LSP. This has to happen inside of a
# thread
addon = T.cast(::RubyLsp::Addon.get("Ruby LSP Rails", ">= 0.3.17", "< 0.4"), ::RubyLsp::Rails::Addon)
@rails_runner_client = addon.rails_runner_client
outgoing_queue << Notification.window_log_message("Activating Tapioca add-on v#{version}")
@outgoing_queue << Notification.window_log_message("Activating Tapioca add-on v#{version}")
@rails_runner_client.register_server_addon(File.expand_path("server_addon.rb", __dir__))
rescue IncompatibleApiError
# The requested version for the Rails add-on no longer matches. We need to upgrade and fix the breaking
# changes
@outgoing_queue << Notification.window_log_message(
"IncompatibleApiError: Cannot activate Tapioca LSP add-on",
type: Constant::MessageType::WARNING,
)
end
end

Expand All @@ -63,7 +74,58 @@ def workspace_did_change_watched_files(changes)
return unless T.must(@global_state).enabled_feature?(:tapiocaAddon)
return unless @rails_runner_client # Client is not ready

nil
constants = changes.flat_map do |change|
path = URI(change[:uri]).to_standardized_path
Copy link
Contributor

@andyw8 andyw8 Nov 26, 2024

Choose a reason for hiding this comment

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

(nit) Perhaps extract the body of this to its own method for readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored the checksum logic into its own method. It's a bit longer and I don't love the boolean returns but I think main loop reads much better now. Lmk what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little uncomfortable that file_updated? has side-effects. I'll take a look to see if there is another way we could structure it.

next if path.end_with?("_test.rb", "_spec.rb")
next unless file_updated?(change, path)

entries = T.must(@index).entries_for(path)
next unless entries

entries.filter_map do |entry|
entry.name if entry.class == RubyIndexer::Entry::Class || entry.class == RubyIndexer::Entry::Module
end
end.compact

return if constants.empty?

@rails_runner_client.trigger_reload
@rails_runner_client.delegate_notification(
server_addon_name: "Tapioca",
request_name: "dsl",
constants: constants,
)
end

private

sig { params(change: T::Hash[Symbol, T.untyped], path: String).returns(T::Boolean) }
def file_updated?(change, path)
case change[:type]
when Constant::FileChangeType::CREATED
@file_checksums[path] = Zlib.crc32(File.read(path)).to_s
return true
when Constant::FileChangeType::CHANGED
current_checksum = Zlib.crc32(File.read(path)).to_s
if @file_checksums[path] == current_checksum
T.must(@outgoing_queue) << Notification.window_log_message(
"File has not changed. Skipping #{path}",
type: Constant::MessageType::INFO,
)
else
@file_checksums[path] = current_checksum
return true
end
when Constant::FileChangeType::DELETED
@file_checksums.delete(path)
else
T.must(@outgoing_queue) << Notification.window_log_message(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we anticipating other change types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, Sorbet was complaining and I thought it'd be nice to have an else here for the future.

"Unexpected file change type: #{change[:type]}",
type: Constant::MessageType::WARNING,
)
end

false
end
end
end
Expand Down
6 changes: 5 additions & 1 deletion lib/ruby_lsp/tapioca/server_addon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@ def name
def execute(request, params)
case request
when "dsl"
dsl(params)
fork do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We run the DSL generation in a short living process because it uses a lot of memory.

dsl(params)
end
end
end

private

def dsl(params)
load("tapioca/cli.rb") # Reload the CLI to reset thor defaults between requests
::Tapioca::Cli.start(["dsl", "--lsp_addon", "--workers=1"] + params[:constants])
end
end
end
Expand Down
4 changes: 4 additions & 0 deletions lib/tapioca/commands/abstract_dsl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ def initialize(

sig { params(outpath: Pathname, quiet: T::Boolean).returns(T::Set[Pathname]) }
def generate_dsl_rbi_files(outpath, quiet:)
if @lsp_addon
pipeline.active_compilers.each(&:reset_state)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe extract this to pipeline#reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think main issue is another method checking for lsp_addon. I don't want this code to be reusable for other clients. It's a unique thing (kind of hacky and not ideal 😬 ) for addon support.

end

existing_rbi_files = existing_rbi_filenames(all_requested_constants)

generated_files = pipeline.run do |constant, contents|
Expand Down
7 changes: 7 additions & 0 deletions lib/tapioca/dsl/compiler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ def requested_constants=(constants)
@@requested_constants = constants # rubocop:disable Style/ClassVars
end

sig { void }
def reset_state
@processable_constants = nil
@all_classes = nil
@all_modules = nil
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are memoized but in LSP mode we want them to be calculated every time.


private

sig do
Expand Down
5 changes: 5 additions & 0 deletions sorbet/rbi/shims/uri.rbi
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,9 @@ module URI
sig { returns(String) }
attr_reader :fragment
end

class Generic
sig { returns(String) }
def to_standardized_path; end
end
end
Loading