Decouple request and listener logic

[st0012]

Motivation

Currently, we treat listener-based requests as both a listener and a request. For example, the Hover request handles the instantiation of addon's hover listeners, while being a listener itself to process dispatcher events. This coupling limits the change we can make to improve the interface for addons or more complicated response merging logic.

Implementation

1. Every request class should have the same root Request class as the parent, which currently only makes sure the response method is called response (instead of run).
2. A ListenerBasedRequest < Request (name to be improved) class was also created to help listener-based requests define and use listeners. This means:
   • Request classes will handle ALL listener instantiation (built-in and addon's) and merge the responses from them.
   • Listener classes will be solely responsible for handling dispatcher events.
3. As a result of 2), ExtensibleListener is not needed anymore.
4. Some request-specific logic in executor.rb are moved to individual request classes as well.

Notes

• Ideally, listener classes' interface shouldn't be called response anymore. But changing it would break addons and makes this already giant PR even bigger. So I decide to do it later.
• @Morriar proposed a few interesting ideas to save us from defining ResponseType on both the listener and request classes. But similar to the above, it'll be a huge change on its own and IMO it's better to implement in a follow up PR.

Feedback Needed

• There are some common pattern for listener classes in the executor.rb, like:

  •   # whole-document-based listener request
      dispatcher = Prism::Dispatcher.new
      document = @store.get(uri)
      request = Requests::DocumentHighlight.new(document, request.dig(:params, :position), dispatcher)
      dispatcher.dispatch(document.tree)
      request.response

  •   # node-based listener request
      dispatcher = Prism::Dispatcher.new
      document = @store.get(uri)
      Requests::Completion.new(
        document,
        @index,
        request.dig(:params, :position),
        document.typechecker_enabled?,
        dispatcher,
      ).response

  Is there a way we can better simplify them (or not)?

Automated Tests

All existing tests should still pass.

Manual Tests

[Morriar]

Once all requests are proper descendants of Request you should be able to enforce overrides on provider:

class Request
  extend T::Sig
  extend T::Helpers

  abstract!

  class << self
    extend T::Sig

    sig { abstract.returns(T.attached_class) }
    def provider; end
  end
end

class RequestA < Request
  class << self
    sig { override.returns(T.attached_class) }
    def provider
      new
    end
  end
end

class RequestB < Request # Error: Missing definition for abstract method `Request.provider`
end

[sorbet.run]

[st0012]

@Morriar But provider is not to return the request itself. It's for options like Interface::CodeLensOptions.new(resolve_provider: false) or just a hash. Also, not every request needs it. For example, definition and selection_range don't need to define it.

[vinistock]

Should this comment be removed? I think the T.must below addresses it, right?

[st0012]

I think the comment means that we shouldn't need to wrap T.must around other_responses as it's a splat assignment:

        first_response, *other_responses = responses

It should always be an array regardless of responses's value:

ruby-lsp(main):008> x, *y = a; y
=> []
ruby-lsp(main):009> a = nil
=> nil
ruby-lsp(main):010> x, *y = a; y
=> []
ruby-lsp(main):011> 

[vinistock]

Indeed, Sorbet is incorrect in this case (unless I'm missing something). I tried a bunch of variations and in the a, *b = pattern, b is always an array and never nilable.

Can we please check if there's an issue for that and, if not, create one? Feels like something that shouldn't be too hard to address (famous last words).

[st0012]

Yes there's an 4-year old issue for it: sorbet/sorbet#2020
To be fair, it was fixed but then the fix was reverted.

[vinistock]

Caught one bug while tophatting

[vinistock]

This line is failing because @dispatcher is nil on certain conditions due to the early returns in initialize.

I think we should move most of the code in the initialize method to response and ensure that the key instance variables are always set.

[st0012]

I prefer not moving most of them into response as it means we'd need to initialise listeners there too, which doesn't align with other requests.
I've rearranged the ordering in initialize a bit instead so all the ivars used in response should always be defined.