Add ignore value suggestion in closure body #135562
Conversation
rustbot
added
S-waiting-on-review
|
I choose to make a |
There was a problem hiding this comment.
I would be concerned with this suggestion triggering for something like map, where should be suggesting removing the call entirely instead of ignoring the result. for for_each it is trickier because it is called for its side-effects, but again if the closure doesn't have any... I'm conflicted, not with the implementation but with the possibility of misleading users accidentally.
r=me after addressing the wording nitpick if I don't come up with actual reasons not to do this before this week is through.
| error[E0308]: mismatched types | ||
| --> $DIR/closure-ty-mismatch-issue-128561.rs:2:32 | ||
| | | ||
| LL | b"abc".iter().for_each(|x| x); | ||
| | ^ | ||
| | | | ||
| | expected `()`, found `&u8` | ||
| | help: consider ignore the value here: `_ =` |
There was a problem hiding this comment.
I could have sworn that this || _ = x syntax didn't work 👀
| error[E0308]: mismatched types | ||
| --> $DIR/closure-ty-mismatch-issue-128561.rs:4:32 | ||
| | | ||
| LL | b"abc".iter().for_each(|x| dbg!(x)); | ||
| | ^^^^^^^ expected `()`, found `&u8` | ||
| | | ||
| = note: this error originates in the macro `dbg` (in Nightly builds, run with -Z macro-backtrace for more info) |
There was a problem hiding this comment.
I notice that the suggestion didn't trigger for the macro case, which is what the original report wanted. I think that in order to sidestep the issues of the macro not being visible in the HIR, to get the span by looking at the closure's tail expression (instead of expression, get the parent node and get the span from there).
There was a problem hiding this comment.
I didn't notice in the macro case the parent type check here is not closure:
I changed the code to avoid suggesting for macro case explicitly.
Seems we need to get the parent hir recursively up to 4 levels in macro case to get the closure body, I tried with some debug code like:
There was a problem hiding this comment.
There are two options that might or might not work: from the expression, climb up its parents until you get a node with a span with root context (hence, not pointing at a macro). That runs the risk of giving you the node of an expression still surrounded by a macro, which isn't great. I think this suggestion is only relevant for the tail expression of closures, right? The second option would be to get the parent closure by looping on getting the parent until you hit one (with get_closure_node, as you point out), and then look down from there. A macro that uses closures internally can still mess your suggestion up though, so you likely still need a check for "what's this span's context", regardless of what you do :-/
Co-authored-by: Esteban Kuber <[email protected]>
Co-authored-by: Esteban Kuber <[email protected]>
chenyukang
force-pushed
the
yukang-fix-128561
branch
from
a833f42 to
9a6d414
Compare
|
|
||
| let parent_id = fcx.tcx.parent_hir_id(block_or_return_id); | ||
| let parent = fcx.tcx.hir_node(parent_id); | ||
| let parent = fcx.tcx.parent_hir_node(block_or_return_id); |
There was a problem hiding this comment.
If you change this for get_closure_node(block_or_return_id)...
| && let hir::Node::Expr(hir::Expr { | ||
| kind: hir::ExprKind::Closure(&hir::Closure { body, .. }), | ||
| .. | ||
| }) = parent |
There was a problem hiding this comment.
...and change the pattern here to surround it with Some. What do you get? It might be interesting to change the last argument to pass in an option of the closure tail expr, instead of a boolean, so that the suggestion can use that.
It is a shame how much macros in general complicate this kind of logic :(
Fixes #128561
r? @estebank