Refactor dependency detector #1042

andyw8 · 2023-09-27T17:16:08Z

Motivation

The current DependencyDetector is somewhat awkward to work with, and because it cannot be instantiated, we are repeated several checks unnecessarily.

Implementation

Convert into a class. Move its use up to the executor layer.

Automated Tests

Updated

Manual Tests

Verified locally.

andyw8 · 2023-09-27T18:27:35Z

lib/ruby_lsp/requests/hover.rb

@@ -71,21 +71,21 @@ def merge_response!(other)

      sig { params(node: YARP::ConstantReadNode).void }
      def on_constant_read(node)
-        return if DependencyDetector::HAS_TYPECHECKER
+        return if DependencyDetector.new.typechecker?


Instead of this, we can pass in typechecker? as a boolean from Executor. (and similarly for the other requests).

andyw8 · 2023-09-28T15:40:25Z

lib/ruby_lsp/requests/definition.rb

@@ -26,25 +26,29 @@ class Definition < ExtensibleListener
      sig { override.returns(ResponseType) }
      attr_reader :_response

+      # rubocop:disable Metrics/ParameterLists


After this, we are thinking of introduce some kind of context object to group common parameters together, and reduce the number of things we need to pass around.

andyw8 · 2023-09-28T16:03:00Z

test/test_helper.rb

    Minitest::Test.make_my_diffs_pretty!
+
+    sig { void }
+    def stub_typechecking


(not sure if this name is clear enough)

Yeah, I'd go with something like stub_missing_typechecker.

vinistock

This is looking good. One thing about naming conventions: I think both type checker and typechecker are correct, but we usually prefer typechecker.

This PR is mixing both a little bit. Can we standardize on typechecker?

lib/ruby_lsp/requests/support/dependency_detector.rb

vinistock · 2023-09-28T18:51:18Z

lib/ruby_lsp/executor.rb

@@ -13,7 +13,7 @@ def initialize(store, message_queue)
      # Requests that mutate the store must be run sequentially! Parallel requests only receive a temporary copy of the
      # store
      @store = store
-      @test_library = T.let(DependencyDetector.detected_test_library, String)
+      @dependency_detector = T.let(DependencyDetector.instance, RubyLsp::DependencyDetector)


We don't need this variable anymore when using the singleton. The requests can just access the singleton directly. It doesn't even need to be passed as a parameter anymore.

andyw8 · 2023-09-28T19:50:52Z

lib/ruby_lsp/requests/completion.rb

@@ -39,6 +39,7 @@ def initialize(index, nesting, emitter, message_queue)
        @_response = T.let([], ResponseType)
        @index = index
        @nesting = nesting
+        @typechecker = T.let(DependencyDetector.instance.typechecker?, T::Boolean)


I'm wondering whether it's worth storing this in an instance var, or just calling the singleton each time.

andyw8 · 2023-09-28T20:02:37Z

lib/ruby_lsp/executor.rb

@@ -97,7 +96,7 @@ def run(request)
        folding_range = Requests::FoldingRanges.new(document.parse_result.comments, emitter, @message_queue)
        document_symbol = Requests::DocumentSymbol.new(emitter, @message_queue)
        document_link = Requests::DocumentLink.new(uri, document.comments, emitter, @message_queue)
-        code_lens = Requests::CodeLens.new(uri, @test_library, emitter, @message_queue)
+        code_lens = Requests::CodeLens.new(uri, emitter, @message_queue)


I removed test_library from the required parameters so that access is through the singleton, making it consistent with approach for other dependency checks.

andyw8 · 2023-09-28T20:16:17Z

sorbet/rbi/shims/singleton.rbi

@@ -0,0 +1,5 @@
+# typed: true
+
+module Singleton


Should we upstream this?

(just noticed we also use this in formatting_test_runner but mark it unsafe)

Yeah, we can push an RBI change to Sorbet.

sorbet/sorbet#7362

andyw8 · 2023-09-29T15:31:33Z

Using the singleton does mean we have to be careful about state leaking between tests - as I discovered, it's easy to accidentally have tests which will fail only occasionally.

I've taken the approach that each test should be responsible for cleaning up after itself, so I've added a teardown method in test_helper. However, this means that wherever we add a teardown method to an individual test file, we should ensure we call super. (It would be handy if there was a cop to catch this.)

(minitest doesn't have an equivalent of RSpec's after(:all). There are lifeycle hoooks, but they are intended for libraries to use.)

This approach also means we're resetting the singleton unnecessarily in many places, so slightly slowing the tests down, but I think it's worth it for the benefit of stability.

vinistock · 2023-09-29T17:16:22Z

lib/ruby_lsp/requests/support/dependency_detector.rb

+    def typechecker?
+      @typechecker
+    end


Let's get rid of the ? and make this an attr reader. They have performance optimizations we'd miss out on with a regular method.

vinistock · 2023-09-29T17:16:45Z

lib/ruby_lsp/requests/support/dependency_detector.rb

+
+    sig { void }
+    def initialize
+      @dependency_keys = T.let(nil, T.nilable(T::Array[String]))


I don't think this instance variable is used anymore.

It's used for the memoization. We could restructure the class to eliminate it, but think it's more readable this way.

You shouldn't need it here in the initialize if you do this in the memoized method.

sig { returns(T::Array[String]) } def something @something ||= T.let(whatever, T.nilable(T::Array[String])) end

vinistock · 2023-09-29T17:19:59Z

test/test_helper.rb

+    def teardown
+      Singleton.__init__(RubyLsp::DependencyDetector)
+    end


I'm not a big fan of this pattern to be honest. Without knowing about this piece of code, there's no way devs will know they have to invoke super on their teardown implementations.

Are there too many tests that depend on the state of the singleton being reset? Can we reset the singleton on each test that requires it instead?

Are there too many tests that depend on the state of the singleton being reset? Can we reset the singleton on each test that requires it instead?

We could do that, but I think it has a similar problem that if we introduce a new test, then we need to remember to add that teardown (if needed). Since it's order dependant, CI may still pass without it.

Another approach would be to switch the tests to use ActiveSupport::TestCase. That provides a teardown {} method that takes a block, which automatically calls super. I know it's another dependency to add, but it would only be required for tests.

IMO, let's not add this teardown. The test for the dependency detector itself can do this in its own teardown.

For all other tests that depend on a specific value being returned by something, we should use stubbing.

vinistock · 2023-09-29T17:20:41Z

test/test_helper.rb

    Minitest::Test.make_my_diffs_pretty!
+
+    sig { void }
+    def stub_typechecking


Yeah, I'd go with something like stub_missing_typechecker.

vinistock · 2023-09-29T17:21:51Z

test/requests/workspace_symbol_test.rb

    @index = RubyIndexer::Index.new
  end

-  def teardown


Would stubbing on the setup method not work here? Do we need to repeat it every time?

I tried that originally but something wasn't working for test_matches_only_gem_symbols_if_typechecker_is_present. I'll take another look.

Fixed in d4476e2

vinistock · 2023-09-29T18:41:28Z

lib/ruby_lsp/requests/support/dependency_detector.rb

+
+    sig { void }
+    def initialize
+      @dependency_keys = T.let(nil, T.nilable(T::Array[String]))


You shouldn't need it here in the initialize if you do this in the memoized method.

sig { returns(T::Array[String]) } def something @something ||= T.let(whatever, T.nilable(T::Array[String])) end

vinistock · 2023-09-29T18:42:03Z

sorbet/rbi/shims/singleton.rbi

@@ -0,0 +1,5 @@
+# typed: true
+
+module Singleton


Yeah, we can push an RBI change to Sorbet.

vinistock · 2023-09-29T18:42:58Z

test/requests/completion_test.rb

@@ -198,7 +199,7 @@ def test_completion_is_not_triggered_if_argument_is_not_a_string
  end

  def test_completion_for_constants
-    RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
+    stub_no_typechecker


Can we do this in the setup method and avoid the duplicates?

vinistock · 2023-09-29T18:44:11Z

test/requests/workspace_symbol_test.rb

@@ -5,14 +5,10 @@

 class WorkspaceSymbolTest < Minitest::Test
  def setup
-    RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
+    RubyLsp::DependencyDetector.instance.stubs(typechecker: false)


Does invoking the new stub_typechecker method work here?

Can we not use the stub_no_typechecker here?

vinistock · 2023-09-29T18:45:18Z

test/test_helper.rb

+    def teardown
+      Singleton.__init__(RubyLsp::DependencyDetector)
+    end


IMO, let's not add this teardown. The test for the dependency detector itself can do this in its own teardown.

For all other tests that depend on a specific value being returned by something, we should use stubbing.

vinistock · 2023-10-02T13:40:34Z

test/requests/support/dependency_detector_test.rb

@@ -5,44 +5,50 @@

 module RubyLsp
  class DependencyDetectorTest < Minitest::Test
+    def teardown


Is this empty teardown necessary?

vinistock · 2023-10-02T13:41:41Z

test/requests/workspace_symbol_test.rb

@@ -5,14 +5,10 @@

 class WorkspaceSymbolTest < Minitest::Test
  def setup
-    RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
+    RubyLsp::DependencyDetector.instance.stubs(typechecker: false)


Can we not use the stub_no_typechecker here?

…nd-patch-6ccc4187eb Bump the minor-and-patch group with 3 updates

andyw8 force-pushed the andyw8/refactor-dependency-detector branch 5 times, most recently from d4571fa to b859892 Compare September 27, 2023 18:25

andyw8 commented Sep 27, 2023

View reviewed changes

andyw8 force-pushed the andyw8/refactor-dependency-detector branch 2 times, most recently from 71b849c to d97052d Compare September 28, 2023 15:33

andyw8 changed the title ~~WIP: Refactor dependency detector~~ Refactor dependency detector Sep 28, 2023

andyw8 force-pushed the andyw8/refactor-dependency-detector branch from d97052d to d425fac Compare September 28, 2023 15:36

andyw8 added the chore Chore task label Sep 28, 2023

andyw8 commented Sep 28, 2023

View reviewed changes

andyw8 marked this pull request as ready for review September 28, 2023 15:58

andyw8 requested a review from a team as a code owner September 28, 2023 15:58

andyw8 requested review from st0012 and vinistock September 28, 2023 15:58

andyw8 commented Sep 28, 2023

View reviewed changes

vinistock reviewed Sep 28, 2023

View reviewed changes

lib/ruby_lsp/requests/support/dependency_detector.rb Outdated Show resolved Hide resolved

vinistock reviewed Sep 28, 2023

View reviewed changes

andyw8 force-pushed the andyw8/refactor-dependency-detector branch from 83c6334 to 9d211fe Compare September 28, 2023 19:38

andyw8 commented Sep 28, 2023

View reviewed changes

andyw8 force-pushed the andyw8/refactor-dependency-detector branch 2 times, most recently from d96ea49 to c13831d Compare September 28, 2023 20:43

vinistock reviewed Sep 29, 2023

View reviewed changes

andyw8 force-pushed the andyw8/refactor-dependency-detector branch 2 times, most recently from 185b6c5 to b53d10d Compare September 29, 2023 20:59

andyw8 added 22 commits October 2, 2023 09:27

Use singleton for Hover

51a2ad7

Remove prefix

e93052a

Use singleton for Completion

9d81a59

Remove unused ivar

c4f89b0

Eagerly determine dependencies

e1df22f

Fixes

0b6d25f

Fix bad paste

ef1bc01

Reset dependency_detector

05493fe

Reset singleton for Formatting request

9f9090f

Avoid unnecessary ivar

a177532

Remove unused unstub method

5db6d4d

Move singleton clearning to teardown

6c8919c

lint

4e06217

Use attr_reader for typechecker

adc3704

Rename method

c67011e

Remove duplication

259aeba

Specify type for shim

bce25b6

Remove unsafe, use shim instead

6fa3849

Remove unnecessary ivar initialization in contructor

8562672

Only reset singleton where necessary

d612876

Unify stub_no_typechecker calls

c997489

Prevent stub being lost

0e68bba

andyw8 force-pushed the andyw8/refactor-dependency-detector branch from b53d10d to 0e68bba Compare October 2, 2023 13:27

andyw8 enabled auto-merge (squash) October 2, 2023 13:28

andyw8 merged commit 1a774de into main Oct 2, 2023
29 checks passed

andyw8 deleted the andyw8/refactor-dependency-detector branch October 2, 2023 13:37

vinistock reviewed Oct 2, 2023

View reviewed changes

andyw8 mentioned this pull request Oct 2, 2023

Follow-ups for DependencyDetector refactoring #1054

Merged

andyw8 mentioned this pull request Oct 12, 2023

Use shortest possible constant name for filtering completions #1074

Merged

vinistock pushed a commit that referenced this pull request Feb 28, 2024

Merge pull request #1042 from Shopify/dependabot/npm_and_yarn/minor-a…

2acabf6

…nd-patch-6ccc4187eb Bump the minor-and-patch group with 3 updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dependency detector #1042

Refactor dependency detector #1042

andyw8 commented Sep 27, 2023 •

edited

Loading

andyw8 Sep 27, 2023 •

edited

Loading

andyw8 Sep 28, 2023

andyw8 Sep 28, 2023

vinistock Sep 29, 2023

vinistock left a comment

vinistock Sep 28, 2023

andyw8 Sep 28, 2023

andyw8 Sep 28, 2023 •

edited

Loading

andyw8 Sep 28, 2023

andyw8 Sep 28, 2023 •

edited

Loading

vinistock Sep 29, 2023

andyw8 Sep 29, 2023

andyw8 commented Sep 29, 2023

vinistock Sep 29, 2023

vinistock Sep 29, 2023

andyw8 Sep 29, 2023

vinistock Sep 29, 2023

vinistock Sep 29, 2023

andyw8 Sep 29, 2023 •

edited

Loading

vinistock Sep 29, 2023

vinistock Sep 29, 2023

vinistock Sep 29, 2023

andyw8 Sep 29, 2023

andyw8 Sep 29, 2023

vinistock Sep 29, 2023

vinistock Sep 29, 2023

vinistock Sep 29, 2023

vinistock Sep 29, 2023

vinistock Oct 2, 2023

vinistock Sep 29, 2023

vinistock Oct 2, 2023

vinistock Oct 2, 2023

Refactor dependency detector #1042

Refactor dependency detector #1042

Conversation

andyw8 commented Sep 27, 2023 • edited Loading

Motivation

Implementation

Automated Tests

Manual Tests

andyw8 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Sep 27, 2023 •

edited

Loading

andyw8 Sep 27, 2023 •

edited

Loading

andyw8 Sep 28, 2023 •

edited

Loading

andyw8 Sep 28, 2023 •

edited

Loading

andyw8 Sep 29, 2023 •

edited

Loading