Skip to content

impl Fn, impl FnMut, ... as inlay hint for closures #12297

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

Closed
Veykril opened this issue May 17, 2022 · 3 comments · Fixed by #14470
Closed

impl Fn, impl FnMut, ... as inlay hint for closures #12297

Veykril opened this issue May 17, 2022 · 3 comments · Fixed by #14470
Labels
A-inlay-hints inlay/inline hints C-feature Category: feature request

Comments

@Veykril
Copy link
Member

Veykril commented May 17, 2022

I would like to see impl Fn, impl FnMut, ... as inlay hint for closures, which I think is a useful data and non trivial in first look. In current state IMO it make sense to enable this by default.

Originally posted by @HKalbasi in #12263 (comment)

@Veykril Veykril added A-inlay-hints inlay/inline hints C-feature Category: feature request labels May 17, 2022
@Veykril Veykril changed the title I would like to see impl Fn, impl FnMut, ... as inlay hint for closures, which I think is a useful data and non trivial in first look. In current state IMO it make sense to enable this by default. I would like to see impl Fn, impl FnMut, ... as inlay hint for closures May 17, 2022
@Veykril Veykril changed the title I would like to see impl Fn, impl FnMut, ... as inlay hint for closures impl Fn, impl FnMut, ... as inlay hint for closures May 17, 2022
@andylizi
Copy link
Contributor

While the idea itself is great, I feel it probably won't work well as type inlay hints though. They only apply to variables, but most closure expressions are directly passed in as function arguments.

I guess we could create a new inlay hint type for it, but on second thought, since inlay hints are always displayed, I'm not sure this is the kind of information that needs to be always available like that.

For example, type inlay hints are almost always useful. Rust has a relatively complicated type system, and without knowing the exact inferred type of the variables at a glance, the code can be hard to write or understand.

Maybe I'm missing something regarding motivation and use cases, but from my personal experience, I rarely need to think anything about Fn* traits when I'm providing a closure. It only matters when the code fails to compile, and the compiler tells me nicely "cannot borrow x as mutable, as it is a captured variable in a Fn closure". This error almost never happens to me, since the strict requirements of the Fn trait are rarely necessary.

Use Fn as a bound when you ... need to call it repeatedly and without mutating state (e.g., when calling it concurrently).

(From grep.app: in libstd there're about 66 places that require FnOnce, 86 places require FnMut and almost no place requires Fn, excluding tests.)

The times where I have to think about Fn/FnMut/FnOnce are when I'm defining a new function that accepts a closure. Inlay hints obviously can't help with that.

I'm not saying that knowing this information is always unnecessary, just that it doesn't need to be always there as an inlay hint, unlike type hints and co. It can become quite verbose, e.g. showing hints for every closure in an iterator chain.

An alternative could be, instead of inlay hints, showing this infomation only when hovering over the closure expression. But I can't seem to get it to work in some situations (it'd highlight the whole expression instead of the closure itself).

@Veykril
Copy link
Member Author

Veykril commented May 18, 2022

I think what was meant here was to configurably render impl FnMut(T, ...) -> R instead of |T, ...| -> R, that is making it visible what kind of closure we have (though r-a can't deduce that yet). I think enabling that via configuration is fine.

Regarding hover for inlays that only got implemented yesterday, before the hover would fall through triggering generic hover

@HKalbasi
Copy link
Member

HKalbasi commented May 18, 2022

@andylizi I think everywhere we render a closure type (which currently is in inlay hints, including complex one like boxed closures, in hover of closure pipes (which is what you are suggesting), in hover of variables that bound a closure, ...) it would be better to show impl Fn* instead of current |i32| -> i32 style, because the former is valid rust, and contains more useful information.

I agree that this is not something that we can't live with it, it is just a nice to have, not for satisfying callers bound, but because these are different classes of closures (FnOnce are destructive closures, FnMut are stateful closures). People may still want to disable inlayhints for closure bindings even after this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inlay-hints inlay/inline hints C-feature Category: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants