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

Implement location link for type inlay hints #13699

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented Nov 29, 2022

fix #11701

This actually doesn't work due a problem in vscode: microsoft/vscode#167564

changelog note: The vscode issue is now fixed, but won't land until the january release, so the feature is disabled serverside for VSCode until then

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2022
@HKalbasi
Copy link
Member Author

@rustbot blocked

@rustbot rustbot added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2022
@bors
Copy link
Collaborator

bors commented Dec 20, 2022

☔ The latest upstream changes (presumably #13804) made this pull request unmergeable. Please resolve the merge conflicts.

@HKalbasi
Copy link
Member Author

The vscode issue is now fixed, but if I understand correctly it will be included in January release, not next release.

@Veykril
Copy link
Member

Veykril commented Dec 21, 2022

Since it won't release until the january release I think we have two options, wait with merging this PR, or check the vscode client version on the server to conditionally enable the link part of this PR as VSCode tells its version in the InitializeParams (current version sends this "clientInfo":{"name":"Visual Studio Code","version":"1.74.1"}) and remove the check some months in the future since we drop support for older VSCode versions pretty fast anyways.

Comment on lines +117 to +120
// FIXME: This is here since it is input of a method in `HirWrite`
// and things outside of hir need to implement that trait. We probably
// should move whole `hir_ty::display` to this crate so we will become
// able to use `ModuleDef` or `Definition` instead of `ModuleDefId`.
Copy link
Member

Choose a reason for hiding this comment

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

I think in general we want to have proper visitors for various things at some point, since the HirWrite interface is tailored to IDE purposes right now and shouldn't really be part of hir-ty either.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should have something in hir-ty that basically turns a type into a tree that directly maps to surface syntax, but still uses IDs instead of names. That representation could then be consumed by the IDE crates

@HKalbasi
Copy link
Member Author

check the vscode client version on the server to conditionally enable the link part of this PR

I think it is not necessary, since it wouldn't cause crash or anything like that, it just doesn't do its job.

@Veykril
Copy link
Member

Veykril commented Dec 21, 2022

it just doesn't do its job.

I know but I imagine people will start reporting this as a bug. Though maybe its fine if we note this down in the changelog that it current VSCode has a bug there.

@Veykril
Copy link
Member

Veykril commented Dec 21, 2022

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Dec 21, 2022

📌 Commit e1aa73e has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Dec 21, 2022

⌛ Testing commit e1aa73e with merge 271f7b4...

@bors
Copy link
Collaborator

bors commented Dec 21, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 271f7b4 to master...

if let Some(client) = client_info {
if client.name.contains("Code") || client.name.contains("Codium") {
if let Some(version) = &client.version {
if version.as_str() < "1.76" {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be 1.75? 1.74 is the November release, they skip December, and 1.75 comes out in January.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I wasn't aware that they will skip december release entirely.

@IWANABETHATGUY
Copy link

VScode has been released January release, could we enable this feature?

@Veykril
Copy link
Member

Veykril commented Feb 6, 2023

It is already enabled

@lnicola
Copy link
Member

lnicola commented Feb 6, 2023

Since #13963.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement inlay hint location links
7 participants