From 369058eacde8ffdfeed9b362b10720799729a835 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 6 Mar 2019 17:54:35 -0800 Subject: [PATCH 1/3] Point at coercion reason for if exprs without else clause ``` error[E0317]: if may be missing an else clause --> $DIR/if-without-else-as-fn-expr.rs:2:5 | LL | fn foo(bar: usize) -> usize { | ----- found `usize` because of this return type LL | / if bar % 5 == 0 { LL | | return 3; LL | | } | |_____^ expected (), found usize | = note: expected type `()` found type `usize` = note: `if` expressions without `else` must evaluate to `()` ``` --- src/librustc_typeck/check/mod.rs | 33 ++++++++++++++++++- src/test/ui/if/if-without-else-as-fn-expr.rs | 10 ++++++ .../ui/if/if-without-else-as-fn-expr.stderr | 17 ++++++++++ src/test/ui/if/if-without-else-result.stderr | 1 + src/test/ui/issues/issue-4201.stderr | 1 + src/test/ui/issues/issue-50577.stderr | 1 + 6 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/if/if-without-else-as-fn-expr.rs create mode 100644 src/test/ui/if/if-without-else-as-fn-expr.stderr diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 183667e224462..be09af093e2ba 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -3472,8 +3472,39 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // We won't diverge unless both branches do (or the condition does). self.diverges.set(cond_diverges | then_diverges & else_diverges); } else { + // If this `if` expr is the parent's function return expr, the cause of the type + // coercion is the return type, point at it. (#25228) + let mut ret_reason = None; + if let Node::Block(block) = self.tcx.hir().get_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id(then_expr.hir_id), + ), + ) { + // check that the body's parent is an fn + let parent = self.tcx.hir().get_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id(block.hir_id), + ), + ); + if let (Some(expr), Node::Item(hir::Item { + node: hir::ItemKind::Fn(..), .. + })) = (&block.expr, parent) { + // check that the `if` expr without `else` is the fn body's expr + if expr.span == sp { + ret_reason = self.get_fn_decl(then_expr.hir_id).map(|(fn_decl, _)| ( + fn_decl.output.span(), + format!("found `{}` because of this return type", fn_decl.output), + )); + } + } + } let else_cause = self.cause(sp, ObligationCauseCode::IfExpressionWithNoElse); - coerce.coerce_forced_unit(self, &else_cause, &mut |_| (), true); + coerce.coerce_forced_unit(self, &else_cause, &mut |err| { + if let Some((sp, msg)) = &ret_reason { + err.span_label(*sp, msg.as_str()); + } + err.note("`if` expressions without `else` must evaluate to `()`"); + }, true); // If the condition is false we can't diverge. self.diverges.set(cond_diverges); diff --git a/src/test/ui/if/if-without-else-as-fn-expr.rs b/src/test/ui/if/if-without-else-as-fn-expr.rs new file mode 100644 index 0000000000000..76ffb49697f53 --- /dev/null +++ b/src/test/ui/if/if-without-else-as-fn-expr.rs @@ -0,0 +1,10 @@ +fn foo(bar: usize) -> usize { + if bar % 5 == 0 { + return 3; + } + //~^^^ ERROR if may be missing an else clause +} + +fn main() { + let _ = foo(1); +} diff --git a/src/test/ui/if/if-without-else-as-fn-expr.stderr b/src/test/ui/if/if-without-else-as-fn-expr.stderr new file mode 100644 index 0000000000000..b8628ee291d25 --- /dev/null +++ b/src/test/ui/if/if-without-else-as-fn-expr.stderr @@ -0,0 +1,17 @@ +error[E0317]: if may be missing an else clause + --> $DIR/if-without-else-as-fn-expr.rs:2:5 + | +LL | fn foo(bar: usize) -> usize { + | ----- found `usize` because of this return type +LL | / if bar % 5 == 0 { +LL | | return 3; +LL | | } + | |_____^ expected (), found usize + | + = note: expected type `()` + found type `usize` + = note: `if` expressions without `else` must evaluate to `()` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0317`. diff --git a/src/test/ui/if/if-without-else-result.stderr b/src/test/ui/if/if-without-else-result.stderr index 2134781088c3d..6e720d59af5bd 100644 --- a/src/test/ui/if/if-without-else-result.stderr +++ b/src/test/ui/if/if-without-else-result.stderr @@ -6,6 +6,7 @@ LL | let a = if true { true }; | = note: expected type `()` found type `bool` + = note: `if` expressions without `else` must evaluate to `()` error: aborting due to previous error diff --git a/src/test/ui/issues/issue-4201.stderr b/src/test/ui/issues/issue-4201.stderr index 4f8ec96d53151..fcda41d78cc2d 100644 --- a/src/test/ui/issues/issue-4201.stderr +++ b/src/test/ui/issues/issue-4201.stderr @@ -13,6 +13,7 @@ LL | | }; | = note: expected type `()` found type `{integer}` + = note: `if` expressions without `else` must evaluate to `()` error: aborting due to previous error diff --git a/src/test/ui/issues/issue-50577.stderr b/src/test/ui/issues/issue-50577.stderr index f26f5a9a9ba22..2747f9375f86b 100644 --- a/src/test/ui/issues/issue-50577.stderr +++ b/src/test/ui/issues/issue-50577.stderr @@ -6,6 +6,7 @@ LL | Drop = assert_eq!(1, 1) | = note: expected type `()` found type `isize` + = note: `if` expressions without `else` must evaluate to `()` = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error: aborting due to previous error From ffa40cb45c195e317602437d8a40178e72807d46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 7 Mar 2019 14:08:20 -0800 Subject: [PATCH 2/3] address review comments --- src/librustc_typeck/check/mod.rs | 58 ++++++++++--------- .../ui/if/if-without-else-as-fn-expr.stderr | 11 ++-- src/test/ui/if/if-without-else-result.stderr | 3 +- src/test/ui/issues/issue-4201.stderr | 3 +- src/test/ui/issues/issue-50577.stderr | 3 +- 5 files changed, 44 insertions(+), 34 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index be09af093e2ba..703a0dae63319 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -3474,37 +3474,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } else { // If this `if` expr is the parent's function return expr, the cause of the type // coercion is the return type, point at it. (#25228) - let mut ret_reason = None; - if let Node::Block(block) = self.tcx.hir().get_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id(then_expr.hir_id), - ), - ) { - // check that the body's parent is an fn - let parent = self.tcx.hir().get_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id(block.hir_id), - ), - ); - if let (Some(expr), Node::Item(hir::Item { - node: hir::ItemKind::Fn(..), .. - })) = (&block.expr, parent) { - // check that the `if` expr without `else` is the fn body's expr - if expr.span == sp { - ret_reason = self.get_fn_decl(then_expr.hir_id).map(|(fn_decl, _)| ( - fn_decl.output.span(), - format!("found `{}` because of this return type", fn_decl.output), - )); - } - } - } + let ret_reason = self.maybe_get_coercion_reason(then_expr.hir_id, sp); + let else_cause = self.cause(sp, ObligationCauseCode::IfExpressionWithNoElse); coerce.coerce_forced_unit(self, &else_cause, &mut |err| { if let Some((sp, msg)) = &ret_reason { err.span_label(*sp, msg.as_str()); } - err.note("`if` expressions without `else` must evaluate to `()`"); - }, true); + err.note("`if` expressions without `else` evaluate to `()`"); + err.help("consider adding an `else` block that evaluates to the expected type"); + }, ret_reason.is_none()); // If the condition is false we can't diverge. self.diverges.set(cond_diverges); @@ -3518,6 +3497,33 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } } + fn maybe_get_coercion_reason(&self, hir_id: hir::HirId, sp: Span) -> Option<(Span, String)> { + if let Node::Block(block) = self.tcx.hir().get_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id(hir_id), + ), + ) { + // check that the body's parent is an fn + let parent = self.tcx.hir().get_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id(block.hir_id), + ), + ); + if let (Some(expr), Node::Item(hir::Item { + node: hir::ItemKind::Fn(..), .. + })) = (&block.expr, parent) { + // check that the `if` expr without `else` is the fn body's expr + if expr.span == sp { + return self.get_fn_decl(hir_id).map(|(fn_decl, _)| ( + fn_decl.output.span(), + format!("expected `{}` because of this return type", fn_decl.output), + )); + } + } + } + None + } + // Check field access expressions fn check_field(&self, expr: &'gcx hir::Expr, diff --git a/src/test/ui/if/if-without-else-as-fn-expr.stderr b/src/test/ui/if/if-without-else-as-fn-expr.stderr index b8628ee291d25..062e0b9c44d79 100644 --- a/src/test/ui/if/if-without-else-as-fn-expr.stderr +++ b/src/test/ui/if/if-without-else-as-fn-expr.stderr @@ -2,15 +2,16 @@ error[E0317]: if may be missing an else clause --> $DIR/if-without-else-as-fn-expr.rs:2:5 | LL | fn foo(bar: usize) -> usize { - | ----- found `usize` because of this return type + | ----- expected `usize` because of this return type LL | / if bar % 5 == 0 { LL | | return 3; LL | | } - | |_____^ expected (), found usize + | |_____^ expected usize, found () | - = note: expected type `()` - found type `usize` - = note: `if` expressions without `else` must evaluate to `()` + = note: expected type `usize` + found type `()` + = note: `if` expressions without `else` evaluate to `()` + = help: consider adding an `else` block that evaluates to the expected type error: aborting due to previous error diff --git a/src/test/ui/if/if-without-else-result.stderr b/src/test/ui/if/if-without-else-result.stderr index 6e720d59af5bd..cb1df141bcb8d 100644 --- a/src/test/ui/if/if-without-else-result.stderr +++ b/src/test/ui/if/if-without-else-result.stderr @@ -6,7 +6,8 @@ LL | let a = if true { true }; | = note: expected type `()` found type `bool` - = note: `if` expressions without `else` must evaluate to `()` + = note: `if` expressions without `else` evaluate to `()` + = help: consider adding an `else` block that evaluates to the expected type error: aborting due to previous error diff --git a/src/test/ui/issues/issue-4201.stderr b/src/test/ui/issues/issue-4201.stderr index fcda41d78cc2d..53397c8ec90b9 100644 --- a/src/test/ui/issues/issue-4201.stderr +++ b/src/test/ui/issues/issue-4201.stderr @@ -13,7 +13,8 @@ LL | | }; | = note: expected type `()` found type `{integer}` - = note: `if` expressions without `else` must evaluate to `()` + = note: `if` expressions without `else` evaluate to `()` + = help: consider adding an `else` block that evaluates to the expected type error: aborting due to previous error diff --git a/src/test/ui/issues/issue-50577.stderr b/src/test/ui/issues/issue-50577.stderr index 2747f9375f86b..323f5ac6547a0 100644 --- a/src/test/ui/issues/issue-50577.stderr +++ b/src/test/ui/issues/issue-50577.stderr @@ -6,7 +6,8 @@ LL | Drop = assert_eq!(1, 1) | = note: expected type `()` found type `isize` - = note: `if` expressions without `else` must evaluate to `()` + = note: `if` expressions without `else` evaluate to `()` + = help: consider adding an `else` block that evaluates to the expected type = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error: aborting due to previous error From dcaec88a57cd0f388af1678f8db155b2add8b175 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 7 Mar 2019 14:37:18 -0800 Subject: [PATCH 3/3] Add more details to elseless if error --- src/librustc_typeck/check/mod.rs | 18 +++++++--- src/test/ui/if/if-without-else-as-fn-expr.rs | 15 +++++++++ .../ui/if/if-without-else-as-fn-expr.stderr | 33 ++++++++++++++++++- src/test/ui/if/if-without-else-result.stderr | 5 ++- src/test/ui/issues/issue-4201.stderr | 1 + src/test/ui/issues/issue-50577.stderr | 5 ++- 6 files changed, 69 insertions(+), 8 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 703a0dae63319..f2ab78e16fb97 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -3480,6 +3480,10 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { coerce.coerce_forced_unit(self, &else_cause, &mut |err| { if let Some((sp, msg)) = &ret_reason { err.span_label(*sp, msg.as_str()); + } else if let ExprKind::Block(block, _) = &then_expr.node { + if let Some(expr) = &block.expr { + err.span_label(expr.span, "found here".to_string()); + } } err.note("`if` expressions without `else` evaluate to `()`"); err.help("consider adding an `else` block that evaluates to the expected type"); @@ -3498,11 +3502,10 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } fn maybe_get_coercion_reason(&self, hir_id: hir::HirId, sp: Span) -> Option<(Span, String)> { - if let Node::Block(block) = self.tcx.hir().get_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id(hir_id), - ), - ) { + let node = self.tcx.hir().get_by_hir_id(self.tcx.hir().get_parent_node_by_hir_id( + self.tcx.hir().get_parent_node_by_hir_id(hir_id), + )); + if let Node::Block(block) = node { // check that the body's parent is an fn let parent = self.tcx.hir().get_by_hir_id( self.tcx.hir().get_parent_node_by_hir_id( @@ -3521,6 +3524,11 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } } } + if let Node::Local(hir::Local { + ty: Some(_), pat, .. + }) = node { + return Some((pat.span, "expected because of this assignment".to_string())); + } None } diff --git a/src/test/ui/if/if-without-else-as-fn-expr.rs b/src/test/ui/if/if-without-else-as-fn-expr.rs index 76ffb49697f53..67e4445629f8c 100644 --- a/src/test/ui/if/if-without-else-as-fn-expr.rs +++ b/src/test/ui/if/if-without-else-as-fn-expr.rs @@ -5,6 +5,21 @@ fn foo(bar: usize) -> usize { //~^^^ ERROR if may be missing an else clause } +fn foo2(bar: usize) -> usize { + let x: usize = if bar % 5 == 0 { + return 3; + }; + //~^^^ ERROR if may be missing an else clause + x +} + +fn foo3(bar: usize) -> usize { + if bar % 5 == 0 { + 3 + } + //~^^^ ERROR if may be missing an else clause +} + fn main() { let _ = foo(1); } diff --git a/src/test/ui/if/if-without-else-as-fn-expr.stderr b/src/test/ui/if/if-without-else-as-fn-expr.stderr index 062e0b9c44d79..0ba72726ca787 100644 --- a/src/test/ui/if/if-without-else-as-fn-expr.stderr +++ b/src/test/ui/if/if-without-else-as-fn-expr.stderr @@ -13,6 +13,37 @@ LL | | } = note: `if` expressions without `else` evaluate to `()` = help: consider adding an `else` block that evaluates to the expected type -error: aborting due to previous error +error[E0317]: if may be missing an else clause + --> $DIR/if-without-else-as-fn-expr.rs:9:20 + | +LL | let x: usize = if bar % 5 == 0 { + | _________-__________^ + | | | + | | expected because of this assignment +LL | | return 3; +LL | | }; + | |_____^ expected usize, found () + | + = note: expected type `usize` + found type `()` + = note: `if` expressions without `else` evaluate to `()` + = help: consider adding an `else` block that evaluates to the expected type + +error[E0317]: if may be missing an else clause + --> $DIR/if-without-else-as-fn-expr.rs:17:5 + | +LL | fn foo3(bar: usize) -> usize { + | ----- expected `usize` because of this return type +LL | / if bar % 5 == 0 { +LL | | 3 +LL | | } + | |_____^ expected usize, found () + | + = note: expected type `usize` + found type `()` + = note: `if` expressions without `else` evaluate to `()` + = help: consider adding an `else` block that evaluates to the expected type + +error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0317`. diff --git a/src/test/ui/if/if-without-else-result.stderr b/src/test/ui/if/if-without-else-result.stderr index cb1df141bcb8d..ddb013ab711fa 100644 --- a/src/test/ui/if/if-without-else-result.stderr +++ b/src/test/ui/if/if-without-else-result.stderr @@ -2,7 +2,10 @@ error[E0317]: if may be missing an else clause --> $DIR/if-without-else-result.rs:2:13 | LL | let a = if true { true }; - | ^^^^^^^^^^^^^^^^ expected (), found bool + | ^^^^^^^^^^----^^ + | | | + | | found here + | expected (), found bool | = note: expected type `()` found type `bool` diff --git a/src/test/ui/issues/issue-4201.stderr b/src/test/ui/issues/issue-4201.stderr index 53397c8ec90b9..4d7116a0ee972 100644 --- a/src/test/ui/issues/issue-4201.stderr +++ b/src/test/ui/issues/issue-4201.stderr @@ -8,6 +8,7 @@ LL | | //~| expected type `()` LL | | //~| found type `{integer}` LL | | //~| expected (), found integer LL | | 1 + | | - found here LL | | }; | |_____^ expected (), found integer | diff --git a/src/test/ui/issues/issue-50577.stderr b/src/test/ui/issues/issue-50577.stderr index 323f5ac6547a0..0c3ba2ea4f94d 100644 --- a/src/test/ui/issues/issue-50577.stderr +++ b/src/test/ui/issues/issue-50577.stderr @@ -2,7 +2,10 @@ error[E0317]: if may be missing an else clause --> $DIR/issue-50577.rs:3:16 | LL | Drop = assert_eq!(1, 1) - | ^^^^^^^^^^^^^^^^ expected (), found isize + | ^^^^^^^^^^^^^^^^ + | | + | expected (), found isize + | found here | = note: expected type `()` found type `isize`