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

[READY] Add support codeAction/resolve requests #1754

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Aug 15, 2024

So...
We advertise that we can handle CodeActions that are either missing an edit or a command (or both).
The server chooses whether it supports either of the two. If it does, it advertises itself as a "code action resolve provider".

For servers that are code action resolve providers:

  • If either edit or command is missing, we need to try resolving the code action via codeAction/resolve.
    • Unless we actually got a LSP Command, in which case codeAction/resove is skipped.
  • After resolving it that way we have a CodeAction in one of these forms:
    • A LSP Command
    • A LSP CodeAction with only an edit.
    • A LSP CodeAction with only a command.
    • A LSP CodeAction with both an edit and a command.
  • Edits are WorkspaceEdits and can easily be converted into ycmd FixIts.
  • Commands are to be executed, yielding ApplyEdits. A single ApplyEdit can be converted into a ycmd FixIt.

For servers that are not code action resolve providers, the steps are the same, but we skip the codeAction/resolve route.

One thing missing is properly defined handling of fixit resolutions that yield multiple fixits. That can happen in a few ways:

  • On /resolve_fixit, by a LSP command yielding multiple ApplyEdits.
  • When eagerly resolving a fixit, again by a LSP command yielding multiple ApplyEdits.
  • Even if all commands always yield a single ApplyEdit, if a CodeAction has both an edit and a command, that's again two fixits.

The first two above don't seem to be used by any server ever. The LSP specs nudges servers away from doing that, but no promises. We are still not supporting any scenario where resolving a single fixit results in more than one fixit.

Another scenario that does not seem to happen:

  • The server is a code action resove provider.
  • The received CodeAction has neither an edit nor a command.
  • After resolving, we get only a command.
  • We then need to execute the command and collect ApplyEdits.

In practice, such code actions resolve to a CodeAction containing an edit.

As for the ycmd<->client side of things... it's a bit ugly on the ycmd side, but we're completely preserving the API, so clients do not need to change a thing.

Previously, clients got { 'fixits': [ { 'command': LSPCommand ... } ] } for unresolved fixits. However, we have not given any guarantees about the value of 'command'. We have only said that it should be returned to ycmd for the purposes of /resolve_fixit. With this pull request, we need to pass the entire CodeAction, but we're still putting it in the command key.


This change is Reviewable

@bstaletic bstaletic changed the title [READY] This is a massive step towards switching omnisharp to LSP. [READY] Add support codeAction/resolve requests Aug 15, 2024
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Thanks!

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

Copy link

codecov bot commented Aug 17, 2024

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.90%. Comparing base (6f1f4f8) to head (a5f4086).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1754      +/-   ##
==========================================
+ Coverage   95.87%   95.90%   +0.03%     
==========================================
  Files          84       84              
  Lines        8437     8450      +13     
  Branches      163      163              
==========================================
+ Hits         8089     8104      +15     
+ Misses        298      296       -2     
  Partials       50       50              

@bstaletic bstaletic force-pushed the codeAction-resolve branch 2 times, most recently from 00b01be to a5f4086 Compare August 17, 2024 12:50
So...
We advertise that we can handle CodeActions that are either missing an
edit or a command (or both).
We also advertise that we will preserve server `data` that we receive in
a CodeAction, when we resolve the said fixit. This allows more servers
to take advantage of the lazy code actions.

The server chooses whether it supports either of the two. If it does, it
advertises itself as a "code action resolve provider".

For servers that are code action resolve providers:
- If either edit or command is missing, we need to try resolving the
  code action via codeAction/resolve.
  - Unless we actually got a LSP Command, in which case
    codeAction/resove is skipped.
- After resolving it that way we have a CodeAction in one of these forms:
  - A LSP Command
  - A LSP CodeAction with only an edit.
  - A LSP CodeAction with only a command.
  - A LSP CodeAction with both an edit and a command.
- Edits are WorkspaceEdits and can easily be converted into ycmd FixIts.
- Commands are to be executed, yielding ApplyEdits. A single ApplyEdit
  can be converted into a ycmd FixIt.

For servers that are not code action resolve providers, the steps are
the same, but we skip the codeAction/resolve route.

One thing missing is properly defined handling of fixit resolutions that
yield multiple fixits. That can happen in a few ways:

- On /resolve_fixit, by a LSP command yielding multiple ApplyEdits.
- When eagerly resolving a fixit, again by a LSP command yielding
  multiple ApplyEdits.
- Even if all commands always yield a single ApplyEdit, if a CodeAction
  has both an edit and a command, that's again two fixits.

The first two above don't seem to be used by any server ever. The LSP
specs nudges servers away from doing that, but no promises.
We are still not supporting any scenario where resolving a single fixit
results in more than one fixit.

Another scenario that does not seem to happen:
- The server is a code action resove provider.
- The received CodeAction has neither an edit nor a command.
- After resolving, we get only a command.
- We then need to execute the command and collect ApplyEdits.

In practice, such code actions resolve to a CodeAction containing an edit.

As for the ycmd<->client side of things... it's a bit ugly on the ycmd
side, but we're completely preserving the API, so clients do not need to
change a thing.

Previously, clients got `{ 'fixits': [ { 'command': LSPCommand ... } ]
}` for unresolved fixits.  However, we have not given any guarantees
about the value of `'command'`.  We have only said that it should be
returned to ycmd for the purposes of `/resolve_fixit`.  With this pull
request, we need to pass the entire CodeAction, but we're still putting
it in the `command` key.
@bstaletic
Copy link
Collaborator Author

I'm finally happy with this pull request. Codecov says that I should have covered the changes better, but the overall coverage is up.

@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Aug 17, 2024
Copy link
Contributor

mergify bot commented Aug 17, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit 1026c83 into ycm-core:master Aug 17, 2024
16 of 18 checks passed
@bstaletic bstaletic deleted the codeAction-resolve branch August 17, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants