diff options
| -rw-r--r-- | compiler/rustc_ast_lowering/src/expr.rs | 45 | ||||
| -rw-r--r-- | compiler/rustc_hir_analysis/src/check/region.rs | 38 | ||||
| -rw-r--r-- | compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs | 6 | ||||
| -rw-r--r-- | compiler/rustc_hir_typeck/src/lib.rs | 6 | ||||
| -rw-r--r-- | compiler/rustc_hir_typeck/src/pat.rs | 12 | ||||
| -rw-r--r-- | compiler/rustc_span/src/hygiene.rs | 7 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_lints/src/booleans.rs | 5 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_lints/src/if_not_else.rs | 1 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_lints/src/implicit_saturating_add.rs | 5 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_lints/src/let_if_seq.rs | 11 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_utils/src/higher.rs | 49 | ||||
| -rw-r--r-- | src/tools/clippy/tests/ui/author/if.stdout | 3 | ||||
| -rw-r--r-- | src/tools/clippy/tests/ui/author/struct.stdout | 3 |
13 files changed, 63 insertions, 128 deletions
diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs index 8747e624a4a..15e736261d5 100644 --- a/compiler/rustc_ast_lowering/src/expr.rs +++ b/compiler/rustc_ast_lowering/src/expr.rs @@ -528,7 +528,7 @@ impl<'hir> LoweringContext<'_, 'hir> { then: &Block, else_opt: Option<&Expr>, ) -> hir::ExprKind<'hir> { - let lowered_cond = self.lower_cond(cond); + let lowered_cond = self.lower_expr(cond); let then_expr = self.lower_block_expr(then); if let Some(rslt) = else_opt { hir::ExprKind::If( @@ -541,44 +541,6 @@ impl<'hir> LoweringContext<'_, 'hir> { } } - // Lowers a condition (i.e. `cond` in `if cond` or `while cond`), wrapping it in a terminating scope - // so that temporaries created in the condition don't live beyond it. - fn lower_cond(&mut self, cond: &Expr) -> &'hir hir::Expr<'hir> { - fn has_let_expr(expr: &Expr) -> bool { - match &expr.kind { - ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs), - ExprKind::Let(..) => true, - _ => false, - } - } - - // We have to take special care for `let` exprs in the condition, e.g. in - // `if let pat = val` or `if foo && let pat = val`, as we _do_ want `val` to live beyond the - // condition in this case. - // - // In order to maintain the drop behavior for the non `let` parts of the condition, - // we still wrap them in terminating scopes, e.g. `if foo && let pat = val` essentially - // gets transformed into `if { let _t = foo; _t } && let pat = val` - match &cond.kind { - ExprKind::Binary(op @ Spanned { node: ast::BinOpKind::And, .. }, lhs, rhs) - if has_let_expr(cond) => - { - let op = self.lower_binop(*op); - let lhs = self.lower_cond(lhs); - let rhs = self.lower_cond(rhs); - - self.arena.alloc(self.expr(cond.span, hir::ExprKind::Binary(op, lhs, rhs))) - } - ExprKind::Let(..) => self.lower_expr(cond), - _ => { - let cond = self.lower_expr(cond); - let reason = DesugaringKind::CondTemporary; - let span_block = self.mark_span_with_reason(reason, cond.span, None); - self.expr_drop_temps(span_block, cond) - } - } - } - // We desugar: `'label: while $cond $body` into: // // ``` @@ -602,7 +564,7 @@ impl<'hir> LoweringContext<'_, 'hir> { body: &Block, opt_label: Option<Label>, ) -> hir::ExprKind<'hir> { - let lowered_cond = self.with_loop_condition_scope(|t| t.lower_cond(cond)); + let lowered_cond = self.with_loop_condition_scope(|t| t.lower_expr(cond)); let then = self.lower_block_expr(body); let expr_break = self.expr_break(span); let stmt_break = self.stmt_expr(span, expr_break); @@ -2091,7 +2053,8 @@ impl<'hir> LoweringContext<'_, 'hir> { /// In terms of drop order, it has the same effect as wrapping `expr` in /// `{ let _t = $expr; _t }` but should provide better compile-time performance. /// - /// The drop order can be important in e.g. `if expr { .. }`. + /// The drop order can be important, e.g. to drop temporaries from an `async fn` + /// body before its parameters. pub(super) fn expr_drop_temps( &mut self, span: Span, diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index 288105dfba7..95f6fba6487 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -167,15 +167,31 @@ fn resolve_block<'tcx>( visitor.cx = prev_cx; } -fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) { - fn has_let_expr(expr: &Expr<'_>) -> bool { - match &expr.kind { - hir::ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs), - hir::ExprKind::Let(..) => true, - _ => false, - } - } +/// Resolve a condition from an `if` expression or match guard so that it is a terminating scope +/// if it doesn't contain `let` expressions. +fn resolve_cond<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, cond: &'tcx hir::Expr<'tcx>) { + let terminate = match cond.kind { + // Temporaries for `let` expressions must live into the success branch. + hir::ExprKind::Let(_) => false, + // Logical operator chains are handled in `resolve_expr`. Since logical operator chains in + // conditions are lowered to control-flow rather than boolean temporaries, there's no + // temporary to drop for logical operators themselves. `resolve_expr` will also recursively + // wrap any operands in terminating scopes, other than `let` expressions (which we shouldn't + // terminate) and other logical operators (which don't need a terminating scope, since their + // operands will be terminated). Any temporaries that would need to be dropped will be + // dropped before we leave this operator's scope; terminating them here would be redundant. + hir::ExprKind::Binary( + source_map::Spanned { node: hir::BinOpKind::And | hir::BinOpKind::Or, .. }, + _, + _, + ) => false, + // Otherwise, conditions should always drop their temporaries. + _ => true, + }; + resolve_expr(visitor, cond, terminate); +} +fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) { let prev_cx = visitor.cx; visitor.enter_node_scope_with_dtor(arm.hir_id.local_id, true); @@ -183,7 +199,7 @@ fn resolve_arm<'tcx>(visitor: &mut ScopeResolutionVisitor<'tcx>, arm: &'tcx hir: resolve_pat(visitor, arm.pat); if let Some(guard) = arm.guard { - resolve_expr(visitor, guard, !has_let_expr(guard)); + resolve_cond(visitor, guard); } resolve_expr(visitor, arm.body, false); @@ -340,7 +356,7 @@ fn resolve_expr<'tcx>( }; visitor.enter_scope(Scope { local_id: then.hir_id.local_id, data }); visitor.cx.var_parent = visitor.cx.parent; - visitor.visit_expr(cond); + resolve_cond(visitor, cond); resolve_expr(visitor, then, true); visitor.cx = expr_cx; resolve_expr(visitor, otherwise, true); @@ -355,7 +371,7 @@ fn resolve_expr<'tcx>( }; visitor.enter_scope(Scope { local_id: then.hir_id.local_id, data }); visitor.cx.var_parent = visitor.cx.parent; - visitor.visit_expr(cond); + resolve_cond(visitor, cond); resolve_expr(visitor, then, true); visitor.cx = expr_cx; } diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs index 58751f232d0..af1fc045ac8 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs @@ -51,12 +51,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }; match span.desugaring_kind() { - // If span arose from a desugaring of `if` or `while`, then it is the condition - // itself, which diverges, that we are about to lint on. This gives suboptimal - // diagnostics. Instead, stop here so that the `if`- or `while`-expression's - // block is linted instead. - Some(DesugaringKind::CondTemporary) => return, - // Don't lint if the result of an async block or async function is `!`. // This does not affect the unreachable lints *within* the body. Some(DesugaringKind::Async) => return, diff --git a/compiler/rustc_hir_typeck/src/lib.rs b/compiler/rustc_hir_typeck/src/lib.rs index ea8c2c6ce23..9e324286fa1 100644 --- a/compiler/rustc_hir_typeck/src/lib.rs +++ b/compiler/rustc_hir_typeck/src/lib.rs @@ -428,11 +428,9 @@ fn report_unexpected_variant_res( } hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Binary(..), hir_id, .. }) => { suggestion.push((expr.span.shrink_to_lo(), "(".to_string())); - if let hir::Node::Expr(drop_temps) = tcx.parent_hir_node(*hir_id) - && let hir::ExprKind::DropTemps(_) = drop_temps.kind - && let hir::Node::Expr(parent) = tcx.parent_hir_node(drop_temps.hir_id) + if let hir::Node::Expr(parent) = tcx.parent_hir_node(*hir_id) && let hir::ExprKind::If(condition, block, None) = parent.kind - && condition.hir_id == drop_temps.hir_id + && condition.hir_id == *hir_id && let hir::ExprKind::Block(block, _) = block.kind && block.stmts.is_empty() && let Some(expr) = block.expr diff --git a/compiler/rustc_hir_typeck/src/pat.rs b/compiler/rustc_hir_typeck/src/pat.rs index 43475521a0f..bf4611e1e34 100644 --- a/compiler/rustc_hir_typeck/src/pat.rs +++ b/compiler/rustc_hir_typeck/src/pat.rs @@ -24,7 +24,6 @@ use rustc_session::lint::builtin::NON_EXHAUSTIVE_OMITTED_PATTERNS; use rustc_session::parse::feature_err; use rustc_span::edit_distance::find_best_match_for_name; use rustc_span::edition::Edition; -use rustc_span::hygiene::DesugaringKind; use rustc_span::source_map::Spanned; use rustc_span::{BytePos, DUMMY_SP, Ident, Span, kw, sym}; use rustc_trait_selection::infer::InferCtxtExt; @@ -902,16 +901,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // then that's equivalent to there existing a LUB. let cause = self.pattern_cause(ti, span); if let Err(err) = self.demand_suptype_with_origin(&cause, expected, pat_ty) { - err.emit_unless( - ti.span - .filter(|&s| { - // In the case of `if`- and `while`-expressions we've already checked - // that `scrutinee: bool`. We know that the pattern is `true`, - // so an error here would be a duplicate and from the wrong POV. - s.is_desugaring(DesugaringKind::CondTemporary) - }) - .is_some(), - ); + err.emit(); } pat_ty diff --git a/compiler/rustc_span/src/hygiene.rs b/compiler/rustc_span/src/hygiene.rs index 29be3b73ee9..c3080875da8 100644 --- a/compiler/rustc_span/src/hygiene.rs +++ b/compiler/rustc_span/src/hygiene.rs @@ -1191,11 +1191,6 @@ impl AstPass { /// The kind of compiler desugaring. #[derive(Clone, Copy, PartialEq, Debug, Encodable, Decodable, HashStable_Generic)] pub enum DesugaringKind { - /// We desugar `if c { i } else { e }` to `match $ExprKind::Use(c) { true => i, _ => e }`. - /// However, we do not want to blame `c` for unreachability but rather say that `i` - /// is unreachable. This desugaring kind allows us to avoid blaming `c`. - /// This also applies to `while` loops. - CondTemporary, QuestionMark, TryBlock, YeetExpr, @@ -1230,7 +1225,6 @@ impl DesugaringKind { /// The description wording should combine well with "desugaring of {}". pub fn descr(self) -> &'static str { match self { - DesugaringKind::CondTemporary => "`if` or `while` condition", DesugaringKind::Async => "`async` block or function", DesugaringKind::Await => "`await` expression", DesugaringKind::QuestionMark => "operator `?`", @@ -1253,7 +1247,6 @@ impl DesugaringKind { /// like `from_desugaring = "QuestionMark"` pub fn matches(&self, value: &str) -> bool { match self { - DesugaringKind::CondTemporary => value == "CondTemporary", DesugaringKind::Async => value == "Async", DesugaringKind::Await => value == "Await", DesugaringKind::QuestionMark => value == "QuestionMark", diff --git a/src/tools/clippy/clippy_lints/src/booleans.rs b/src/tools/clippy/clippy_lints/src/booleans.rs index bf43234ff50..61c2fc49bd7 100644 --- a/src/tools/clippy/clippy_lints/src/booleans.rs +++ b/src/tools/clippy/clippy_lints/src/booleans.rs @@ -1,5 +1,6 @@ use clippy_config::Conf; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; +use clippy_utils::higher::has_let_expr; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::SpanRangeExt; use clippy_utils::sugg::Sugg; @@ -646,7 +647,9 @@ impl<'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'_, 'tcx> { fn visit_expr(&mut self, e: &'tcx Expr<'_>) { if !e.span.from_expansion() { match &e.kind { - ExprKind::Binary(binop, _, _) if binop.node == BinOpKind::Or || binop.node == BinOpKind::And => { + ExprKind::Binary(binop, _, _) + if binop.node == BinOpKind::Or || binop.node == BinOpKind::And && !has_let_expr(e) => + { self.bool_expr(e); }, ExprKind::Unary(UnOp::Not, inner) => { diff --git a/src/tools/clippy/clippy_lints/src/if_not_else.rs b/src/tools/clippy/clippy_lints/src/if_not_else.rs index ab7a965b367..25fed0d4dd1 100644 --- a/src/tools/clippy/clippy_lints/src/if_not_else.rs +++ b/src/tools/clippy/clippy_lints/src/if_not_else.rs @@ -51,7 +51,6 @@ declare_lint_pass!(IfNotElse => [IF_NOT_ELSE]); impl LateLintPass<'_> for IfNotElse { fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) { if let ExprKind::If(cond, cond_inner, Some(els)) = e.kind - && let ExprKind::DropTemps(cond) = cond.kind && let ExprKind::Block(..) = els.kind { let (msg, help) = match cond.kind { diff --git a/src/tools/clippy/clippy_lints/src/implicit_saturating_add.rs b/src/tools/clippy/clippy_lints/src/implicit_saturating_add.rs index 185fc2aa2d4..0fdbf679738 100644 --- a/src/tools/clippy/clippy_lints/src/implicit_saturating_add.rs +++ b/src/tools/clippy/clippy_lints/src/implicit_saturating_add.rs @@ -41,8 +41,7 @@ declare_lint_pass!(ImplicitSaturatingAdd => [IMPLICIT_SATURATING_ADD]); impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingAdd { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { if let ExprKind::If(cond, then, None) = expr.kind - && let ExprKind::DropTemps(expr1) = cond.kind - && let Some((c, op_node, l)) = get_const(cx, expr1) + && let Some((c, op_node, l)) = get_const(cx, cond) && let BinOpKind::Ne | BinOpKind::Lt = op_node && let ExprKind::Block(block, None) = then.kind && let Block { @@ -66,7 +65,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingAdd { && Some(c) == get_int_max(ty) && let ctxt = expr.span.ctxt() && ex.span.ctxt() == ctxt - && expr1.span.ctxt() == ctxt + && cond.span.ctxt() == ctxt && clippy_utils::SpanlessEq::new(cx).eq_expr(l, target) && AssignOpKind::AddAssign == op1.node && let ExprKind::Lit(lit) = value.kind diff --git a/src/tools/clippy/clippy_lints/src/let_if_seq.rs b/src/tools/clippy/clippy_lints/src/let_if_seq.rs index 5db28e9ae9b..e480c8fbed5 100644 --- a/src/tools/clippy/clippy_lints/src/let_if_seq.rs +++ b/src/tools/clippy/clippy_lints/src/let_if_seq.rs @@ -62,15 +62,8 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq { if let hir::StmtKind::Let(local) = stmt.kind && let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind && let hir::StmtKind::Expr(if_) = next.kind - && let hir::ExprKind::If( - hir::Expr { - kind: hir::ExprKind::DropTemps(cond), - .. - }, - then, - else_, - ) = if_.kind - && !is_local_used(cx, *cond, canonical_id) + && let hir::ExprKind::If(cond, then, else_) = if_.kind + && !is_local_used(cx, cond, canonical_id) && let hir::ExprKind::Block(then, _) = then.kind && let Some(value) = check_assign(cx, canonical_id, then) && !is_local_used(cx, value, canonical_id) diff --git a/src/tools/clippy/clippy_utils/src/higher.rs b/src/tools/clippy/clippy_utils/src/higher.rs index 6971b488013..4e0b00df950 100644 --- a/src/tools/clippy/clippy_utils/src/higher.rs +++ b/src/tools/clippy/clippy_utils/src/higher.rs @@ -54,7 +54,7 @@ impl<'tcx> ForLoop<'tcx> { } } -/// An `if` expression without `DropTemps` +/// An `if` expression without `let` pub struct If<'hir> { /// `if` condition pub cond: &'hir Expr<'hir>, @@ -66,16 +66,10 @@ pub struct If<'hir> { impl<'hir> If<'hir> { #[inline] - /// Parses an `if` expression + /// Parses an `if` expression without `let` pub const fn hir(expr: &Expr<'hir>) -> Option<Self> { - if let ExprKind::If( - Expr { - kind: ExprKind::DropTemps(cond), - .. - }, - then, - r#else, - ) = expr.kind + if let ExprKind::If(cond, then, r#else) = expr.kind + && !has_let_expr(cond) { Some(Self { cond, then, r#else }) } else { @@ -198,18 +192,10 @@ impl<'hir> IfOrIfLet<'hir> { /// Parses an `if` or `if let` expression pub const fn hir(expr: &Expr<'hir>) -> Option<Self> { if let ExprKind::If(cond, then, r#else) = expr.kind { - if let ExprKind::DropTemps(new_cond) = cond.kind { - return Some(Self { - cond: new_cond, - then, - r#else, - }); - } - if let ExprKind::Let(..) = cond.kind { - return Some(Self { cond, then, r#else }); - } + Some(Self { cond, then, r#else }) + } else { + None } - None } } @@ -343,15 +329,7 @@ impl<'hir> While<'hir> { Block { expr: Some(Expr { - kind: - ExprKind::If( - Expr { - kind: ExprKind::DropTemps(condition), - .. - }, - body, - _, - ), + kind: ExprKind::If(condition, body, _), .. }), .. @@ -360,6 +338,7 @@ impl<'hir> While<'hir> { LoopSource::While, span, ) = expr.kind + && !has_let_expr(condition) { return Some(Self { condition, body, span }); } @@ -493,3 +472,13 @@ pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) - } None } + +/// Checks that a condition doesn't have a `let` expression, to keep `If` and `While` from accepting +/// `if let` and `while let`. +pub const fn has_let_expr<'tcx>(cond: &'tcx Expr<'tcx>) -> bool { + match &cond.kind { + ExprKind::Let(_) => true, + ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs), + _ => false, + } +} diff --git a/src/tools/clippy/tests/ui/author/if.stdout b/src/tools/clippy/tests/ui/author/if.stdout index da359866bff..dbff55634ea 100644 --- a/src/tools/clippy/tests/ui/author/if.stdout +++ b/src/tools/clippy/tests/ui/author/if.stdout @@ -1,8 +1,7 @@ if let StmtKind::Let(local) = stmt.kind && let Some(init) = local.init && let ExprKind::If(cond, then, Some(else_expr)) = init.kind - && let ExprKind::DropTemps(expr) = cond.kind - && let ExprKind::Lit(ref lit) = expr.kind + && let ExprKind::Lit(ref lit) = cond.kind && let LitKind::Bool(true) = lit.node && let ExprKind::Block(block, None) = then.kind && block.stmts.len() == 1 diff --git a/src/tools/clippy/tests/ui/author/struct.stdout b/src/tools/clippy/tests/ui/author/struct.stdout index 1e8fbafd30c..2dedab56dce 100644 --- a/src/tools/clippy/tests/ui/author/struct.stdout +++ b/src/tools/clippy/tests/ui/author/struct.stdout @@ -2,8 +2,7 @@ if let ExprKind::Struct(qpath, fields, None) = expr.kind && fields.len() == 1 && fields[0].ident.as_str() == "field" && let ExprKind::If(cond, then, Some(else_expr)) = fields[0].expr.kind - && let ExprKind::DropTemps(expr1) = cond.kind - && let ExprKind::Lit(ref lit) = expr1.kind + && let ExprKind::Lit(ref lit) = cond.kind && let LitKind::Bool(true) = lit.node && let ExprKind::Block(block, None) = then.kind && block.stmts.is_empty() |