From 125f722e59301511b8b3df3418b10a961bcadcef Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 16 Jan 2025 10:23:16 -0500 Subject: [PATCH] Change our internal RuboCop integration identifier (#3067) ### Motivation This PR addresses point 1 and 2 from #3046 Now that the RuboCop add-on is using the `rubocop` identifier, we need to change ours to prevent conflicts, while still ensuring that users who are on older versions of RuboCop are getting the right behaviour. ### Implementation Essentially, we are changing our identifier to `rubocop_internal`. The only extra logic is falling back when users have explicitly configured their formatter or linter as `rubocop`. Since that is now owned by RuboCop, but only present on versions higher than v1.70, we need to check the version and ensure that it's actually available or fallback to our internal integration. ### Automated Tests Added tests. --- lib/ruby_lsp/global_state.rb | 30 ++++++++- lib/ruby_lsp/server.rb | 11 ++-- test/global_state_test.rb | 65 +++++++++++++++++-- test/requests/code_actions_formatting_test.rb | 4 +- .../requests/diagnostics_expectations_test.rb | 4 +- test/requests/diagnostics_test.rb | 6 +- test/requests/formatting_expectations_test.rb | 4 +- test/server_test.rb | 10 +-- vscode/package.json | 4 +- 9 files changed, 108 insertions(+), 30 deletions(-) diff --git a/lib/ruby_lsp/global_state.rb b/lib/ruby_lsp/global_state.rb index dfe2b0114..02df8eac2 100644 --- a/lib/ruby_lsp/global_state.rb +++ b/lib/ruby_lsp/global_state.rb @@ -94,6 +94,7 @@ def apply_options(options) @workspace_uri = URI(workspace_uri) if workspace_uri specified_formatter = options.dig(:initializationOptions, :formatter) + rubocop_has_addon = Gem::Requirement.new(">= 1.70.0").satisfied_by?(Gem::Version.new(::RuboCop::Version::STRING)) if specified_formatter @formatter = specified_formatter @@ -101,6 +102,12 @@ def apply_options(options) if specified_formatter != "auto" notifications << Notification.window_log_message("Using formatter specified by user: #{@formatter}") end + + # If the user had originally configured to use `rubocop`, but their version doesn't provide the add-on yet, + # fallback to the internal integration + if specified_formatter == "rubocop" && !rubocop_has_addon + @formatter = "rubocop_internal" + end end if @formatter == "auto" @@ -109,6 +116,23 @@ def apply_options(options) end specified_linters = options.dig(:initializationOptions, :linters) + + if specified_formatter == "rubocop" || specified_linters&.include?("rubocop") + notifications << Notification.window_log_message(<<~MESSAGE, type: Constant::MessageType::WARNING) + Formatter is configured to be `rubocop`. As of RuboCop v1.70.0, this identifier activates the add-on + implemented in the rubocop gem itself instead of the internal integration provided by the Ruby LSP. + + If you wish to use the internal integration, please configure the formatter as `rubocop_internal`. + MESSAGE + end + + # If the user had originally configured to use `rubocop`, but their version doesn't provide the add-on yet, + # fall back to the internal integration + if specified_linters&.include?("rubocop") && !rubocop_has_addon + specified_linters.delete("rubocop") + specified_linters << "rubocop_internal" + end + @linters = specified_linters || detect_linters(direct_dependencies, all_dependencies) notifications << if specified_linters @@ -185,13 +209,13 @@ def supports_watching_files sig { params(direct_dependencies: T::Array[String], all_dependencies: T::Array[String]).returns(String) } def detect_formatter(direct_dependencies, all_dependencies) # NOTE: Intentionally no $ at end, since we want to match rubocop-shopify, etc. - return "rubocop" if direct_dependencies.any?(/^rubocop/) + return "rubocop_internal" if direct_dependencies.any?(/^rubocop/) syntax_tree_is_direct_dependency = direct_dependencies.include?("syntax_tree") return "syntax_tree" if syntax_tree_is_direct_dependency rubocop_is_transitive_dependency = all_dependencies.include?("rubocop") - return "rubocop" if dot_rubocop_yml_present && rubocop_is_transitive_dependency + return "rubocop_internal" if dot_rubocop_yml_present && rubocop_is_transitive_dependency "none" end @@ -203,7 +227,7 @@ def detect_linters(dependencies, all_dependencies) linters = [] if dependencies.any?(/^rubocop/) || (all_dependencies.include?("rubocop") && dot_rubocop_yml_present) - linters << "rubocop" + linters << "rubocop_internal" end linters diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index 458dbd565..92aac17ee 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -338,7 +338,7 @@ def run_initialized unless @setup_error if defined?(Requests::Support::RuboCopFormatter) begin - @global_state.register_formatter("rubocop", Requests::Support::RuboCopFormatter.new) + @global_state.register_formatter("rubocop_internal", Requests::Support::RuboCopFormatter.new) rescue ::RuboCop::Error => e # The user may have provided unknown config switches in .rubocop or # is trying to load a non-existent config file. @@ -1040,7 +1040,7 @@ def handle_rubocop_config_change(uri) return unless defined?(Requests::Support::RuboCopFormatter) send_log_message("Reloading RuboCop since #{uri} changed") - @global_state.register_formatter("rubocop", Requests::Support::RuboCopFormatter.new) + @global_state.register_formatter("rubocop_internal", Requests::Support::RuboCopFormatter.new) # Clear all existing diagnostics since the config changed. This has to happen under a mutex because the `state` # hash cannot be mutated during iteration or that will throw an error @@ -1211,16 +1211,15 @@ def end_progress(id) sig { void } def check_formatter_is_available return if @setup_error - # Warn of an unavailable `formatter` setting, e.g. `rubocop` on a project which doesn't have RuboCop. - # Syntax Tree will always be available via Ruby LSP so we don't need to check for it. - return unless @global_state.formatter == "rubocop" + # Warn of an unavailable `formatter` setting, e.g. `rubocop_internal` on a project which doesn't have RuboCop. + return unless @global_state.formatter == "rubocop_internal" unless defined?(RubyLsp::Requests::Support::RuboCopRunner) @global_state.formatter = "none" send_message( Notification.window_show_message( - "Ruby LSP formatter is set to `rubocop` but RuboCop was not found in the Gemfile or gemspec.", + "Ruby LSP formatter is set to `rubocop_internal` but RuboCop was not found in the Gemfile or gemspec.", type: Constant::MessageType::ERROR, ), ) diff --git a/test/global_state_test.rb b/test/global_state_test.rb index b655dcbfe..f47c8586d 100644 --- a/test/global_state_test.rb +++ b/test/global_state_test.rb @@ -63,14 +63,14 @@ def test_apply_option_selects_formatter def test_applying_auto_formatter_invokes_detection state = GlobalState.new state.apply_options({ initializationOptions: { formatter: "auto" } }) - assert_equal("rubocop", state.formatter) + assert_equal("rubocop_internal", state.formatter) end def test_applying_auto_formatter_with_rubocop_extension state = GlobalState.new stub_direct_dependencies("rubocop-rails" => "1.2.3") state.apply_options({ initializationOptions: { formatter: "auto" } }) - assert_equal("rubocop", state.formatter) + assert_equal("rubocop_internal", state.formatter) end def test_applying_auto_formatter_with_rubocop_as_transitive_dependency @@ -82,7 +82,7 @@ def test_applying_auto_formatter_with_rubocop_as_transitive_dependency state.apply_options({ initializationOptions: { formatter: "auto" } }) - assert_equal("rubocop", state.formatter) + assert_equal("rubocop_internal", state.formatter) end def test_applying_auto_formatter_with_rubocop_as_transitive_dependency_without_config @@ -150,12 +150,13 @@ def test_watching_files_if_not_reported end def test_linter_specification + ::RuboCop::Version.const_set(:STRING, "1.68.0") state = GlobalState.new state.apply_options({ initializationOptions: { linters: ["rubocop", "brakeman"] }, }) - assert_equal(["rubocop", "brakeman"], state.instance_variable_get(:@linters)) + assert_equal(["brakeman", "rubocop_internal"], state.instance_variable_get(:@linters)) end def test_linter_auto_detection @@ -163,7 +164,7 @@ def test_linter_auto_detection state = GlobalState.new state.apply_options({}) - assert_equal(["rubocop"], state.instance_variable_get(:@linters)) + assert_equal(["rubocop_internal"], state.instance_variable_get(:@linters)) end def test_specifying_empty_linters @@ -185,7 +186,7 @@ def test_linter_auto_detection_with_rubocop_as_transitive_dependency state.apply_options({}) - assert_includes(state.instance_variable_get(:@linters), "rubocop") + assert_includes(state.instance_variable_get(:@linters), "rubocop_internal") end def test_type_checker_is_detected_based_on_transitive_sorbet_static @@ -249,6 +250,58 @@ def test_enabled_feature_always_returns_true_if_all_are_enabled assert(state.enabled_feature?(:whatever)) end + def test_notifies_the_user_when_using_rubocop_addon_through_linters + ::RuboCop::Version.const_set(:STRING, "1.70.0") + + state = GlobalState.new + notifications = state.apply_options({ initializationOptions: { linters: ["rubocop"] } }) + + log = notifications.find do |n| + n.method == "window/logMessage" && T.unsafe(n.params).message.include?("RuboCop v1.70.0") + end + refute_nil(log) + assert_equal(["rubocop"], state.instance_variable_get(:@linters)) + end + + def test_notifies_the_user_when_using_rubocop_addon_through_formatter + ::RuboCop::Version.const_set(:STRING, "1.70.0") + + state = GlobalState.new + notifications = state.apply_options({ initializationOptions: { formatter: "rubocop" } }) + + log = notifications.find do |n| + n.method == "window/logMessage" && T.unsafe(n.params).message.include?("RuboCop v1.70.0") + end + refute_nil(log) + assert_equal("rubocop", state.formatter) + end + + def test_falls_back_to_internal_integration_for_linters_if_rubocop_has_no_addon + ::RuboCop::Version.const_set(:STRING, "1.68.0") + + state = GlobalState.new + notifications = state.apply_options({ initializationOptions: { linters: ["rubocop"] } }) + + log = notifications.find do |n| + n.method == "window/logMessage" && T.unsafe(n.params).message.include?("RuboCop v1.70.0") + end + refute_nil(log) + assert_equal(["rubocop_internal"], state.instance_variable_get(:@linters)) + end + + def test_falls_back_to_internal_integration_for_formatters_if_rubocop_has_no_addon + ::RuboCop::Version.const_set(:STRING, "1.68.0") + + state = GlobalState.new + notifications = state.apply_options({ initializationOptions: { formatter: "rubocop" } }) + + log = notifications.find do |n| + n.method == "window/logMessage" && T.unsafe(n.params).message.include?("RuboCop v1.70.0") + end + refute_nil(log) + assert_equal("rubocop_internal", state.formatter) + end + private def stub_direct_dependencies(dependencies) diff --git a/test/requests/code_actions_formatting_test.rb b/test/requests/code_actions_formatting_test.rb index 3e3d54e1a..03e776934 100644 --- a/test/requests/code_actions_formatting_test.rb +++ b/test/requests/code_actions_formatting_test.rb @@ -70,10 +70,10 @@ def assert_fixtures_match(name, diagnostic_code, code_action_title) def assert_corrects_to_expected(diagnostic_code, code_action_title, source, expected) global_state = RubyLsp::GlobalState.new global_state.apply_options({ - initializationOptions: { linters: ["rubocop"] }, + initializationOptions: { linters: ["rubocop_internal"] }, }) global_state.register_formatter( - "rubocop", + "rubocop_internal", RubyLsp::Requests::Support::RuboCopFormatter.new, ) diff --git a/test/requests/diagnostics_expectations_test.rb b/test/requests/diagnostics_expectations_test.rb index 3a43c12c8..0c80b0708 100644 --- a/test/requests/diagnostics_expectations_test.rb +++ b/test/requests/diagnostics_expectations_test.rb @@ -10,10 +10,10 @@ class DiagnosticsExpectationsTest < ExpectationsTestRunner def run_expectations(source) result = T.let(nil, T.nilable(T::Array[RubyLsp::Interface::Diagnostic])) @global_state.apply_options({ - initializationOptions: { linters: ["rubocop"] }, + initializationOptions: { linters: ["rubocop_internal"] }, }) @global_state.register_formatter( - "rubocop", + "rubocop_internal", RubyLsp::Requests::Support::RuboCopFormatter.new, ) diff --git a/test/requests/diagnostics_test.rb b/test/requests/diagnostics_test.rb index 86dcef442..a49ed52d0 100644 --- a/test/requests/diagnostics_test.rb +++ b/test/requests/diagnostics_test.rb @@ -8,10 +8,10 @@ def setup @uri = URI("file:///fake/file.rb") @global_state = RubyLsp::GlobalState.new @global_state.apply_options({ - initializationOptions: { linters: ["rubocop"] }, + initializationOptions: { linters: ["rubocop_internal"] }, }) @global_state.register_formatter( - "rubocop", + "rubocop_internal", RubyLsp::Requests::Support::RuboCopFormatter.new, ) end @@ -52,7 +52,7 @@ def foo klass = RubyLsp::Requests::Support::RuboCopFormatter RubyLsp::Requests::Support.send(:remove_const, :RuboCopFormatter) - @global_state.instance_variable_get(:@supported_formatters).delete("rubocop") + @global_state.instance_variable_get(:@supported_formatters).delete("rubocop_internal") diagnostics = T.must(RubyLsp::Requests::Diagnostics.new(@global_state, document).perform) diff --git a/test/requests/formatting_expectations_test.rb b/test/requests/formatting_expectations_test.rb index 441e414a8..6f17bfeaf 100644 --- a/test/requests/formatting_expectations_test.rb +++ b/test/requests/formatting_expectations_test.rb @@ -8,9 +8,9 @@ class FormattingExpectationsTest < ExpectationsTestRunner expectations_tests RubyLsp::Requests::Formatting, "formatting" def run_expectations(source) - @global_state.formatter = "rubocop" + @global_state.formatter = "rubocop_internal" @global_state.register_formatter( - "rubocop", + "rubocop_internal", RubyLsp::Requests::Support::RuboCopFormatter.new, ) document = RubyLsp::RubyDocument.new( diff --git a/test/server_test.rb b/test/server_test.rb index 5151faf09..201f886d2 100644 --- a/test/server_test.rb +++ b/test/server_test.rb @@ -171,7 +171,7 @@ def test_server_info_includes_version end def test_server_info_includes_formatter - @server.global_state.expects(:formatter).twice.returns("rubocop") + @server.global_state.expects(:formatter).twice.returns("rubocop_internal") capture_subprocess_io do @server.process_message({ id: 1, @@ -185,7 +185,7 @@ def test_server_info_includes_formatter result = find_message(RubyLsp::Result, id: 1) hash = JSON.parse(result.response.to_json) - assert_equal("rubocop", hash.dig("formatter")) + assert_equal("rubocop_internal", hash.dig("formatter")) end def test_initialized_recovers_from_indexing_failures @@ -375,10 +375,10 @@ def test_handles_invalid_configuration def test_shows_error_if_formatter_set_to_rubocop_but_rubocop_not_available capture_subprocess_io do @server.process_message(id: 1, method: "initialize", params: { - initializationOptions: { formatter: "rubocop" }, + initializationOptions: { formatter: "rubocop_internal" }, }) - @server.global_state.register_formatter("rubocop", RubyLsp::Requests::Support::RuboCopFormatter.new) + @server.global_state.register_formatter("rubocop_internal", RubyLsp::Requests::Support::RuboCopFormatter.new) with_uninstalled_rubocop do @server.process_message({ method: "initialized" }) end @@ -388,7 +388,7 @@ def test_shows_error_if_formatter_set_to_rubocop_but_rubocop_not_available notification = find_message(RubyLsp::Notification, "window/showMessage") assert_equal( - "Ruby LSP formatter is set to `rubocop` but RuboCop was not found in the Gemfile or gemspec.", + "Ruby LSP formatter is set to `rubocop_internal` but RuboCop was not found in the Gemfile or gemspec.", T.cast(notification.params, RubyLsp::Interface::ShowMessageParams).message, ) end diff --git a/vscode/package.json b/vscode/package.json index 78a74e386..1d44fd9ff 100644 --- a/vscode/package.json +++ b/vscode/package.json @@ -373,6 +373,7 @@ "enum": [ "auto", "rubocop", + "rubocop_internal", "syntax_tree", "standard", "rubyfmt", @@ -381,6 +382,7 @@ "enumDescriptions": [ "Automatically detect formatter", "RuboCop", + "Ruby LSP RuboCop integration", "Syntax Tree", "Standard (supported by community addon)", "Rubyfmt (supported by community addon)", @@ -393,7 +395,7 @@ "type": "array", "examples": [ [ - "rubocop" + "rubocop_internal" ] ], "default": null