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

fix: Advertise completions and inlay hints resolve server capabilities based on the client capabilities #18589

Merged

Conversation

SomeoneToIgnore
Copy link
Contributor

No description provided.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2024
@Veykril Veykril enabled auto-merge December 3, 2024 08:48
auto-merge was automatically disabled December 3, 2024 08:51

Head branch was pushed to by a user without write access

@SomeoneToIgnore SomeoneToIgnore force-pushed the proper-resolve-advertisement branch from 6367410 to 4261ac7 Compare December 3, 2024 08:51
@Veykril Veykril added this pull request to the merge queue Dec 3, 2024
@bstaletic
Copy link
Contributor

@SomeoneToIgnore Thank you! I have just tested with my client and this pull request resolves the regression I have seen.

Merged via the queue into rust-lang:master with commit c4e040e Dec 3, 2024
9 checks passed
@SomeoneToIgnore SomeoneToIgnore deleted the proper-resolve-advertisement branch December 3, 2024 09:53
@Veykril
Copy link
Member

Veykril commented Dec 3, 2024

I'm not up to speed with what happened the past weeks wrt to this but does this also need a beta backport?

Edit: Actually this seems separate from the other problems I suppose, though backporting that might not be bad eitherway

@SomeoneToIgnore
Copy link
Contributor Author

So far it seems that most of the completion-related issues were in "rich" editors that were always requiring the additionalTextEdits, so this fix changes nothing for them and is indeed, somewhat separate.

I am trying not to do anything extra on this front without the confirmation now, so let me know if you think it's a good idea to backport.
For that, I need to open this PR against the rustc:beta branch, don't I?

@Veykril
Copy link
Member

Veykril commented Dec 3, 2024

I am trying not to do anything extra on this front without the confirmation now, so let me know if you think it's a good idea to backport. For that, I need to open this PR against the rustc:beta branch, don't I?

It wouldn't hurt I'd say, and yes, we'd need to file the changes here on the in-tree version against https://github.com/rust-lang/rust/tree/beta

@lnicola
Copy link
Member

lnicola commented Dec 3, 2024

Wasn't this disabled on beta in rust-lang/rust#133546?

@SomeoneToIgnore
Copy link
Contributor Author

Oh, sorry, I'm sick and not thinking straight.
It was, indeed, so we're "good" for now modulo inlays' resolve (which no one seem to care about given how much time it's there)

@lnicola
Copy link
Member

lnicola commented Dec 3, 2024

I'd say not backport if it's not strictly needed, it's fine if it hits stable in the next version.

@Veykril
Copy link
Member

Veykril commented Dec 3, 2024

Ah okay, then I agree. No need to backport this 👍

@lnicola lnicola changed the title Advertise completions and inlay hints resolve server capabilities based on the client capabilities fix: Advertise completions and inlay hints resolve server capabilities based on the client capabilities Dec 3, 2024
@bstaletic
Copy link
Contributor

May I ask if there's any policy regarding when this change will be available through rustup-installed rust-anayzer?
I'd like to get these changes into the default config of my client. To be honest, I'm not in a hurry.

@lnicola
Copy link
Member

lnicola commented Dec 10, 2024

I think the problematic code was disabled in 1.83. I'll try to do another subtree sync tomorrow, so this should hit nightly in a couple of days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants