From 27c6af7f9635f3375214996fe4e4dfaa16007e3f Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 14 Nov 2024 18:19:13 -0500 Subject: [PATCH 01/27] Log when trying to format files outside of workspace (#2853) --- lib/ruby_lsp/server.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index 83eaf2a05..d0c24451b 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -604,6 +604,11 @@ def text_document_formatting(message) # don't want to format it path = uri.to_standardized_path unless path.nil? || path.start_with?(@global_state.workspace_path) + send_log_message(<<~MESSAGE) + Ignoring formatting request for file outside of the workspace. + Workspace path was set by editor as #{@global_state.workspace_path}. + File path requested for formatting was #{path} + MESSAGE send_empty_response(message[:id]) return end From 9e24cff30b825855254aeaebbce025ac104d60e4 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Fri, 15 Nov 2024 13:01:16 -0600 Subject: [PATCH 02/27] Use predicates in tests for checking visibility (#2859) --- .../test/classes_and_modules_test.rb | 4 ++-- lib/ruby_indexer/test/constant_test.rb | 16 ++++++++-------- lib/ruby_indexer/test/method_test.rb | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/ruby_indexer/test/classes_and_modules_test.rb b/lib/ruby_indexer/test/classes_and_modules_test.rb index 61351d4ac..4e3abdf20 100644 --- a/lib/ruby_indexer/test/classes_and_modules_test.rb +++ b/lib/ruby_indexer/test/classes_and_modules_test.rb @@ -302,10 +302,10 @@ class D; end RUBY b_const = @index["A::B"].first - assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) + assert_predicate(b_const, :private?) c_const = @index["A::C"].first - assert_equal(Entry::Visibility::PRIVATE, c_const.visibility) + assert_predicate(c_const, :private?) d_const = @index["A::D"].first assert_equal(Entry::Visibility::PUBLIC, d_const.visibility) diff --git a/lib/ruby_indexer/test/constant_test.rb b/lib/ruby_indexer/test/constant_test.rb index cf0f83329..30faa0aac 100644 --- a/lib/ruby_indexer/test/constant_test.rb +++ b/lib/ruby_indexer/test/constant_test.rb @@ -130,13 +130,13 @@ class A RUBY b_const = @index["A::B"].first - assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) + assert_predicate(b_const, :private?) c_const = @index["A::C"].first - assert_equal(Entry::Visibility::PRIVATE, c_const.visibility) + assert_predicate(c_const, :private?) d_const = @index["A::D"].first - assert_equal(Entry::Visibility::PUBLIC, d_const.visibility) + assert_predicate(d_const, :public?) end def test_marking_constants_as_private_reopening_namespaces @@ -163,13 +163,13 @@ module B RUBY a_const = @index["A::B::CONST_A"].first - assert_equal(Entry::Visibility::PRIVATE, a_const.visibility) + assert_predicate(a_const, :private?) b_const = @index["A::B::CONST_B"].first - assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) + assert_predicate(b_const, :private?) c_const = @index["A::B::CONST_C"].first - assert_equal(Entry::Visibility::PRIVATE, c_const.visibility) + assert_predicate(c_const, :private?) end def test_marking_constants_as_private_with_receiver @@ -187,10 +187,10 @@ module B RUBY a_const = @index["A::B::CONST_A"].first - assert_equal(Entry::Visibility::PRIVATE, a_const.visibility) + assert_predicate(a_const, :private?) b_const = @index["A::B::CONST_B"].first - assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) + assert_predicate(b_const, :private?) end def test_indexing_constant_aliases diff --git a/lib/ruby_indexer/test/method_test.rb b/lib/ruby_indexer/test/method_test.rb index 809980e18..312730188 100644 --- a/lib/ruby_indexer/test/method_test.rb +++ b/lib/ruby_indexer/test/method_test.rb @@ -141,7 +141,7 @@ def bar; end # The first entry points to the location of the module_function call assert_equal("Test", first_entry.owner.name) assert_instance_of(Entry::Module, first_entry.owner) - assert_equal(Entry::Visibility::PRIVATE, first_entry.visibility) + assert_predicate(first_entry, :private?) # The second entry points to the public singleton method assert_equal("Test::", second_entry.owner.name) assert_instance_of(Entry::SingletonClass, second_entry.owner) From bec32c05cd0ca4dc35b3d033c70ceeba349826c1 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 18 Nov 2024 02:10:36 +0000 Subject: [PATCH 03/27] Bump test/fixtures/prism from `3a34845` to `7366114` Bumps [test/fixtures/prism](https://github.com/ruby/prism) from `3a34845` to `7366114`. - [Release notes](https://github.com/ruby/prism/releases) - [Commits](https://github.com/ruby/prism/compare/3a3484570cd1920428062863efd12188ddb5837a...73661142e47ee1b00fbcb3984cb4eb2e93d38ac9) --- updated-dependencies: - dependency-name: test/fixtures/prism dependency-type: direct:production ... Signed-off-by: dependabot[bot] --- test/fixtures/prism | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/prism b/test/fixtures/prism index 3a3484570..73661142e 160000 --- a/test/fixtures/prism +++ b/test/fixtures/prism @@ -1 +1 @@ -Subproject commit 3a3484570cd1920428062863efd12188ddb5837a +Subproject commit 73661142e47ee1b00fbcb3984cb4eb2e93d38ac9 From b68d4bbe79154db59b8966e40335a2d3e2f5a4da Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 18 Nov 2024 02:37:25 +0000 Subject: [PATCH 04/27] Bump the minor-and-patch group in /vscode with 2 updates Bumps the minor-and-patch group in /vscode with 2 updates: [@eslint/eslintrc](https://github.com/eslint/eslintrc) and [@eslint/js](https://github.com/eslint/eslint/tree/HEAD/packages/js). Updates `@eslint/eslintrc` from 3.1.0 to 3.2.0 - [Release notes](https://github.com/eslint/eslintrc/releases) - [Changelog](https://github.com/eslint/eslintrc/blob/main/CHANGELOG.md) - [Commits](https://github.com/eslint/eslintrc/compare/v3.1.0...v3.2.0) Updates `@eslint/js` from 9.14.0 to 9.15.0 - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) - [Commits](https://github.com/eslint/eslint/commits/v9.15.0/packages/js) --- updated-dependencies: - dependency-name: "@eslint/eslintrc" dependency-type: direct:development update-type: version-update:semver-minor dependency-group: minor-and-patch - dependency-name: "@eslint/js" dependency-type: direct:development update-type: version-update:semver-minor dependency-group: minor-and-patch ... Signed-off-by: dependabot[bot] --- vscode/package.json | 4 ++-- vscode/yarn.lock | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/vscode/package.json b/vscode/package.json index a43474426..f7641c617 100644 --- a/vscode/package.json +++ b/vscode/package.json @@ -728,8 +728,8 @@ }, "devDependencies": { "@babel/core": "^7.26.0", - "@eslint/eslintrc": "^3.1.0", - "@eslint/js": "^9.14.0", + "@eslint/eslintrc": "^3.2.0", + "@eslint/js": "^9.15.0", "@shopify/eslint-plugin": "^46.0.0", "@shopify/prettier-config": "^1.1.2", "@types/glob": "^8.1.0", diff --git a/vscode/yarn.lock b/vscode/yarn.lock index 4207c71b4..1f108eb21 100644 --- a/vscode/yarn.lock +++ b/vscode/yarn.lock @@ -437,10 +437,10 @@ minimatch "^3.1.2" strip-json-comments "^3.1.1" -"@eslint/eslintrc@^3.1.0": - version "3.1.0" - resolved "https://registry.yarnpkg.com/@eslint/eslintrc/-/eslintrc-3.1.0.tgz#dbd3482bfd91efa663cbe7aa1f506839868207b6" - integrity sha512-4Bfj15dVJdoy3RfZmmo86RK1Fwzn6SstsvK9JS+BaVKqC6QQQQyXekNaC+g+LKNgkQ+2VhGAzm6hO40AhMR3zQ== +"@eslint/eslintrc@^3.2.0": + version "3.2.0" + resolved "https://registry.yarnpkg.com/@eslint/eslintrc/-/eslintrc-3.2.0.tgz#57470ac4e2e283a6bf76044d63281196e370542c" + integrity sha512-grOjVNN8P3hjJn/eIETF1wwd12DdnwFDoyceUJLYYdkpbwq3nLi+4fqrTAONx7XDALqlL220wC/RHSC/QTI/0w== dependencies: ajv "^6.12.4" debug "^4.3.2" @@ -457,10 +457,10 @@ resolved "https://registry.yarnpkg.com/@eslint/js/-/js-8.57.0.tgz#a5417ae8427873f1dd08b70b3574b453e67b5f7f" integrity sha512-Ys+3g2TaW7gADOJzPt83SJtCDhMjndcDMFVQ/Tj9iA1BfJzFKD9mAUXT3OenpuPHbI6P/myECxRJrofUsDx/5g== -"@eslint/js@^9.14.0": - version "9.14.0" - resolved "https://registry.yarnpkg.com/@eslint/js/-/js-9.14.0.tgz#2347a871042ebd11a00fd8c2d3d56a265ee6857e" - integrity sha512-pFoEtFWCPyDOl+C6Ift+wC7Ro89otjigCf5vcuWqWgqNSQbRrpjSvdeE6ofLz4dHmyxD5f7gIdGT4+p36L6Twg== +"@eslint/js@^9.15.0": + version "9.15.0" + resolved "https://registry.yarnpkg.com/@eslint/js/-/js-9.15.0.tgz#df0e24fe869143b59731942128c19938fdbadfb5" + integrity sha512-tMTqrY+EzbXmKJR5ToI8lxu7jaN5EdmrBFJpQk5JmSlyLsx6o4t27r883K5xsLuCYCpfKBCGswMSWXsM+jB7lg== "@humanwhocodes/config-array@^0.11.14": version "0.11.14" From 357202f8c3cf4809be7282002a2cdea66cdab8b0 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 18 Nov 2024 03:02:12 +0000 Subject: [PATCH 05/27] Bump cross-spawn from 7.0.3 to 7.0.5 in /vscode Bumps [cross-spawn](https://github.com/moxystudio/node-cross-spawn) from 7.0.3 to 7.0.5. - [Changelog](https://github.com/moxystudio/node-cross-spawn/blob/master/CHANGELOG.md) - [Commits](https://github.com/moxystudio/node-cross-spawn/compare/v7.0.3...v7.0.5) --- updated-dependencies: - dependency-name: cross-spawn dependency-type: indirect ... Signed-off-by: dependabot[bot] --- vscode/yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/vscode/yarn.lock b/vscode/yarn.lock index 1f108eb21..56ae61817 100644 --- a/vscode/yarn.lock +++ b/vscode/yarn.lock @@ -1465,9 +1465,9 @@ core-util-is@~1.0.0: node-fetch "^2.6.12" cross-spawn@^7.0.0, cross-spawn@^7.0.2: - version "7.0.3" - resolved "https://registry.yarnpkg.com/cross-spawn/-/cross-spawn-7.0.3.tgz#f73a85b9d5d41d045551c177e2882d4ac85728a6" - integrity sha512-iRDPJKUPVEND7dHPO8rkbOnPpyDygcDFtWjpeWNCgy8WP2rXcxXL8TskReQl6OrB2G7+UJrags1q15Fudc7G6w== + version "7.0.5" + resolved "https://registry.yarnpkg.com/cross-spawn/-/cross-spawn-7.0.5.tgz#910aac880ff5243da96b728bc6521a5f6c2f2f82" + integrity sha512-ZVJrKKYunU38/76t0RMOulHOnUcbU9GbpWKAOZ0mhjr7CX6FVrH+4FrAapSOekrgFQ3f/8gwMEuIft0aKq6Hug== dependencies: path-key "^3.1.0" shebang-command "^2.0.0" From 9926887f3bccf2175923c5e88825b9db6142f1e3 Mon Sep 17 00:00:00 2001 From: Koji NAKAMURA Date: Tue, 19 Nov 2024 00:08:56 +0900 Subject: [PATCH 06/27] Update sample codes on Add-ons page to fix execution errors (#2869) --- jekyll/add-ons.markdown | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/jekyll/add-ons.markdown b/jekyll/add-ons.markdown index c9f7dd996..d4451cc4d 100644 --- a/jekyll/add-ons.markdown +++ b/jekyll/add-ons.markdown @@ -204,10 +204,10 @@ module RubyLsp "0.1.0" end - def create_hover_listener(response_builder, node_context, index, dispatcher) + def create_hover_listener(response_builder, node_context, dispatcher) # Use the listener factory methods to instantiate listeners with parameters sent by the LSP combined with any # pre-computed information in the add-on. These factory methods are invoked on every request - Hover.new(client, response_builder, @config, dispatcher) + Hover.new(response_builder, @config, dispatcher) end end @@ -222,10 +222,7 @@ module RubyLsp # to this object, which will then build the Ruby LSP's response. # Additionally, listeners are instantiated with a message_queue to push notifications (not used in this example). # See "Sending notifications to the client" for more information. - def initialize(client, response_builder, config, dispatcher) - super(dispatcher) - - @client = client + def initialize(response_builder, config, dispatcher) @response_builder = response_builder @config = config @@ -293,18 +290,17 @@ class MyIndexingEnhancement < RubyIndexer::Enhancement # Create the array of signatures that this method will accept. Every signatures is composed of a list of # parameters. The parameter classes represent each type of parameter signatures = [ - Entry::Signature.new([Entry::RequiredParameter.new(name: :a)]) + RubyIndexer::Entry::Signature.new([RubyIndexer::Entry::RequiredParameter.new(name: :a)]) ] - new_entry = Entry::Method.new( + new_entry = RubyIndexer::Entry::Method.new( "new_method", # The name of the method that gets created via meta-programming file_path, # The file_path where the DSL call was found. This should always just be the file_path received location, # The Prism node location where the DSL call was found location, # The Prism node location for the DSL name location. May or not be the same nil, # The documentation for this DSL call. This should always be `nil` to ensure lazy fetching of docs - @index.configuration.encoding, # The negotiated encoding. This should always be `indexing.configuration.encoding` signatures, # All signatures for this method (every way it can be invoked) - Entry::Visibility::PUBLIC, # The method's visibility + RubyIndexer::Entry::Visibility::PUBLIC, # The method's visibility owner, # The method's owner. This is almost always going to be the same owner received ) @@ -326,7 +322,7 @@ module RubyLsp class Addon < ::RubyLsp::Addon def activate(global_state, message_queue) # Register the enhancement as part of the indexing process - @index.register_enhancement(MyIndexingEnhancement.new(@index)) + global_state.index.register_enhancement(MyIndexingEnhancement.new(global_state.index)) end def deactivate From 50daa5369ddb1d5fa015361f477072a084b49462 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Mon, 18 Nov 2024 11:04:28 -0500 Subject: [PATCH 07/27] Check target precision for call node in hover and definition (#2870) ### Motivation Closes #2846 We had a regression in our call node target precisions. Essentially, the call node covers both the receiver and the message, but we only want to make the call node the target is the position covers the message location exactly. ### Implementation Added another branch to the checks for whether the position is covered or not. ### Automated Tests Added tests to ensure we don't regress. --- lib/ruby_lsp/requests/definition.rb | 2 ++ lib/ruby_lsp/requests/hover.rb | 2 ++ test/requests/definition_expectations_test.rb | 34 +++++++++++++++++++ test/requests/hover_expectations_test.rb | 34 +++++++++++++++++++ 4 files changed, 72 insertions(+) diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb index 8305f0ec6..e4817e3db 100644 --- a/lib/ruby_lsp/requests/definition.rb +++ b/lib/ruby_lsp/requests/definition.rb @@ -118,6 +118,8 @@ def position_outside_target?(position, target) Prism::InstanceVariableWriteNode !covers_position?(target.name_loc, position) + when Prism::CallNode + !covers_position?(target.message_loc, position) else false end diff --git a/lib/ruby_lsp/requests/hover.rb b/lib/ruby_lsp/requests/hover.rb index 06fe522f9..55c5ea8ba 100644 --- a/lib/ruby_lsp/requests/hover.rb +++ b/lib/ruby_lsp/requests/hover.rb @@ -103,6 +103,8 @@ def position_outside_target?(position, target) Prism::GlobalVariableOrWriteNode, Prism::GlobalVariableWriteNode !covers_position?(target.name_loc, position) + when Prism::CallNode + !covers_position?(target.message_loc, position) else false end diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb index b74090c7b..d24d178c5 100644 --- a/test/requests/definition_expectations_test.rb +++ b/test/requests/definition_expectations_test.rb @@ -1097,6 +1097,40 @@ def baz end end + def test_definition_call_node_precision + source = <<~RUBY + class Foo + def message + "hello!" + end + end + + class Bar + def with_foo(foo) + @foo_message = foo.message + end + end + RUBY + + with_server(source) do |server, uri| + # On the `foo` receiver, we should not show any results + server.process_message( + id: 1, + method: "textDocument/definition", + params: { textDocument: { uri: uri }, position: { character: 19, line: 8 } }, + ) + assert_empty(server.pop_response.response) + + # On `message`, we should + server.process_message( + id: 2, + method: "textDocument/definition", + params: { textDocument: { uri: uri }, position: { character: 23, line: 8 } }, + ) + refute_empty(server.pop_response.response) + end + end + private def create_definition_addon diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index 6b08c8d24..b43ae8709 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -820,6 +820,40 @@ def foo end end + def test_hover_call_node_precision + source = <<~RUBY + class Foo + def message + "hello!" + end + end + + class Bar + def with_foo(foo) + @foo_message = foo.message + end + end + RUBY + + with_server(source) do |server, uri| + # On the `foo` receiver, we should not show any results + server.process_message( + id: 1, + method: "textDocument/hover", + params: { textDocument: { uri: uri }, position: { character: 19, line: 8 } }, + ) + assert_nil(server.pop_response.response) + + # On `message`, we should + server.process_message( + id: 2, + method: "textDocument/hover", + params: { textDocument: { uri: uri }, position: { character: 23, line: 8 } }, + ) + refute_nil(server.pop_response.response) + end + end + private def create_hover_addon From 150f83451e65bc3e3da9fe82bad69f646eed96aa Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Nov 2024 11:11:14 -0500 Subject: [PATCH 08/27] Prevent full indexing (`index_all`) from being called multiple times (#2864) Prevent `index_all` being called multiple times --- lib/ruby_indexer/lib/ruby_indexer/index.rb | 10 ++++++++++ lib/ruby_indexer/test/index_test.rb | 7 +++++++ 2 files changed, 17 insertions(+) diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb index 7effc5b77..ee09c773b 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/index.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/index.rb @@ -7,6 +7,7 @@ class Index class UnresolvableAliasError < StandardError; end class NonExistingNamespaceError < StandardError; end + class IndexNotEmptyError < StandardError; end # The minimum Jaro-Winkler similarity score for an entry to be considered a match for a given fuzzy search query ENTRY_SIMILARITY_THRESHOLD = 0.7 @@ -360,6 +361,15 @@ def resolve(name, nesting, seen_names = []) ).void end def index_all(indexable_paths: @configuration.indexables, &block) + # When troubleshooting an indexing issue, e.g. through irb, it's not obvious that `index_all` will augment the + # existing index values, meaning it may contain 'stale' entries. This check ensures that the user is aware of this + # behavior and can take appropriate action. + # binding.break + if @entries.any? + raise IndexNotEmptyError, + "The index is not empty. To prevent invalid entries, `index_all` can only be called once." + end + RBSIndexer.new(self).index_ruby_core # Calculate how many paths are worth 1% of progress progress_step = (indexable_paths.length / 100.0).ceil diff --git a/lib/ruby_indexer/test/index_test.rb b/lib/ruby_indexer/test/index_test.rb index c83d3b0e6..2723edc0a 100644 --- a/lib/ruby_indexer/test/index_test.rb +++ b/lib/ruby_indexer/test/index_test.rb @@ -2023,5 +2023,12 @@ def test_build_non_redundant_name ), ) end + + def test_prevents_multiple_calls_to_index_all + # For this test class, `index_all` is already called once in `setup`. + assert_raises(Index::IndexNotEmptyError) do + @index.index_all + end + end end end From 592495eace9286f6845d2dbafc003d399e689000 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Nov 2024 15:14:08 -0500 Subject: [PATCH 09/27] Update docs and code for remaining references to 'custom bundle' (#2732) Rename custom bundle to composed bundle --- exe/ruby-lsp | 2 +- jekyll/design-and-roadmap.markdown | 1 - jekyll/troubleshooting.markdown | 14 +++-------- lib/ruby_lsp/setup_bundler.rb | 40 +++++++++++++++--------------- test/setup_bundler_test.rb | 36 +++++++++++++-------------- vscode/README.md | 4 +-- vscode/package.json | 2 +- vscode/src/client.ts | 2 +- 8 files changed, 46 insertions(+), 55 deletions(-) diff --git a/exe/ruby-lsp b/exe/ruby-lsp index 431a9024b..31e1acf69 100755 --- a/exe/ruby-lsp +++ b/exe/ruby-lsp @@ -54,7 +54,7 @@ rescue OptionParser::InvalidOption => e exit(1) end -# When we're running without bundler, then we need to make sure the custom bundle is fully configured and re-execute +# When we're running without bundler, then we need to make sure the composed bundle is fully configured and re-execute # using `BUNDLE_GEMFILE=.ruby-lsp/Gemfile bundle exec ruby-lsp` so that we have access to the gems that are a part of # the application's bundle if ENV["BUNDLE_GEMFILE"].nil? diff --git a/jekyll/design-and-roadmap.markdown b/jekyll/design-and-roadmap.markdown index 1e27c1b07..90b38d0f5 100644 --- a/jekyll/design-and-roadmap.markdown +++ b/jekyll/design-and-roadmap.markdown @@ -201,7 +201,6 @@ Interested in contributing? Check out the issues tagged with [help-wanted] or [g - [Add rename support] - [Add show type hierarchy support] - [Show index view on the VS Code extension allowing users to browse indexed gems] -- Remove custom bundle in favor of using bundler-compose - [Add more refactoring code actions such as extract to method, extract to class/module, etc] - [Explore speeding up indexing by caching the index for gems] - Explore speeding up indexing by making Prism AST allocations lazy diff --git a/jekyll/troubleshooting.markdown b/jekyll/troubleshooting.markdown index b6c07a5cd..0c3d8539b 100644 --- a/jekyll/troubleshooting.markdown +++ b/jekyll/troubleshooting.markdown @@ -29,16 +29,8 @@ As an example, the activation script for `zsh` using `rbenv` as a version manage ``` After activating the Ruby version, we then proceed to boot the server gem (`ruby-lsp`). To avoid having users include -the `ruby-lsp` in their `Gemfile`, we currently create a custom bundle under the `.ruby-lsp` directory inside your -project. That directory contains another `Gemfile`, that includes the `ruby-lsp` gem in addition to your project's -dependencies. This approach allows us to automatically detect which formatter your project uses and which gems we need -to index for features such as go to definition. - -{: .note } -We are working with the RubyGems/Bundler team to have this type of mechanism properly supported from within -Bundler itself, which is currently being experimented with in a plugin called `bundler-compose`. Once -> `bundler-compose`is production ready, the entire custom bundle created under the `.ruby-lsp` directory will go away -> and we'll rely on Bundler to compose the LOAD_PATH including the `ruby-lsp` gem. +the `ruby-lsp` in their `Gemfile`, we create a [composed +bundle](https://shopify.github.io/ruby-lsp/composed-bundle.html) under the `.ruby-lsp` directory inside your project. ## Common issues @@ -88,7 +80,7 @@ More context about this issue on https://github.com/Shopify/vscode-ruby-lsp/issu ### Bundler issues -If the extension successfully activated the Ruby environment, it may still fail when trying to compose the custom bundle +If the extension successfully activated the Ruby environment, it may still fail when trying to compose the composed bundle to run the server gem. This could be a regular Bundler issue, like not being able to satisfy dependencies due to a conflicting version requirement, or it could be a configuration issue. diff --git a/lib/ruby_lsp/setup_bundler.rb b/lib/ruby_lsp/setup_bundler.rb index 769dfe757..ac425144a 100644 --- a/lib/ruby_lsp/setup_bundler.rb +++ b/lib/ruby_lsp/setup_bundler.rb @@ -12,7 +12,7 @@ require "time" require "uri" -# This file is a script that will configure a custom bundle for the Ruby LSP. The custom bundle allows developers to use +# This file is a script that will configure a composed bundle for the Ruby LSP. The composed bundle allows developers to use # the Ruby LSP without including the gem in their application's Gemfile while at the same time giving us access to the # exact locked versions of dependencies. @@ -62,7 +62,7 @@ def initialize(project_path, **options) @retry = T.let(false, T::Boolean) end - # Sets up the custom bundle and returns the `BUNDLE_GEMFILE`, `BUNDLE_PATH` and `BUNDLE_APP_CONFIG` that should be + # Sets up the composed bundle and returns the `BUNDLE_GEMFILE`, `BUNDLE_PATH` and `BUNDLE_APP_CONFIG` that should be # used for running the server sig { returns(T::Hash[String, String]) } def setup! @@ -73,12 +73,12 @@ def setup! ignore_file = @custom_dir + ".gitignore" ignore_file.write("*") unless ignore_file.exist? - # Do not set up a custom bundle if LSP dependencies are already in the Gemfile + # Do not set up a composed bundle if LSP dependencies are already in the Gemfile if @dependencies["ruby-lsp"] && @dependencies["debug"] && (@rails_app ? @dependencies["ruby-lsp-rails"] : true) $stderr.puts( - "Ruby LSP> Skipping custom bundle setup since LSP dependencies are already in #{@gemfile}", + "Ruby LSP> Skipping composed bundle setup since LSP dependencies are already in #{@gemfile}", ) return run_bundle_install @@ -96,7 +96,7 @@ def setup! if @custom_lockfile.exist? && @lockfile_hash_path.exist? && @lockfile_hash_path.read == current_lockfile_hash $stderr.puts( - "Ruby LSP> Skipping custom bundle setup since #{@custom_lockfile} already exists and is up to date", + "Ruby LSP> Skipping composed bundle setup since #{@custom_lockfile} already exists and is up to date", ) return run_bundle_install(@custom_gemfile) end @@ -110,8 +110,8 @@ def setup! private sig { returns(T::Hash[String, T.untyped]) } - def custom_bundle_dependencies - @custom_bundle_dependencies ||= T.let( + def composed_bundle_dependencies + @composed_bundle_dependencies ||= T.let( begin original_bundle_gemfile = ENV["BUNDLE_GEMFILE"] @@ -136,8 +136,8 @@ def write_custom_gemfile "", ] - # If there's a top level Gemfile, we want to evaluate from the custom bundle. We get the source from the top level - # Gemfile, so if there isn't one we need to add a default source + # If there's a top level Gemfile, we want to evaluate from the composed bundle. We get the source from the top + # level Gemfile, so if there isn't one we need to add a default source if @gemfile&.exist? && @lockfile&.exist? parts << "eval_gemfile(File.expand_path(\"../#{@gemfile_name}\", __dir__))" else @@ -187,7 +187,7 @@ def run_bundle_install(bundle_gemfile = @gemfile) env = bundler_settings_as_env env["BUNDLE_GEMFILE"] = bundle_gemfile.to_s - # If the user has a custom bundle path configured, we need to ensure that we will use the absolute and not + # If the user has a composed bundle path configured, we need to ensure that we will use the absolute and not # relative version of it when running `bundle install`. This is necessary to avoid installing the gems under the # `.ruby-lsp` folder, which is not the user's intention. For example, if the path is configured as `vendor`, we # want to install it in the top level `vendor` and not `.ruby-lsp/vendor` @@ -244,7 +244,7 @@ def run_bundle_install_through_command(env) base_bundle = base_bundle_command(env) # If `ruby-lsp` and `debug` (and potentially `ruby-lsp-rails`) are already in the Gemfile, then we shouldn't try - # to upgrade them or else we'll produce undesired source control changes. If the custom bundle was just created + # to upgrade them or else we'll produce undesired source control changes. If the composed bundle was just created # and any of `ruby-lsp`, `ruby-lsp-rails` or `debug` weren't a part of the Gemfile, then we need to run `bundle # install` for the first time to generate the Gemfile.lock with them included or else Bundler will complain that # they're missing. We can only update if the custom `.ruby-lsp/Gemfile.lock` already exists and includes all gems @@ -274,16 +274,16 @@ def run_bundle_install_through_command(env) command << "1>&2" # Add bundle update - $stderr.puts("Ruby LSP> Running bundle install for the custom bundle. This may take a while...") + $stderr.puts("Ruby LSP> Running bundle install for the composed bundle. This may take a while...") $stderr.puts("Ruby LSP> Command: #{command}") - # Try to run the bundle install or update command. If that fails, it normally means that the custom lockfile is in - # a bad state that no longer reflects the top level one. In that case, we can remove the whole directory, try + # Try to run the bundle install or update command. If that fails, it normally means that the composed lockfile is + # in a bad state that no longer reflects the top level one. In that case, we can remove the whole directory, try # another time and give up if it fails again if !system(env, command) && !@retry && @custom_gemfile.exist? @retry = true @custom_dir.rmtree - $stderr.puts("Ruby LSP> Running bundle install failed. Trying to re-generate the custom bundle from scratch") + $stderr.puts("Ruby LSP> Running bundle install failed. Trying to re-generate the composed bundle from scratch") return setup! end @@ -330,14 +330,14 @@ def should_bundle_update? if @rails_app return false if @dependencies.values_at("ruby-lsp", "ruby-lsp-rails", "debug").all? - # If the custom lockfile doesn't include `ruby-lsp`, `ruby-lsp-rails` or `debug`, we need to run bundle install - # before updating - return false if custom_bundle_dependencies.values_at("ruby-lsp", "debug", "ruby-lsp-rails").any?(&:nil?) + # If the composed lockfile doesn't include `ruby-lsp`, `ruby-lsp-rails` or `debug`, we need to run bundle + # install before updating + return false if composed_bundle_dependencies.values_at("ruby-lsp", "debug", "ruby-lsp-rails").any?(&:nil?) else return false if @dependencies.values_at("ruby-lsp", "debug").all? - # If the custom lockfile doesn't include `ruby-lsp` or `debug`, we need to run bundle install before updating - return false if custom_bundle_dependencies.values_at("ruby-lsp", "debug").any?(&:nil?) + # If the composed lockfile doesn't include `ruby-lsp` or `debug`, we need to run bundle install before updating + return false if composed_bundle_dependencies.values_at("ruby-lsp", "debug").any?(&:nil?) end # If the last updated file doesn't exist or was updated more than 4 hours ago, we should update diff --git a/test/setup_bundler_test.rb b/test/setup_bundler_test.rb index a9a3793dc..58315ee43 100644 --- a/test/setup_bundler_test.rb +++ b/test/setup_bundler_test.rb @@ -24,7 +24,7 @@ def test_does_not_create_composed_gemfile_if_all_gems_are_in_the_bundle_for_rail refute_path_exists(".ruby-lsp/Gemfile") end - def test_creates_custom_bundle + def test_creates_composed_bundle stub_bundle_with_env(bundle_env(Dir.pwd, ".ruby-lsp/Gemfile")) Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}).at_least_once run_script @@ -40,7 +40,7 @@ def test_creates_custom_bundle FileUtils.rm_r(".ruby-lsp") if Dir.exist?(".ruby-lsp") end - def test_creates_custom_bundle_for_a_rails_app + def test_creates_composed_bundle_for_a_rails_app stub_bundle_with_env(bundle_env(Dir.pwd, ".ruby-lsp/Gemfile")) FileUtils.mkdir("config") FileUtils.cp("test/fixtures/rails_application.rb", "config/application.rb") @@ -59,7 +59,7 @@ def test_creates_custom_bundle_for_a_rails_app FileUtils.rm_r("config") if Dir.exist?("config") end - def test_changing_lockfile_causes_custom_bundle_to_be_rebuilt + def test_changing_lockfile_causes_composed_bundle_to_be_rebuilt Dir.mktmpdir do |dir| Dir.chdir(dir) do File.write(File.join(dir, "Gemfile"), <<~GEMFILE) @@ -72,7 +72,7 @@ def test_changing_lockfile_causes_custom_bundle_to_be_rebuilt # Run bundle install to generate the lockfile system("bundle install") - # Run the script once to generate a custom bundle + # Run the script once to generate a composed bundle run_script(dir) end end @@ -92,11 +92,11 @@ def test_changing_lockfile_causes_custom_bundle_to_be_rebuilt end end - # At this point, the custom bundle includes the `ruby-lsp` in its lockfile, but that will be overwritten when we - # copy the top level lockfile. If custom bundle dependencies are eagerly evaluated, then we would think the - # ruby-lsp is a part of the custom lockfile and would try to run `bundle update ruby-lsp`, which would fail. If - # we evaluate lazily, then we only find dependencies after the lockfile was copied, and then run bundle install - # instead, which re-locks and adds the ruby-lsp + # At this point, the composed bundle includes the `ruby-lsp` in its lockfile, but that will be overwritten when + # we copy the top level lockfile. If composed bundle dependencies are eagerly evaluated, then we would think the + # ruby-lsp is a part of the composed lockfile and would try to run `bundle update ruby-lsp`, which would fail. + # If we evaluate lazily, then we only find dependencies after the lockfile was copied, and then run bundle + # install instead, which re-locks and adds the ruby-lsp Bundler.with_unbundled_env do stub_bundle_with_env(bundle_env(dir, ".ruby-lsp/Gemfile")) run_script(dir) @@ -118,7 +118,7 @@ def test_does_not_copy_gemfile_lock_when_not_modified # Run bundle install to generate the lockfile system("bundle install") - # Run the script once to generate a custom bundle + # Run the script once to generate a composed bundle run_script(dir) end end @@ -155,7 +155,7 @@ def test_does_only_updates_every_4_hours # Run bundle install to generate the lockfile system("bundle install") - # Run the script once to generate a custom bundle + # Run the script once to generate a composed bundle run_script(dir) end end @@ -186,7 +186,7 @@ def test_uses_absolute_bundle_path_for_bundle_install Bundler.settings.set_global(:path, original) end - def test_creates_custom_bundle_if_no_gemfile + def test_creates_composed_bundle_if_no_gemfile # Create a temporary directory with no Gemfile or Gemfile.lock Dir.mktmpdir do |dir| Dir.chdir(dir) do @@ -263,7 +263,7 @@ def test_does_not_create_composed_gemfile_if_both_ruby_lsp_and_debug_are_gemspec end end - def test_creates_custom_bundle_with_specified_branch + def test_creates_composed_bundle_with_specified_branch Dir.mktmpdir do |dir| Dir.chdir(dir) do bundle_gemfile = Pathname.new(".ruby-lsp").expand_path(Dir.pwd) + "Gemfile" @@ -295,7 +295,7 @@ def test_returns_bundle_app_config_if_there_is_local_config end end - def test_custom_bundle_uses_alternative_gemfiles + def test_composed_bundle_uses_alternative_gemfiles Dir.mktmpdir do |dir| Dir.chdir(dir) do File.write(File.join(dir, "gems.rb"), <<~GEMFILE) @@ -323,8 +323,8 @@ def test_custom_bundle_uses_alternative_gemfiles def test_ensures_lockfile_remotes_are_relative_to_default_gemfile Dir.mktmpdir do |dir| Dir.chdir(dir) do - # The structure used in Rails uncovered a bug in our custom bundle logic. Rails is an empty gem with a bunch of - # nested gems. The lockfile includes remotes that use relative paths and we need to adjust those when we copy + # The structure used in Rails uncovered a bug in our composed bundle logic. Rails is an empty gem with a bunch + # of nested gems. The lockfile includes remotes that use relative paths and we need to adjust those when we copy # the lockfile File.write(File.join(dir, "Gemfile"), <<~GEMFILE) @@ -534,7 +534,7 @@ def test_recovers_from_stale_lockfiles # Write the lockfile hash based on the valid file File.write(File.join(custom_dir, "main_lockfile_hash"), Digest::SHA256.hexdigest(lockfile_contents)) - # Write the custom bundle's lockfile using a fake version that doesn't exist to force bundle install to fail + # Write the composed bundle's lockfile using a fake version that doesn't exist to force bundle install to fail File.write(File.join(custom_dir, "Gemfile"), <<~GEMFILE) source "https://rubygems.org" gem "stringio" @@ -560,7 +560,7 @@ def test_recovers_from_stale_lockfiles run_script(dir) end - # Verify that the script recovered and re-generated the custom bundle from scratch + # Verify that the script recovered and re-generated the composed bundle from scratch assert_path_exists(".ruby-lsp/Gemfile") assert_path_exists(".ruby-lsp/Gemfile.lock") refute_match("999.1.555", File.read(".ruby-lsp/Gemfile.lock")) diff --git a/vscode/README.md b/vscode/README.md index e6f6dff3c..2b8ecd297 100644 --- a/vscode/README.md +++ b/vscode/README.md @@ -56,7 +56,7 @@ If you have feedback about this feature, you can let us know in the [DX Slack](h Search for `Shopify.ruby-lsp` in the extensions tab and click install. -By default, the Ruby LSP will generate a `.ruby-lsp` directory with a custom bundle that includes the server gem. +By default, the Ruby LSP will generate a `.ruby-lsp` directory with a composed bundle that includes the server gem. Additionally, it will attempt to use available version managers to select the correct Ruby version for any given project. Refer to configuration for more options. @@ -158,7 +158,7 @@ separate `Gemfile` for development tools. **Note**: when using this, gems will not be installed automatically and neither will `ruby-lsp` upgrades. -Create a directory to store the custom bundle outside of the project that uses the old Ruby version. Inside that +Create a directory to store the composed bundle outside of the project that uses the old Ruby version. Inside that directory, add your preferred version manager configuration to select a supported Ruby version. For example, if using `chruby`, it would look like this: diff --git a/vscode/package.json b/vscode/package.json index f7641c617..e8db5e5dd 100644 --- a/vscode/package.json +++ b/vscode/package.json @@ -415,7 +415,7 @@ "default": "both" }, "rubyLsp.useBundlerCompose": { - "description": "This is a temporary setting for testing purposes, do not use it! Replace the custom bundle logic by bundler-compose.", + "description": "This is a temporary setting for testing purposes, do not use it! Replace the composed bundle logic by bundler-compose.", "type": "boolean", "default": false }, diff --git a/vscode/src/client.ts b/vscode/src/client.ts index 49263d415..3313b9803 100644 --- a/vscode/src/client.ts +++ b/vscode/src/client.ts @@ -83,7 +83,7 @@ function getLspExecutables( }; // If there's a user defined custom bundle, we run the LSP with `bundle exec` and just trust the user configured - // their bundle. Otherwise, we run the global install of the LSP and use our custom bundle logic in the server + // their bundle. Otherwise, we run the global install of the LSP and use our composed bundle logic in the server if (customBundleGemfile.length > 0) { run = { command: "bundle", From adf28bd13739054dce706a56678f99581871905f Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Nov 2024 15:32:55 -0500 Subject: [PATCH 10/27] Add Sorbet troubleshooting note (#2871) --- jekyll/troubleshooting.markdown | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/jekyll/troubleshooting.markdown b/jekyll/troubleshooting.markdown index 0c3d8539b..c86cb23dc 100644 --- a/jekyll/troubleshooting.markdown +++ b/jekyll/troubleshooting.markdown @@ -119,6 +119,14 @@ that prevents it from responding to new requests coming from the editor. If you report [here](https://github.com/Shopify/ruby-lsp/issues/new?labels=bug&template=bug_template.yml) including the steps that led to the server getting stuck. +### Missing Features + +If you find that some features are working (such as formatting), but others aren't (such as go to definition), +and are working on a codebase that uses Sorbet, then this may indicate the +[Sorbet LSP isn't running](https://sorbet.org/docs/lsp#instructions-for-specific-language-clients). +To avoid duplicate/conflicting behavior, Ruby LSP disables some features when a Sorbet codebase is detected, with the +intention that Sorbet can provide better accuracy. + ### Gem installation locations and permissions To launch the Ruby LSP server, the `ruby-lsp` gem must be installed. And in order to automatically index your project's From 670defb2650c07df9a47d0ad424b746177115d0c Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Mon, 18 Nov 2024 15:58:06 -0500 Subject: [PATCH 11/27] Prevent Shadowenv from mutating BUNDLE_GEMFILE (#2874) ### Motivation I finally understood the issue we were seeing of some folks requiring completely incompatible versions of add-ons. If they used Shadowenv to override BUNDLE_GEMFILE, then it would cause the LSP to skip composing the bundle, which then leads to finding add-ons with `Gem.find_files` without having setup the bundle first. That results in finding all `addon.rb` files defined by every installed gem, which might be ordered alphabetically and then results in requiring an old and incompatible Rails add-on version. ### Implementation We cannot let Shadowenv override `BUNDLE_GEMFILE` because that fully messes up our entire composed bundle setup. I started deleting that key from the activated environment. --- vscode/src/ruby/shadowenv.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vscode/src/ruby/shadowenv.ts b/vscode/src/ruby/shadowenv.ts index 0f03cfcf0..06caff075 100644 --- a/vscode/src/ruby/shadowenv.ts +++ b/vscode/src/ruby/shadowenv.ts @@ -32,6 +32,10 @@ export class Shadowenv extends VersionManager { `${shadowenvExec} exec -- ruby`, ); + // Do not let Shadowenv change the BUNDLE_GEMFILE. The server has to be able to control this in order to properly + // set up the environment + delete parsedResult.env.BUNDLE_GEMFILE; + return { env: { ...process.env, ...parsedResult.env }, yjit: parsedResult.yjit, From baf6599de650f6092ea23b22e012b2cf66a66da7 Mon Sep 17 00:00:00 2001 From: William Fan Date: Mon, 18 Nov 2024 17:21:25 -0500 Subject: [PATCH 12/27] Fix documentation reference when methods are too close (#2806) * delete line key from @comments_by_line when comment has been visited * fix 'test_completion_resolve_with_owner_present' test * add source_lines and refactor collect_comments * minimize the changes --- .../lib/ruby_indexer/declaration_listener.rb | 9 +++-- lib/ruby_indexer/test/method_test.rb | 33 +++++++++++++++++++ test/requests/completion_resolve_test.rb | 2 +- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb index b29bdd7db..2815ec1a8 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb @@ -37,6 +37,7 @@ def initialize(index, dispatcher, parse_result, file_path, collect_comments: fal parse_result.code_units_cache(@index.configuration.encoding), T.any(T.proc.params(arg0: Integer).returns(Integer), Prism::CodeUnitsCache), ) + @source_lines = T.let(parse_result.source.lines, T::Array[String]) # The nesting stack we're currently inside. Used to determine the fully qualified name of constants, but only # stored by unresolved aliases which need the original nesting to be lazily resolved @@ -661,8 +662,7 @@ def collect_comments(node) comments = +"" start_line = node.location.start_line - 1 - start_line -= 1 unless @comments_by_line.key?(start_line) - + start_line -= 1 unless comment_exists_at?(start_line) start_line.downto(1) do |line| comment = @comments_by_line[line] break unless comment @@ -683,6 +683,11 @@ def collect_comments(node) comments end + sig { params(line: Integer).returns(T::Boolean) } + def comment_exists_at?(line) + @comments_by_line.key?(line) || !@source_lines[line - 1].to_s.strip.empty? + end + sig { params(name: String).returns(String) } def fully_qualify_name(name) if @stack.empty? || name.start_with?("::") diff --git a/lib/ruby_indexer/test/method_test.rb b/lib/ruby_indexer/test/method_test.rb index 312730188..8e45a54b5 100644 --- a/lib/ruby_indexer/test/method_test.rb +++ b/lib/ruby_indexer/test/method_test.rb @@ -149,6 +149,39 @@ def bar; end end end + def test_comments_documentation + index(<<~RUBY) + # Documentation for Foo + + class Foo + # #################### + # Documentation for bar + # #################### + # + def bar + end + + # test + + # Documentation for baz + def baz; end + def ban; end + end + RUBY + + foo_comment = @index["Foo"].first.comments + assert_equal("Documentation for Foo", foo_comment) + + bar_comment = @index["bar"].first.comments + assert_equal("####################\nDocumentation for bar\n####################\n", bar_comment) + + baz_comment = @index["baz"].first.comments + assert_equal("Documentation for baz", baz_comment) + + ban_comment = @index["ban"].first.comments + assert_empty(ban_comment) + end + def test_method_with_parameters index(<<~RUBY) class Foo diff --git a/test/requests/completion_resolve_test.rb b/test/requests/completion_resolve_test.rb index 343596708..15cadf66d 100644 --- a/test/requests/completion_resolve_test.rb +++ b/test/requests/completion_resolve_test.rb @@ -50,8 +50,8 @@ def initialize end class Foo - # Foo! def initialize + # Foo! @a = 1 end end From 4770fdbb8227b7976277a2f8afb6ac46044ebce1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Reeves?= <111763415+aurelien-reeves-scalingo@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:02:38 +0100 Subject: [PATCH 13/27] Allow configuring rbenv executable path (#2875) * Allow configuring rbenv executable path * Remove useless eslint-disable instruction * Fix linting offenses --- vscode/package.json | 4 ++ vscode/src/ruby/rbenv.ts | 36 ++++++++++++-- vscode/src/test/suite/ruby/rbenv.test.ts | 61 ++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 4 deletions(-) diff --git a/vscode/package.json b/vscode/package.json index e8db5e5dd..928714f85 100644 --- a/vscode/package.json +++ b/vscode/package.json @@ -345,6 +345,10 @@ "description": "The path to the Mise executable, if not installed in ~/.local/bin/mise", "type": "string" }, + "rbenvExecutablePath": { + "description": "The path to the rbenv executable, if not installed on one of the standard locations", + "type": "string" + }, "chrubyRubies": { "description": "An array of extra directories to search for Ruby installations when using chruby. Equivalent to the RUBIES environment variable", "type": "array" diff --git a/vscode/src/ruby/rbenv.ts b/vscode/src/ruby/rbenv.ts index b58d9cfee..2666db0d2 100644 --- a/vscode/src/ruby/rbenv.ts +++ b/vscode/src/ruby/rbenv.ts @@ -8,10 +8,7 @@ import { VersionManager, ActivationResult } from "./versionManager"; // Learn more: https://github.com/rbenv/rbenv export class Rbenv extends VersionManager { async activate(): Promise { - const rbenvExec = await this.findExec( - [vscode.Uri.file("/opt/homebrew/bin"), vscode.Uri.file("/usr/local/bin")], - "rbenv", - ); + const rbenvExec = await this.findRbenv(); const parsedResult = await this.runEnvActivationScript( `${rbenvExec} exec ruby`, @@ -24,4 +21,35 @@ export class Rbenv extends VersionManager { gemPath: parsedResult.gemPath, }; } + + private async findRbenv(): Promise { + const config = vscode.workspace.getConfiguration("rubyLsp"); + const configuredRbenvPath = config.get( + "rubyVersionManager.rbenvExecutablePath", + ); + + if (configuredRbenvPath) { + return this.ensureRbenvExistsAt(configuredRbenvPath); + } else { + return this.findExec( + [ + vscode.Uri.file("/opt/homebrew/bin"), + vscode.Uri.file("/usr/local/bin"), + ], + "rbenv", + ); + } + } + + private async ensureRbenvExistsAt(path: string): Promise { + try { + await vscode.workspace.fs.stat(vscode.Uri.file(path)); + + return path; + } catch (error: any) { + throw new Error( + `The Ruby LSP version manager is configured to be rbenv, but ${path} does not exist`, + ); + } + } } diff --git a/vscode/src/test/suite/ruby/rbenv.test.ts b/vscode/src/test/suite/ruby/rbenv.test.ts index db33705f8..a39a12cea 100644 --- a/vscode/src/test/suite/ruby/rbenv.test.ts +++ b/vscode/src/test/suite/ruby/rbenv.test.ts @@ -1,6 +1,7 @@ import assert from "assert"; import path from "path"; import os from "os"; +import fs from "fs"; import * as vscode from "vscode"; import sinon from "sinon"; @@ -59,6 +60,66 @@ suite("Rbenv", () => { execStub.restore(); }); + test("Allows configuring where rbenv is installed", async () => { + const workspacePath = fs.mkdtempSync( + path.join(os.tmpdir(), "ruby-lsp-test-"), + ); + const workspaceFolder = { + uri: vscode.Uri.from({ scheme: "file", path: workspacePath }), + name: path.basename(workspacePath), + index: 0, + }; + const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); + const rbenv = new Rbenv(workspaceFolder, outputChannel); + + const envStub = { + env: { ANY: "true" }, + yjit: true, + version: "3.0.0", + }; + + const execStub = sinon.stub(common, "asyncExec").resolves({ + stdout: "", + stderr: `${ACTIVATION_SEPARATOR}${JSON.stringify(envStub)}${ACTIVATION_SEPARATOR}`, + }); + + const rbenvPath = path.join(workspacePath, "rbenv"); + fs.writeFileSync(rbenvPath, "fakeRbenvBinary"); + + const configStub = sinon + .stub(vscode.workspace, "getConfiguration") + .returns({ + get: (name: string) => { + if (name === "rubyVersionManager.rbenvExecutablePath") { + return rbenvPath; + } + return ""; + }, + } as any); + + const { env, version, yjit } = await rbenv.activate(); + + assert.ok( + execStub.calledOnceWithExactly( + `${rbenvPath} exec ruby -W0 -rjson -e '${rbenv.activationScript}'`, + { + cwd: workspacePath, + shell: vscode.env.shell, + // eslint-disable-next-line no-process-env + env: process.env, + }, + ), + ); + + assert.strictEqual(version, "3.0.0"); + assert.strictEqual(yjit, true); + assert.deepStrictEqual(env.ANY, "true"); + + execStub.restore(); + configStub.restore(); + fs.rmSync(workspacePath, { recursive: true, force: true }); + }); + test("Reports invalid JSON environments", async () => { // eslint-disable-next-line no-process-env const workspacePath = process.env.PWD!; From a3ed38099e2754f9e7d6b4d55822a6100e1d97ea Mon Sep 17 00:00:00 2001 From: Tyler Lemburg Date: Tue, 19 Nov 2024 09:33:55 -0600 Subject: [PATCH 14/27] Add support for `extend self` to `handle_module_operation` (#2855) By adding the instance methods as class methods through the singleton mixin operations, we are able to get all instance methods "copied" over into the class methods implicitly. I don't think it's necessary to test whether this extend self applies when the module is opened after being closed again, or re-opened in another file. Closes #2782 --- .../lib/ruby_indexer/declaration_listener.rb | 24 ++++++++------ lib/ruby_indexer/test/index_test.rb | 32 +++++++++++++++++++ 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb index 2815ec1a8..0aa89ea66 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb @@ -751,16 +751,22 @@ def handle_module_operation(node, operation) return unless arguments arguments.each do |node| - next unless node.is_a?(Prism::ConstantReadNode) || node.is_a?(Prism::ConstantPathNode) - - case operation - when :include - owner.mixin_operations << Entry::Include.new(node.full_name) - when :prepend - owner.mixin_operations << Entry::Prepend.new(node.full_name) - when :extend + next unless node.is_a?(Prism::ConstantReadNode) || node.is_a?(Prism::ConstantPathNode) || + (node.is_a?(Prism::SelfNode) && operation == :extend) + + if node.is_a?(Prism::SelfNode) singleton = @index.existing_or_new_singleton_class(owner.name) - singleton.mixin_operations << Entry::Include.new(node.full_name) + singleton.mixin_operations << Entry::Include.new(owner.name) + else + case operation + when :include + owner.mixin_operations << Entry::Include.new(node.full_name) + when :prepend + owner.mixin_operations << Entry::Prepend.new(node.full_name) + when :extend + singleton = @index.existing_or_new_singleton_class(owner.name) + singleton.mixin_operations << Entry::Include.new(node.full_name) + end end rescue Prism::ConstantPathNode::DynamicPartsInConstantPathError, Prism::ConstantPathNode::MissingNodesInConstantPathError diff --git a/lib/ruby_indexer/test/index_test.rb b/lib/ruby_indexer/test/index_test.rb index 2723edc0a..69b5b68af 100644 --- a/lib/ruby_indexer/test/index_test.rb +++ b/lib/ruby_indexer/test/index_test.rb @@ -1672,6 +1672,38 @@ def test_linearizing_singleton_object ) end + def test_extend_self + @index.index_single(IndexablePath.new(nil, "/fake/path/foo.rb"), <<~RUBY) + module Foo + def bar + end + + extend self + + def baz + end + end + RUBY + + ["bar", "baz"].product(["Foo", "Foo::"]).each do |method, receiver| + entry = @index.resolve_method(method, receiver)&.first + refute_nil(entry) + assert_equal(method, T.must(entry).name) + end + + assert_equal( + [ + "Foo::", + "Foo", + "Module", + "Object", + "Kernel", + "BasicObject", + ], + @index.linearized_ancestors_of("Foo::"), + ) + end + def test_linearizing_singleton_ancestors @index.index_single(IndexablePath.new(nil, "/fake/path/foo.rb"), <<~RUBY) module First From 6bdc52ddf91a1dd568751e01f992be63f55272c6 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Tue, 19 Nov 2024 12:53:18 -0500 Subject: [PATCH 15/27] Remove deprecated experimental features from global state (#2829) --- lib/ruby_lsp/global_state.rb | 4 +--- test/global_state_test.rb | 11 ----------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/lib/ruby_lsp/global_state.rb b/lib/ruby_lsp/global_state.rb index fb6c2cab2..df0eee261 100644 --- a/lib/ruby_lsp/global_state.rb +++ b/lib/ruby_lsp/global_state.rb @@ -21,7 +21,7 @@ class GlobalState attr_reader :encoding sig { returns(T::Boolean) } - attr_reader :experimental_features, :top_level_bundle + attr_reader :top_level_bundle sig { returns(TypeInferrer) } attr_reader :type_inferrer @@ -40,7 +40,6 @@ def initialize @has_type_checker = T.let(true, T::Boolean) @index = T.let(RubyIndexer::Index.new, RubyIndexer::Index) @supported_formatters = T.let({}, T::Hash[String, Requests::Support::Formatter]) - @experimental_features = T.let(false, T::Boolean) @type_inferrer = T.let(TypeInferrer.new(@index), TypeInferrer) @addon_settings = T.let({}, T::Hash[String, T.untyped]) @top_level_bundle = T.let( @@ -131,7 +130,6 @@ def apply_options(options) end @index.configuration.encoding = @encoding - @experimental_features = options.dig(:initializationOptions, :experimentalFeaturesEnabled) || false @client_capabilities.apply_client_capabilities(options[:capabilities]) if options[:capabilities] addon_settings = options.dig(:initializationOptions, :addonSettings) diff --git a/test/global_state_test.rb b/test/global_state_test.rb index c28fa8b18..02dc0ee66 100644 --- a/test/global_state_test.rb +++ b/test/global_state_test.rb @@ -188,17 +188,6 @@ def test_linter_auto_detection_with_rubocop_as_transitive_dependency assert_includes(state.instance_variable_get(:@linters), "rubocop") end - def test_apply_options_sets_experimental_features - state = GlobalState.new - refute_predicate(state, :experimental_features) - - state.apply_options({ - initializationOptions: { experimentalFeaturesEnabled: true }, - }) - - assert_predicate(state, :experimental_features) - end - def test_type_checker_is_detected_based_on_transitive_sorbet_static state = GlobalState.new From 7d067e507f7ee7308a621a7fb26c19ee350c5c27 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Tue, 19 Nov 2024 13:17:14 -0500 Subject: [PATCH 16/27] Eagerly compute SHA's for watched files (#2861) ### Motivation This PR fixes two issues related to automated restarts. The first one is preventing undesired restarts immediately after the server finishes booting. The second one is preventing unwanted restarts if a create event is fired for an existing file. In theory, this shouldn't happen, but our tests tell a different story and we consistently see a create event getting fired for updates. ### Implementation The two fixes are: 1. Unified all of our watchers to only check the top level configuration files and then started eagerly computing the SHAs for all of them (if they are present). This will prevent unwanted restarts if these files are touched immediately after boot 2. Started using the hash check for `didCreate` as well. If the file being updated fires a create event and the content SHA matches ### Automated Tests Added a new test and reduced some of the sleeps on the other test, which is taking a bit too long. --- vscode/src/test/suite/workspace.test.ts | 44 ++++++++++-- vscode/src/workspace.ts | 95 +++++++++++++++---------- 2 files changed, 96 insertions(+), 43 deletions(-) diff --git a/vscode/src/test/suite/workspace.test.ts b/vscode/src/test/suite/workspace.test.ts index 69b550019..ed91e2619 100644 --- a/vscode/src/test/suite/workspace.test.ts +++ b/vscode/src/test/suite/workspace.test.ts @@ -59,15 +59,15 @@ suite("Workspace", () => { await workspace.activate(); - for (let i = 0; i < 5; i++) { - await new Promise((resolve) => setTimeout(resolve, 200)); + for (let i = 0; i < 4; i++) { + await new Promise((resolve) => setTimeout(resolve, 50)); fs.writeFileSync(path.join(gitDir, "rebase-apply"), "1"); - await new Promise((resolve) => setTimeout(resolve, 200)); + await new Promise((resolve) => setTimeout(resolve, 50)); fs.rmSync(path.join(gitDir, "rebase-apply")); } // Give enough time for all watchers to fire and all debounces to run off - await new Promise((resolve) => setTimeout(resolve, 10000)); + await new Promise((resolve) => setTimeout(resolve, 6000)); startStub.restore(); restartSpy.restore(); @@ -77,4 +77,40 @@ suite("Workspace", () => { // The restart call only happens once because of the debounce assert.strictEqual(restartSpy.callCount, 1); }).timeout(60000); + + test("does not restart when watched files are touched without modifying contents", async () => { + const lockfileUri = vscode.Uri.file( + path.join(workspacePath, "Gemfile.lock"), + ); + const contents = Buffer.from("hello"); + await vscode.workspace.fs.writeFile(lockfileUri, contents); + + const workspace = new Workspace( + context, + workspaceFolder, + FAKE_TELEMETRY, + () => {}, + new Map(), + ); + + await workspace.activate(); + + const startStub = sinon.stub(workspace, "start"); + const restartSpy = sinon.spy(workspace, "restart"); + + for (let i = 0; i < 4; i++) { + const updateDate = new Date(); + fs.utimesSync(lockfileUri.fsPath, updateDate, updateDate); + await new Promise((resolve) => setTimeout(resolve, 50)); + } + + // Give enough time for all watchers to fire and all debounces to run off + await new Promise((resolve) => setTimeout(resolve, 6000)); + + startStub.restore(); + restartSpy.restore(); + + assert.strictEqual(startStub.callCount, 0); + assert.strictEqual(restartSpy.callCount, 0); + }).timeout(10000); }); diff --git a/vscode/src/workspace.ts b/vscode/src/workspace.ts index dc054a6b1..5fcf55220 100644 --- a/vscode/src/workspace.ts +++ b/vscode/src/workspace.ts @@ -14,6 +14,13 @@ import { } from "./common"; import { WorkspaceChannel } from "./workspaceChannel"; +const WATCHED_FILES = [ + "Gemfile.lock", + "gems.locked", + ".rubocop.yml", + ".rubocop", +]; + export class Workspace implements WorkspaceInterface { public lspClient?: Client; public readonly ruby: Ruby; @@ -58,8 +65,6 @@ export class Workspace implements WorkspaceInterface { this.createTestItems = createTestItems; this.isMainWorkspace = isMainWorkspace; this.virtualDocuments = virtualDocuments; - - this.registerRestarts(context); } // Activate this workspace. This method is intended to be invoked only once, unlikely `start` which may be invoked @@ -82,6 +87,20 @@ export class Workspace implements WorkspaceInterface { rootGitUri, ".git/{rebase-merge,rebase-apply,BISECT_START,CHERRY_PICK_HEAD}", ); + + this.registerRestarts(); + + // Eagerly calculate SHAs for the watched files to avoid unnecessary restarts + for (const file of WATCHED_FILES) { + const uri = vscode.Uri.joinPath(this.workspaceFolder.uri, file); + const currentSha = await this.fileContentsSha(uri); + + if (!currentSha) { + continue; + } + + this.restartDocumentShas.set(uri.fsPath, currentSha); + } } async start(debugMode?: boolean) { @@ -164,7 +183,9 @@ export class Workspace implements WorkspaceInterface { // server can now handle shutdown requests if (this.needsRestart) { this.needsRestart = false; - await this.restart(); + await this.debouncedRestart( + "a restart was requested while the server was still booting", + ); } } catch (error: any) { this.error = true; @@ -342,39 +363,21 @@ export class Workspace implements WorkspaceInterface { return result; } - private registerRestarts(context: vscode.ExtensionContext) { - this.createRestartWatcher(context, "Gemfile.lock"); - this.createRestartWatcher(context, "gems.locked"); - this.createRestartWatcher(context, "**/.rubocop.yml"); - this.createRestartWatcher(context, ".rubocop"); + private registerRestarts() { + this.createRestartWatcher(`{${WATCHED_FILES.join(",")}}`); // If a configuration that affects the Ruby LSP has changed, update the client options using the latest // configuration and restart the server - context.subscriptions.push( + this.context.subscriptions.push( vscode.workspace.onDidChangeConfiguration(async (event) => { if (event.affectsConfiguration("rubyLsp")) { - // Re-activate Ruby if the version manager changed - if ( - event.affectsConfiguration("rubyLsp.rubyVersionManager") || - event.affectsConfiguration("rubyLsp.bundleGemfile") || - event.affectsConfiguration("rubyLsp.customRubyCommand") - ) { - await this.ruby.activateRuby(); - } - - this.outputChannel.info( - "Restarting the Ruby LSP because configuration changed", - ); - await this.restart(); + await this.debouncedRestart("configuration changed"); } }), ); } - private createRestartWatcher( - context: vscode.ExtensionContext, - pattern: string, - ) { + private createRestartWatcher(pattern: string) { const watcher = vscode.workspace.createFileSystemWatcher( new vscode.RelativePattern(this.workspaceFolder, pattern), ); @@ -382,27 +385,27 @@ export class Workspace implements WorkspaceInterface { // Handler for only triggering restart if the contents of the file have been modified. If the file was just touched, // but the contents are the same, we don't want to restart const debouncedRestartWithHashCheck = async (uri: vscode.Uri) => { - const fileContents = await vscode.workspace.fs.readFile(uri); const fsPath = uri.fsPath; + const currentSha = await this.fileContentsSha(uri); - const hash = createHash("sha256"); - hash.update(fileContents.toString()); - const currentSha = hash.digest("hex"); - - if (this.restartDocumentShas.get(fsPath) !== currentSha) { + if (currentSha && this.restartDocumentShas.get(fsPath) !== currentSha) { this.restartDocumentShas.set(fsPath, currentSha); - await this.debouncedRestart(`${pattern} changed`); + await this.debouncedRestart(`${fsPath} changed, matching ${pattern}`); } }; - const debouncedRestart = async () => { - await this.debouncedRestart(`${pattern} changed`); + const debouncedRestart = async (uri: vscode.Uri) => { + this.restartDocumentShas.delete(uri.fsPath); + await this.debouncedRestart(`${uri.fsPath} changed, matching ${pattern}`); }; - context.subscriptions.push( + this.context.subscriptions.push( watcher, watcher.onDidChange(debouncedRestartWithHashCheck), - watcher.onDidCreate(debouncedRestart), + // Interestingly, we are seeing create events being fired even when the file already exists. If a create event is + // fired for an update to an existing file, then we need to check if the contents still match to prevent unwanted + // restarts + watcher.onDidCreate(debouncedRestartWithHashCheck), watcher.onDidDelete(debouncedRestart), ); } @@ -416,9 +419,9 @@ export class Workspace implements WorkspaceInterface { this.#inhibitRestart = true; }; - const stop = async () => { + const stop = async (uri: vscode.Uri) => { this.#inhibitRestart = false; - await this.debouncedRestart(`${glob} changed`); + await this.debouncedRestart(`${uri.fsPath} changed, matching ${glob}`); }; this.context.subscriptions.push( @@ -429,4 +432,18 @@ export class Workspace implements WorkspaceInterface { workspaceWatcher.onDidDelete(stop), ); } + + private async fileContentsSha(uri: vscode.Uri): Promise { + let fileContents; + + try { + fileContents = await vscode.workspace.fs.readFile(uri); + } catch (error: any) { + return undefined; + } + + const hash = createHash("sha256"); + hash.update(fileContents.toString()); + return hash.digest("hex"); + } } From 5f13ecda1fe9057268dcd0dd7ae99420162f4916 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Tue, 19 Nov 2024 13:24:10 -0500 Subject: [PATCH 17/27] Remove experimental features from extension and migrate old settings (#2830) --- vscode/package.json | 5 ----- vscode/src/client.ts | 3 --- vscode/src/extension.ts | 17 +++++++++++++++ vscode/src/status.ts | 24 --------------------- vscode/src/test/suite/common.test.ts | 8 +++++++ vscode/src/test/suite/status.test.ts | 32 ---------------------------- 6 files changed, 25 insertions(+), 64 deletions(-) diff --git a/vscode/package.json b/vscode/package.json index 928714f85..99ced2a9a 100644 --- a/vscode/package.json +++ b/vscode/package.json @@ -158,11 +158,6 @@ "configuration": { "title": "Ruby LSP", "properties": { - "rubyLsp.enableExperimentalFeatures": { - "description": "Enable experimental and under development features", - "type": "boolean", - "default": false - }, "rubyLsp.enabledFeatures": { "description": "List of enabled LSP features", "type": "object", diff --git a/vscode/src/client.ts b/vscode/src/client.ts index 3313b9803..4731289c0 100644 --- a/vscode/src/client.ts +++ b/vscode/src/client.ts @@ -218,9 +218,6 @@ function collectClientOptions( errorHandler: new ClientErrorHandler(workspaceFolder, telemetry), initializationOptions: { enabledFeatures, - experimentalFeaturesEnabled: configuration.get( - "enableExperimentalFeatures", - ), featuresConfiguration: configuration.get("featuresConfiguration"), formatter: configuration.get("formatter"), linters: configuration.get("linters"), diff --git a/vscode/src/extension.ts b/vscode/src/extension.ts index 82595e7f5..842cf4fe3 100644 --- a/vscode/src/extension.ts +++ b/vscode/src/extension.ts @@ -25,6 +25,8 @@ export async function activate(context: vscode.ExtensionContext) { return; } + await migrateExperimentalFeaturesSetting(); + const logger = await createLogger(context); context.subscriptions.push(logger); @@ -36,6 +38,21 @@ export async function deactivate(): Promise { await extension.deactivate(); } +// Remove after ~2 months. This code migrates the old experimental features setting to the new feature flag rollout +// setting +async function migrateExperimentalFeaturesSetting() { + const config = vscode.workspace.getConfiguration("rubyLsp"); + const experimentalFeatures = config.get("enableExperimentalFeatures"); + + if (experimentalFeatures) { + // Remove the old setting + await config.update("enableExperimentalFeatures", undefined, true); + + // Add the new one + await config.update("featureFlags", { all: true }, true); + } +} + async function createLogger(context: vscode.ExtensionContext) { let sender; diff --git a/vscode/src/status.ts b/vscode/src/status.ts index ad53709d8..665546d7c 100644 --- a/vscode/src/status.ts +++ b/vscode/src/status.ts @@ -119,29 +119,6 @@ export class ServerStatus extends StatusItem { } } -export class ExperimentalFeaturesStatus extends StatusItem { - constructor() { - super("experimentalFeatures"); - - const experimentalFeaturesEnabled = - vscode.workspace - .getConfiguration("rubyLsp") - .get("enableExperimentalFeatures") === true; - const message = experimentalFeaturesEnabled - ? "Experimental features enabled" - : "Experimental features disabled"; - - this.item.name = "Ruby LSP Experimental Features"; - this.item.text = message; - this.item.command = { - title: experimentalFeaturesEnabled ? "Disable" : "Enable", - command: Command.ToggleExperimentalFeatures, - }; - } - - refresh(_workspace: WorkspaceInterface): void {} -} - export class FeaturesStatus extends StatusItem { constructor() { super("features"); @@ -225,7 +202,6 @@ export class StatusItems { this.items = [ new RubyVersionStatus(), new ServerStatus(), - new ExperimentalFeaturesStatus(), new FeaturesStatus(), new FormatterStatus(), new AddonsStatus(), diff --git a/vscode/src/test/suite/common.test.ts b/vscode/src/test/suite/common.test.ts index cb1e99bdc..23f1bc389 100644 --- a/vscode/src/test/suite/common.test.ts +++ b/vscode/src/test/suite/common.test.ts @@ -33,6 +33,12 @@ suite("Common", () => { }); test("maintains enabled state when increasing rollout percentage", () => { + const stub = sandbox.stub(vscode.workspace, "getConfiguration").returns({ + get: () => { + return { all: undefined }; + }, + } as any); + // For the fake machine of 42 in base 16 and the name `fakeFeature`, the feature flag activation percetange is // 0.357. For every percetange below that, the feature should appear as disabled [0.25, 0.3, 0.35].forEach((percentage) => { @@ -45,6 +51,8 @@ suite("Common", () => { (FEATURE_FLAGS as any).fakeFeature = percentage; assert.strictEqual(featureEnabled("fakeFeature" as any), true); }); + + stub.restore(); }); test("returns false if user opted out of specific feature", () => { diff --git a/vscode/src/test/suite/status.test.ts b/vscode/src/test/suite/status.test.ts index 7956ee5c8..cefb6ed17 100644 --- a/vscode/src/test/suite/status.test.ts +++ b/vscode/src/test/suite/status.test.ts @@ -9,7 +9,6 @@ import { Ruby } from "../../ruby"; import { RubyVersionStatus, ServerStatus, - ExperimentalFeaturesStatus, StatusItem, FeaturesStatus, FormatterStatus, @@ -21,7 +20,6 @@ suite("StatusItems", () => { let ruby: Ruby; let status: StatusItem; let workspace: WorkspaceInterface; - let formatter: string; afterEach(() => { status.dispose(); @@ -145,36 +143,6 @@ suite("StatusItems", () => { }); }); - suite("ExperimentalFeaturesStatus", () => { - beforeEach(() => { - ruby = {} as Ruby; - workspace = { - ruby, - lspClient: { - addons: [], - state: State.Running, - formatter, - serverVersion: "1.0.0", - sendRequest: () => Promise.resolve([] as T), - degraded: false, - }, - error: false, - }; - status = new ExperimentalFeaturesStatus(); - status.refresh(workspace); - }); - - test("Status is initialized with the right values", () => { - assert.match(status.item.text, /Experimental features (dis|en)abled/); - assert.strictEqual(status.item.name, "Ruby LSP Experimental Features"); - assert.match(status.item.command?.title!, /Enable|Disable/); - assert.strictEqual( - status.item.command!.command, - Command.ToggleExperimentalFeatures, - ); - }); - }); - suite("FeaturesStatus", () => { beforeEach(() => { ruby = {} as Ruby; From 74d2ab0926198ac2771b35b79d4f81574365497a Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Tue, 19 Nov 2024 16:45:20 -0500 Subject: [PATCH 18/27] Show multi-root workspace warning as progress instead (#2879) ### Motivation We noticed that the warning for multi-root workspaces is sometimes ignored by users. The problem is that if you don't click anything, the LSP will never start because the promise is never resolved. Let's show the warning under a timer and auto-dismiss so that the behaviour is less confusing. ### Implementation I put the same warning inside a progress notification that auto-dismisses under 5 seconds. --- vscode/src/rubyLsp.ts | 68 ++++++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/vscode/src/rubyLsp.ts b/vscode/src/rubyLsp.ts index 4a055f743..83e5870ee 100644 --- a/vscode/src/rubyLsp.ts +++ b/vscode/src/rubyLsp.ts @@ -175,7 +175,6 @@ export class RubyLsp { workspaceFolder: vscode.WorkspaceFolder, eager: boolean, ) { - const workspaceDir = workspaceFolder.uri.fsPath; const customBundleGemfile: string = vscode.workspace .getConfiguration("rubyLsp") .get("bundleGemfile")!; @@ -191,31 +190,8 @@ export class RubyLsp { // If no lockfile exists and we're activating lazily (if the user opened a Ruby file inside a workspace we hadn't // activated before), then we start the language server, but we warn the user that they may be missing multi-root // workspace configuration - if ( - customBundleGemfile.length === 0 && - !lockfileExists && - !this.context.globalState.get("rubyLsp.disableMultirootLockfileWarning") - ) { - const answer = await vscode.window.showWarningMessage( - `Activating the Ruby LSP in ${workspaceDir}, but no lockfile was found. Are you using a monorepo setup?`, - "See the multi-root workspace docs", - "Don't show again", - ); - - if (answer === "See the multi-root workspace docs") { - await vscode.env.openExternal( - vscode.Uri.parse( - "https://github.com/Shopify/ruby-lsp/blob/main/vscode/README.md?tab=readme-ov-file#multi-root-workspaces", - ), - ); - } - - if (answer === "Don't show again") { - await this.context.globalState.update( - "rubyLsp.disableMultirootLockfileWarning", - true, - ); - } + if (customBundleGemfile.length === 0 && !lockfileExists) { + await this.showStandaloneWarning(workspaceFolder.uri.fsPath); } const workspace = new Workspace( @@ -797,4 +773,44 @@ export class RubyLsp { return false; } + + private async showStandaloneWarning(workspaceDir: string) { + await vscode.window.withProgress( + { + location: vscode.ProgressLocation.Notification, + title: "No bundle found. Launching in standalone mode in 5 seconds", + cancellable: true, + }, + async (progress, token) => { + progress.report({ + message: + "If working in a monorepo, cancel to see configuration instructions", + }); + + await new Promise((resolve) => { + token.onCancellationRequested(() => { + resolve(); + }); + + setTimeout(resolve, 5000); + }); + + if (token.isCancellationRequested) { + const answer = await vscode.window.showWarningMessage( + `Could not find a lockfile in ${workspaceDir}. Are you using a monorepo setup?`, + "See the multi-root workspace docs", + "Launch anyway", + ); + + if (answer === "See the multi-root workspace docs") { + const uri = vscode.Uri.parse( + "https://github.com/Shopify/ruby-lsp/blob/main/vscode/README.md?tab=readme-ov-file#multi-root-workspaces", + ); + + await vscode.env.openExternal(uri); + } + } + }, + ); + } } From 071aa8481a473df6b52647b4ec26ce0dfd69c8f4 Mon Sep 17 00:00:00 2001 From: Adam Daniels Date: Tue, 19 Nov 2024 17:29:09 -0500 Subject: [PATCH 19/27] Add callout on editors section of website about Mason risks (#2885) When using Mason to manage Ruby LSP, unless only a single Ruby version is used, there are chances of strange errors due to conflicts of the Ruby ABI. For anything other than the simplest of deployments, Mason should likely not be used to manage the Ruby LSP or any Ruby IDE tooling. --- jekyll/editors.markdown | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/jekyll/editors.markdown b/jekyll/editors.markdown index da3aa5ffc..9cfd1c2f2 100644 --- a/jekyll/editors.markdown +++ b/jekyll/editors.markdown @@ -169,6 +169,16 @@ mason_lspconfig.setup_handlers { } ``` +{: .important } +> Using Mason to manage your installation of the Ruby LSP may cause errors + +Mason installs the Ruby LSP in a folder shared among all your Rubies. Some of the +Ruby LSP dependencies are C extensions, and they rely on the Ruby ABI to look and +act a certain way when they were linked to Ruby. This causes issues when a shared +folder is used. + +See [this issue][mason-abi] for further information. + ### Additional setup (optional) `rubyLsp/workspace/dependencies` is a custom method currently supported only in the VS Code plugin. @@ -293,3 +303,6 @@ To use it with Ruby LSP, you can override particular configuration items in the Kate will start an instance of the Ruby LSP server in the background for any Ruby project matching the `rootIndicationFileNames`. If starting Ruby LSP succeeds, the entries in the LSP-Client menu are activated. Otherwise the error output can be inspected in the Output window. + + +[mason-abi]: https://github.com/williamboman/mason.nvim/issues/1292 From 3f62b8ead0547659ae2142c87ca7245b2a6fa289 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 20 Nov 2024 09:26:58 -0500 Subject: [PATCH 20/27] Only include workspace name as part of telemetry (#2883) ### Motivation I made a mistake when adding the workspace as part of the telemetry. I thought it was the workspace name, but it's actually the path to the workspace, which may include things like `/Users/name/...`. Let's return only the workspace name to fully anonymize the telemetry. --- vscode/src/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vscode/src/client.ts b/vscode/src/client.ts index 4731289c0..594ce633c 100644 --- a/vscode/src/client.ts +++ b/vscode/src/client.ts @@ -498,7 +498,7 @@ export default class Client extends LanguageClient implements ClientInterface { ...error.data, serverVersion: this.serverVersion, workspace: new vscode.TelemetryTrustedValue( - this.workingDirectory, + path.basename(this.workingDirectory), ), }, ); From 5704eb3cc92b54109395ad48d89320c1b2ca1803 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Wed, 20 Nov 2024 17:43:19 +0000 Subject: [PATCH 21/27] Display output channel when addons are clicked (#2850) This way, when users see that an addon is errored, they can quickly navigate to the output channel to see the error. --- vscode/src/rubyLsp.ts | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/vscode/src/rubyLsp.ts b/vscode/src/rubyLsp.ts index 83e5870ee..6bc84d00e 100644 --- a/vscode/src/rubyLsp.ts +++ b/vscode/src/rubyLsp.ts @@ -263,14 +263,7 @@ export class RubyLsp { } const options: vscode.QuickPickItem[] = client.addons - .sort((addon) => { - // Display errored addons last - if (addon.errored) { - return 1; - } - - return -1; - }) + .sort((addon) => (addon.errored ? 1 : -1)) .map((addon) => { const icon = addon.errored ? "$(error)" : "$(pass)"; return { @@ -278,9 +271,24 @@ export class RubyLsp { }; }); - await vscode.window.showQuickPick(options, { - placeHolder: "Addons (readonly)", + const quickPick = vscode.window.createQuickPick(); + quickPick.items = options; + quickPick.placeholder = "Addons (click to view output)"; + + quickPick.onDidAccept(() => { + const selected = quickPick.selectedItems[0]; + // Ideally, we should display information that's specific to the selected addon + if (selected) { + this.currentActiveWorkspace()?.outputChannel.show(); + } + quickPick.hide(); }); + + quickPick.onDidHide(() => { + quickPick.dispose(); + }); + + quickPick.show(); }), vscode.commands.registerCommand(Command.ToggleFeatures, async () => { // Extract feature descriptions from our package.json From afe3705db5aafb87cdd958e1e1ddfb11c8f3aad7 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 20 Nov 2024 13:33:44 -0500 Subject: [PATCH 22/27] Allow individual version managers to trigger manual Ruby selection (#2835) ### Motivation First step for #2793 This PR passes down the `manuallySelectRuby` function to version managers in order to allow specific version managers to offer configuring fallbacks under certain situations. ### Implementation This PR only starts passing the function down to managers. The next PR in the stack starts using it. ### Automated Tests Updated existing tests. --- vscode/src/ruby.ts | 68 ++++++++++++++++--- vscode/src/ruby/chruby.ts | 3 +- vscode/src/ruby/none.ts | 3 +- vscode/src/ruby/versionManager.ts | 3 + vscode/src/test/suite/ruby/asdf.test.ts | 4 +- vscode/src/test/suite/ruby/chruby.test.ts | 16 ++--- vscode/src/test/suite/ruby/custom.test.ts | 2 +- vscode/src/test/suite/ruby/mise.test.ts | 4 +- vscode/src/test/suite/ruby/none.test.ts | 2 +- vscode/src/test/suite/ruby/rbenv.test.ts | 6 +- .../src/test/suite/ruby/rubyInstaller.test.ts | 18 ++++- vscode/src/test/suite/ruby/rvm.test.ts | 2 +- vscode/src/test/suite/ruby/shadowenv.test.ts | 30 ++++++-- 13 files changed, 122 insertions(+), 39 deletions(-) diff --git a/vscode/src/ruby.ts b/vscode/src/ruby.ts index f06786d83..fad69bb63 100644 --- a/vscode/src/ruby.ts +++ b/vscode/src/ruby.ts @@ -114,7 +114,12 @@ export class Ruby implements RubyInterface { if (workspaceRubyPath) { // If a workspace specific Ruby path is configured, then we use that to activate the environment await this.runActivation( - new None(this.workspaceFolder, this.outputChannel, workspaceRubyPath), + new None( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + workspaceRubyPath, + ), ); } else { // If the version manager is auto, discover the actual manager before trying to activate anything @@ -139,7 +144,12 @@ export class Ruby implements RubyInterface { if (globalRubyPath) { await this.runActivation( - new None(this.workspaceFolder, this.outputChannel, globalRubyPath), + new None( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + globalRubyPath, + ), ); } else { this._error = true; @@ -266,47 +276,83 @@ export class Ruby implements RubyInterface { switch (this.versionManager.identifier) { case ManagerIdentifier.Asdf: await this.runActivation( - new Asdf(this.workspaceFolder, this.outputChannel), + new Asdf( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; case ManagerIdentifier.Chruby: await this.runActivation( - new Chruby(this.workspaceFolder, this.outputChannel), + new Chruby( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; case ManagerIdentifier.Rbenv: await this.runActivation( - new Rbenv(this.workspaceFolder, this.outputChannel), + new Rbenv( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; case ManagerIdentifier.Rvm: await this.runActivation( - new Rvm(this.workspaceFolder, this.outputChannel), + new Rvm( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; case ManagerIdentifier.Mise: await this.runActivation( - new Mise(this.workspaceFolder, this.outputChannel), + new Mise( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; case ManagerIdentifier.RubyInstaller: await this.runActivation( - new RubyInstaller(this.workspaceFolder, this.outputChannel), + new RubyInstaller( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; case ManagerIdentifier.Custom: await this.runActivation( - new Custom(this.workspaceFolder, this.outputChannel), + new Custom( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; case ManagerIdentifier.None: await this.runActivation( - new None(this.workspaceFolder, this.outputChannel), + new None( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; default: await this.runActivation( - new Shadowenv(this.workspaceFolder, this.outputChannel), + new Shadowenv( + this.workspaceFolder, + this.outputChannel, + this.manuallySelectRuby.bind(this), + ), ); break; } diff --git a/vscode/src/ruby/chruby.ts b/vscode/src/ruby/chruby.ts index cabb26ee0..81a8d9915 100644 --- a/vscode/src/ruby/chruby.ts +++ b/vscode/src/ruby/chruby.ts @@ -29,8 +29,9 @@ export class Chruby extends VersionManager { constructor( workspaceFolder: vscode.WorkspaceFolder, outputChannel: WorkspaceChannel, + manuallySelectRuby: () => Promise, ) { - super(workspaceFolder, outputChannel); + super(workspaceFolder, outputChannel, manuallySelectRuby); const configuredRubies = vscode.workspace .getConfiguration("rubyLsp") diff --git a/vscode/src/ruby/none.ts b/vscode/src/ruby/none.ts index faea1690f..13d65facb 100644 --- a/vscode/src/ruby/none.ts +++ b/vscode/src/ruby/none.ts @@ -19,9 +19,10 @@ export class None extends VersionManager { constructor( workspaceFolder: vscode.WorkspaceFolder, outputChannel: WorkspaceChannel, + manuallySelectRuby: () => Promise, rubyPath?: string, ) { - super(workspaceFolder, outputChannel); + super(workspaceFolder, outputChannel, manuallySelectRuby); this.rubyPath = rubyPath ?? "ruby"; } diff --git a/vscode/src/ruby/versionManager.ts b/vscode/src/ruby/versionManager.ts index 506fc8a0c..f24837490 100644 --- a/vscode/src/ruby/versionManager.ts +++ b/vscode/src/ruby/versionManager.ts @@ -26,15 +26,18 @@ export abstract class VersionManager { protected readonly outputChannel: WorkspaceChannel; protected readonly workspaceFolder: vscode.WorkspaceFolder; protected readonly bundleUri: vscode.Uri; + protected readonly manuallySelectRuby: () => Promise; private readonly customBundleGemfile?: string; constructor( workspaceFolder: vscode.WorkspaceFolder, outputChannel: WorkspaceChannel, + manuallySelectRuby: () => Promise, ) { this.workspaceFolder = workspaceFolder; this.outputChannel = outputChannel; + this.manuallySelectRuby = manuallySelectRuby; const customBundleGemfile: string = vscode.workspace .getConfiguration("rubyLsp") .get("bundleGemfile")!; diff --git a/vscode/src/test/suite/ruby/asdf.test.ts b/vscode/src/test/suite/ruby/asdf.test.ts index e5e8aeab5..46a4449ad 100644 --- a/vscode/src/test/suite/ruby/asdf.test.ts +++ b/vscode/src/test/suite/ruby/asdf.test.ts @@ -26,7 +26,7 @@ suite("Asdf", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const asdf = new Asdf(workspaceFolder, outputChannel); + const asdf = new Asdf(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, yjit: true, @@ -75,7 +75,7 @@ suite("Asdf", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const asdf = new Asdf(workspaceFolder, outputChannel); + const asdf = new Asdf(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, yjit: true, diff --git a/vscode/src/test/suite/ruby/chruby.test.ts b/vscode/src/test/suite/ruby/chruby.test.ts index 8e1339457..b997caba2 100644 --- a/vscode/src/test/suite/ruby/chruby.test.ts +++ b/vscode/src/test/suite/ruby/chruby.test.ts @@ -74,7 +74,7 @@ suite("Chruby", () => { test("Finds Ruby when .ruby-version is inside workspace", async () => { fs.writeFileSync(path.join(workspacePath, ".ruby-version"), RUBY_VERSION); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); chruby.rubyInstallationUris = [ vscode.Uri.file(path.join(rootPath, "opt", "rubies")), ]; @@ -90,7 +90,7 @@ suite("Chruby", () => { test("Finds Ruby when .ruby-version is inside on parent directories", async () => { fs.writeFileSync(path.join(rootPath, ".ruby-version"), RUBY_VERSION); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); chruby.rubyInstallationUris = [ vscode.Uri.file(path.join(rootPath, "opt", "rubies")), ]; @@ -126,7 +126,7 @@ suite("Chruby", () => { fs.writeFileSync(path.join(rootPath, ".ruby-version"), RUBY_VERSION); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); chruby.rubyInstallationUris = [ vscode.Uri.file(path.join(rootPath, "opt", "rubies")), ]; @@ -171,7 +171,7 @@ suite("Chruby", () => { `${RUBY_VERSION}-rc1`, ); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); chruby.rubyInstallationUris = [ vscode.Uri.file(path.join(rootPath, "opt", "rubies")), ]; @@ -201,7 +201,7 @@ suite("Chruby", () => { fs.writeFileSync(path.join(rootPath, ".ruby-version"), RUBY_VERSION); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); chruby.rubyInstallationUris = [vscode.Uri.file(rubyHome)]; const { env, version, yjit } = await chruby.activate(); @@ -223,7 +223,7 @@ suite("Chruby", () => { : "", } as any); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); configStub.restore(); const { env, version, yjit } = await chruby.activate(); @@ -247,7 +247,7 @@ suite("Chruby", () => { `${major}.${minor}`, ); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); chruby.rubyInstallationUris = [ vscode.Uri.file(path.join(rootPath, "opt", "rubies")), ]; @@ -278,7 +278,7 @@ suite("Chruby", () => { `${major}.${minor}`, ); - const chruby = new Chruby(workspaceFolder, outputChannel); + const chruby = new Chruby(workspaceFolder, outputChannel, async () => {}); chruby.rubyInstallationUris = [ vscode.Uri.file(path.join(rootPath, ".rubies")), vscode.Uri.file(path.join(rootPath, "opt", "rubies")), diff --git a/vscode/src/test/suite/ruby/custom.test.ts b/vscode/src/test/suite/ruby/custom.test.ts index 27bc824f1..5e1482d1c 100644 --- a/vscode/src/test/suite/ruby/custom.test.ts +++ b/vscode/src/test/suite/ruby/custom.test.ts @@ -23,7 +23,7 @@ suite("Custom", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const custom = new Custom(workspaceFolder, outputChannel); + const custom = new Custom(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, diff --git a/vscode/src/test/suite/ruby/mise.test.ts b/vscode/src/test/suite/ruby/mise.test.ts index b2e6a1f2c..5d31fa244 100644 --- a/vscode/src/test/suite/ruby/mise.test.ts +++ b/vscode/src/test/suite/ruby/mise.test.ts @@ -27,7 +27,7 @@ suite("Mise", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const mise = new Mise(workspaceFolder, outputChannel); + const mise = new Mise(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, @@ -82,7 +82,7 @@ suite("Mise", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const mise = new Mise(workspaceFolder, outputChannel); + const mise = new Mise(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, diff --git a/vscode/src/test/suite/ruby/none.test.ts b/vscode/src/test/suite/ruby/none.test.ts index 1b5bbaf3e..f42fbaf64 100644 --- a/vscode/src/test/suite/ruby/none.test.ts +++ b/vscode/src/test/suite/ruby/none.test.ts @@ -23,7 +23,7 @@ suite("None", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const none = new None(workspaceFolder, outputChannel); + const none = new None(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, diff --git a/vscode/src/test/suite/ruby/rbenv.test.ts b/vscode/src/test/suite/ruby/rbenv.test.ts index a39a12cea..1463c2f3e 100644 --- a/vscode/src/test/suite/ruby/rbenv.test.ts +++ b/vscode/src/test/suite/ruby/rbenv.test.ts @@ -27,7 +27,7 @@ suite("Rbenv", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const rbenv = new Rbenv(workspaceFolder, outputChannel); + const rbenv = new Rbenv(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, @@ -70,7 +70,7 @@ suite("Rbenv", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const rbenv = new Rbenv(workspaceFolder, outputChannel); + const rbenv = new Rbenv(workspaceFolder, outputChannel, async () => {}); const envStub = { env: { ANY: "true" }, @@ -129,7 +129,7 @@ suite("Rbenv", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const rbenv = new Rbenv(workspaceFolder, outputChannel); + const rbenv = new Rbenv(workspaceFolder, outputChannel, async () => {}); const execStub = sinon.stub(common, "asyncExec").resolves({ stdout: "", diff --git a/vscode/src/test/suite/ruby/rubyInstaller.test.ts b/vscode/src/test/suite/ruby/rubyInstaller.test.ts index 6e478da36..ca773618d 100644 --- a/vscode/src/test/suite/ruby/rubyInstaller.test.ts +++ b/vscode/src/test/suite/ruby/rubyInstaller.test.ts @@ -60,7 +60,11 @@ suite("RubyInstaller", () => { fs.writeFileSync(path.join(workspacePath, ".ruby-version"), RUBY_VERSION); - const windows = new RubyInstaller(workspaceFolder, outputChannel); + const windows = new RubyInstaller( + workspaceFolder, + outputChannel, + async () => {}, + ); const { env, version, yjit } = await windows.activate(); assert.match(env.GEM_PATH!, /ruby\/3\.3\.0/); @@ -90,7 +94,11 @@ suite("RubyInstaller", () => { fs.writeFileSync(path.join(workspacePath, ".ruby-version"), RUBY_VERSION); - const windows = new RubyInstaller(workspaceFolder, outputChannel); + const windows = new RubyInstaller( + workspaceFolder, + outputChannel, + async () => {}, + ); const { env, version, yjit } = await windows.activate(); assert.match(env.GEM_PATH!, /ruby\/3\.3\.0/); @@ -120,7 +128,11 @@ suite("RubyInstaller", () => { fs.writeFileSync(path.join(workspacePath, ".ruby-version"), RUBY_VERSION); - const windows = new RubyInstaller(workspaceFolder, outputChannel); + const windows = new RubyInstaller( + workspaceFolder, + outputChannel, + async () => {}, + ); const result = ["/fake/dir", "/other/fake/dir", true, RUBY_VERSION].join( ACTIVATION_SEPARATOR, ); diff --git a/vscode/src/test/suite/ruby/rvm.test.ts b/vscode/src/test/suite/ruby/rvm.test.ts index ff6bcba41..a1ff3c5b4 100644 --- a/vscode/src/test/suite/ruby/rvm.test.ts +++ b/vscode/src/test/suite/ruby/rvm.test.ts @@ -26,7 +26,7 @@ suite("RVM", () => { index: 0, }; const outputChannel = new WorkspaceChannel("fake", common.LOG_CHANNEL); - const rvm = new Rvm(workspaceFolder, outputChannel); + const rvm = new Rvm(workspaceFolder, outputChannel, async () => {}); const installationPathStub = sinon .stub(rvm, "findRvmInstallation") diff --git a/vscode/src/test/suite/ruby/shadowenv.test.ts b/vscode/src/test/suite/ruby/shadowenv.test.ts index 8d1666c8e..0ba34ceab 100644 --- a/vscode/src/test/suite/ruby/shadowenv.test.ts +++ b/vscode/src/test/suite/ruby/shadowenv.test.ts @@ -121,7 +121,11 @@ suite("Shadowenv", () => { `(env/prepend-to-pathlist "PATH" "${rubyBinPath}")`, ); - const shadowenv = new Shadowenv(workspaceFolder, outputChannel); + const shadowenv = new Shadowenv( + workspaceFolder, + outputChannel, + async () => {}, + ); const { env, version, yjit } = await shadowenv.activate(); assert.match(env.PATH!, new RegExp(rubyBinPath)); @@ -137,7 +141,11 @@ suite("Shadowenv", () => { shadowLispFile, ); - const shadowenv = new Shadowenv(workspaceFolder, outputChannel); + const shadowenv = new Shadowenv( + workspaceFolder, + outputChannel, + async () => {}, + ); const { env, version, yjit } = await shadowenv.activate(); assert.match(env.PATH!, new RegExp(rubyBinPath)); @@ -159,7 +167,11 @@ suite("Shadowenv", () => { .stub(vscode.window, "showErrorMessage") .resolves("Trust workspace" as any); - const shadowenv = new Shadowenv(workspaceFolder, outputChannel); + const shadowenv = new Shadowenv( + workspaceFolder, + outputChannel, + async () => {}, + ); const { env, version, yjit } = await shadowenv.activate(); assert.match(env.PATH!, new RegExp(rubyBinPath)); @@ -185,7 +197,11 @@ suite("Shadowenv", () => { .stub(vscode.window, "showErrorMessage") .resolves("Cancel" as any); - const shadowenv = new Shadowenv(workspaceFolder, outputChannel); + const shadowenv = new Shadowenv( + workspaceFolder, + outputChannel, + async () => {}, + ); await assert.rejects(async () => { await shadowenv.activate(); @@ -204,7 +220,11 @@ suite("Shadowenv", () => { shadowLispFile, ); - const shadowenv = new Shadowenv(workspaceFolder, outputChannel); + const shadowenv = new Shadowenv( + workspaceFolder, + outputChannel, + async () => {}, + ); // First, reject the call to `shadowenv exec`. Then resolve the call to `which shadowenv` to return nothing const execStub = sinon From a738d5ba7318dfb9bc29d6b96b8534ad60310de6 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 20 Nov 2024 14:00:30 -0500 Subject: [PATCH 23/27] Start returning composed bundle environment from initialize request (#2880) ### Motivation Currently, we are always spawning the debug client with the incorrect environment. The reason is because the composed bundle process does a lot of heavy lifting to determine the exact environment variables that need to be set to apply an accurate Bundler configuration. We need the debug client to have access to the composed environment if we want to be able to launch the debugger properly for all scenarios. ### Implementation Started reading the bundle env and returning it as part of the initialize request. ### Automated Tests Added a test to verify that we're returning the environment. --- exe/ruby-lsp-launcher | 3 --- lib/ruby_lsp/internal.rb | 1 + lib/ruby_lsp/server.rb | 8 ++++++++ project-words | 1 + test/server_test.rb | 23 +++++++++++++++++++++++ 5 files changed, 33 insertions(+), 3 deletions(-) diff --git a/exe/ruby-lsp-launcher b/exe/ruby-lsp-launcher index 44b915421..a89095c71 100755 --- a/exe/ruby-lsp-launcher +++ b/exe/ruby-lsp-launcher @@ -74,9 +74,6 @@ rescue StandardError => e # If Bundler.setup fails, we need to restore the original $LOAD_PATH so that we can still require the Ruby LSP server # in degraded mode $LOAD_PATH.unshift(File.expand_path("../lib", __dir__)) -ensure - require "fileutils" - FileUtils.rm(bundle_env_path) if File.exist?(bundle_env_path) end error_path = File.join(".ruby-lsp", "install_error") diff --git a/lib/ruby_lsp/internal.rb b/lib/ruby_lsp/internal.rb index 78823fcfe..540e93f08 100644 --- a/lib/ruby_lsp/internal.rb +++ b/lib/ruby_lsp/internal.rb @@ -21,6 +21,7 @@ require "prism/visitor" require "language_server-protocol" require "rbs" +require "fileutils" require "ruby-lsp" require "ruby_lsp/base_server" diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index d0c24451b..5f939190c 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -216,6 +216,13 @@ def run_initialize(message) Hash.new(true) end + bundle_env_path = File.join(".ruby-lsp", "bundle_env") + bundle_env = if File.exist?(bundle_env_path) + env = File.readlines(bundle_env_path).to_h { |line| T.cast(line.chomp.split("=", 2), [String, String]) } + FileUtils.rm(bundle_env_path) + env + end + document_symbol_provider = Requests::DocumentSymbol.provider if enabled_features["documentSymbols"] document_link_provider = Requests::DocumentLink.provider if enabled_features["documentLink"] code_lens_provider = Requests::CodeLens.provider if enabled_features["codeLens"] @@ -269,6 +276,7 @@ def run_initialize(message) }, formatter: @global_state.formatter, degraded_mode: !!(@install_error || @setup_error), + bundle_env: bundle_env, } send_message(Result.new(id: message[:id], response: response)) diff --git a/project-words b/project-words index 5f182c097..47ceaf33c 100644 --- a/project-words +++ b/project-words @@ -58,6 +58,7 @@ quxx quux qorge rdbg +readlines realpath reparsing requireds diff --git a/test/server_test.rb b/test/server_test.rb index 44e469624..bbeed3e0b 100644 --- a/test/server_test.rb +++ b/test/server_test.rb @@ -33,6 +33,29 @@ def test_initialize_enabled_features_with_array assert_includes(capabilities, "semanticTokensProvider") end + def test_initialize_returns_bundle_env + bundle_env_path = File.join(".ruby-lsp", "bundle_env") + FileUtils.mkdir(".ruby-lsp") unless File.exist?(".ruby-lsp") + File.write(bundle_env_path, "BUNDLE_PATH=vendor/bundle") + @server.process_message({ + id: 1, + method: "initialize", + params: { + initializationOptions: { enabledFeatures: [] }, + capabilities: { general: { positionEncodings: ["utf-8"] } }, + }, + }) + + result = find_message(RubyLsp::Result, id: 1) + hash = JSON.parse(result.response.to_json) + + begin + assert_equal({ "BUNDLE_PATH" => "vendor/bundle" }, hash["bundle_env"]) + ensure + FileUtils.rm(bundle_env_path) if File.exist?(bundle_env_path) + end + end + def test_initialize_enabled_features_with_hash capture_subprocess_io do @server.process_message({ From f8cdbfad353cf33884b6d8d1ad230908054438f6 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 20 Nov 2024 14:55:17 -0500 Subject: [PATCH 24/27] Allow indexing enhancements to create namespaces (#2857) ### Motivation We realized that in order to support concerns `class_methods do...end`, indexing enhancements needed to be more powerful and have the liberty to create fake/temporary namespaces. When trying to make that possible, we also realized that the API had many shortcomings that made enhancements harder than they had to be. ### Implementation The idea is to instantiate enhancements with the declaration listener and then expose the API from it. That way, the declaration listener can hold important state like the code units cache and the current file path and enhancements only make flow adjustments and additions to the indexing process. This PR also allows enhancements to create modules and classes, which allows us to support `class_methods do...end` in the Rails add-on and allows the RSpec add-on to support `let` and `subject` properly. ### Automated Tests Added tests. --- jekyll/add-ons.markdown | 30 +-- .../lib/ruby_indexer/declaration_listener.rb | 161 +++++++++++----- .../lib/ruby_indexer/enhancement.rb | 59 +++--- lib/ruby_indexer/lib/ruby_indexer/index.rb | 10 - lib/ruby_indexer/test/enhancements_test.rb | 172 ++++++++++++++---- 5 files changed, 288 insertions(+), 144 deletions(-) diff --git a/jekyll/add-ons.markdown b/jekyll/add-ons.markdown index d4451cc4d..ee51f875c 100644 --- a/jekyll/add-ons.markdown +++ b/jekyll/add-ons.markdown @@ -271,15 +271,11 @@ This is how you could write an enhancement to teach the Ruby LSP to understand t class MyIndexingEnhancement < RubyIndexer::Enhancement # This on call node handler is invoked any time during indexing when we find a method call. It can be used to insert # more entries into the index depending on the conditions - def on_call_node_enter(owner, node, file_path, code_units_cache) - return unless owner + def on_call_node_enter(node) + return unless @listener.current_owner - # Get the ancestors of the current class - ancestors = @index.linearized_ancestors_of(owner.name) - - # Return early unless the method call is the one we want to handle and the class invoking the DSL inherits from - # our library's parent class - return unless node.name == :my_dsl_that_creates_methods && ancestors.include?("MyLibrary::ParentClass") + # Return early unless the method call is the one we want to handle + return unless node.name == :my_dsl_that_creates_methods # Create a new entry to be inserted in the index. This entry will represent the declaration that is created via # meta-programming. All entries are defined in the `entry.rb` file. @@ -293,24 +289,16 @@ class MyIndexingEnhancement < RubyIndexer::Enhancement RubyIndexer::Entry::Signature.new([RubyIndexer::Entry::RequiredParameter.new(name: :a)]) ] - new_entry = RubyIndexer::Entry::Method.new( - "new_method", # The name of the method that gets created via meta-programming - file_path, # The file_path where the DSL call was found. This should always just be the file_path received - location, # The Prism node location where the DSL call was found - location, # The Prism node location for the DSL name location. May or not be the same - nil, # The documentation for this DSL call. This should always be `nil` to ensure lazy fetching of docs - signatures, # All signatures for this method (every way it can be invoked) - RubyIndexer::Entry::Visibility::PUBLIC, # The method's visibility - owner, # The method's owner. This is almost always going to be the same owner received + @listener.add_method( + "new_method", # Name of the method + location, # Prism location for the node defining this method + signatures # Signatures available to invoke this method ) - - # Push the new entry to the index - @index.add(new_entry) end # This method is invoked when the parser has finished processing the method call node. # It can be used to perform cleanups like popping a stack...etc. - def on_call_node_leave(owner, node, file_path, code_units_cache); end + def on_call_node_leave(node); end end ``` diff --git a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb index 0aa89ea66..4d977e317 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb @@ -18,13 +18,12 @@ class DeclarationListener parse_result: Prism::ParseResult, file_path: String, collect_comments: T::Boolean, - enhancements: T::Array[Enhancement], ).void end - def initialize(index, dispatcher, parse_result, file_path, collect_comments: false, enhancements: []) + def initialize(index, dispatcher, parse_result, file_path, collect_comments: false) @index = index @file_path = file_path - @enhancements = enhancements + @enhancements = T.let(Enhancement.all(self), T::Array[Enhancement]) @visibility_stack = T.let([Entry::Visibility::PUBLIC], T::Array[Entry::Visibility]) @comments_by_line = T.let( parse_result.comments.to_h do |c| @@ -86,15 +85,9 @@ def initialize(index, dispatcher, parse_result, file_path, collect_comments: fal sig { params(node: Prism::ClassNode).void } def on_class_node_enter(node) - @visibility_stack.push(Entry::Visibility::PUBLIC) constant_path = node.constant_path - name = constant_path.slice - - comments = collect_comments(node) - superclass = node.superclass - - nesting = actual_nesting(name) + nesting = actual_nesting(constant_path.slice) parent_class = case superclass when Prism::ConstantReadNode, Prism::ConstantPathNode @@ -113,53 +106,29 @@ def on_class_node_enter(node) end end - entry = Entry::Class.new( + add_class( nesting, - @file_path, - Location.from_prism_location(node.location, @code_units_cache), - Location.from_prism_location(constant_path.location, @code_units_cache), - comments, - parent_class, + node.location, + constant_path.location, + parent_class_name: parent_class, + comments: collect_comments(node), ) - - @owner_stack << entry - @index.add(entry) - @stack << name end sig { params(node: Prism::ClassNode).void } def on_class_node_leave(node) - @stack.pop - @owner_stack.pop - @visibility_stack.pop + pop_namespace_stack end sig { params(node: Prism::ModuleNode).void } def on_module_node_enter(node) - @visibility_stack.push(Entry::Visibility::PUBLIC) constant_path = node.constant_path - name = constant_path.slice - - comments = collect_comments(node) - - entry = Entry::Module.new( - actual_nesting(name), - @file_path, - Location.from_prism_location(node.location, @code_units_cache), - Location.from_prism_location(constant_path.location, @code_units_cache), - comments, - ) - - @owner_stack << entry - @index.add(entry) - @stack << name + add_module(constant_path.slice, node.location, constant_path.location, comments: collect_comments(node)) end sig { params(node: Prism::ModuleNode).void } def on_module_node_leave(node) - @stack.pop - @owner_stack.pop - @visibility_stack.pop + pop_namespace_stack end sig { params(node: Prism::SingletonClassNode).void } @@ -201,9 +170,7 @@ def on_singleton_class_node_enter(node) sig { params(node: Prism::SingletonClassNode).void } def on_singleton_class_node_leave(node) - @stack.pop - @owner_stack.pop - @visibility_stack.pop + pop_namespace_stack end sig { params(node: Prism::MultiWriteNode).void } @@ -318,7 +285,7 @@ def on_call_node_enter(node) end @enhancements.each do |enhancement| - enhancement.on_call_node_enter(@owner_stack.last, node, @file_path, @code_units_cache) + enhancement.on_call_node_enter(node) rescue StandardError => e @indexing_errors << <<~MSG Indexing error in #{@file_path} with '#{enhancement.class.name}' on call node enter enhancement: #{e.message} @@ -339,7 +306,7 @@ def on_call_node_leave(node) end @enhancements.each do |enhancement| - enhancement.on_call_node_leave(@owner_stack.last, node, @file_path, @code_units_cache) + enhancement.on_call_node_leave(node) rescue StandardError => e @indexing_errors << <<~MSG Indexing error in #{@file_path} with '#{enhancement.class.name}' on call node leave enhancement: #{e.message} @@ -464,6 +431,98 @@ def on_alias_method_node_enter(node) ) end + sig do + params( + name: String, + node_location: Prism::Location, + signatures: T::Array[Entry::Signature], + visibility: Entry::Visibility, + comments: T.nilable(String), + ).void + end + def add_method(name, node_location, signatures, visibility: Entry::Visibility::PUBLIC, comments: nil) + location = Location.from_prism_location(node_location, @code_units_cache) + + @index.add(Entry::Method.new( + name, + @file_path, + location, + location, + comments, + signatures, + visibility, + @owner_stack.last, + )) + end + + sig do + params( + name: String, + full_location: Prism::Location, + name_location: Prism::Location, + comments: T.nilable(String), + ).void + end + def add_module(name, full_location, name_location, comments: nil) + location = Location.from_prism_location(full_location, @code_units_cache) + name_loc = Location.from_prism_location(name_location, @code_units_cache) + + entry = Entry::Module.new( + actual_nesting(name), + @file_path, + location, + name_loc, + comments, + ) + + advance_namespace_stack(name, entry) + end + + sig do + params( + name_or_nesting: T.any(String, T::Array[String]), + full_location: Prism::Location, + name_location: Prism::Location, + parent_class_name: T.nilable(String), + comments: T.nilable(String), + ).void + end + def add_class(name_or_nesting, full_location, name_location, parent_class_name: nil, comments: nil) + nesting = name_or_nesting.is_a?(Array) ? name_or_nesting : actual_nesting(name_or_nesting) + entry = Entry::Class.new( + nesting, + @file_path, + Location.from_prism_location(full_location, @code_units_cache), + Location.from_prism_location(name_location, @code_units_cache), + comments, + parent_class_name, + ) + + advance_namespace_stack(T.must(nesting.last), entry) + end + + sig { params(block: T.proc.params(index: Index, base: Entry::Namespace).void).void } + def register_included_hook(&block) + owner = @owner_stack.last + return unless owner + + @index.register_included_hook(owner.name) do |index, base| + block.call(index, base) + end + end + + sig { void } + def pop_namespace_stack + @stack.pop + @owner_stack.pop + @visibility_stack.pop + end + + sig { returns(T.nilable(Entry::Namespace)) } + def current_owner + @owner_stack.last + end + private sig do @@ -921,5 +980,13 @@ def actual_nesting(name) corrected_nesting end + + sig { params(short_name: String, entry: Entry::Namespace).void } + def advance_namespace_stack(short_name, entry) + @visibility_stack.push(Entry::Visibility::PUBLIC) + @owner_stack << entry + @index.add(entry) + @stack << short_name + end end end diff --git a/lib/ruby_indexer/lib/ruby_indexer/enhancement.rb b/lib/ruby_indexer/lib/ruby_indexer/enhancement.rb index a654b87e8..2d4fef3f7 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/enhancement.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/enhancement.rb @@ -8,38 +8,41 @@ class Enhancement abstract! - sig { params(index: Index).void } - def initialize(index) - @index = index + @enhancements = T.let([], T::Array[T::Class[Enhancement]]) + + class << self + extend T::Sig + + sig { params(child: T::Class[Enhancement]).void } + def inherited(child) + @enhancements << child + super + end + + sig { params(listener: DeclarationListener).returns(T::Array[Enhancement]) } + def all(listener) + @enhancements.map { |enhancement| enhancement.new(listener) } + end + + # Only available for testing purposes + sig { void } + def clear + @enhancements.clear + end + end + + sig { params(listener: DeclarationListener).void } + def initialize(listener) + @listener = listener end # The `on_extend` indexing enhancement is invoked whenever an extend is encountered in the code. It can be used to # register for an included callback, similar to what `ActiveSupport::Concern` does in order to auto-extend the # `ClassMethods` modules - sig do - overridable.params( - owner: T.nilable(Entry::Namespace), - node: Prism::CallNode, - file_path: String, - code_units_cache: T.any( - T.proc.params(arg0: Integer).returns(Integer), - Prism::CodeUnitsCache, - ), - ).void - end - def on_call_node_enter(owner, node, file_path, code_units_cache); end - - sig do - overridable.params( - owner: T.nilable(Entry::Namespace), - node: Prism::CallNode, - file_path: String, - code_units_cache: T.any( - T.proc.params(arg0: Integer).returns(Integer), - Prism::CodeUnitsCache, - ), - ).void - end - def on_call_node_leave(owner, node, file_path, code_units_cache); end + sig { overridable.params(node: Prism::CallNode).void } + def on_call_node_enter(node); end # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod + + sig { overridable.params(node: Prism::CallNode).void } + def on_call_node_leave(node); end # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod end end diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb index ee09c773b..f46be2756 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/index.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/index.rb @@ -40,9 +40,6 @@ def initialize # Holds the linearized ancestors list for every namespace @ancestors = T.let({}, T::Hash[String, T::Array[String]]) - # List of classes that are enhancing the index - @enhancements = T.let([], T::Array[Enhancement]) - # Map of module name to included hooks that have to be executed when we include the given module @included_hooks = T.let( {}, @@ -52,12 +49,6 @@ def initialize @configuration = T.let(RubyIndexer::Configuration.new, Configuration) end - # Register an enhancement to the index. Enhancements must conform to the `Enhancement` interface - sig { params(enhancement: Enhancement).void } - def register_enhancement(enhancement) - @enhancements << enhancement - end - # Register an included `hook` that will be executed when `module_name` is included into any namespace sig { params(module_name: String, hook: T.proc.params(index: Index, base: Entry::Namespace).void).void } def register_included_hook(module_name, &hook) @@ -396,7 +387,6 @@ def index_single(indexable_path, source = nil, collect_comments: true) result, indexable_path.full_path, collect_comments: collect_comments, - enhancements: @enhancements, ) dispatcher.dispatch(result.value) diff --git a/lib/ruby_indexer/test/enhancements_test.rb b/lib/ruby_indexer/test/enhancements_test.rb index 029bf81b8..6086eb9be 100644 --- a/lib/ruby_indexer/test/enhancements_test.rb +++ b/lib/ruby_indexer/test/enhancements_test.rb @@ -5,24 +5,28 @@ module RubyIndexer class EnhancementTest < TestCase + def teardown + super + Enhancement.clear + end + def test_enhancing_indexing_included_hook - enhancement_class = Class.new(Enhancement) do - def on_call_node_enter(owner, node, file_path, code_units_cache) + Class.new(Enhancement) do + def on_call_node_enter(call_node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod + owner = @listener.current_owner return unless owner - return unless node.name == :extend + return unless call_node.name == :extend - arguments = node.arguments&.arguments + arguments = call_node.arguments&.arguments return unless arguments - location = Location.from_prism_location(node.location, code_units_cache) - arguments.each do |node| next unless node.is_a?(Prism::ConstantReadNode) || node.is_a?(Prism::ConstantPathNode) module_name = node.full_name next unless module_name == "ActiveSupport::Concern" - @index.register_included_hook(owner.name) do |index, base| + @listener.register_included_hook do |index, base| class_methods_name = "#{owner.name}::ClassMethods" if index.indexed?(class_methods_name) @@ -31,16 +35,11 @@ def on_call_node_enter(owner, node, file_path, code_units_cache) end end - @index.add(Entry::Method.new( + @listener.add_method( "new_method", - file_path, - location, - location, - nil, + call_node.location, [Entry::Signature.new([Entry::RequiredParameter.new(name: :a)])], - Entry::Visibility::PUBLIC, - owner, - )) + ) rescue Prism::ConstantPathNode::DynamicPartsInConstantPathError, Prism::ConstantPathNode::MissingNodesInConstantPathError # Do nothing @@ -48,7 +47,6 @@ def on_call_node_enter(owner, node, file_path, code_units_cache) end end - @index.register_enhancement(enhancement_class.new(@index)) index(<<~RUBY) module ActiveSupport module Concern @@ -96,9 +94,9 @@ class User < ActiveRecord::Base end def test_enhancing_indexing_configuration_dsl - enhancement_class = Class.new(Enhancement) do - def on_call_node_enter(owner, node, file_path, code_units_cache) - return unless owner + Class.new(Enhancement) do + def on_call_node_enter(node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod + return unless @listener.current_owner name = node.name return unless name == :has_many @@ -109,22 +107,14 @@ def on_call_node_enter(owner, node, file_path, code_units_cache) association_name = arguments.first return unless association_name.is_a?(Prism::SymbolNode) - location = Location.from_prism_location(association_name.location, code_units_cache) - - @index.add(Entry::Method.new( + @listener.add_method( T.must(association_name.value), - file_path, - location, - location, - nil, + association_name.location, [], - Entry::Visibility::PUBLIC, - owner, - )) + ) end end - @index.register_enhancement(enhancement_class.new(@index)) index(<<~RUBY) module ActiveSupport module Concern @@ -157,8 +147,8 @@ class User < ActiveRecord::Base end def test_error_handling_in_on_call_node_enter_enhancement - enhancement_class = Class.new(Enhancement) do - def on_call_node_enter(owner, node, file_path, code_units_cache) + Class.new(Enhancement) do + def on_call_node_enter(node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod raise "Error" end @@ -169,8 +159,6 @@ def name end end - @index.register_enhancement(enhancement_class.new(@index)) - _stdout, stderr = capture_io do index(<<~RUBY) module ActiveSupport @@ -192,8 +180,8 @@ def self.extended(base) end def test_error_handling_in_on_call_node_leave_enhancement - enhancement_class = Class.new(Enhancement) do - def on_call_node_leave(owner, node, file_path, code_units_cache) + Class.new(Enhancement) do + def on_call_node_leave(node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod raise "Error" end @@ -204,8 +192,6 @@ def name end end - @index.register_enhancement(enhancement_class.new(@index)) - _stdout, stderr = capture_io do index(<<~RUBY) module ActiveSupport @@ -225,5 +211,115 @@ def self.extended(base) # The module should still be indexed assert_entry("ActiveSupport::Concern", Entry::Module, "/fake/path/foo.rb:1-2:5-5") end + + def test_advancing_namespace_stack_from_enhancement + Class.new(Enhancement) do + def on_call_node_enter(call_node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod + owner = @listener.current_owner + return unless owner + + case call_node.name + when :class_methods + @listener.add_module("ClassMethods", call_node.location, call_node.location) + when :extend + arguments = call_node.arguments&.arguments + return unless arguments + + arguments.each do |node| + next unless node.is_a?(Prism::ConstantReadNode) || node.is_a?(Prism::ConstantPathNode) + + module_name = node.full_name + next unless module_name == "ActiveSupport::Concern" + + @listener.register_included_hook do |index, base| + class_methods_name = "#{owner.name}::ClassMethods" + + if index.indexed?(class_methods_name) + singleton = index.existing_or_new_singleton_class(base.name) + singleton.mixin_operations << Entry::Include.new(class_methods_name) + end + end + end + end + end + + def on_call_node_leave(call_node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod + return unless call_node.name == :class_methods + + @listener.pop_namespace_stack + end + end + + index(<<~RUBY) + module ActiveSupport + module Concern + end + end + + module MyConcern + extend ActiveSupport::Concern + + class_methods do + def foo; end + end + end + + class User + include MyConcern + end + RUBY + + assert_equal( + [ + "User::", + "MyConcern::ClassMethods", + "Object::", + "BasicObject::", + "Class", + "Module", + "Object", + "Kernel", + "BasicObject", + ], + @index.linearized_ancestors_of("User::"), + ) + + refute_nil(@index.resolve_method("foo", "User::")) + end + + def test_creating_anonymous_classes_from_enhancement + Class.new(Enhancement) do + def on_call_node_enter(call_node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod + case call_node.name + when :context + arguments = call_node.arguments&.arguments + first_argument = arguments&.first + return unless first_argument.is_a?(Prism::StringNode) + + @listener.add_class( + "", + call_node.location, + first_argument.location, + ) + when :subject + @listener.add_method("subject", call_node.location, []) + end + end + + def on_call_node_leave(call_node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod + return unless call_node.name == :context + + @listener.pop_namespace_stack + end + end + + index(<<~RUBY) + context "does something" do + subject { call_whatever } + end + RUBY + + refute_nil(@index.resolve_method("subject", "")) + end end end From 51e76fabfc836256b03e9936947ae8b89877a688 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 20 Nov 2024 15:02:32 -0500 Subject: [PATCH 25/27] Bump version to v0.22.0 --- Gemfile.lock | 2 +- VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index fe1d83deb..1a08173e9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - ruby-lsp (0.21.4) + ruby-lsp (0.22.0) language_server-protocol (~> 3.17.0) prism (>= 1.2, < 2.0) rbs (>= 3, < 4) diff --git a/VERSION b/VERSION index 6aec9e544..215740905 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.21.4 +0.22.0 From dd3968cd6a6185dc9143473c833883a6e3cbb10d Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 20 Nov 2024 15:08:30 -0500 Subject: [PATCH 26/27] Merge composed bundle environment into Ruby object (#2881) ### Motivation Closes #1767, closes #1785 Merge the composed bundle environment into the workspace's Ruby object. The language server sets up the composed bundle environment, returns it, and then we merge that into the Ruby object to ensure that any other parts of the extension are using the exact same environment. This fixes a few issues people have been experiencing with the debug client. ### Implementation Started merging the composed environment into the Ruby object and then started relying on that for the debug client. ### Automated Tests Added some tests. I will follow up with another PR that creates an integration test for a scenario that currently fails. --- vscode/src/client.ts | 4 ++++ vscode/src/debugger.ts | 6 ++++++ vscode/src/ruby.ts | 4 ++++ vscode/src/test/suite/ruby.test.ts | 17 +++++++++++++++++ 4 files changed, 31 insertions(+) diff --git a/vscode/src/client.ts b/vscode/src/client.ts index 594ce633c..114b385f2 100644 --- a/vscode/src/client.ts +++ b/vscode/src/client.ts @@ -389,6 +389,10 @@ export default class Client extends LanguageClient implements ClientInterface { this.degraded = this.initializeResult?.degraded_mode; } + if (this.initializeResult?.bundle_env) { + this.ruby.mergeComposedEnvironment(this.initializeResult.bundle_env); + } + await this.fetchAddons(); } diff --git a/vscode/src/debugger.ts b/vscode/src/debugger.ts index 12e7547dc..4b39681cf 100644 --- a/vscode/src/debugger.ts +++ b/vscode/src/debugger.ts @@ -127,6 +127,12 @@ export class Debugger name: workspace.workspaceFolder.name, }; + // In newer versions of the server, the composed bundle environment is merged directly into the Ruby object and no + // adjustments have to be made here + if (debugConfiguration.env.BUNDLE_GEMFILE) { + return debugConfiguration; + } + const customBundleUri = vscode.Uri.joinPath(workspaceUri, ".ruby-lsp"); return vscode.workspace.fs.readDirectory(customBundleUri).then( diff --git a/vscode/src/ruby.ts b/vscode/src/ruby.ts index fad69bb63..bf0d6c8e7 100644 --- a/vscode/src/ruby.ts +++ b/vscode/src/ruby.ts @@ -218,6 +218,10 @@ export class Ruby implements RubyInterface { return this.activateRuby(); } + mergeComposedEnvironment(env: Record) { + this._env = { ...this._env, ...env }; + } + private async runActivation(manager: VersionManager) { const { env, version, yjit, gemPath } = await manager.activate(); const [major, minor, _patch] = version.split(".").map(Number); diff --git a/vscode/src/test/suite/ruby.test.ts b/vscode/src/test/suite/ruby.test.ts index b3832b439..e1c4abb34 100644 --- a/vscode/src/test/suite/ruby.test.ts +++ b/vscode/src/test/suite/ruby.test.ts @@ -150,4 +150,21 @@ suite("Ruby environment activation", () => { "/opt/rubies/3.3.5/lib/ruby/gems/3.3.0", ]); }); + + test("mergeComposedEnv merges environment variables", () => { + const ruby = new Ruby( + context, + workspaceFolder, + outputChannel, + FAKE_TELEMETRY, + ); + + assert.deepStrictEqual(ruby.env, {}); + + ruby.mergeComposedEnvironment({ + BUNDLE_GEMFILE: ".ruby-lsp/Gemfile", + }); + + assert.deepStrictEqual(ruby.env, { BUNDLE_GEMFILE: ".ruby-lsp/Gemfile" }); + }); }); From fc2d91cfaba708ee380aafb6fe0a86e80ad0b00d Mon Sep 17 00:00:00 2001 From: "shopify-github-actions-access[bot]" <109624739+shopify-github-actions-access[bot]@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:41:02 -0500 Subject: [PATCH 27/27] Bump version to v0.22.1 (#2888) Co-authored-by: github-actions --- Gemfile.lock | 2 +- VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 1a08173e9..05aa91624 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - ruby-lsp (0.22.0) + ruby-lsp (0.22.1) language_server-protocol (~> 3.17.0) prism (>= 1.2, < 2.0) rbs (>= 3, < 4) diff --git a/VERSION b/VERSION index 215740905..a723ece79 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.22.0 +0.22.1