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

Add ignore value suggestion in closure body #135562

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions compiler/rustc_hir_typeck/src/coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1883,25 +1883,24 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
fcx.err_ctxt().report_mismatched_types(cause, fcx.param_env, expected, found, ty_err);

let due_to_block = matches!(fcx.tcx.hir_node(block_or_return_id), hir::Node::Block(..));

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change this for get_closure_node(block_or_return_id)...

if let Some(expr) = expression
&& let hir::Node::Expr(hir::Expr {
kind: hir::ExprKind::Closure(&hir::Closure { body, .. }),
..
}) = parent
Comment on lines 1888 to 1891
Copy link
Contributor

Choose a reason for hiding this comment

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

...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 :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's complicated.
Let's merge this PR to resolve the scenario cases except for macros, since the original issue describes a macro case, let's keep it open?

&& !matches!(fcx.tcx.hir_body(body).value.kind, hir::ExprKind::Block(..))
{
fcx.suggest_missing_semicolon(&mut err, expr, expected, true);
let needs_block =
!matches!(fcx.tcx.hir_body(body).value.kind, hir::ExprKind::Block(..));
fcx.suggest_missing_semicolon(&mut err, expr, expected, needs_block, true);
}
// Verify that this is a tail expression of a function, otherwise the
// label pointing out the cause for the type coercion will be wrong
// as prior return coercions would not be relevant (#57664).
if let Some(expr) = expression
&& due_to_block
{
fcx.suggest_missing_semicolon(&mut err, expr, expected, false);
fcx.suggest_missing_semicolon(&mut err, expr, expected, false, false);
let pointing_at_return_type = fcx.suggest_mismatched_types_on_tail(
&mut err,
expr,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self,
&cause,
|mut err| {
self.suggest_missing_semicolon(&mut err, expr, e_ty, false);
self.suggest_missing_semicolon(&mut err, expr, e_ty, false, false);
self.suggest_mismatched_types_on_tail(
&mut err, expr, ty, e_ty, target_id,
);
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expression: &'tcx hir::Expr<'tcx>,
expected: Ty<'tcx>,
needs_block: bool,
parent_is_closure: bool,
) {
if expected.is_unit() {
// `BlockTailExpression` only relevant if the tail expr would be
Expand Down Expand Up @@ -789,6 +790,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
}
}
ExprKind::Path(..) | ExprKind::Lit(_)
if parent_is_closure
&& !expression.span.in_external_macro(self.tcx.sess.source_map()) =>
{
err.span_suggestion_verbose(
expression.span.shrink_to_lo(),
"consider ignoring the value",
"_ = ",
Applicability::MachineApplicable,
);
}
_ => (),
}
}
Expand Down
10 changes: 10 additions & 0 deletions tests/ui/typeck/closure-ty-mismatch-issue-128561.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
fn main() {
b"abc".iter().for_each(|x| x); //~ ERROR: mismatched types

b"abc".iter().for_each(|x| dbg!(x)); //~ ERROR: mismatched types

b"abc".iter().for_each(|x| {
println!("{}", x);
x //~ ERROR: mismatched types
})
}
28 changes: 28 additions & 0 deletions tests/ui/typeck/closure-ty-mismatch-issue-128561.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
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 ignoring the value
|
LL | b"abc".iter().for_each(|x| _ = x);
| +++

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)
Comment on lines +12 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't notice in the macro case the parent type check here is not closure:

https://github.com/rust-lang/rust/pull/135562/files#diff-4931c060d9b3dc4c4a95c29607277bce4b456a157c7a72401c3f0441d03b76b3L1896-L1899

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:

https://github.com/chenyukang/rust/blob/3e380a8e3b2d7b9416ac7dedce6b5b055ca9a9a3/compiler/rustc_middle/src/hir/map/mod.rs#L169-L183

https://github.com/chenyukang/rust/blob/3e380a8e3b2d7b9416ac7dedce6b5b055ca9a9a3/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs#L813-L814

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :-/


error[E0308]: mismatched types
--> $DIR/closure-ty-mismatch-issue-128561.rs:8:9
|
LL | x
| ^ expected `()`, found `&u8`

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0308`.
Loading