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

feat: make inlay hints insertable #14533

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Apr 8, 2023

Part of #13812

This PR implements text edit for inlay hints. When an inlay hint contain text edit, user can "accept" it (e.g. by double-clicking in VS Code) to make the hint actual code (effectively deprecating the hint itself).

This PR does not implement auto import despite the original request; text edits only insert qualified types along with necessary punctuation. I feel there are some missing pieces to implement efficient auto import (in particular, type traversal function with early exit) so left it for future work. Even without it, user can use replace_qualified_name_with_use assist after accepting the edit to achieve the same result.

I implemented for the following inlay hints:

  • top-level identifier pattern in let statements
  • top-level identifier pattern in closure parameters
  • closure return type when it has block body

One somewhat strange interaction can be observed when top-level identifier pattern has subpattern: text edit inserts type annotation in different place than the inlay hint. Do we want to allow it or should we not provide text edits for these cases at all?

let a /* inlay hint shown here */ @ (b, c) = foo();
let a @ (b, c) /* text edit inserts types here */ = foo();

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 8, 2023
@@ -147,7 +148,7 @@ pub trait HirDisplay {
curr_size: 0,
max_size: None,
omit_verbose_types: false,
display_target: DisplayTarget::SourceCode { module_id },
display_target: DisplayTarget::SourceCode { module_id, allow_opaque: allows_opaque },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently render opaque types for DisplayTarget::SourceCode unconditionally, but it should fail unless we're rendering it for function param type or return type. I've looked through every HirDisplay::display_source_code() call sites and passed the flag accordingly, which I'd like to get careful review.

@bors
Copy link
Collaborator

bors commented Apr 10, 2023

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

@Veykril
Copy link
Member

Veykril commented Apr 11, 2023

One somewhat strange interaction can be observed when top-level identifier pattern has subpattern:

I think its fine if we offer the edit still. The reason we render the hint after the identifier instead of the proper position is because of nesting, if we render it in the correct place we can only actually render @ bind pat hints in top level patterns.

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, I am worried about perf now though (especially if we text edits to more inlay hints), ideally we should calculate these in a resolve step (if the client supports it). #13962, it is mainly a question of how. We probably need to restructure this a bit so we can do a resolve step efficiently.

@lowr lowr force-pushed the feat/text-edits-for-inlay-hints branch from c24eb5e to f1981e1 Compare April 12, 2023 09:49
@lowr lowr force-pushed the feat/text-edits-for-inlay-hints branch from f1981e1 to c978d4b Compare April 12, 2023 10:04
@lowr
Copy link
Contributor Author

lowr commented Apr 12, 2023

I understand the concern on the performance implication (and I actually gave a bit of thoughts on inlayHint/resolve!). We gotta call hir_def::find_path() for every def in the type being rendered, which can be quite expensive especially when it's some complex type coming from combinators and such.

Do we want to put this feature behind a configuration until #13962 is resolved? Or would it suffice to limit the length of the rendered type and bail on excess (which I haven't implemented but actually thought of and put a FIXME note on)?

@Veykril
Copy link
Member

Veykril commented Apr 12, 2023

I think its fine to add it as is, since we arent generating edits for all hints yet. But we want resolve support before we add more I think (or before making them more complex)

@Veykril
Copy link
Member

Veykril commented Apr 12, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 12, 2023

📌 Commit c978d4b has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 12, 2023

⌛ Testing commit c978d4b with merge 1ee88db...

@bors
Copy link
Collaborator

bors commented Apr 12, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 1ee88db to master...

@lnicola
Copy link
Member

lnicola commented Apr 17, 2023

insert_inlay_hint.mp4

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