Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor listener registration #829

Merged
merged 1 commit into from
Aug 3, 2023
Merged

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Jul 20, 2023

Motivation

Allowing extensions to perform listener registration using class methods does not give the LSP enough control to deactivate extensions properly. It also doesn't allow extensions to perform store ahead of time configuration in the Extension instance and pass that to their listeners.

This PR proposes a refactor in how listener registration happens. Instead of registering listener classes that we instantiate from the LSP, we invert the responsibility and require the extensions to implement factory methods that will create the listener instances based on parameters. This allows extensions to use information stored in the Extension instance in their listeners and also allows us to easily disable extensions, since the factory methods are invoked on every request.

Example

class MyExtension < RubyLsp::Extension
  def activate
    @important_info = 123
  end

  # This is invoked on every request. Disabling the extension would then mean just removing
  # it from the list which automatically makes us not invoke these factory methods
  def create_code_lens_listener(uri, emitter, queue)
    MyCodeLensListener.new(@important_info, uri, emitter, queue)
  end
end

Implementation

There are tradeoffs with each approach, so I'd love to hear opinions on this. The idea is that we'd need to provide empty factory methods from the base Extension class that are overridden to do the right thing if your extension needs it. I don't love invoking a bunch of empty methods, but I couldn't come up with an alternative.

There's also the question of formatters. Those are still registered at a class level since a factory method doesn't make much sense for them. We just need to know an identifier and an instance of the formatter.

Automated Tests

Adapted tests.

@vinistock vinistock added the breaking-change Non-backward compatible change label Jul 20, 2023
@vinistock vinistock added this to the 2023-Q3 milestone Jul 20, 2023
@vinistock vinistock self-assigned this Jul 20, 2023
@vinistock vinistock requested a review from a team as a code owner July 20, 2023 21:21
@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.322914 std_dev: 0.010211
          textDocument/diagnostic average: 0.059061 std_dev: 0.012654
          textDocument/definition average: 0.005671 std_dev: 0.003306
      textDocument/selectionRange average: 0.004376 std_dev: 0.000739
        textDocument/documentLink average: 0.002443 std_dev: 0.000452
 textDocument/semanticTokens/full average: 0.002441 std_dev: 0.000441
   textDocument/documentHighlight average: 0.00233 std_dev: 0.000452
      textDocument/documentSymbol average: 0.002322 std_dev: 0.000263
            textDocument/codeLens average: 0.002321 std_dev: 0.000341
        textDocument/foldingRange average: 0.002042 std_dev: 0.000337
textDocument/semanticTokens/range average: 0.00117 std_dev: 0.000167
               codeAction/resolve average: 0.000843 std_dev: 0.000399
           textDocument/inlayHint average: 0.000752 std_dev: 0.000143
               textDocument/hover average: 0.000751 std_dev: 0.000164
    textDocument/onTypeFormatting average: 0.000116 std_dev: 8.4e-05
          textDocument/formatting average: 0.00011 std_dev: 0.000385
          textDocument/codeAction average: 6.6e-05 std_dev: 0.000162


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range slower by 17.477 %
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion unchanged
            textDocument/codeLens unchanged
          textDocument/definition unchanged


At least one benchmark is slower than the main branch.


================================================================================
Missing benchmarks:

RubyLsp::Requests::ShowSyntaxTree

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I like the approach better 👍
Just a nitpick on the PR name: I feel calling it Refactor is a bit misleading because it actually changes the interface for extensions.

@andyw8
Copy link
Contributor

andyw8 commented Jul 24, 2023

I expect https://github.com/Shopify/ruby-lsp/blob/main/SERVER_EXTENSIONS.md should also be updated?

lib/ruby_lsp/extension.rb Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs/refactor_listener_registration branch from e01a655 to 7a1189d Compare July 25, 2023 16:13
@vinistock
Copy link
Member Author

Good point. Changed the documentation to reflect the changes. I'll hold off merging this for a bit to get some bug fixes out before having to bump to v0.8.0 and ship these breaking changes.

@vinistock vinistock merged commit d182220 into main Aug 3, 2023
25 checks passed
@vinistock vinistock deleted the vs/refactor_listener_registration branch August 3, 2023 13:31
@shopify-shipit shopify-shipit bot temporarily deployed to production August 8, 2023 20:03 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Non-backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants