From 2d9f2eae84dc3988c5463fdd1b124a9b695447d7 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Thu, 25 Feb 2021 22:37:22 +0000 Subject: [PATCH 1/2] Use correct drop scopes for if expressions --- compiler/rustc_ast_lowering/src/expr.rs | 17 ++++++-- .../rustc_mir_build/src/build/matches/mod.rs | 41 +++++++++++++++++++ compiler/rustc_passes/src/region.rs | 24 +++++++++-- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs index bacf5662bc005..3bf7c7c37a477 100644 --- a/compiler/rustc_ast_lowering/src/expr.rs +++ b/compiler/rustc_ast_lowering/src/expr.rs @@ -443,9 +443,20 @@ impl<'hir> LoweringContext<'_, 'hir> { else_opt: Option<&Expr>, ) -> hir::ExprKind<'hir> { let cond = self.lower_expr(cond); - let then = self.arena.alloc(self.lower_block_expr(then)); - let els = else_opt.map(|els| self.lower_expr(els)); - hir::ExprKind::If(cond, then, els) + let wrapped_cond = match cond.kind { + hir::ExprKind::Let(..) => cond, + _ => self.expr_drop_temps(cond.span, cond, AttrVec::new()), + }; + let then_expr = self.lower_block_expr(then); + if let Some(rslt) = else_opt { + hir::ExprKind::If( + wrapped_cond, + self.arena.alloc(then_expr), + Some(self.lower_expr(rslt)), + ) + } else { + hir::ExprKind::If(wrapped_cond, self.arena.alloc(then_expr), None) + } } fn lower_expr_if_let( diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 8164529dd1ff7..9ca4425a1b505 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -35,6 +35,47 @@ use std::convert::TryFrom; use std::mem; impl<'a, 'tcx> Builder<'a, 'tcx> { + pub(crate) fn then_else_blocks( + &mut self, + mut block: BasicBlock, + expr: ExprRef<'tcx>, + source_info: SourceInfo, + ) -> (BasicBlock, BasicBlock) { + let this = self; + let expr = this.hir.mirror(expr); + let expr_span = expr.span; + + match expr.kind { + ExprKind::Scope { region_scope, lint_level, value } => { + let region_scope = (region_scope, source_info); + let then_block; + let else_block = unpack!( + then_block = this.in_scope(region_scope, lint_level, |this| { + let (then_block, else_block) = + this.then_else_blocks(block, value, source_info); + then_block.and(else_block) + }) + ); + (then_block, else_block) + } + ExprKind::Let { expr, pat } => { + // TODO: Use correct span. + this.lower_let(block, &expr, &pat, expr_span) + } + _ => { + let local_scope = Some(this.local_scope()); + let place = + unpack!(block = this.as_temp(block, local_scope, expr, Mutability::Mut)); + let operand = Operand::Move(Place::from(place)); + let then_block = this.cfg.start_new_block(); + let else_block = this.cfg.start_new_block(); + let term = TerminatorKind::if_(this.hir.tcx(), operand, then_block, else_block); + this.cfg.terminate(block, source_info, term); + (then_block, else_block) + } + } + } + /// Generates MIR for a `match` expression. /// /// The MIR that we generate for a match looks like this. diff --git a/compiler/rustc_passes/src/region.rs b/compiler/rustc_passes/src/region.rs index c133f1a041719..7403e51c7341b 100644 --- a/compiler/rustc_passes/src/region.rs +++ b/compiler/rustc_passes/src/region.rs @@ -233,14 +233,12 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h terminating(r.hir_id.local_id); } - hir::ExprKind::If(ref expr, ref then, Some(ref otherwise)) => { - terminating(expr.hir_id.local_id); + hir::ExprKind::If(_, ref then, Some(ref otherwise)) => { terminating(then.hir_id.local_id); terminating(otherwise.hir_id.local_id); } - hir::ExprKind::If(ref expr, ref then, None) => { - terminating(expr.hir_id.local_id); + hir::ExprKind::If(_, ref then, None) => { terminating(then.hir_id.local_id); } @@ -392,6 +390,24 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h } } + hir::ExprKind::If(ref cond, ref then, Some(ref otherwise)) => { + // FIXME(matthewjasper): ideally the scope we use here would only + // contain the condition and then expression. This works, but + // can result in some extra drop flags. + visitor.cx.var_parent = visitor.cx.parent; + visitor.visit_expr(cond); + visitor.cx.var_parent = prev_cx.var_parent; + visitor.visit_expr(then); + visitor.visit_expr(otherwise); + } + + hir::ExprKind::If(ref cond, ref then, None) => { + visitor.cx.var_parent = visitor.cx.parent; + visitor.visit_expr(cond); + visitor.cx.var_parent = prev_cx.var_parent; + visitor.visit_expr(then); + } + _ => intravisit::walk_expr(visitor, expr), } From 6aa9937a768bf13e5f7bd0ee6dd8579403b39058 Mon Sep 17 00:00:00 2001 From: Caio Date: Sun, 8 Aug 2021 11:49:13 -0300 Subject: [PATCH 2/2] Introduce hir::ExprKind::Let - Take 2 --- compiler/rustc_ast/src/ast.rs | 4 +- compiler/rustc_ast/src/mut_visit.rs | 2 +- compiler/rustc_ast/src/visit.rs | 4 +- compiler/rustc_ast_lowering/src/expr.rs | 263 ++------- compiler/rustc_ast_lowering/src/lib.rs | 10 - .../rustc_ast_passes/src/ast_validation.rs | 69 ++- compiler/rustc_ast_pretty/src/pprust/state.rs | 27 +- compiler/rustc_expand/src/base.rs | 1 + compiler/rustc_hir/src/hir.rs | 26 +- compiler/rustc_hir/src/intravisit.rs | 4 + compiler/rustc_hir_pretty/src/lib.rs | 124 +++-- .../src/infer/error_reporting/mod.rs | 18 - compiler/rustc_lint/src/unused.rs | 12 +- compiler/rustc_middle/src/thir.rs | 4 + .../src/build/expr/as_place.rs | 1 + .../src/build/expr/as_rvalue.rs | 1 + .../src/build/expr/category.rs | 1 + .../rustc_mir_build/src/build/expr/into.rs | 66 ++- .../rustc_mir_build/src/build/matches/mod.rs | 103 ++-- .../rustc_mir_build/src/check_unsafety.rs | 9 + compiler/rustc_mir_build/src/thir/cx/expr.rs | 3 + .../src/thir/pattern/check_match.rs | 192 ++++--- compiler/rustc_mir_build/src/thir/visit.rs | 3 + compiler/rustc_parse/src/parser/expr.rs | 2 +- compiler/rustc_passes/src/check_const.rs | 9 +- compiler/rustc_passes/src/liveness.rs | 17 +- compiler/rustc_passes/src/naked_functions.rs | 1 + compiler/rustc_resolve/src/late.rs | 2 +- compiler/rustc_typeck/src/check/_match.rs | 307 ++++------- compiler/rustc_typeck/src/check/expr.rs | 44 +- compiler/rustc_typeck/src/expr_use_visitor.rs | 28 +- .../rustc_typeck/src/mem_categorization.rs | 1 + src/test/incremental/hashes/if_expressions.rs | 2 +- .../incremental/hashes/while_let_loops.rs | 6 +- src/test/incremental/hashes/while_loops.rs | 4 +- ...float_to_exponential_common.ConstProp.diff | 46 +- .../issue_41888.main.ElaborateDrops.after.mir | 66 +-- ...e_75439.foo.MatchBranchSimplification.diff | 18 +- ..._locals_fixedpoint.foo.SimplifyLocals.diff | 14 +- ...reachable.main.UnreachablePropagation.diff | 11 +- ...hable_asm.main.UnreachablePropagation.diff | 13 +- ...ble_asm_2.main.UnreachablePropagation.diff | 17 +- ...diverging.main.UnreachablePropagation.diff | 27 +- ...oops.change_loop_body.ConstProp.32bit.diff | 6 +- ...oops.change_loop_body.ConstProp.64bit.diff | 6 +- ...le_storage.while_loop.PreCodegen.after.mir | 18 +- src/test/ui-fulldeps/pprust-expr-roundtrip.rs | 2 +- .../migrations/issue-78720.stderr | 9 +- src/test/ui/expr/if/if-let.rs | 2 +- src/test/ui/expr/if/if-let.stderr | 46 +- src/test/ui/expr/if/if-ret.rs | 2 +- src/test/ui/expr/if/if-ret.stderr | 4 +- src/test/ui/issues/issue-14091.stderr | 2 - src/test/ui/match/issue-82392.stdout | 11 +- src/test/ui/pattern/issue-82290.rs | 4 +- src/test/ui/pattern/issue-82290.stderr | 22 +- .../deny-irrefutable-let-patterns.stderr | 12 +- src/test/ui/reachable/expr_if.rs | 2 +- src/test/ui/reachable/expr_if.stderr | 4 +- src/test/ui/reachable/expr_while.rs | 4 +- src/test/ui/reachable/expr_while.stderr | 8 +- .../ui/rfc-2294-if-let-guard/feature-gate.rs | 16 - .../rfc-2294-if-let-guard/feature-gate.stderr | 164 +----- .../disallowed-positions.rs | 1 - .../disallowed-positions.stderr | 189 ++++--- .../ui/rfc-2497-if-let-chains/feature-gate.rs | 32 -- .../feature-gate.stderr | 334 ++---------- .../protect-precedences.rs | 3 +- .../protect-precedences.stderr | 4 +- src/test/ui/union/union-unsafe.thir.stderr | 4 +- src/test/ui/while-let.stderr | 17 +- .../src/assertions_on_constants.rs | 5 +- .../src/blocks_in_if_conditions.rs | 3 +- .../clippy_lints/src/collapsible_match.rs | 77 ++- src/tools/clippy/clippy_lints/src/copies.rs | 17 +- .../clippy/clippy_lints/src/dereference.rs | 1 + src/tools/clippy/clippy_lints/src/entry.rs | 8 +- .../clippy/clippy_lints/src/eta_reduction.rs | 2 +- .../src/floating_point_arithmetic.rs | 7 +- .../clippy/clippy_lints/src/if_let_mutex.rs | 27 +- .../clippy_lints/src/if_let_some_result.rs | 15 +- .../src/if_then_some_else_none.rs | 4 +- .../src/implicit_saturating_sub.rs | 3 +- .../clippy_lints/src/indexing_slicing.rs | 2 +- .../clippy/clippy_lints/src/infinite_iter.rs | 2 +- .../clippy/clippy_lints/src/let_if_seq.rs | 4 +- .../clippy_lints/src/loops/manual_flatten.rs | 13 +- .../clippy_lints/src/loops/manual_memcpy.rs | 2 +- .../clippy/clippy_lints/src/loops/mod.rs | 6 +- .../clippy_lints/src/loops/mut_range_bound.rs | 2 +- .../src/loops/needless_range_loop.rs | 2 +- .../clippy_lints/src/loops/never_loop.rs | 9 +- .../clippy_lints/src/loops/while_let_loop.rs | 98 ++-- .../src/loops/while_let_on_iterator.rs | 25 +- .../clippy/clippy_lints/src/manual_map.rs | 301 ++++++----- .../clippy/clippy_lints/src/manual_strip.rs | 4 +- src/tools/clippy/clippy_lints/src/matches.rs | 265 +++++---- .../src/methods/iter_next_slice.rs | 4 +- src/tools/clippy/clippy_lints/src/mut_mut.rs | 2 +- .../clippy/clippy_lints/src/needless_bool.rs | 14 +- .../clippy_lints/src/non_expressive_names.rs | 3 +- .../clippy_lints/src/option_if_let_else.rs | 43 +- .../clippy_lints/src/pattern_type_mismatch.rs | 33 +- .../clippy/clippy_lints/src/question_mark.rs | 27 +- src/tools/clippy/clippy_lints/src/ranges.rs | 14 +- .../clippy_lints/src/redundant_clone.rs | 2 +- src/tools/clippy/clippy_lints/src/returns.rs | 8 - .../src/suspicious_operation_groupings.rs | 2 +- .../clippy_lints/src/unnested_or_patterns.rs | 2 +- src/tools/clippy/clippy_lints/src/unwrap.rs | 7 +- .../clippy/clippy_lints/src/utils/author.rs | 9 + .../clippy_lints/src/utils/inspector.rs | 33 +- src/tools/clippy/clippy_lints/src/vec.rs | 8 +- .../clippy/clippy_utils/src/ast_utils.rs | 2 +- .../clippy/clippy_utils/src/eager_or_lazy.rs | 1 + src/tools/clippy/clippy_utils/src/higher.rs | 507 +++++++++++++----- .../clippy/clippy_utils/src/hir_utils.rs | 7 + src/tools/clippy/clippy_utils/src/lib.rs | 19 +- src/tools/clippy/clippy_utils/src/sugg.rs | 3 +- src/tools/clippy/tests/ui/author/if.stdout | 3 +- .../clippy/tests/ui/collapsible_match.stderr | 8 +- src/tools/clippy/tests/ui/crashes/ice-7410.rs | 1 + .../tests/ui/crashes/issues_loop_mut_cond.rs | 1 + src/tools/clippy/tests/ui/infinite_loop.rs | 2 + .../clippy/tests/ui/infinite_loop.stderr | 22 +- .../clippy/tests/ui/match_overlapping_arm.rs | 1 + .../tests/ui/match_overlapping_arm.stderr | 20 +- src/tools/rustfmt/src/expr.rs | 2 +- 128 files changed, 2054 insertions(+), 2170 deletions(-) diff --git a/compiler/rustc_ast/src/ast.rs b/compiler/rustc_ast/src/ast.rs index bd6e4c30fc34c..575a00cdd0e43 100644 --- a/compiler/rustc_ast/src/ast.rs +++ b/compiler/rustc_ast/src/ast.rs @@ -1302,7 +1302,9 @@ pub enum ExprKind { Type(P, P), /// A `let pat = expr` expression that is only semantically allowed in the condition /// of `if` / `while` expressions. (e.g., `if let 0 = x { .. }`). - Let(P, P), + /// + /// `Span` represents the whole `let pat = expr` statement. + Let(P, P, Span), /// An `if` block, with an optional `else` block. /// /// `if expr { block } else { expr }` diff --git a/compiler/rustc_ast/src/mut_visit.rs b/compiler/rustc_ast/src/mut_visit.rs index 87950b44083ef..c824583118722 100644 --- a/compiler/rustc_ast/src/mut_visit.rs +++ b/compiler/rustc_ast/src/mut_visit.rs @@ -1237,7 +1237,7 @@ pub fn noop_visit_expr( vis.visit_ty(ty); } ExprKind::AddrOf(_, _, ohs) => vis.visit_expr(ohs), - ExprKind::Let(pat, scrutinee) => { + ExprKind::Let(pat, scrutinee, _) => { vis.visit_pat(pat); vis.visit_expr(scrutinee); } diff --git a/compiler/rustc_ast/src/visit.rs b/compiler/rustc_ast/src/visit.rs index 1ebfcf367110f..a377763983a4b 100644 --- a/compiler/rustc_ast/src/visit.rs +++ b/compiler/rustc_ast/src/visit.rs @@ -779,9 +779,9 @@ pub fn walk_expr<'a, V: Visitor<'a>>(visitor: &mut V, expression: &'a Expr) { visitor.visit_expr(subexpression); visitor.visit_ty(typ) } - ExprKind::Let(ref pat, ref scrutinee) => { + ExprKind::Let(ref pat, ref expr, _) => { visitor.visit_pat(pat); - visitor.visit_expr(scrutinee); + visitor.visit_expr(expr); } ExprKind::If(ref head_expression, ref if_block, ref optional_else) => { visitor.visit_expr(head_expression); diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs index 3bf7c7c37a477..bf7589e84adc4 100644 --- a/compiler/rustc_ast_lowering/src/expr.rs +++ b/compiler/rustc_ast_lowering/src/expr.rs @@ -86,32 +86,12 @@ impl<'hir> LoweringContext<'_, 'hir> { let ohs = self.lower_expr(ohs); hir::ExprKind::AddrOf(k, m, ohs) } - ExprKind::Let(ref pat, ref scrutinee) => { - self.lower_expr_let(e.span, pat, scrutinee) + ExprKind::Let(ref pat, ref scrutinee, span) => { + hir::ExprKind::Let(self.lower_pat(pat), self.lower_expr(scrutinee), span) + } + ExprKind::If(ref cond, ref then, ref else_opt) => { + self.lower_expr_if(cond, then, else_opt.as_deref()) } - ExprKind::If(ref cond, ref then, ref else_opt) => match cond.kind { - ExprKind::Let(ref pat, ref scrutinee) => { - self.lower_expr_if_let(e.span, pat, scrutinee, then, else_opt.as_deref()) - } - ExprKind::Paren(ref paren) => match paren.peel_parens().kind { - ExprKind::Let(ref pat, ref scrutinee) => { - // A user has written `if (let Some(x) = foo) {`, we want to avoid - // confusing them with mentions of nightly features. - // If this logic is changed, you will also likely need to touch - // `unused::UnusedParens::check_expr`. - self.if_let_expr_with_parens(cond, &paren.peel_parens()); - self.lower_expr_if_let( - e.span, - pat, - scrutinee, - then, - else_opt.as_deref(), - ) - } - _ => self.lower_expr_if(cond, then, else_opt.as_deref()), - }, - _ => self.lower_expr_if(cond, then, else_opt.as_deref()), - }, ExprKind::While(ref cond, ref body, opt_label) => self .with_loop_scope(e.id, |this| { this.lower_expr_while_in_loop_scope(e.span, cond, body, opt_label) @@ -368,126 +348,51 @@ impl<'hir> LoweringContext<'_, 'hir> { hir::ExprKind::Call(f, self.lower_exprs(&real_args)) } - fn if_let_expr_with_parens(&mut self, cond: &Expr, paren: &Expr) { - let start = cond.span.until(paren.span); - let end = paren.span.shrink_to_hi().until(cond.span.shrink_to_hi()); - self.sess - .struct_span_err( - vec![start, end], - "invalid parentheses around `let` expression in `if let`", - ) - .multipart_suggestion( - "`if let` needs to be written without parentheses", - vec![(start, String::new()), (end, String::new())], - rustc_errors::Applicability::MachineApplicable, - ) - .emit(); - // Ideally, we'd remove the feature gating of a `let` expression since we are already - // complaining about it here, but `feature_gate::check_crate` has already run by now: - // self.sess.parse_sess.gated_spans.ungate_last(sym::let_chains, paren.span); - } - - /// Emit an error and lower `ast::ExprKind::Let(pat, scrutinee)` into: - /// ```rust - /// match scrutinee { pats => true, _ => false } - /// ``` - fn lower_expr_let(&mut self, span: Span, pat: &Pat, scrutinee: &Expr) -> hir::ExprKind<'hir> { - // If we got here, the `let` expression is not allowed. - - if self.sess.opts.unstable_features.is_nightly_build() { - self.sess - .struct_span_err(span, "`let` expressions are not supported here") - .note( - "only supported directly without parentheses in conditions of `if`- and \ - `while`-expressions, as well as in `let` chains within parentheses", - ) - .emit(); - } else { - self.sess - .struct_span_err(span, "expected expression, found statement (`let`)") - .note("variable declaration using `let` is a statement") - .emit(); - } - - // For better recovery, we emit: - // ``` - // match scrutinee { pat => true, _ => false } - // ``` - // While this doesn't fully match the user's intent, it has key advantages: - // 1. We can avoid using `abort_if_errors`. - // 2. We can typeck both `pat` and `scrutinee`. - // 3. `pat` is allowed to be refutable. - // 4. The return type of the block is `bool` which seems like what the user wanted. - let scrutinee = self.lower_expr(scrutinee); - let then_arm = { - let pat = self.lower_pat(pat); - let expr = self.expr_bool(span, true); - self.arm(pat, expr) - }; - let else_arm = { - let pat = self.pat_wild(span); - let expr = self.expr_bool(span, false); - self.arm(pat, expr) - }; - hir::ExprKind::Match( - scrutinee, - arena_vec![self; then_arm, else_arm], - hir::MatchSource::Normal, - ) - } - fn lower_expr_if( &mut self, cond: &Expr, then: &Block, else_opt: Option<&Expr>, ) -> hir::ExprKind<'hir> { - let cond = self.lower_expr(cond); - let wrapped_cond = match cond.kind { - hir::ExprKind::Let(..) => cond, - _ => self.expr_drop_temps(cond.span, cond, AttrVec::new()), - }; + let lowered_cond = self.lower_expr(cond); + let new_cond = self.manage_let_cond(lowered_cond); let then_expr = self.lower_block_expr(then); if let Some(rslt) = else_opt { - hir::ExprKind::If( - wrapped_cond, - self.arena.alloc(then_expr), - Some(self.lower_expr(rslt)), - ) + hir::ExprKind::If(new_cond, self.arena.alloc(then_expr), Some(self.lower_expr(rslt))) } else { - hir::ExprKind::If(wrapped_cond, self.arena.alloc(then_expr), None) + hir::ExprKind::If(new_cond, self.arena.alloc(then_expr), None) } } - fn lower_expr_if_let( - &mut self, - span: Span, - pat: &Pat, - scrutinee: &Expr, - then: &Block, - else_opt: Option<&Expr>, - ) -> hir::ExprKind<'hir> { - // FIXME(#53667): handle lowering of && and parens. - - // `_ => else_block` where `else_block` is `{}` if there's `None`: - let else_pat = self.pat_wild(span); - let (else_expr, contains_else_clause) = match else_opt { - None => (self.expr_block_empty(span.shrink_to_hi()), false), - Some(els) => (self.lower_expr(els), true), - }; - let else_arm = self.arm(else_pat, else_expr); - - // Handle then + scrutinee: - let scrutinee = self.lower_expr(scrutinee); - let then_pat = self.lower_pat(pat); - - let then_expr = self.lower_block_expr(then); - let then_arm = self.arm(then_pat, self.arena.alloc(then_expr)); - - let desugar = hir::MatchSource::IfLetDesugar { contains_else_clause }; - hir::ExprKind::Match(scrutinee, arena_vec![self; then_arm, else_arm], desugar) + // If `cond` kind is `let`, returns `let`. Otherwise, wraps and returns `cond` + // in a temporary block. + fn manage_let_cond(&mut self, cond: &'hir hir::Expr<'hir>) -> &'hir hir::Expr<'hir> { + match cond.kind { + hir::ExprKind::Let(..) => cond, + _ => { + let span_block = + self.mark_span_with_reason(DesugaringKind::CondTemporary, cond.span, None); + self.expr_drop_temps(span_block, cond, AttrVec::new()) + } + } } + // We desugar: `'label: while $cond $body` into: + // + // ``` + // 'label: loop { + // if { let _t = $cond; _t } { + // $body + // } + // else { + // break; + // } + // } + // ``` + // + // Wrap in a construct equivalent to `{ let _t = $cond; _t }` + // to preserve drop semantics since `while $cond { ... }` does not + // let temporaries live outside of `cond`. fn lower_expr_while_in_loop_scope( &mut self, span: Span, @@ -495,72 +400,17 @@ impl<'hir> LoweringContext<'_, 'hir> { body: &Block, opt_label: Option