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

Return VOID by default for unhandled requests #961

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

vinistock
Copy link
Member

Motivation

Closes #882

Currently, if we receive a request we don't handle, we are returning nil back to the editor. Based on the issue reported, not every editor handles this and some completely stop the execution of the LSP.

This is probably happening when the server makes a request to the client to initiate progress. Because this is not a notification, but a request to the client, it actually sends back a response. When we receive that response back, we should not return nil back to the editor, since it does not expect anything back.

Generally, I don't think the LSP should ever return anything back to the editor for requests we don't handle explicitly.

Implementation

Added an else branch to our case statement to always return VOID for unhandled requests.

Automated Tests

Added a test verifying the new behaviour. Also moved the private since there were some helper methods defined outside of that section.

@vinistock vinistock added the bugfix This PR will fix an existing bug label Aug 31, 2023
@vinistock vinistock added this to the 2023-Q3 milestone Aug 31, 2023
@vinistock vinistock self-assigned this Aug 31, 2023
@vinistock vinistock requested a review from a team as a code owner August 31, 2023 18:41
@github-actions
Copy link
Contributor

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.207273 std_dev: 0.00343
          textDocument/diagnostic average: 0.039906 std_dev: 0.009841
          textDocument/definition average: 0.004901 std_dev: 0.002822
      textDocument/selectionRange average: 0.004072 std_dev: 0.000441
   textDocument/documentHighlight average: 0.002511 std_dev: 0.000171
            textDocument/codeLens average: 0.002498 std_dev: 0.000151
        textDocument/documentLink average: 0.002497 std_dev: 0.000205
      textDocument/documentSymbol average: 0.002459 std_dev: 0.000145
 textDocument/semanticTokens/full average: 0.002453 std_dev: 0.00031
        textDocument/foldingRange average: 0.002211 std_dev: 0.000136
textDocument/semanticTokens/range average: 0.001639 std_dev: 7.5e-05
           textDocument/inlayHint average: 0.001406 std_dev: 9.6e-05
               codeAction/resolve average: 0.001394 std_dev: 0.00011
               textDocument/hover average: 0.00134 std_dev: 8.6e-05
    textDocument/onTypeFormatting average: 0.000838 std_dev: 8.2e-05
          textDocument/formatting average: 0.000829 std_dev: 0.000281
          textDocument/codeAction average: 0.000811 std_dev: 7.4e-05


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

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      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


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

RubyLsp::Requests::ShowSyntaxTree

Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

Any way we could have leveraged static typing to avoid this?

I see the return being typed as T.untyped.

@vinistock
Copy link
Member Author

I'm not sure how we'd go about typing in this case. The method returns a different type depending on what request we received, which is why it's currently untyped.

case request[:method]
when "textDocument/documentSymbol"
  # returns document symbols array
when "textDocument/hover"
  # returns hover object
else
  # unknown requests, we don't return anything
  VOID
end

@vinistock vinistock merged commit 193c6f1 into main Sep 1, 2023
31 checks passed
@vinistock vinistock deleted the vs/return_void_by_default branch September 1, 2023 13:15
@Morriar
Copy link
Contributor

Morriar commented Sep 1, 2023

I'm not sure how we'd go about typing in this case. The method returns a different type depending on what request we received, which is why it's currently untyped.

A first option would be to use T.any(...) this would work better with responses such as T::Array[Interface::WorkspaceSymbol]:

POSSIBLE_RESPONSE_TYPES = T.type_alias { T.any(all the types we handle right now) }

A more involved solution would be to create type wrappers with a common ancestor:

Do the returned types have a common ancestor we could use?

If not, we could add one ourselves:

```rb
module ResponseType
  interface!
end

class ::Interface::SemanticTokens
  include ResponseType
end

I wonder if we could handle the arrays the same way:

class WorkspaceSymbols < Array
  include ResponseType

  Elem = type_member{{fixed: Interface::WorkspaceSymbol}}
end

And interesting case would be to avoid implicit nil returns to avoid mistake, maybe with something like this:

class NilReponse < NilClass
  include ResponseType
end

So it would force us to use return NilResponse.new rather than just return and catch branches where we just forgot to return something.

@vinistock
Copy link
Member Author

There's no common ancestor unfortunately. We could try using the T.any, but I'm not sure it would make much of a difference in terms of catching errors.

The untyped response is only used when writing it to STDOUT to send it back to the editor here.

andyw8 pushed a commit that referenced this pull request Sep 5, 2023
vinistock pushed a commit that referenced this pull request Feb 28, 2024
assign testItem from children only if matching child
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INVALID_SERVER_MESSAGE
4 participants