From 6a701eae42385eb54b47a211ac3e3ab0083703db Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Mon, 19 Aug 2024 16:19:00 -0400 Subject: [PATCH] Always use workspace URI instead of Dir.pwd --- .../lib/ruby_indexer/configuration.rb | 37 ++++++--- lib/ruby_indexer/test/configuration_test.rb | 48 +++++++++-- lib/ruby_lsp/listeners/definition.rb | 2 +- lib/ruby_lsp/server.rb | 6 +- lib/ruby_lsp/setup_bundler.rb | 6 +- sorbet/rbi/shims/file.rbi | 8 ++ test/setup_bundler_test.rb | 81 +++++++++---------- 7 files changed, 121 insertions(+), 67 deletions(-) create mode 100644 sorbet/rbi/shims/file.rbi diff --git a/lib/ruby_indexer/lib/ruby_indexer/configuration.rb b/lib/ruby_indexer/lib/ruby_indexer/configuration.rb index a3377fbf8..330c64b70 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/configuration.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/configuration.rb @@ -16,15 +16,24 @@ class Configuration T::Hash[String, T.untyped], ) + sig { params(workspace_path: String).void } + attr_writer :workspace_path + sig { void } def initialize + @workspace_path = T.let(Dir.pwd, String) @excluded_gems = T.let(initial_excluded_gems, T::Array[String]) @included_gems = T.let([], T::Array[String]) - @excluded_patterns = T.let([File.join("**", "*_test.rb"), File.join("**", "tmp", "**", "*")], T::Array[String]) + @excluded_patterns = T.let([File.join("**", "*_test.rb"), File.join("tmp", "**", "*")], T::Array[String]) + path = Bundler.settings["path"] - @excluded_patterns << File.join(File.expand_path(path, Dir.pwd), "**", "*.rb") if path + if path + # Substitute Windows backslashes into forward slashes, which are used in glob patterns + glob = path.gsub(/[\\]+/, "/") + @excluded_patterns << File.join(glob, "**", "*.rb") + end - @included_patterns = T.let([File.join(Dir.pwd, "**", "*.rb")], T::Array[String]) + @included_patterns = T.let([File.join("**", "*.rb")], T::Array[String]) @excluded_magic_comments = T.let( [ "frozen_string_literal:", @@ -55,12 +64,12 @@ def indexables indexables = @included_patterns.flat_map do |pattern| load_path_entry = T.let(nil, T.nilable(String)) - Dir.glob(pattern, File::FNM_PATHNAME | File::FNM_EXTGLOB).map! do |path| + Dir.glob(File.join(@workspace_path, pattern), File::FNM_PATHNAME | File::FNM_EXTGLOB).map! do |path| path = File.expand_path(path) # All entries for the same pattern match the same $LOAD_PATH entry. Since searching the $LOAD_PATH for every # entry is expensive, we memoize it until we find a path that doesn't belong to that $LOAD_PATH. This happens - # on repositories that define multiple gems, like Rails. All frameworks are defined inside the Dir.pwd, but - # each one of them belongs to a different $LOAD_PATH entry + # on repositories that define multiple gems, like Rails. All frameworks are defined inside the current + # workspace directory, but each one of them belongs to a different $LOAD_PATH entry if load_path_entry.nil? || !path.start_with?(load_path_entry) load_path_entry = $LOAD_PATH.find { |load_path| path.start_with?(load_path) } end @@ -69,9 +78,19 @@ def indexables end end + # If the patterns are relative, we make it relative to the workspace path. If they are absolute, then we shouldn't + # concatenate anything + excluded_patterns = @excluded_patterns.map do |pattern| + if File.absolute_path?(pattern) + pattern + else + File.join(@workspace_path, pattern) + end + end + # Remove user specified patterns indexables.reject! do |indexable| - @excluded_patterns.any? do |pattern| + excluded_patterns.any? do |pattern| File.fnmatch?(pattern, indexable.full_path, File::FNM_PATHNAME | File::FNM_EXTGLOB) end end @@ -122,7 +141,7 @@ def indexables # When working on a gem, it will be included in the locked_gems list. Since these are the project's own files, # we have already included and handled exclude patterns for it and should not re-include or it'll lead to # duplicates or accidentally ignoring exclude patterns - next if spec.full_gem_path == Dir.pwd + next if spec.full_gem_path == @workspace_path indexables.concat( spec.require_paths.flat_map do |require_path| @@ -185,7 +204,7 @@ def initial_excluded_gems # If the dependency is prerelease, `to_spec` may return `nil` due to a bug in older version of Bundler/RubyGems: # https://github.com/Shopify/ruby-lsp/issues/1246 this_gem = Bundler.definition.dependencies.find do |d| - d.to_spec&.full_gem_path == Dir.pwd + d.to_spec&.full_gem_path == @workspace_path rescue Gem::MissingSpecError false end diff --git a/lib/ruby_indexer/test/configuration_test.rb b/lib/ruby_indexer/test/configuration_test.rb index 9ba72f9a3..2078571c0 100644 --- a/lib/ruby_indexer/test/configuration_test.rb +++ b/lib/ruby_indexer/test/configuration_test.rb @@ -7,10 +7,12 @@ module RubyIndexer class ConfigurationTest < Minitest::Test def setup @config = Configuration.new + @workspace_path = File.expand_path(File.join("..", "..", ".."), __dir__) + @config.workspace_path = @workspace_path end def test_load_configuration_executes_configure_block - @config.apply_config({ "excluded_patterns" => ["**/test/fixtures/**/*.rb"] }) + @config.apply_config({ "excluded_patterns" => ["**/fixtures/**/*.rb"] }) indexables = @config.indexables assert(indexables.none? { |indexable| indexable.full_path.include?("test/fixtures") }) @@ -25,7 +27,7 @@ def test_indexables_have_expanded_full_paths indexables = @config.indexables # All paths should be expanded - assert(indexables.none? { |indexable| indexable.full_path.start_with?("lib/") }) + assert(indexables.all? { |indexable| File.absolute_path?(indexable.full_path) }) end def test_indexables_only_includes_gem_require_paths @@ -71,17 +73,20 @@ def test_indexables_avoids_duplicates_if_bundle_path_is_inside_project Bundler.settings.temporary(path: "vendor/bundle") do config = Configuration.new - assert_includes(config.instance_variable_get(:@excluded_patterns), "#{Dir.pwd}/vendor/bundle/**/*.rb") + assert_includes(config.instance_variable_get(:@excluded_patterns), "vendor/bundle/**/*.rb") end end def test_indexables_does_not_include_gems_own_installed_files indexables = @config.indexables + indexables_inside_bundled_lsp = indexables.select do |indexable| + indexable.full_path.start_with?(Bundler.bundle_path.join("gems", "ruby-lsp").to_s) + end - assert( - indexables.none? do |indexable| - indexable.full_path.start_with?(Bundler.bundle_path.join("gems", "ruby-lsp").to_s) - end, + assert_empty( + indexables_inside_bundled_lsp, + "Indexables should not include files from the gem currently being worked on. " \ + "Included: #{indexables_inside_bundled_lsp.map(&:full_path)}", ) end @@ -126,5 +131,34 @@ def test_magic_comments_regex assert_match(regex, comment) end end + + def test_indexables_respect_given_workspace_path + Dir.mktmpdir do |dir| + FileUtils.mkdir(File.join(dir, "ignore")) + FileUtils.touch(File.join(dir, "ignore", "file0.rb")) + FileUtils.touch(File.join(dir, "file1.rb")) + FileUtils.touch(File.join(dir, "file2.rb")) + + @config.apply_config({ "excluded_patterns" => ["ignore/**/*.rb"] }) + @config.workspace_path = dir + indexables = @config.indexables + + assert(indexables.none? { |indexable| indexable.full_path.start_with?(File.join(dir, "ignore")) }) + + # After switching the workspace path, all indexables will be found in one of these places: + # - The new workspace path + # - The Ruby LSP's own code (because Bundler is requiring the dependency from source) + # - Bundled gems + # - Default gems + assert( + indexables.all? do |i| + i.full_path.start_with?(dir) || + i.full_path.start_with?(File.join(Dir.pwd, "lib")) || + i.full_path.start_with?(Bundler.bundle_path.to_s) || + i.full_path.start_with?(RbConfig::CONFIG["rubylibdir"]) + end, + ) + end + end end end diff --git a/lib/ruby_lsp/listeners/definition.rb b/lib/ruby_lsp/listeners/definition.rb index 14f98a73e..1a9999677 100644 --- a/lib/ruby_lsp/listeners/definition.rb +++ b/lib/ruby_lsp/listeners/definition.rb @@ -250,7 +250,7 @@ def handle_require_definition(node, message) when :require_relative required_file = "#{node.content}.rb" path = @uri.to_standardized_path - current_folder = path ? Pathname.new(CGI.unescape(path)).dirname : Dir.pwd + current_folder = path ? Pathname.new(CGI.unescape(path)).dirname : @global_state.workspace_path candidate = File.expand_path(File.join(current_folder, required_file)) @response_builder << Interface::Location.new( diff --git a/lib/ruby_lsp/server.rb b/lib/ruby_lsp/server.rb index e38442b18..950ca05e9 100644 --- a/lib/ruby_lsp/server.rb +++ b/lib/ruby_lsp/server.rb @@ -1000,10 +1000,10 @@ def process_indexing_configuration(indexing_options) return unless indexing_options + configuration = @global_state.index.configuration + configuration.workspace_path = @global_state.workspace_path # The index expects snake case configurations, but VS Code standardizes on camel case settings - @global_state.index.configuration.apply_config( - indexing_options.transform_keys { |key| key.to_s.gsub(/([A-Z])/, "_\\1").downcase }, - ) + configuration.apply_config(indexing_options.transform_keys { |key| key.to_s.gsub(/([A-Z])/, "_\\1").downcase }) end end end diff --git a/lib/ruby_lsp/setup_bundler.rb b/lib/ruby_lsp/setup_bundler.rb index 9f1ba0397..b8a5eacc5 100644 --- a/lib/ruby_lsp/setup_bundler.rb +++ b/lib/ruby_lsp/setup_bundler.rb @@ -43,7 +43,7 @@ def initialize(project_path, **options) @gemfile_name = T.let(@gemfile&.basename&.to_s || "Gemfile", String) # Custom bundle paths - @custom_dir = T.let(Pathname.new(".ruby-lsp").expand_path(Dir.pwd), Pathname) + @custom_dir = T.let(Pathname.new(".ruby-lsp").expand_path(@project_path), Pathname) @custom_gemfile = T.let(@custom_dir + @gemfile_name, Pathname) @custom_lockfile = T.let(@custom_dir + (@lockfile&.basename || "Gemfile.lock"), Pathname) @lockfile_hash_path = T.let(@custom_dir + "main_lockfile_hash", Pathname) @@ -183,14 +183,14 @@ def run_bundle_install(bundle_gemfile = @gemfile) # `.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` path = Bundler.settings["path"] - expanded_path = File.expand_path(path, Dir.pwd) if path + expanded_path = File.expand_path(path, @project_path) if path # Use the absolute `BUNDLE_PATH` to prevent accidentally creating unwanted folders under `.ruby-lsp` env = {} env["BUNDLE_GEMFILE"] = bundle_gemfile.to_s env["BUNDLE_PATH"] = expanded_path if expanded_path - local_config_path = File.join(Dir.pwd, ".bundle") + local_config_path = File.join(@project_path, ".bundle") env["BUNDLE_APP_CONFIG"] = local_config_path if Dir.exist?(local_config_path) # If `ruby-lsp` and `debug` (and potentially `ruby-lsp-rails`) are already in the Gemfile, then we shouldn't try diff --git a/sorbet/rbi/shims/file.rbi b/sorbet/rbi/shims/file.rbi new file mode 100644 index 000000000..a00e12209 --- /dev/null +++ b/sorbet/rbi/shims/file.rbi @@ -0,0 +1,8 @@ +# typed: true + +class File + class << self + sig { params(path: String).returns(T::Boolean) } + def absolute_path?(path); end + end +end diff --git a/test/setup_bundler_test.rb b/test/setup_bundler_test.rb index 09b1f2760..afe3d3910 100644 --- a/test/setup_bundler_test.rb +++ b/test/setup_bundler_test.rb @@ -34,16 +34,6 @@ def test_removes_ruby_lsp_folder_if_both_gems_were_added_to_the_bundle FileUtils.rm_r(".ruby-lsp") if Dir.exist?(".ruby-lsp") end - def test_in_a_rails_app_does_nothing_if_ruby_lsp_and_ruby_lsp_rails_and_debug_are_in_the_bundle - Object.any_instance.expects(:system).with(bundle_env, "(bundle check || bundle install) 1>&2").returns(true) - Bundler::LockfileParser.any_instance.expects(:dependencies) - .returns({ "ruby-lsp" => true, "ruby-lsp-rails" => true, "debug" => true }) - run_script - refute_path_exists(".ruby-lsp") - ensure - FileUtils.rm_r(".ruby-lsp") if Dir.exist?(".ruby-lsp") - end - def test_in_a_rails_app_removes_ruby_lsp_folder_if_all_gems_were_added_to_the_bundle Object.any_instance.expects(:system).with(bundle_env, "(bundle check || bundle install) 1>&2").returns(true) Bundler::LockfileParser.any_instance.expects(:dependencies) @@ -57,7 +47,7 @@ def test_in_a_rails_app_removes_ruby_lsp_folder_if_all_gems_were_added_to_the_bu def test_creates_custom_bundle Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(Dir.pwd, ".ruby-lsp/Gemfile"), "(bundle check || bundle install) 1>&2", ).returns(true) Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}).at_least_once @@ -76,7 +66,7 @@ def test_creates_custom_bundle def test_creates_custom_bundle_for_a_rails_app Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(Dir.pwd, ".ruby-lsp/Gemfile"), "(bundle check || bundle install) 1>&2", ).returns(true) @@ -111,7 +101,7 @@ def test_changing_lockfile_causes_custom_bundle_to_be_rebuilt system("bundle install") # Run the script once to generate a custom bundle - run_script + run_script(dir) end end @@ -136,11 +126,11 @@ def test_changing_lockfile_causes_custom_bundle_to_be_rebuilt # 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 Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(dir, ".ruby-lsp/Gemfile"), "(bundle check || bundle install) 1>&2", ).returns(true) Bundler.with_unbundled_env do - run_script + run_script(dir) end end end @@ -160,7 +150,7 @@ def test_does_not_copy_gemfile_lock_when_not_modified system("bundle install") # Run the script once to generate a custom bundle - run_script + run_script(dir) end end @@ -168,7 +158,7 @@ def test_does_not_copy_gemfile_lock_when_not_modified capture_subprocess_io do Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(dir, ".ruby-lsp/Gemfile"), "((bundle check && bundle update ruby-lsp debug) || bundle install) 1>&2", ).returns(true) @@ -176,7 +166,7 @@ def test_does_not_copy_gemfile_lock_when_not_modified Bundler.with_unbundled_env do # Run the script again without having the lockfile modified - run_script + run_script(dir) end end end @@ -197,7 +187,7 @@ def test_does_only_updates_every_4_hours system("bundle install") # Run the script once to generate a custom bundle - run_script + run_script(dir) end end @@ -205,13 +195,13 @@ def test_does_only_updates_every_4_hours capture_subprocess_io do Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(dir, ".ruby-lsp/Gemfile"), "(bundle check || bundle install) 1>&2", ).returns(true) Bundler.with_unbundled_env do # Run the script again without having the lockfile modified - run_script + run_script(dir) end end end @@ -221,7 +211,7 @@ def test_does_only_updates_every_4_hours def test_uses_absolute_bundle_path_for_bundle_install Bundler.settings.temporary(path: "vendor/bundle") do Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(Dir.pwd, ".ruby-lsp/Gemfile"), "(bundle check || bundle install) 1>&2", ).returns(true) Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}).at_least_once @@ -235,14 +225,14 @@ def test_creates_custom_bundle_if_no_gemfile # Create a temporary directory with no Gemfile or Gemfile.lock Dir.mktmpdir do |dir| Dir.chdir(dir) do - bundle_gemfile = Pathname.new(".ruby-lsp").expand_path(Dir.pwd) + "Gemfile" + bundle_gemfile = Pathname.new(".ruby-lsp").expand_path(dir) + "Gemfile" Object.any_instance.expects(:system).with( - bundle_env(bundle_gemfile.to_s), + bundle_env(dir, bundle_gemfile.to_s), "(bundle check || bundle install) 1>&2", ).returns(true) Bundler.with_unbundled_env do - run_script + run_script(dir) end assert_path_exists(".ruby-lsp") @@ -261,7 +251,7 @@ def test_raises_if_bundle_is_not_locked Bundler.with_unbundled_env do assert_raises(RubyLsp::SetupBundler::BundleNotLocked) do - run_script + run_script(dir) end end end @@ -301,9 +291,12 @@ def test_does_nothing_if_both_ruby_lsp_and_debug_are_gemspec_dependencies FileUtils.touch(File.join(dir, "Gemfile.lock")) Bundler.with_unbundled_env do - Object.any_instance.expects(:system).with(bundle_env, "(bundle check || bundle install) 1>&2").returns(true) + Object.any_instance.expects(:system).with( + bundle_env(File.realpath(dir)), + "(bundle check || bundle install) 1>&2", + ).returns(true) Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}) - run_script + run_script(dir) end refute_path_exists(".ruby-lsp") @@ -316,12 +309,12 @@ def test_creates_custom_bundle_with_specified_branch Dir.chdir(dir) do bundle_gemfile = Pathname.new(".ruby-lsp").expand_path(Dir.pwd) + "Gemfile" Object.any_instance.expects(:system).with( - bundle_env(bundle_gemfile.to_s), + bundle_env(dir, bundle_gemfile.to_s), "(bundle check || bundle install) 1>&2", ).returns(true) Bundler.with_unbundled_env do - run_script(branch: "test-branch") + run_script(File.realpath(dir), branch: "test-branch") end assert_path_exists(".ruby-lsp") @@ -346,18 +339,18 @@ def test_install_prerelease_versions_if_experimental_is_true system("bundle install") # Run the script once to generate a custom bundle - run_script + run_script(dir) end end capture_subprocess_io do Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(dir, ".ruby-lsp/Gemfile"), "((bundle check && bundle update ruby-lsp debug --pre) || bundle install) 1>&2", ).returns(true) Bundler.with_unbundled_env do - run_script(experimental: true) + run_script(dir, experimental: true) end end end @@ -371,11 +364,11 @@ def test_returns_bundle_app_config_if_there_is_local_config Bundler.with_unbundled_env do Bundler.settings.temporary(without: "production") do Object.any_instance.expects(:system).with( - bundle_env(bundle_gemfile.to_s), + bundle_env(dir, bundle_gemfile.to_s), "(bundle check || bundle install) 1>&2", ).returns(true) - run_script + run_script(File.realpath(dir)) end end end @@ -393,7 +386,7 @@ def test_custom_bundle_uses_alternative_gemfiles Bundler.with_unbundled_env do capture_subprocess_io do system("bundle install") - run_script + run_script(dir) end end @@ -471,7 +464,7 @@ def test_ensures_lockfile_remotes_are_relative_to_default_gemfile Bundler.with_unbundled_env do capture_subprocess_io do system("bundle install") - run_script + run_script(File.realpath(dir)) end end @@ -500,11 +493,11 @@ def test_ruby_lsp_rails_is_automatically_included_in_rails_apps end Object.any_instance.expects(:system).with( - bundle_env(".ruby-lsp/Gemfile"), + bundle_env(dir, ".ruby-lsp/Gemfile"), "(bundle check || bundle install) 1>&2", ).returns(true) Bundler.with_unbundled_env do - run_script + run_script(dir) end assert_path_exists(".ruby-lsp/Gemfile") @@ -569,7 +562,7 @@ def test_recovers_from_stale_lockfiles LOCKFILE Bundler.with_unbundled_env do - run_script + run_script(dir) end # Verify that the script recovered and re-generated the custom bundle from scratch @@ -584,7 +577,7 @@ def test_recovers_from_stale_lockfiles # This method runs the script and then immediately unloads it. This allows us to make assertions against the effects # of running the script multiple times - def run_script(path = "/fake/project/path", expected_path: nil, **options) + def run_script(path = Dir.pwd, expected_path: nil, **options) bundle_path = T.let(nil, T.nilable(String)) stdout, _stderr = capture_subprocess_io do @@ -595,12 +588,12 @@ def run_script(path = "/fake/project/path", expected_path: nil, **options) assert_equal(expected_path, bundle_path) if expected_path end - def bundle_env(bundle_gemfile = "Gemfile") - bundle_gemfile_path = Pathname.new(bundle_gemfile) + def bundle_env(base_path = Dir.pwd, bundle_gemfile = "Gemfile") + bundle_gemfile_path = Pathname.new(base_path).join(bundle_gemfile) path = Bundler.settings["path"] env = {} - env["BUNDLE_PATH"] = File.expand_path(path, Dir.pwd) if path + env["BUNDLE_PATH"] = File.expand_path(path, base_path) if path env["BUNDLE_GEMFILE"] = bundle_gemfile_path.absolute? ? bundle_gemfile_path.to_s : bundle_gemfile_path.expand_path(Dir.pwd).to_s