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] Rust analyzer code action fixes #1771

Merged

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Dec 2, 2024

New rust-analyzer, with the following changes:

  1. For the past month and some change, rust-analzer takes advantage of completionItem/resolve requests, but does not advertise that server capability.
    1.1. Link: internal: Send less data during textDocument/completion if possible rust-lang/rust-analyzer#18167 (comment)
  2. Sometimes it produces unresolved code action and sometimes it produces a code action literal. Then, if dealing with a code action literal, it errors on codeAction/resolve request.
    2.1. Link: codeAction/resolve throws an exception for a code action without data rust-lang/rust-analyzer#18428
    2.2. This is what the added test exercises.
  3. Diagnostics are getting more erratic on startup, so I had to modify the message poll test.

This change is Reviewable

@bstaletic bstaletic force-pushed the rust-analyzer-naughty-code-actions branch from dace0ca to 68840b4 Compare December 2, 2024 21:57
@bstaletic
Copy link
Collaborator Author

Let's wait on rust-lang/rust-analyzer#18589 so we don't have to introduce new hacks.

@bstaletic bstaletic force-pushed the rust-analyzer-naughty-code-actions branch 6 times, most recently from e3d7b40 to 2594df3 Compare December 13, 2024 07:45
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:

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


ycmd/tests/rust/get_completions_proc_macro_test.py line 75 at r2 (raw file):

    for candidate in results:
      if candidate[ 'insertion_text' ] == 'checkpoint':
        checked_candidate = candidate

break?

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.91%. Comparing base (c485180) to head (b96501f).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
+ Coverage   95.51%   95.91%   +0.40%     
==========================================
  Files          52       84      +32     
  Lines        6975     8469    +1494     
  Branches        0      163     +163     
==========================================
+ Hits         6662     8123    +1461     
+ Misses        313      296      -17     
- Partials        0       50      +50     

@bstaletic bstaletic force-pushed the rust-analyzer-naughty-code-actions branch from 2594df3 to b96501f Compare December 13, 2024 09:12
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.

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


ycmd/tests/rust/get_completions_proc_macro_test.py line 75 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

break?

Done.

The protocol expects us to call `codeAction/resolve` whenever a
codeaction lacks either an `edit` or a `command`.
Code action literals can lack the `data` field, which some servers
(hint: rust-analyzer) require to respond to `codeAction/resolve`.
However, we can still apply such code actions, so we do.
@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Dec 13, 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:

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

Copy link
Contributor

mergify bot commented Dec 13, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit bd150e6 into ycm-core:master Dec 13, 2024
14 of 15 checks passed
@bstaletic bstaletic deleted the rust-analyzer-naughty-code-actions branch December 13, 2024 10:26
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