Skip to content

Commit d669882

Browse files
estebankestebank
committed
Do not suggest ; if expression is side effect free
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.
1 parent 020edd9 commit d669882

File tree

11 files changed

+92
-51
lines changed

11 files changed

+92
-51
lines changed

‎compiler/rustc_hir/src/hir.rs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// ignore-tidy-filelength
22
use crate::def::{DefKind, Namespace, Res};
3+
use crate::def::{CtorKind, DefKind, Namespace, Res};
34
use crate::def_id::DefId;
45
crate use crate::hir_id::HirId;
56
use crate::{itemlikevisit, LangItem};
@@ -1554,6 +1555,63 @@ impl Expr<'_> {
15541555
}
15551556
expr
15561557
}
1558+
1559+
pub fn can_have_side_effects(&self) -> bool {
1560+
match self.peel_drop_temps().kind {
1561+
ExprKind::Path(_) | ExprKind::Lit(_) => false,
1562+
ExprKind::Type(base, _)
1563+
| ExprKind::Unary(_, base)
1564+
| ExprKind::Field(base, _)
1565+
| ExprKind::Index(base, _)
1566+
| ExprKind::AddrOf(.., base)
1567+
| ExprKind::Cast(base, _) => {
1568+
// This isn't exactly true for `Index` and all `Unnary`, but we are using this
1569+
// method exclusively for diagnostics and there's a *cultural* pressure against
1570+
// them being used only for its side-effects.
1571+
base.can_have_side_effects()
1572+
}
1573+
ExprKind::Struct(_, fields, init) => fields
1574+
.iter()
1575+
.map(|field| field.expr)
1576+
.chain(init.into_iter())
1577+
.all(|e| e.can_have_side_effects()),
1578+
1579+
ExprKind::Array(args)
1580+
| ExprKind::Tup(args)
1581+
| ExprKind::Call(
1582+
Expr {
1583+
kind:
1584+
ExprKind::Path(QPath::Resolved(
1585+
None,
1586+
Path { res: Res::Def(DefKind::Ctor(_, CtorKind::Fn), _), .. },
1587+
)),
1588+
..
1589+
},
1590+
args,
1591+
) => args.iter().all(|arg| arg.can_have_side_effects()),
1592+
ExprKind::If(..)
1593+
| ExprKind::Match(..)
1594+
| ExprKind::MethodCall(..)
1595+
| ExprKind::Call(..)
1596+
| ExprKind::Closure(..)
1597+
| ExprKind::Block(..)
1598+
| ExprKind::Repeat(..)
1599+
| ExprKind::Break(..)
1600+
| ExprKind::Continue(..)
1601+
| ExprKind::Ret(..)
1602+
| ExprKind::Loop(..)
1603+
| ExprKind::Assign(..)
1604+
| ExprKind::InlineAsm(..)
1605+
| ExprKind::LlvmInlineAsm(..)
1606+
| ExprKind::AssignOp(..)
1607+
| ExprKind::ConstBlock(..)
1608+
| ExprKind::Box(..)
1609+
| ExprKind::Binary(..)
1610+
| ExprKind::Yield(..)
1611+
| ExprKind::DropTemps(..)
1612+
| ExprKind::Err => true,
1613+
}
1614+
}
15571615
}
15581616

15591617
/// Checks if the specified expression is a built-in range literal.

‎compiler/rustc_typeck/src/check/coercion.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1450,7 +1450,9 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
14501450
) {
14511451
if cond_expr.span.desugaring_kind().is_none() {
14521452
err.span_label(cond_expr.span, "expected this to be `()`");
1453-
fcx.suggest_semicolon_at_end(cond_expr.span, &mut err);
1453+
if expr.can_have_side_effects() {
1454+
fcx.suggest_semicolon_at_end(cond_expr.span, &mut err);
1455+
}
14541456
}
14551457
}
14561458
fcx.get_node_fn_decl(parent).map(|(fn_decl, _, is_main)| (fn_decl, is_main))

‎compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
561561
hir::StmtKind::Expr(ref expr) => {
562562
// Check with expected type of `()`.
563563
self.check_expr_has_type_or_error(&expr, self.tcx.mk_unit(), |err| {
564-
self.suggest_semicolon_at_end(expr.span, err);
564+
if expr.can_have_side_effects() {
565+
self.suggest_semicolon_at_end(expr.span, err);
566+
}
565567
});
566568
}
567569
hir::StmtKind::Semi(ref expr) => {

‎compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
4444
blk_id: hir::HirId,
4545
) -> bool {
4646
let expr = expr.peel_drop_temps();
47-
self.suggest_missing_semicolon(err, expr, expected, cause_span);
47+
if expr.can_have_side_effects() {
48+
self.suggest_missing_semicolon(err, expr, expected, cause_span);
49+
}
4850
let mut pointing_at_return_type = false;
4951
if let Some((fn_decl, can_suggest)) = self.get_fn_decl(blk_id) {
5052
pointing_at_return_type =
5153
self.suggest_missing_return_type(err, &fn_decl, expected, found, can_suggest);
54+
self.suggest_missing_return_expr(err, expr, &fn_decl, expected, found);
5255
}
5356
pointing_at_return_type
5457
}
@@ -392,7 +395,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
392395
| ExprKind::Loop(..)
393396
| ExprKind::If(..)
394397
| ExprKind::Match(..)
395-
| ExprKind::Block(..) => {
398+
| ExprKind::Block(..)
399+
if expression.can_have_side_effects() =>
400+
{
396401
err.span_suggestion(
397402
cause_span.shrink_to_hi(),
398403
"consider using a semicolon here",

‎src/test/ui/block-result/block-must-not-have-result-while.stderr

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,7 @@ LL | | true
1414
| | ^^^^ expected `()`, found `bool`
1515
LL | |
1616
LL | | }
17-
| | -- help: consider using a semicolon here
18-
| |_____|
19-
| expected this to be `()`
17+
| |_____- expected this to be `()`
2018

2119
error: aborting due to previous error; 1 warning emitted
2220

‎src/test/ui/parser/expr-as-stmt-2.stderr

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,6 @@ LL | if let Some(x) = a { true } else { false }
77
| | expected `()`, found `bool`
88
| expected this to be `()`
99
|
10-
help: consider using a semicolon here
11-
|
12-
LL | if let Some(x) = a { true } else { false };
13-
| ^
1410
help: you might have meant to return this value
1511
|
1612
LL | if let Some(x) = a { return true; } else { false }
@@ -25,10 +21,6 @@ LL | if let Some(x) = a { true } else { false }
2521
| | expected `()`, found `bool`
2622
| expected this to be `()`
2723
|
28-
help: consider using a semicolon here
29-
|
30-
LL | if let Some(x) = a { true } else { false };
31-
| ^
3224
help: you might have meant to return this value
3325
|
3426
LL | if let Some(x) = a { true } else { return false; }

‎src/test/ui/parser/struct-literal-variant-in-if.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ error[E0308]: mismatched types
5757
--> $DIR/struct-literal-variant-in-if.rs:10:20
5858
|
5959
LL | if x == E::V { field } {}
60-
| ---------------^^^^^--- help: consider using a semicolon here
60+
| ---------------^^^^^--
6161
| | |
6262
| | expected `()`, found `bool`
6363
| expected this to be `()`

‎src/test/ui/return/tail-expr-as-potential-return.stderr

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,6 @@ LL | | }
99
|
1010
= note: expected unit type `()`
1111
found enum `std::result::Result<_, {integer}>`
12-
help: consider using a semicolon here
13-
|
14-
LL | Err(42);
15-
| ^
16-
help: consider using a semicolon here
17-
|
18-
LL | };
19-
| ^
2012
help: you might have meant to return this value
2113
|
2214
LL | return Err(42);

‎src/test/ui/suggestions/match-needing-semi.fixed

Lines changed: 0 additions & 18 deletions
This file was deleted.

‎src/test/ui/suggestions/match-needing-semi.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
// check-only
2-
// run-rustfix
32

43
fn main() {
54
match 3 {
65
4 => 1,
76
3 => {
8-
2 //~ ERROR mismatched types
7+
foo() //~ ERROR mismatched types
98
}
109
_ => 2
1110
}
@@ -16,3 +15,7 @@ fn main() {
1615
}
1716
let _ = ();
1817
}
18+
19+
fn foo() -> i32 {
20+
42
21+
}

‎src/test/ui/suggestions/match-needing-semi.stderr

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,27 @@
11
error[E0308]: mismatched types
2-
--> $DIR/match-needing-semi.rs:8:13
2+
--> $DIR/match-needing-semi.rs:7:13
33
|
44
LL | / match 3 {
55
LL | | 4 => 1,
66
LL | | 3 => {
7-
LL | | 2
8-
| | ^ expected `()`, found integer
7+
LL | | foo()
8+
| | ^^^^^ expected `()`, found `i32`
99
LL | | }
1010
LL | | _ => 2
1111
LL | | }
12-
| | -- help: consider using a semicolon here
13-
| |_____|
14-
| expected this to be `()`
12+
| |_____- expected this to be `()`
13+
|
14+
help: consider using a semicolon here
15+
|
16+
LL | foo();
17+
| ^
18+
help: consider using a semicolon here
19+
|
20+
LL | };
21+
| ^
1522

1623
error[E0308]: mismatched types
17-
--> $DIR/match-needing-semi.rs:12:5
24+
--> $DIR/match-needing-semi.rs:11:5
1825
|
1926
LL | / match 3 {
2027
LL | | 4 => 1,

0 commit comments

Comments
 (0)