]> git.lizzy.rs Git - rust.git/commitdiff
Do not suggest `;` if expression is side effect free
authorEsteban Küber <esteban@kuber.com.ar>
Fri, 5 Feb 2021 01:36:06 +0000 (17:36 -0800)
committerEsteban Küber <esteban@kuber.com.ar>
Mon, 22 Feb 2021 00:34:37 +0000 (16:34 -0800)
When a tail expression isn't unit, we previously always suggested adding
a trailing `;` to turn it into a statement. This suggestion isn't
appropriate for any expression that doesn't have side-effects, as the
user will have likely wanted to call something else or do something with
the resulting value, instead of just discarding it.

compiler/rustc_hir/src/hir.rs
compiler/rustc_typeck/src/check/coercion.rs
compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs
src/test/ui/block-result/block-must-not-have-result-while.stderr
src/test/ui/parser/expr-as-stmt-2.stderr
src/test/ui/parser/struct-literal-variant-in-if.stderr
src/test/ui/return/tail-expr-as-potential-return.stderr
src/test/ui/suggestions/match-needing-semi.fixed [deleted file]
src/test/ui/suggestions/match-needing-semi.rs
src/test/ui/suggestions/match-needing-semi.stderr

index 4df8c44e62b38a0506cb89cce4bca53ef3c21d94..840099841839ab47f55e4f71f4d044be6ad60ebe 100644 (file)
@@ -1,5 +1,6 @@
 // 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};
@@ -1554,6 +1555,63 @@ pub fn peel_drop_temps(&self) -> &Self {
         }
         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.
index f95627cfdee83d6f27100e1cf2e24836deab1076..159c97d8bfaa917d3a937300a80e0babe6beda42 100644 (file)
@@ -1450,7 +1450,9 @@ fn report_return_mismatched_types<'a>(
             ) {
                 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))
index 3326be796cee37831c2c816514d138c4abf61df6..155c10e891652f1b9cf75f8f5c401bdce8091e55 100644 (file)
@@ -561,7 +561,9 @@ pub fn check_stmt(&self, stmt: &'tcx hir::Stmt<'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) => {
index e5a2e1398178dfa0bab1cf70ada2f55d53ee40dd..416b75d9e2e0c0c51bbfae4c2c1753272c1dcf92 100644 (file)
@@ -44,11 +44,14 @@ pub fn suggest_mismatched_types_on_tail(
         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,7 +395,9 @@ fn suggest_missing_semicolon(
                 | ExprKind::Loop(..)
                 | ExprKind::If(..)
                 | ExprKind::Match(..)
-                | ExprKind::Block(..) => {
+                | ExprKind::Block(..)
+                    if expression.can_have_side_effects() =>
+                {
                     err.span_suggestion(
                         cause_span.shrink_to_hi(),
                         "consider using a semicolon here",
index d4845290d8a908d3528444f8d0f275dc0a28f759..7f96aa289d0abc565b87aa2ef6ae47f0abdf4c7e 100644 (file)
@@ -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
 
index 75c0e7bb47560f8894850074d55fb1812411a8d5..2a70127485785783128b9bd5c905fc9d6fa4b004 100644 (file)
@@ -7,10 +7,6 @@ LL |     if let Some(x) = a { true } else { false }
    |     |                    expected `()`, found `bool`
    |     expected this to be `()`
    |
-help: consider using a semicolon here
-   |
-LL |     if let Some(x) = a { true } else { false };
-   |                                               ^
 help: you might have meant to return this value
    |
 LL |     if let Some(x) = a { return true; } else { false }
@@ -25,10 +21,6 @@ LL |     if let Some(x) = a { true } else { false }
    |     |                                  expected `()`, found `bool`
    |     expected this to be `()`
    |
-help: consider using a semicolon here
-   |
-LL |     if let Some(x) = a { true } else { false };
-   |                                               ^
 help: you might have meant to return this value
    |
 LL |     if let Some(x) = a { true } else { return false; }
index a2252d4e4d2822943a42d3e28d5b2366f8ff1d23..3ea5ca565c5e530e9cd4525cc6dffafd6863dfab 100644 (file)
@@ -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 `()`
index 6eeaf5b3412e7a6040e395fe0943b0aa4315d421..52c63c8e223e47ff7c9d371449fb68d13b958769 100644 (file)
@@ -9,14 +9,6 @@ LL | |     }
    |
    = note: expected unit type `()`
                    found enum `std::result::Result<_, {integer}>`
-help: consider using a semicolon here
-   |
-LL |         Err(42);
-   |                ^
-help: consider using a semicolon here
-   |
-LL |     };
-   |      ^
 help: you might have meant to return this value
    |
 LL |         return Err(42);
diff --git a/src/test/ui/suggestions/match-needing-semi.fixed b/src/test/ui/suggestions/match-needing-semi.fixed
deleted file mode 100644 (file)
index 03cbed1..0000000
+++ /dev/null
@@ -1,18 +0,0 @@
-// check-only
-// run-rustfix
-
-fn main() {
-    match 3 {
-        4 => 1,
-        3 => {
-            2 //~ ERROR mismatched types
-        }
-        _ => 2
-    };
-    match 3 { //~ ERROR mismatched types
-        4 => 1,
-        3 => 2,
-        _ => 2
-    };
-    let _ = ();
-}
index f34071ac758868b9fb4c10b5244288dcebb8ebc7..833555d0e406e95e8b36f1a832223ba1ad43057d 100644 (file)
@@ -1,11 +1,10 @@
 // check-only
-// run-rustfix
 
 fn main() {
     match 3 {
         4 => 1,
         3 => {
-            2 //~ ERROR mismatched types
+            foo() //~ ERROR mismatched types
         }
         _ => 2
     }
@@ -16,3 +15,7 @@ fn main() {
     }
     let _ = ();
 }
+
+fn foo() -> i32 {
+    42
+}
index 28abd089525df66cabcb4158bced6672cb42b070..3739c9940f0cc268c13938e49aaecd05c6752367 100644 (file)
@@ -1,20 +1,27 @@
 error[E0308]: mismatched types
-  --> $DIR/match-needing-semi.rs:8:13
+  --> $DIR/match-needing-semi.rs:7:13
    |
 LL | /     match 3 {
 LL | |         4 => 1,
 LL | |         3 => {
-LL | |             2
-   | |             ^ expected `()`, found integer
+LL | |             foo()
+   | |             ^^^^^ expected `()`, found `i32`
 LL | |         }
 LL | |         _ => 2
 LL | |     }
-   | |     -- help: consider using a semicolon here
-   | |_____|
-   |       expected this to be `()`
+   | |_____- expected this to be `()`
+   |
+help: consider using a semicolon here
+   |
+LL |             foo();
+   |                  ^
+help: consider using a semicolon here
+   |
+LL |     };
+   |      ^
 
 error[E0308]: mismatched types
-  --> $DIR/match-needing-semi.rs:12:5
+  --> $DIR/match-needing-semi.rs:11:5
    |
 LL | /     match 3 {
 LL | |         4 => 1,