Rollup merge of #81769 - estebank:tail-expr-as-potential-return, r=lcnr

Dylan-DPC · web-flow · commit 5d90e89c3646 · 2021-02-23T02:51:46.000+01:00
Suggest `return`ing tail expressions that match return type

Some newcomers are confused by the behavior of tail expressions,
interpreting that "leaving out the `;` makes it the return value".
To help them go in the right direction, suggest using `return` instead
when applicable.
diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs
@@ -1,5 +1,5 @@
 // ignore-tidy-filelength
-use crate::def::{DefKind, Namespace, Res};
+use crate::def::{CtorKind, DefKind, Namespace, Res};
 use crate::def_id::DefId;
 crate use crate::hir_id::HirId;
 use crate::{itemlikevisit, LangItem};
@@ -1576,6 +1576,63 @@ impl Expr<'_> {
         }
         expr
     }
+
+    pub fn can_have_side_effects(&self) -> bool {
+        match self.peel_drop_temps().kind {
+            ExprKind::Path(_) | ExprKind::Lit(_) => false,
+            ExprKind::Type(base, _)
+            | ExprKind::Unary(_, base)
+            | ExprKind::Field(base, _)
+            | ExprKind::Index(base, _)
+            | ExprKind::AddrOf(.., base)
+            | ExprKind::Cast(base, _) => {
+                // This isn't exactly true for `Index` and all `Unnary`, but we are using this
+                // method exclusively for diagnostics and there's a *cultural* pressure against
+                // them being used only for its side-effects.
+                base.can_have_side_effects()
+            }
+            ExprKind::Struct(_, fields, init) => fields
+                .iter()
+                .map(|field| field.expr)
+                .chain(init.into_iter())
+                .all(|e| e.can_have_side_effects()),
+
+            ExprKind::Array(args)
+            | ExprKind::Tup(args)
+            | ExprKind::Call(
+                Expr {
+                    kind:
+                        ExprKind::Path(QPath::Resolved(
+                            None,
+                            Path { res: Res::Def(DefKind::Ctor(_, CtorKind::Fn), _), .. },
+                        )),
+                    ..
+                },
+                args,
+            ) => args.iter().all(|arg| arg.can_have_side_effects()),
+            ExprKind::If(..)
+            | ExprKind::Match(..)
+            | ExprKind::MethodCall(..)
+            | ExprKind::Call(..)
+            | ExprKind::Closure(..)
+            | ExprKind::Block(..)
+            | ExprKind::Repeat(..)
+            | ExprKind::Break(..)
+            | ExprKind::Continue(..)
+            | ExprKind::Ret(..)
+            | ExprKind::Loop(..)
+            | ExprKind::Assign(..)
+            | ExprKind::InlineAsm(..)
+            | ExprKind::LlvmInlineAsm(..)
+            | ExprKind::AssignOp(..)
+            | ExprKind::ConstBlock(..)
+            | ExprKind::Box(..)
+            | ExprKind::Binary(..)
+            | ExprKind::Yield(..)
+            | ExprKind::DropTemps(..)
+            | ExprKind::Err => true,
+        }
+    }
 }
 
 /// Checks if the specified expression is a built-in range literal.
diff --git a/compiler/rustc_typeck/src/check/callee.rs b/compiler/rustc_typeck/src/check/callee.rs
@@ -346,7 +346,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                             if call_is_multiline {
                                 err.span_suggestion(
                                     callee.span.shrink_to_hi(),
-                                    "try adding a semicolon",
+                                    "consider using a semicolon here",
                                     ";".to_owned(),
                                     Applicability::MaybeIncorrect,
                                 );
diff --git a/compiler/rustc_typeck/src/check/coercion.rs b/compiler/rustc_typeck/src/check/coercion.rs
@@ -1450,15 +1450,17 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
             ) {
                 if cond_expr.span.desugaring_kind().is_none() {
                     err.span_label(cond_expr.span, "expected this to be `()`");
-                    fcx.suggest_semicolon_at_end(cond_expr.span, &mut err);
+                    if expr.can_have_side_effects() {
+                        fcx.suggest_semicolon_at_end(cond_expr.span, &mut err);
+                    }
                 }
             }
             fcx.get_node_fn_decl(parent).map(|(fn_decl, _, is_main)| (fn_decl, is_main))
         } else {
             fcx.get_fn_decl(parent_id)
         };
 
-        if let (Some((fn_decl, can_suggest)), _) = (fn_decl, pointing_at_return_type) {
+        if let Some((fn_decl, can_suggest)) = fn_decl {
             if expression.is_none() {
                 pointing_at_return_type |= fcx.suggest_missing_return_type(
                     &mut err,
@@ -1472,6 +1474,16 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                 fn_output = Some(&fn_decl.output); // `impl Trait` return type
             }
         }
+
+        let parent_id = fcx.tcx.hir().get_parent_item(id);
+        let parent_item = fcx.tcx.hir().get(parent_id);
+
+        if let (Some((expr, _)), Some((fn_decl, _, _))) =
+            (expression, fcx.get_node_fn_decl(parent_item))
+        {
+            fcx.suggest_missing_return_expr(&mut err, expr, fn_decl, expected, found);
+        }
+
         if let (Some(sp), Some(fn_output)) = (fcx.ret_coercion_span.get(), fn_output) {
             self.add_impl_trait_explanation(&mut err, cause, fcx, expected, sp, fn_output);
         }
diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
@@ -561,7 +561,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             hir::StmtKind::Expr(ref expr) => {
                 // Check with expected type of `()`.
                 self.check_expr_has_type_or_error(&expr, self.tcx.mk_unit(), |err| {
-                    self.suggest_semicolon_at_end(expr.span, err);
+                    if expr.can_have_side_effects() {
+                        self.suggest_semicolon_at_end(expr.span, err);
+                    }
                 });
             }
             hir::StmtKind::Semi(ref expr) => {
diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs b/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs
@@ -44,11 +44,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         blk_id: hir::HirId,
     ) -> bool {
         let expr = expr.peel_drop_temps();
-        self.suggest_missing_semicolon(err, expr, expected, cause_span);
+        if expr.can_have_side_effects() {
+            self.suggest_missing_semicolon(err, expr, expected, cause_span);
+        }
         let mut pointing_at_return_type = false;
         if let Some((fn_decl, can_suggest)) = self.get_fn_decl(blk_id) {
             pointing_at_return_type =
                 self.suggest_missing_return_type(err, &fn_decl, expected, found, can_suggest);
+            self.suggest_missing_return_expr(err, expr, &fn_decl, expected, found);
         }
         pointing_at_return_type
     }
@@ -392,10 +395,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 | ExprKind::Loop(..)
                 | ExprKind::If(..)
                 | ExprKind::Match(..)
-                | ExprKind::Block(..) => {
+                | ExprKind::Block(..)
+                    if expression.can_have_side_effects() =>
+                {
                     err.span_suggestion(
                         cause_span.shrink_to_hi(),
-                        "try adding a semicolon",
+                        "consider using a semicolon here",
                         ";".to_string(),
                         Applicability::MachineApplicable,
                     );
@@ -464,6 +469,34 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 
+    pub(in super::super) fn suggest_missing_return_expr(
+        &self,
+        err: &mut DiagnosticBuilder<'_>,
+        expr: &'tcx hir::Expr<'tcx>,
+        fn_decl: &hir::FnDecl<'_>,
+        expected: Ty<'tcx>,
+        found: Ty<'tcx>,
+    ) {
+        if !expected.is_unit() {
+            return;
+        }
+        let found = self.resolve_vars_with_obligations(found);
+        if let hir::FnRetTy::Return(ty) = fn_decl.output {
+            let ty = AstConv::ast_ty_to_ty(self, ty);
+            let ty = self.normalize_associated_types_in(expr.span, ty);
+            if self.can_coerce(found, ty) {
+                err.multipart_suggestion(
+                    "you might have meant to return this value",
+                    vec![
+                        (expr.span.shrink_to_lo(), "return ".to_string()),
+                        (expr.span.shrink_to_hi(), ";".to_string()),
+                    ],
+                    Applicability::MaybeIncorrect,
+                );
+            }
+        }
+    }
+
     pub(in super::super) fn suggest_missing_parentheses(
         &self,
         err: &mut DiagnosticBuilder<'_>,
diff --git a/src/test/ui/async-await/suggest-missing-await.rs b/src/test/ui/async-await/suggest-missing-await.rs
@@ -21,7 +21,7 @@ async fn dummy() {}
 async fn suggest_await_in_async_fn_return() {
     dummy()
     //~^ ERROR mismatched types [E0308]
-    //~| HELP try adding a semicolon
+    //~| HELP consider using a semicolon here
     //~| HELP consider `await`ing on the `Future`
     //~| SUGGESTION .await
 }
diff --git a/src/test/ui/async-await/suggest-missing-await.stderr b/src/test/ui/async-await/suggest-missing-await.stderr
@@ -29,7 +29,7 @@ help: consider `await`ing on the `Future`
    |
 LL |     dummy().await
    |            ^^^^^^
-help: try adding a semicolon
+help: consider using a semicolon here
    |
 LL |     dummy();
    |            ^
diff --git a/src/test/ui/block-result/block-must-not-have-result-while.stderr b/src/test/ui/block-result/block-must-not-have-result-while.stderr
@@ -14,9 +14,7 @@ LL | |         true
    | |         ^^^^ expected `()`, found `bool`
 LL | |
 LL | |     }
-   | |     -- help: consider using a semicolon here
-   | |_____|
-   |       expected this to be `()`
+   | |_____- expected this to be `()`
 
 error: aborting due to previous error; 1 warning emitted
 
diff --git a/src/test/ui/block-result/unexpected-return-on-unit.stderr b/src/test/ui/block-result/unexpected-return-on-unit.stderr
@@ -4,7 +4,7 @@ error[E0308]: mismatched types
 LL |     foo()
    |     ^^^^^ expected `()`, found `usize`
    |
-help: try adding a semicolon
+help: consider using a semicolon here
    |
 LL |     foo();
    |          ^
diff --git a/src/test/ui/macros/empty-trailing-stmt.stderr b/src/test/ui/macros/empty-trailing-stmt.stderr
@@ -3,6 +3,11 @@ error[E0308]: mismatched types
    |
 LL |     { true }
    |       ^^^^ expected `()`, found `bool`
+   |
+help: you might have meant to return this value
+   |
+LL |     { return true; }
+   |       ^^^^^^     ^
 
 error[E0308]: mismatched types
   --> $DIR/empty-trailing-stmt.rs:5:13
diff --git a/src/test/ui/parser/expr-as-stmt-2.stderr b/src/test/ui/parser/expr-as-stmt-2.stderr
@@ -2,19 +2,29 @@ error[E0308]: mismatched types
   --> $DIR/expr-as-stmt-2.rs:3:26
    |
 LL |     if let Some(x) = a { true } else { false }
-   |     ---------------------^^^^------------------ help: consider using a semicolon here
+   |     ---------------------^^^^-----------------
    |     |                    |
    |     |                    expected `()`, found `bool`
    |     expected this to be `()`
+   |
+help: you might have meant to return this value
+   |
+LL |     if let Some(x) = a { return true; } else { false }
+   |                          ^^^^^^     ^
 
 error[E0308]: mismatched types
   --> $DIR/expr-as-stmt-2.rs:3:40
    |
 LL |     if let Some(x) = a { true } else { false }
-   |     -----------------------------------^^^^^--- help: consider using a semicolon here
+   |     -----------------------------------^^^^^--
    |     |                                  |
    |     |                                  expected `()`, found `bool`
    |     expected this to be `()`
+   |
+help: you might have meant to return this value
+   |
+LL |     if let Some(x) = a { true } else { return false; }
+   |                                        ^^^^^^      ^
 
 error[E0308]: mismatched types
   --> $DIR/expr-as-stmt-2.rs:6:5
diff --git a/src/test/ui/parser/expr-as-stmt.stderr b/src/test/ui/parser/expr-as-stmt.stderr
@@ -40,24 +40,44 @@ error[E0308]: mismatched types
    |
 LL |     {2} + {2}
    |      ^ expected `()`, found integer
+   |
+help: you might have meant to return this value
+   |
+LL |     {return 2;} + {2}
+   |      ^^^^^^  ^
 
 error[E0308]: mismatched types
   --> $DIR/expr-as-stmt.rs:12:6
    |
 LL |     {2} + 2
    |      ^ expected `()`, found integer
+   |
+help: you might have meant to return this value
+   |
+LL |     {return 2;} + 2
+   |      ^^^^^^  ^
 
 error[E0308]: mismatched types
   --> $DIR/expr-as-stmt.rs:18:7
    |
 LL |     { 42 } + foo;
    |       ^^ expected `()`, found integer
+   |
+help: you might have meant to return this value
+   |
+LL |     { return 42; } + foo;
+   |       ^^^^^^   ^
 
 error[E0308]: mismatched types
   --> $DIR/expr-as-stmt.rs:24:7
    |
 LL |     { 3 } * 3
    |       ^ expected `()`, found integer
+   |
+help: you might have meant to return this value
+   |
+LL |     { return 3; } * 3
+   |       ^^^^^^  ^
 
 error[E0614]: type `{integer}` cannot be dereferenced
   --> $DIR/expr-as-stmt.rs:24:11
diff --git a/src/test/ui/parser/struct-literal-variant-in-if.stderr b/src/test/ui/parser/struct-literal-variant-in-if.stderr
@@ -57,7 +57,7 @@ error[E0308]: mismatched types
   --> $DIR/struct-literal-variant-in-if.rs:10:20
    |
 LL |     if x == E::V { field } {}
-   |     ---------------^^^^^--- help: consider using a semicolon here
+   |     ---------------^^^^^--
    |     |              |
    |     |              expected `()`, found `bool`
    |     expected this to be `()`
diff --git a/src/test/ui/proc-macro/issue-37788.stderr b/src/test/ui/proc-macro/issue-37788.stderr
@@ -5,7 +5,7 @@ LL | fn main() {
    |           - expected `()` because of default return type
 LL |     // Test that constructing the `visible_parent_map` (in `cstore_impl.rs`) does not ICE.
 LL |     std::cell::Cell::new(0)
-   |     ^^^^^^^^^^^^^^^^^^^^^^^- help: try adding a semicolon: `;`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^- help: consider using a semicolon here: `;`
    |     |
    |     expected `()`, found struct `Cell`
    |
diff --git a/src/test/ui/return/return-type.stderr b/src/test/ui/return/return-type.stderr
@@ -6,7 +6,7 @@ LL |     foo(4 as usize)
    |
    = note: expected unit type `()`
                  found struct `S<usize>`
-help: try adding a semicolon
+help: consider using a semicolon here
    |
 LL |     foo(4 as usize);
    |                    ^
diff --git a/src/test/ui/return/tail-expr-as-potential-return.rs b/src/test/ui/return/tail-expr-as-potential-return.rs
@@ -0,0 +1,10 @@
+fn main() {
+    let _ = foo(true);
+}
+
+fn foo(x: bool) -> Result<f64, i32> {
+    if x {
+        Err(42) //~ ERROR mismatched types
+    }
+    Ok(42.0)
+}
diff --git a/src/test/ui/return/tail-expr-as-potential-return.stderr b/src/test/ui/return/tail-expr-as-potential-return.stderr
@@ -0,0 +1,19 @@
+error[E0308]: mismatched types
+  --> $DIR/tail-expr-as-potential-return.rs:7:9
+   |
+LL | /     if x {
+LL | |         Err(42)
+   | |         ^^^^^^^ expected `()`, found enum `Result`
+LL | |     }
+   | |_____- expected this to be `()`
+   |
+   = note: expected unit type `()`
+                   found enum `Result<_, {integer}>`
+help: you might have meant to return this value
+   |
+LL |         return Err(42);
+   |         ^^^^^^        ^
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0308`.
diff --git a/src/test/ui/specialization/specialization-default-projection.stderr b/src/test/ui/specialization/specialization-default-projection.stderr
@@ -29,7 +29,7 @@ LL | fn monomorphic() -> () {
    |                     -- expected `()` because of return type
 ...
 LL |     generic::<()>()
-   |     ^^^^^^^^^^^^^^^- help: try adding a semicolon: `;`
+   |     ^^^^^^^^^^^^^^^- help: consider using a semicolon here: `;`
    |     |
    |     expected `()`, found associated type
    |
diff --git a/src/test/ui/suggestions/issue-51055-missing-semicolon-between-call-and-tuple.stderr b/src/test/ui/suggestions/issue-51055-missing-semicolon-between-call-and-tuple.stderr
@@ -5,7 +5,7 @@ LL |   fn vindictive() -> bool { true }
    |   ----------------------- `vindictive` defined here returns `bool`
 ...
 LL |       vindictive()
-   |       -^^^^^^^^^^^- help: try adding a semicolon: `;`
+   |       -^^^^^^^^^^^- help: consider using a semicolon here: `;`
    |  _____|
    | |
 LL | |     (1, 2)
diff --git a/src/test/ui/suggestions/match-needing-semi.fixed b/src/test/ui/suggestions/match-needing-semi.fixed
diff --git a/src/test/ui/suggestions/match-needing-semi.rs b/src/test/ui/suggestions/match-needing-semi.rs
diff --git a/src/test/ui/suggestions/match-needing-semi.stderr b/src/test/ui/suggestions/match-needing-semi.stderr

Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@ async fn dummy() {}`
`21`	`21`	`async fn suggest_await_in_async_fn_return() {`
`22`	`22`	`dummy()`
`23`	`23`	`//~^ ERROR mismatched types [E0308]`
`24`		`- //~\| HELP try adding a semicolon`
	`24`	`+ //~\| HELP consider using a semicolon here`
`25`	`25`	//~\| HELP consider `await`ing on the `Future`
`26`	`26`	`//~\| SUGGESTION .await`
`27`	`27`	`}`
Original file line number	Diff line number	Diff line change
@@ -29,7 +29,7 @@ help: consider `await`ing on the `Future`
`29`	`29`	`\|`
`30`	`30`	`LL \| dummy().await`
`31`	`31`	`\| ^^^^^^`
`32`		`-help: try adding a semicolon`
	`32`	`+help: consider using a semicolon here`
`33`	`33`	`\|`
`34`	`34`	`LL \| dummy();`
`35`	`35`	`\| ^`
Original file line number	Diff line number	Diff line change
`@@ -4,7 +4,7 @@ error[E0308]: mismatched types`
`4`	`4`	`LL \| foo()`
`5`	`5`	\| ^^^^^ expected `()`, found `usize`
`6`	`6`	`\|`
`7`		`-help: try adding a semicolon`
	`7`	`+help: consider using a semicolon here`
`8`	`8`	`\|`
`9`	`9`	`LL \| foo();`
`10`	`10`	`\| ^`
Original file line number	Diff line number	Diff line change
`@@ -57,7 +57,7 @@ error[E0308]: mismatched types`
`57`	`57`	`--> $DIR/struct-literal-variant-in-if.rs:10:20`
`58`	`58`	`\|`
`59`	`59`	`LL \| if x == E::V { field } {}`
`60`		`- \| ---------------^^^^^--- help: consider using a semicolon here`
	`60`	`+ \| ---------------^^^^^--`
`61`	`61`	`\| \| \|`
`62`	`62`	\| \| expected `()`, found `bool`
`63`	`63`	\| expected this to be `()`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ LL \| fn main() {`
`5`	`5`	\| - expected `()` because of default return type
`6`	`6`	LL \| // Test that constructing the `visible_parent_map` (in `cstore_impl.rs`) does not ICE.
`7`	`7`	`LL \| std::cell::Cell::new(0)`
`8`		- \| ^^^^^^^^^^^^^^^^^^^^^^^- help: try adding a semicolon: `;`
	`8`	+ \| ^^^^^^^^^^^^^^^^^^^^^^^- help: consider using a semicolon here: `;`
`9`	`9`	`\| \|`
`10`	`10`	\| expected `()`, found struct `Cell`
`11`	`11`	`\|`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@ LL \| foo(4 as usize)`
`6`	`6`	`\|`
`7`	`7`	= note: expected unit type `()`
`8`	`8`	found struct `S<usize>`
`9`		`-help: try adding a semicolon`
	`9`	`+help: consider using a semicolon here`
`10`	`10`	`\|`
`11`	`11`	`LL \| foo(4 as usize);`
`12`	`12`	`\| ^`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ LL \| fn monomorphic() -> () {`
`29`	`29`	\| -- expected `()` because of return type
`30`	`30`	`...`
`31`	`31`	`LL \| generic::<()>()`
`32`		- \| ^^^^^^^^^^^^^^^- help: try adding a semicolon: `;`
	`32`	+ \| ^^^^^^^^^^^^^^^- help: consider using a semicolon here: `;`
`33`	`33`	`\| \|`
`34`	`34`	\| expected `()`, found associated type
`35`	`35`	`\|`