-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Shorten projection types in inlay hints #13834
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the motivation and the logic looks correct, I'm a bit concerned about the performance here; iterating through every associated item of every trait in scope sounds a bit expensive especially for large files. I guess it's okay for typical codebases as it's fairly rare to show projection types, but we might want to assess the effect or add a configuration to opt it out.
Another thing is that it might be somewhat confusing unless the self type of the projection is either placeholder type or ADT. T::Assoc and Vec<i32>::Assoc seem nice, but (T, U)::Assoc, impl Trait + Clone::Assoc, and &dyn Trait + Send::Assoc look foreign to me. Also, <&T as Trait>::Assoc would be displayed as &T::Assoc, which is indistinguishable from &<T as Trait>::Assoc.
|
☔ The latest upstream changes (presumably #13860) made this pull request unmergeable. Please resolve the merge conflicts. |
e7ccf34 to
3a97175
Compare
|
☔ The latest upstream changes (presumably #13946) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I agree with lowr's comment here, this does seem like a performance pitfall and the ambiguity cases mentioned aren't too nice either. So I am inclined to close this for now. |
|
Just to be clear: is this an issue of "the implementation is not good enough, needs more work" or "this looks hard/impossible to do in a good way in general"? |
|
I would say, more work required. I do think having this wouldn't be too bad, but I also don't know if this can be done in a way that is not too performance heavy (inlay hints already take way too long to calculate as is). So unfortunately I can only give a vague answer here :) |
|
Okay, I don't have enough time/energy to work on this, but hopefully I'll eventually try to revive this :) |
In examples like this, where we have a projection type, which is the only associated item with its name in a trait that is in scope:
Show
T::Associnstead of<T as Trait>::Assoc.Rationale: making inlay hints less verbose is generally good,
Traitis not that useful there.I'm not sure if the implementation is good and correct, but it seems to be working at least 😅