From 615c23f6ecc07e87501f2f52190e3fb08b50a17e Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 30 Mar 2019 21:49:52 +0000 Subject: [PATCH 01/11] Remove unused parameter from in(_opt)?_scope --- src/librustc_mir/build/block.rs | 12 ++++++------ src/librustc_mir/build/expr/as_operand.rs | 2 +- src/librustc_mir/build/expr/as_place.rs | 2 +- src/librustc_mir/build/expr/as_rvalue.rs | 2 +- src/librustc_mir/build/expr/as_temp.rs | 2 +- src/librustc_mir/build/expr/into.rs | 2 +- src/librustc_mir/build/expr/stmt.rs | 2 +- src/librustc_mir/build/mod.rs | 4 ++-- src/librustc_mir/build/scope.rs | 8 ++++---- 9 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/librustc_mir/build/block.rs b/src/librustc_mir/build/block.rs index 7469aceee3a9e..d93223a4292c4 100644 --- a/src/librustc_mir/build/block.rs +++ b/src/librustc_mir/build/block.rs @@ -23,8 +23,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { safety_mode } = self.hir.mirror(ast_block); - self.in_opt_scope(opt_destruction_scope.map(|de|(de, source_info)), block, move |this| { - this.in_scope((region_scope, source_info), LintLevel::Inherited, block, move |this| { + self.in_opt_scope(opt_destruction_scope.map(|de|(de, source_info)), move |this| { + this.in_scope((region_scope, source_info), LintLevel::Inherited, move |this| { if targeted_by_break { // This is a `break`-able block let exit_block = this.cfg.start_new_block(); @@ -83,9 +83,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { StmtKind::Expr { scope, expr } => { this.block_context.push(BlockFrame::Statement { ignores_expr_result: true }); unpack!(block = this.in_opt_scope( - opt_destruction_scope.map(|de|(de, source_info)), block, |this| { + opt_destruction_scope.map(|de|(de, source_info)), |this| { let si = (scope, source_info); - this.in_scope(si, LintLevel::Inherited, block, |this| { + this.in_scope(si, LintLevel::Inherited, |this| { let expr = this.hir.mirror(expr); this.stmt_expr(block, expr, Some(stmt_span)) }) @@ -128,9 +128,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { Some((None, initializer_span)), ); unpack!(block = this.in_opt_scope( - opt_destruction_scope.map(|de|(de, source_info)), block, |this| { + opt_destruction_scope.map(|de|(de, source_info)), |this| { let scope = (init_scope, source_info); - this.in_scope(scope, lint_level, block, |this| { + this.in_scope(scope, lint_level, |this| { this.expr_into_pattern(block, pattern, init) }) })); diff --git a/src/librustc_mir/build/expr/as_operand.rs b/src/librustc_mir/build/expr/as_operand.rs index e354a2ee8160b..ed80cb1a16369 100644 --- a/src/librustc_mir/build/expr/as_operand.rs +++ b/src/librustc_mir/build/expr/as_operand.rs @@ -57,7 +57,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { { let source_info = this.source_info(expr.span); let region_scope = (region_scope, source_info); - return this.in_scope(region_scope, lint_level, block, |this| { + return this.in_scope(region_scope, lint_level, |this| { this.as_operand(block, scope, value) }); } diff --git a/src/librustc_mir/build/expr/as_place.rs b/src/librustc_mir/build/expr/as_place.rs index 5f444d4ceeb88..a956eacb0699f 100644 --- a/src/librustc_mir/build/expr/as_place.rs +++ b/src/librustc_mir/build/expr/as_place.rs @@ -52,7 +52,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { region_scope, lint_level, value, - } => this.in_scope((region_scope, source_info), lint_level, block, |this| { + } => this.in_scope((region_scope, source_info), lint_level, |this| { if mutability == Mutability::Not { this.as_read_only_place(block, value) } else { diff --git a/src/librustc_mir/build/expr/as_rvalue.rs b/src/librustc_mir/build/expr/as_rvalue.rs index fbc4835a6557b..a0b504a99de9a 100644 --- a/src/librustc_mir/build/expr/as_rvalue.rs +++ b/src/librustc_mir/build/expr/as_rvalue.rs @@ -58,7 +58,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { value, } => { let region_scope = (region_scope, source_info); - this.in_scope(region_scope, lint_level, block, |this| { + this.in_scope(region_scope, lint_level, |this| { this.as_rvalue(block, scope, value) }) } diff --git a/src/librustc_mir/build/expr/as_temp.rs b/src/librustc_mir/build/expr/as_temp.rs index cba771f27065d..c60e197010067 100644 --- a/src/librustc_mir/build/expr/as_temp.rs +++ b/src/librustc_mir/build/expr/as_temp.rs @@ -43,7 +43,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { value, } = expr.kind { - return this.in_scope((region_scope, source_info), lint_level, block, |this| { + return this.in_scope((region_scope, source_info), lint_level, |this| { this.as_temp(block, temp_lifetime, value, mutability) }); } diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index 15795a64e3b7d..7bdfdf0b0895f 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -46,7 +46,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { value, } => { let region_scope = (region_scope, source_info); - this.in_scope(region_scope, lint_level, block, |this| { + this.in_scope(region_scope, lint_level, |this| { this.into(destination, block, value) }) } diff --git a/src/librustc_mir/build/expr/stmt.rs b/src/librustc_mir/build/expr/stmt.rs index b58914b017fd4..ac690f89264bf 100644 --- a/src/librustc_mir/build/expr/stmt.rs +++ b/src/librustc_mir/build/expr/stmt.rs @@ -29,7 +29,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { value, } => { let value = this.hir.mirror(value); - this.in_scope((region_scope, source_info), lint_level, block, |this| { + this.in_scope((region_scope, source_info), lint_level, |this| { this.stmt_expr(block, value, opt_stmt_span) }) } diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index 16ab233bd2e36..b432ed47d0b8d 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -702,13 +702,13 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>, let mut block = START_BLOCK; let source_info = builder.source_info(span); let call_site_s = (call_site_scope, source_info); - unpack!(block = builder.in_scope(call_site_s, LintLevel::Inherited, block, |builder| { + unpack!(block = builder.in_scope(call_site_s, LintLevel::Inherited, |builder| { if should_abort_on_panic(tcx, fn_def_id, abi) { builder.schedule_abort(); } let arg_scope_s = (arg_scope, source_info); - unpack!(block = builder.in_scope(arg_scope_s, LintLevel::Inherited, block, |builder| { + unpack!(block = builder.in_scope(arg_scope_s, LintLevel::Inherited, |builder| { builder.args_and_body(block, &arguments, arg_scope, &body.value) })); // Attribute epilogue to function's closing brace diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 4aa463b37ab77..471304012c9a8 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -279,13 +279,13 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { pub fn in_opt_scope(&mut self, opt_scope: Option<(region::Scope, SourceInfo)>, - mut block: BasicBlock, f: F) -> BlockAnd where F: FnOnce(&mut Builder<'a, 'gcx, 'tcx>) -> BlockAnd { - debug!("in_opt_scope(opt_scope={:?}, block={:?})", opt_scope, block); + debug!("in_opt_scope(opt_scope={:?})", opt_scope); if let Some(region_scope) = opt_scope { self.push_scope(region_scope); } + let mut block; let rv = unpack!(block = f(self)); if let Some(region_scope) = opt_scope { unpack!(block = self.pop_scope(region_scope, block)); @@ -299,12 +299,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { pub fn in_scope(&mut self, region_scope: (region::Scope, SourceInfo), lint_level: LintLevel, - mut block: BasicBlock, f: F) -> BlockAnd where F: FnOnce(&mut Builder<'a, 'gcx, 'tcx>) -> BlockAnd { - debug!("in_scope(region_scope={:?}, block={:?})", region_scope, block); + debug!("in_scope(region_scope={:?})", region_scope); let source_scope = self.source_scope; let tcx = self.hir.tcx(); if let LintLevel::Explicit(current_hir_id) = lint_level { @@ -330,6 +329,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } self.push_scope(region_scope); + let mut block; let rv = unpack!(block = f(self)); unpack!(block = self.pop_scope(region_scope, block)); self.source_scope = source_scope; From 4bfb0453f537b2927574f29bdf90c9a22ea98add Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 30 Mar 2019 22:54:29 +0000 Subject: [PATCH 02/11] Give match arms an HirId and a Span --- src/librustc/hir/intravisit.rs | 1 + src/librustc/hir/lowering.rs | 8 ++++++++ src/librustc/hir/map/collector.rs | 10 ++++++++++ src/librustc/hir/map/mod.rs | 7 +++++++ src/librustc/hir/mod.rs | 4 ++++ src/librustc/hir/print.rs | 2 +- src/librustc_passes/hir_stats.rs | 2 +- src/librustc_typeck/check/_match.rs | 15 +++++++++------ src/libsyntax/ast.rs | 1 + src/libsyntax/ext/build.rs | 3 ++- src/libsyntax/mut_visit.rs | 6 +++++- src/libsyntax/parse/parser.rs | 4 ++++ 12 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs index 38d6d710868c0..517c99f99efea 100644 --- a/src/librustc/hir/intravisit.rs +++ b/src/librustc/hir/intravisit.rs @@ -1102,6 +1102,7 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr) { } pub fn walk_arm<'v, V: Visitor<'v>>(visitor: &mut V, arm: &'v Arm) { + visitor.visit_id(arm.hir_id); walk_list!(visitor, visit_pat, &arm.pats); if let Some(ref g) = arm.guard { match g { diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index 3ec4d4e8cc8f6..8dba7491bf504 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -1313,7 +1313,10 @@ impl<'a> LoweringContext<'a> { } fn lower_arm(&mut self, arm: &Arm) -> hir::Arm { + let LoweredNodeId { node_id: _, hir_id } = self.next_id(); + hir::Arm { + hir_id, attrs: self.lower_attrs(&arm.attrs), pats: arm.pats.iter().map(|x| self.lower_pat(x)).collect(), guard: match arm.guard { @@ -1321,6 +1324,7 @@ impl<'a> LoweringContext<'a> { _ => None, }, body: P(self.lower_expr(&arm.body)), + span: arm.span, } } @@ -5023,10 +5027,14 @@ impl<'a> LoweringContext<'a> { // Helper methods for building HIR. fn arm(&mut self, pats: hir::HirVec>, expr: P) -> hir::Arm { + let LoweredNodeId { node_id: _, hir_id } = self.next_id(); + hir::Arm { + hir_id, attrs: hir_vec![], pats, guard: None, + span: expr.span, body: expr, } } diff --git a/src/librustc/hir/map/collector.rs b/src/librustc/hir/map/collector.rs index eeba628b3bf21..b5203f9ec1f72 100644 --- a/src/librustc/hir/map/collector.rs +++ b/src/librustc/hir/map/collector.rs @@ -430,6 +430,16 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> { }); } + fn visit_arm(&mut self, arm: &'hir Arm) { + let node = Node::Arm(arm); + + self.insert(arm.span, arm.hir_id, node); + + self.with_parent(arm.hir_id, |this| { + intravisit::walk_arm(this, arm); + }); + } + fn visit_anon_const(&mut self, constant: &'hir AnonConst) { self.insert(DUMMY_SP, constant.hir_id, Node::AnonConst(constant)); diff --git a/src/librustc/hir/map/mod.rs b/src/librustc/hir/map/mod.rs index 4b94f772554e7..d8fe90d40481b 100644 --- a/src/librustc/hir/map/mod.rs +++ b/src/librustc/hir/map/mod.rs @@ -373,6 +373,7 @@ impl<'hir> Map<'hir> { Node::Pat(_) | Node::Binding(_) | Node::Local(_) | + Node::Arm(_) | Node::Lifetime(_) | Node::Visibility(_) | Node::Block(_) | @@ -1000,6 +1001,7 @@ impl<'hir> Map<'hir> { Some(Node::Field(ref f)) => Some(&f.attrs[..]), Some(Node::Expr(ref e)) => Some(&*e.attrs), Some(Node::Stmt(ref s)) => Some(s.node.attrs()), + Some(Node::Arm(ref a)) => Some(&*a.attrs), Some(Node::GenericParam(param)) => Some(¶m.attrs[..]), // Unit/tuple structs/variants take the attributes straight from // the struct/variant definition. @@ -1073,6 +1075,7 @@ impl<'hir> Map<'hir> { Some(Node::TraitRef(tr)) => tr.path.span, Some(Node::Binding(pat)) => pat.span, Some(Node::Pat(pat)) => pat.span, + Some(Node::Arm(arm)) => arm.span, Some(Node::Block(block)) => block.span, Some(Node::Ctor(..)) => match self.find_by_hir_id( self.get_parent_node_by_hir_id(hir_id)) @@ -1288,6 +1291,7 @@ impl<'a> print::State<'a> { Node::TraitRef(a) => self.print_trait_ref(&a), Node::Binding(a) | Node::Pat(a) => self.print_pat(&a), + Node::Arm(a) => self.print_arm(&a), Node::Block(a) => { use syntax::print::pprust::PrintState; @@ -1417,6 +1421,9 @@ fn hir_id_to_string(map: &Map<'_>, id: HirId, include_id: bool) -> String { Some(Node::Pat(_)) => { format!("pat {}{}", map.hir_to_pretty_string(id), id_str) } + Some(Node::Arm(_)) => { + format!("arm {}{}", map.hir_to_pretty_string(id), id_str) + } Some(Node::Block(_)) => { format!("block {}{}", map.hir_to_pretty_string(id), id_str) } diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs index 57304c5ed37ae..08feea3ccce42 100644 --- a/src/librustc/hir/mod.rs +++ b/src/librustc/hir/mod.rs @@ -1228,6 +1228,9 @@ pub struct Local { /// ` (if ) => `. #[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable)] pub struct Arm { + #[stable_hasher(ignore)] + pub hir_id: HirId, + pub span: Span, pub attrs: HirVec, /// Multiple patterns can be combined with `|` pub pats: HirVec>, @@ -2656,6 +2659,7 @@ pub enum Node<'hir> { TraitRef(&'hir TraitRef), Binding(&'hir Pat), Pat(&'hir Pat), + Arm(&'hir Arm), Block(&'hir Block), Local(&'hir Local), MacroDef(&'hir MacroDef), diff --git a/src/librustc/hir/print.rs b/src/librustc/hir/print.rs index 8a9028e544391..ef9fee5cab694 100644 --- a/src/librustc/hir/print.rs +++ b/src/librustc/hir/print.rs @@ -1862,7 +1862,7 @@ impl<'a> State<'a> { self.ann.post(self, AnnNode::Pat(pat)) } - fn print_arm(&mut self, arm: &hir::Arm) -> io::Result<()> { + pub fn print_arm(&mut self, arm: &hir::Arm) -> io::Result<()> { // I have no idea why this check is necessary, but here it // is :( if arm.attrs.is_empty() { diff --git a/src/librustc_passes/hir_stats.rs b/src/librustc_passes/hir_stats.rs index c74314ce0c4b5..0088c97679c66 100644 --- a/src/librustc_passes/hir_stats.rs +++ b/src/librustc_passes/hir_stats.rs @@ -149,7 +149,7 @@ impl<'v> hir_visit::Visitor<'v> for StatCollector<'v> { } fn visit_arm(&mut self, a: &'v hir::Arm) { - self.record("Arm", Id::None, a); + self.record("Arm", Id::Node(a.hir_id), a); hir_visit::walk_arm(self, a) } diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index a69f639e8941f..99b350b833274 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/src/librustc_typeck/check/_match.rs @@ -781,14 +781,17 @@ https://doc.rust-lang.org/reference/types.html#trait-objects"); fn maybe_get_coercion_reason(&self, hir_id: hir::HirId, span: Span) -> Option<(Span, String)> { use hir::Node::{Block, Item, Local}; - let node = self.tcx.hir().get_by_hir_id(self.tcx.hir().get_parent_node_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id(hir_id), - )); + let hir = self.tcx.hir(); + let arm_id = hir.get_parent_node_by_hir_id(hir_id); + let match_id = hir.get_parent_node_by_hir_id(arm_id); + let containing_id = hir.get_parent_node_by_hir_id(match_id); + + let node = hir.get_by_hir_id(containing_id); if let Block(block) = node { // check that the body's parent is an fn - let parent = self.tcx.hir().get_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id( - self.tcx.hir().get_parent_node_by_hir_id(block.hir_id), + let parent = hir.get_by_hir_id( + hir.get_parent_node_by_hir_id( + hir.get_parent_node_by_hir_id(block.hir_id), ), ); if let (Some(expr), Item(hir::Item { diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index a6bb47bef87e0..e2c2c46abf6b2 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -908,6 +908,7 @@ pub struct Arm { pub pats: Vec>, pub guard: Option, pub body: P, + pub span: Span, } #[derive(Clone, RustcEncodable, RustcDecodable, Debug)] diff --git a/src/libsyntax/ext/build.rs b/src/libsyntax/ext/build.rs index d24106f697e19..cb967a76822c9 100644 --- a/src/libsyntax/ext/build.rs +++ b/src/libsyntax/ext/build.rs @@ -890,12 +890,13 @@ impl<'a> AstBuilder for ExtCtxt<'a> { self.pat_tuple_struct(span, path, vec![pat]) } - fn arm(&self, _span: Span, pats: Vec>, expr: P) -> ast::Arm { + fn arm(&self, span: Span, pats: Vec>, expr: P) -> ast::Arm { ast::Arm { attrs: vec![], pats, guard: None, body: expr, + span, } } diff --git a/src/libsyntax/mut_visit.rs b/src/libsyntax/mut_visit.rs index f587e63e12b94..cb21014ec7646 100644 --- a/src/libsyntax/mut_visit.rs +++ b/src/libsyntax/mut_visit.rs @@ -392,11 +392,15 @@ pub fn noop_visit_use_tree(use_tree: &mut UseTree, vis: &mut T) { vis.visit_span(span); } -pub fn noop_visit_arm(Arm { attrs, pats, guard, body }: &mut Arm, vis: &mut T) { +pub fn noop_visit_arm( + Arm { attrs, pats, guard, body, span }: &mut Arm, + vis: &mut T, +) { visit_attrs(attrs, vis); visit_vec(pats, |pat| vis.visit_pat(pat)); visit_opt(guard, |guard| vis.visit_guard(guard)); vis.visit_expr(body); + vis.visit_span(span); } pub fn noop_visit_guard(g: &mut Guard, vis: &mut T) { diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 24d120376def1..ba36783e11e95 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -3946,6 +3946,7 @@ impl<'a> Parser<'a> { crate fn parse_arm(&mut self) -> PResult<'a, Arm> { let attrs = self.parse_outer_attributes()?; + let lo = self.span; let pats = self.parse_pats()?; let guard = if self.eat_keyword(keywords::If) { Some(Guard::If(self.parse_expr()?)) @@ -3965,6 +3966,8 @@ impl<'a> Parser<'a> { let require_comma = classify::expr_requires_semi_to_be_stmt(&expr) && self.token != token::CloseDelim(token::Brace); + let hi = self.span; + if require_comma { let cm = self.sess.source_map(); self.expect_one_of(&[token::Comma], &[token::CloseDelim(token::Brace)]) @@ -4008,6 +4011,7 @@ impl<'a> Parser<'a> { pats, guard, body: expr, + span: lo.to(hi), }) } From e784595c280da04c98e76a5ce9d603b58f6a88e2 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 30 Mar 2019 23:00:07 +0000 Subject: [PATCH 03/11] Respect lint attributes on match arms --- src/librustc/lint/mod.rs | 6 ++++++ src/test/ui/lint/lint-match-arms.rs | 18 ++++++++++++++++++ src/test/ui/lint/lint-match-arms.stderr | 14 ++++++++++++++ 3 files changed, 38 insertions(+) create mode 100644 src/test/ui/lint/lint-match-arms.rs create mode 100644 src/test/ui/lint/lint-match-arms.stderr diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 9c4683e094634..512e4d434434c 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -852,6 +852,12 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for LintLevelMapBuilder<'a, 'tcx> { }) } + fn visit_arm(&mut self, a: &'tcx hir::Arm) { + self.with_lint_attrs(a.hir_id, &a.attrs, |builder| { + intravisit::walk_arm(builder, a); + }) + } + fn visit_trait_item(&mut self, trait_item: &'tcx hir::TraitItem) { self.with_lint_attrs(trait_item.hir_id, &trait_item.attrs, |builder| { intravisit::walk_trait_item(builder, trait_item); diff --git a/src/test/ui/lint/lint-match-arms.rs b/src/test/ui/lint/lint-match-arms.rs new file mode 100644 index 0000000000000..2c471a61054b2 --- /dev/null +++ b/src/test/ui/lint/lint-match-arms.rs @@ -0,0 +1,18 @@ +fn deny_on_arm() { + match 0 { + #[deny(unused_variables)] + //~^ NOTE lint level defined here + y => (), + //~^ ERROR unused variable + } +} + +#[deny(unused_variables)] +fn allow_on_arm() { + match 0 { + #[allow(unused_variables)] + y => (), // OK + } +} + +fn main() {} diff --git a/src/test/ui/lint/lint-match-arms.stderr b/src/test/ui/lint/lint-match-arms.stderr new file mode 100644 index 0000000000000..e4e3adab0a9b2 --- /dev/null +++ b/src/test/ui/lint/lint-match-arms.stderr @@ -0,0 +1,14 @@ +error: unused variable: `y` + --> $DIR/lint-match-arms.rs:5:9 + | +LL | y => (), + | ^ help: consider prefixing with an underscore: `_y` + | +note: lint level defined here + --> $DIR/lint-match-arms.rs:3:16 + | +LL | #[deny(unused_variables)] + | ^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + From af6a9a2c62ba2e7fe54f53884b94fcd2f9a9265b Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Tue, 2 Apr 2019 22:29:28 +0100 Subject: [PATCH 04/11] Handle the visibility/lint scope distinction better * Don't generate an extra lint scope for each `let` statement. * Place match guards inside the visiblility scope of the bindings for their arm. --- src/librustc/hir/lowering.rs | 8 ++--- src/librustc_mir/build/block.rs | 36 +++++++++++-------- src/librustc_mir/build/matches/mod.rs | 27 ++++---------- src/librustc_mir/build/mod.rs | 11 +++--- src/librustc_mir/hair/mod.rs | 11 ++---- src/test/mir-opt/box_expr.rs | 4 +-- src/test/mir-opt/issue-41110.rs | 7 ++-- src/test/mir-opt/issue-49232.rs | 4 +-- .../mir-opt/packed-struct-drop-aligned.rs | 4 +-- 9 files changed, 45 insertions(+), 67 deletions(-) diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index 8dba7491bf504..591f713f81f79 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -1313,10 +1313,8 @@ impl<'a> LoweringContext<'a> { } fn lower_arm(&mut self, arm: &Arm) -> hir::Arm { - let LoweredNodeId { node_id: _, hir_id } = self.next_id(); - hir::Arm { - hir_id, + hir_id: self.next_id(), attrs: self.lower_attrs(&arm.attrs), pats: arm.pats.iter().map(|x| self.lower_pat(x)).collect(), guard: match arm.guard { @@ -5027,10 +5025,8 @@ impl<'a> LoweringContext<'a> { // Helper methods for building HIR. fn arm(&mut self, pats: hir::HirVec>, expr: P) -> hir::Arm { - let LoweredNodeId { node_id: _, hir_id } = self.next_id(); - hir::Arm { - hir_id, + hir_id: self.next_id(), attrs: hir_vec![], pats, guard: None, diff --git a/src/librustc_mir/build/block.rs b/src/librustc_mir/build/block.rs index d93223a4292c4..b5bab1585342a 100644 --- a/src/librustc_mir/build/block.rs +++ b/src/librustc_mir/build/block.rs @@ -113,31 +113,39 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let remainder_span = remainder_scope.span(this.hir.tcx(), &this.hir.region_scope_tree); - let scope; + let visibility_scope = + Some(this.new_source_scope(remainder_span, LintLevel::Inherited, None)); // Evaluate the initializer, if present. if let Some(init) = initializer { let initializer_span = init.span(); - scope = this.declare_bindings( - None, - remainder_span, - lint_level, - &pattern, - ArmHasGuard(false), - Some((None, initializer_span)), - ); unpack!(block = this.in_opt_scope( opt_destruction_scope.map(|de|(de, source_info)), |this| { let scope = (init_scope, source_info); this.in_scope(scope, lint_level, |this| { + this.declare_bindings( + visibility_scope, + remainder_span, + &pattern, + ArmHasGuard(false), + Some((None, initializer_span)), + ); this.expr_into_pattern(block, pattern, init) }) })); } else { - scope = this.declare_bindings( - None, remainder_span, lint_level, &pattern, - ArmHasGuard(false), None); + let scope = (init_scope, source_info); + unpack!(this.in_scope(scope, lint_level, |this| { + this.declare_bindings( + visibility_scope, + remainder_span, + &pattern, + ArmHasGuard(false), + None, + ); + block.unit() + })); debug!("ast_block_stmts: pattern={:?}", pattern); this.visit_bindings( @@ -149,8 +157,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { }) } - // Enter the source scope, after evaluating the initializer. - if let Some(source_scope) = scope { + // Enter the visibility scope, after evaluating the initializer. + if let Some(source_scope) = visibility_scope { this.source_scope = source_scope; } } diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 51d5c96083d8f..93a702dc44e25 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -259,12 +259,15 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let scope = self.declare_bindings( None, body.span, - LintLevel::Inherited, &arm.patterns[0], ArmHasGuard(arm.guard.is_some()), Some((Some(&scrutinee_place), scrutinee_span)), ); + if let Some(source_scope) = scope { + this.source_scope = source_scope; + } + for candidate in candidates { self.bind_and_guard_matched_candidate( candidate, @@ -275,9 +278,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ); } - if let Some(source_scope) = scope { - self.source_scope = source_scope; - } unpack!(arm_block = self.into(destination, arm_block, body)); @@ -489,33 +489,20 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { &mut self, mut visibility_scope: Option, scope_span: Span, - lint_level: LintLevel, pattern: &Pattern<'tcx>, has_guard: ArmHasGuard, opt_match_place: Option<(Option<&Place<'tcx>>, Span)>, ) -> Option { - assert!( - !(visibility_scope.is_some() && lint_level.is_explicit()), - "can't have both a visibility and a lint scope at the same time" - ); - let mut scope = self.source_scope; debug!("declare_bindings: pattern={:?}", pattern); self.visit_bindings( &pattern, UserTypeProjections::none(), &mut |this, mutability, name, mode, var, span, ty, user_ty| { if visibility_scope.is_none() { - // If we have lints, create a new source scope - // that marks the lints for the locals. See the comment - // on the `source_info` field for why this is needed. - if lint_level.is_explicit() { - scope = this.new_source_scope(scope_span, lint_level, None); - } - visibility_scope = Some(this.new_source_scope(scope_span, - LintLevel::Inherited, - None)); + visibility_scope = + Some(this.new_source_scope(scope_span, LintLevel::Inherited, None)); } - let source_info = SourceInfo { span, scope }; + let source_info = SourceInfo { span, this.source_scope }; let visibility_scope = visibility_scope.unwrap(); this.declare_binding( source_info, diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index b432ed47d0b8d..8cd4ac0ad3ade 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -945,10 +945,13 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { self.var_indices.insert(var, LocalsForNode::One(local)); } _ => { - scope = self.declare_bindings(scope, ast_body.span, - LintLevel::Inherited, &pattern, - matches::ArmHasGuard(false), - Some((Some(&place), span))); + scope = self.declare_bindings( + scope, + ast_body.span, + &pattern, + matches::ArmHasGuard(false), + Some((Some(&place), span)), + ); unpack!(block = self.place_into_pattern(block, pattern, &place, false)); } } diff --git a/src/librustc_mir/hair/mod.rs b/src/librustc_mir/hair/mod.rs index d4f139e103a64..8e19913f4df26 100644 --- a/src/librustc_mir/hair/mod.rs +++ b/src/librustc_mir/hair/mod.rs @@ -31,15 +31,6 @@ pub enum LintLevel { Explicit(hir::HirId) } -impl LintLevel { - pub fn is_explicit(self) -> bool { - match self { - LintLevel::Inherited => false, - LintLevel::Explicit(_) => true - } - } -} - #[derive(Clone, Debug)] pub struct Block<'tcx> { pub targeted_by_break: bool, @@ -311,6 +302,8 @@ pub struct Arm<'tcx> { pub guard: Option>, pub body: ExprRef<'tcx>, pub lint_level: LintLevel, + pub scope: region::Scope, + pub span: Span, } #[derive(Clone, Debug)] diff --git a/src/test/mir-opt/box_expr.rs b/src/test/mir-opt/box_expr.rs index d4852db6d475e..ee6adfefe3e36 100644 --- a/src/test/mir-opt/box_expr.rs +++ b/src/test/mir-opt/box_expr.rs @@ -22,13 +22,11 @@ impl Drop for S { // END RUST SOURCE // START rustc.main.ElaborateDrops.before.mir // let mut _0: (); +// let _1: std::boxed::Box; // let mut _2: std::boxed::Box; // let mut _3: (); // let mut _4: std::boxed::Box; // scope 1 { -// let _1: std::boxed::Box; -// } -// scope 2 { // } // bb0: { // StorageLive(_1); diff --git a/src/test/mir-opt/issue-41110.rs b/src/test/mir-opt/issue-41110.rs index 023440af0eb10..0b678be2ab319 100644 --- a/src/test/mir-opt/issue-41110.rs +++ b/src/test/mir-opt/issue-41110.rs @@ -29,27 +29,24 @@ impl S { // END RUST SOURCE // START rustc.main.ElaborateDrops.after.mir // let mut _0: (); +// let _1: (); // let mut _2: S; // let mut _3: S; // let mut _4: S; // let mut _5: bool; // scope 1 { -// let _1: (); -// } -// scope 2 { // } // ... // bb0: { // END rustc.main.ElaborateDrops.after.mir // START rustc.test.ElaborateDrops.after.mir // let mut _0: (); +// let _1: S; // let mut _3: (); // let mut _4: S; // let mut _5: S; // let mut _6: bool; // ... -// let _1: S; -// ... // let mut _2: S; // ... // bb0: { diff --git a/src/test/mir-opt/issue-49232.rs b/src/test/mir-opt/issue-49232.rs index 29446d2ecc23e..3910183dee789 100644 --- a/src/test/mir-opt/issue-49232.rs +++ b/src/test/mir-opt/issue-49232.rs @@ -18,14 +18,12 @@ fn main() { // fn main() -> (){ // let mut _0: (); // let mut _1: (); +// let _2: i32; // let mut _3: bool; // let mut _4: !; // let mut _5: (); // let mut _6: &i32; // scope 1 { -// let _2: i32; -// } -// scope 2 { // } // bb0: { // goto -> bb1; diff --git a/src/test/mir-opt/packed-struct-drop-aligned.rs b/src/test/mir-opt/packed-struct-drop-aligned.rs index 7e8c58e64c28d..da73cc96348f0 100644 --- a/src/test/mir-opt/packed-struct-drop-aligned.rs +++ b/src/test/mir-opt/packed-struct-drop-aligned.rs @@ -18,15 +18,13 @@ impl Drop for Droppy { // START rustc.main.EraseRegions.before.mir // fn main() -> () { // let mut _0: (); +// let mut _1: Packed; // let mut _2: Aligned; // let mut _3: Droppy; // let mut _4: Aligned; // let mut _5: Droppy; // let mut _6: Aligned; // scope 1 { -// let mut _1: Packed; -// } -// scope 2 { // } // // bb0: { From f506aea1fac0977c7215b4240f4d99b45bf7ae97 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 3 Apr 2019 19:21:51 +0100 Subject: [PATCH 05/11] Give match arms a drop/region scope Also give arms the correct lint scope in MIR. --- src/librustc/cfg/construct.rs | 10 +- src/librustc/middle/region.rs | 34 +++-- src/librustc_mir/build/matches/mod.rs | 87 +++++++----- src/librustc_mir/build/scope.rs | 84 +++++++++++- src/librustc_mir/hair/cx/expr.rs | 8 +- src/test/mir-opt/match_false_edges.rs | 126 +++++++++++------- src/test/mir-opt/match_test.rs | 33 ++--- src/test/mir-opt/remove_fake_borrows.rs | 50 +++---- src/test/ui/lint/lint-unused-mut-variables.rs | 8 ++ .../ui/lint/lint-unused-mut-variables.stderr | 4 +- 10 files changed, 299 insertions(+), 145 deletions(-) diff --git a/src/librustc/cfg/construct.rs b/src/librustc/cfg/construct.rs index 2e54165be1f1b..ef0d4be268eaf 100644 --- a/src/librustc/cfg/construct.rs +++ b/src/librustc/cfg/construct.rs @@ -419,7 +419,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> { for arm in arms { // Add an exit node for when we've visited all the // patterns and the guard (if there is one) in the arm. - let arm_exit = self.add_dummy_node(&[]); + let bindings_exit = self.add_dummy_node(&[]); for pat in &arm.pats { // Visit the pattern, coming from the discriminant exit @@ -453,14 +453,16 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> { // Add an edge from the exit of this pattern to the // exit of the arm - self.add_contained_edge(pat_exit, arm_exit); + self.add_contained_edge(pat_exit, bindings_exit); } // Visit the body of this arm - let body_exit = self.expr(&arm.body, arm_exit); + let body_exit = self.expr(&arm.body, bindings_exit); + + let arm_exit = self.add_ast_node(arm.hir_id.local_id, &[body_exit]); // Link the body to the exit of the expression - self.add_contained_edge(body_exit, expr_exit); + self.add_contained_edge(arm_exit, expr_exit); } expr_exit diff --git a/src/librustc/middle/region.rs b/src/librustc/middle/region.rs index 37681ad7fcdd2..fa4e8e3d4769d 100644 --- a/src/librustc/middle/region.rs +++ b/src/librustc/middle/region.rs @@ -119,18 +119,18 @@ impl fmt::Debug for Scope { pub enum ScopeData { Node, - // Scope of the call-site for a function or closure - // (outlives the arguments as well as the body). + /// Scope of the call-site for a function or closure + /// (outlives the arguments as well as the body). CallSite, - // Scope of arguments passed to a function or closure - // (they outlive its body). + /// Scope of arguments passed to a function or closure + /// (they outlive its body). Arguments, - // Scope of destructors for temporaries of node-id. + /// Scope of destructors for temporaries of node-id. Destruction, - // Scope following a `let id = expr;` binding in a block. + /// Scope following a `let id = expr;` binding in a block. Remainder(FirstStatementIndex) } @@ -152,11 +152,11 @@ newtype_index! { /// /// * The subscope with `first_statement_index == 1` is scope of `c`, /// and thus does not include EXPR_2, but covers the `...`. - pub struct FirstStatementIndex { .. } + pub struct FirstStatementIndex { + derive [HashStable] + } } -impl_stable_hash_for!(struct crate::middle::region::FirstStatementIndex { private }); - // compilation error if size of `ScopeData` is not the same as a `u32` static_assert_size!(ScopeData, 4); @@ -814,6 +814,16 @@ fn resolve_block<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, blk: } fn resolve_arm<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, arm: &'tcx hir::Arm) { + let prev_cx = visitor.cx; + + visitor.enter_scope( + Scope { + id: arm.hir_id.local_id, + data: ScopeData::Node, + } + ); + visitor.cx.var_parent = visitor.cx.parent; + visitor.terminating_scopes.insert(arm.body.hir_id.local_id); if let Some(hir::Guard::If(ref expr)) = arm.guard { @@ -821,6 +831,8 @@ fn resolve_arm<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, arm: & } intravisit::walk_arm(visitor, arm); + + visitor.cx = prev_cx; } fn resolve_pat<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, pat: &'tcx hir::Pat) { @@ -893,10 +905,6 @@ fn resolve_expr<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, expr: terminating(body.hir_id.local_id); } - hir::ExprKind::Match(..) => { - visitor.cx.var_parent = visitor.cx.parent; - } - hir::ExprKind::DropTemps(ref expr) => { // `DropTemps(expr)` does not denote a conditional scope. // Rather, we want to achieve the same behavior as `{ let _t = expr; _t }`. diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 93a702dc44e25..091e39630d6cb 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -12,6 +12,7 @@ use crate::build::{GuardFrame, GuardFrameLocal, LocalsForNode}; use crate::hair::{self, *}; use rustc::hir::HirId; use rustc::mir::*; +use rustc::middle::region; use rustc::ty::{self, CanonicalUserTypeAnnotation, Ty}; use rustc::ty::layout::VariantIdx; use rustc_data_structures::bit_set::BitSet; @@ -251,37 +252,39 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // Step 5. Create everything else: the guards and the arms. - let outer_source_info = self.source_info(span); let arm_end_blocks: Vec<_> = arm_candidates.into_iter().map(|(arm, candidates)| { - let mut arm_block = self.cfg.start_new_block(); - - let body = self.hir.mirror(arm.body.clone()); - let scope = self.declare_bindings( - None, - body.span, - &arm.patterns[0], - ArmHasGuard(arm.guard.is_some()), - Some((Some(&scrutinee_place), scrutinee_span)), - ); - - if let Some(source_scope) = scope { - this.source_scope = source_scope; - } - - for candidate in candidates { - self.bind_and_guard_matched_candidate( - candidate, - arm.guard.clone(), - arm_block, - &fake_borrow_temps, - scrutinee_span, + let arm_source_info = self.source_info(arm.span); + let region_scope = (arm.scope, arm_source_info); + self.in_scope(region_scope, arm.lint_level, |this| { + let arm_block = this.cfg.start_new_block(); + + let body = this.hir.mirror(arm.body.clone()); + let scope = this.declare_bindings( + None, + arm.span, + &arm.patterns[0], + ArmHasGuard(arm.guard.is_some()), + Some((Some(&scrutinee_place), scrutinee_span)), ); - } + if let Some(source_scope) = scope { + this.source_scope = source_scope; + } - unpack!(arm_block = self.into(destination, arm_block, body)); + for candidate in candidates { + this.clear_top_scope(arm.scope); + this.bind_and_guard_matched_candidate( + candidate, + arm.guard.clone(), + arm_block, + &fake_borrow_temps, + scrutinee_span, + region_scope, + ); + } - arm_block + this.into(destination, arm_block, body) + }) }).collect(); // all the arm blocks will rejoin here @@ -289,7 +292,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { for arm_block in arm_end_blocks { self.cfg.terminate( - arm_block, + unpack!(arm_block), outer_source_info, TerminatorKind::Goto { target: end_block }, ); @@ -502,7 +505,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { visibility_scope = Some(this.new_source_scope(scope_span, LintLevel::Inherited, None)); } - let source_info = SourceInfo { span, this.source_scope }; + let source_info = SourceInfo { span, scope: this.source_scope }; let visibility_scope = visibility_scope.unwrap(); this.declare_binding( source_info, @@ -1315,6 +1318,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { arm_block: BasicBlock, fake_borrows: &Vec<(&Place<'tcx>, Local)>, scrutinee_span: Span, + region_scope: (region::Scope, SourceInfo), ) { debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate); @@ -1497,17 +1501,40 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // // and that is clearly not correct. let post_guard_block = self.cfg.start_new_block(); + let otherwise_post_guard_block = self.cfg.start_new_block(); self.cfg.terminate( block, source_info, TerminatorKind::if_( self.hir.tcx(), - cond, + cond.clone(), post_guard_block, - candidate.otherwise_block.unwrap() + otherwise_post_guard_block, ), ); + self.exit_scope( + source_info.span, + region_scope, + otherwise_post_guard_block, + candidate.otherwise_block.unwrap(), + ); + + if let Operand::Copy(cond_place) | Operand::Move(cond_place) = cond { + if let Place::Base(PlaceBase::Local(cond_temp)) = cond_place { + // We will call `clear_top_scope` if there's another guard. So + // we have to drop this variable now or it will be "storage + // leaked". + self.pop_variable( + post_guard_block, + region_scope.0, + cond_temp + ); + } else { + bug!("Expected as_local_operand to produce a temporary"); + } + } + let by_value_bindings = candidate.bindings.iter().filter(|binding| { if let BindingMode::ByValue = binding.binding_mode { true } else { false } }); diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 471304012c9a8..0d1d40a8af633 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -19,13 +19,18 @@ paragraph). This is because region scopes are tied to them. Eventually, when we shift to non-lexical lifetimes, there should be no need to remember this mapping. -There is one additional wrinkle, actually, that I wanted to hide from -you but duty compels me to mention. In the course of building -matches, it sometimes happen that certain code (namely guards) gets -executed multiple times. This means that the scope lexical scope may -in fact correspond to multiple, disjoint SEME regions. So in fact our +### Not so SEME Regions + +In the course of building matches, it sometimes happens that certain code +(namely guards) gets executed multiple times. This means that the scope lexical +scope may in fact correspond to multiple, disjoint SEME regions. So in fact our mapping is from one scope to a vector of SEME regions. +Also in matches, the scopes assigned to arms are not even SEME regions! Each +arm has a single region with one entry for each pattern. We manually +manipulate the scheduled drops in this scope to avoid dropping things multiple +times, although drop elaboration would clean this up for value drops. + ### Drops The primary purpose for scopes is to insert drops: while building @@ -731,7 +736,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // Note that this code iterates scopes from the inner-most to the outer-most, // invalidating caches of each scope visited. This way bare minimum of the // caches gets invalidated. i.e., if a new drop is added into the middle scope, the - // cache of outer scpoe stays intact. + // cache of outer scope stays intact. scope.invalidate_cache(!needs_drop, this_scope); if this_scope { if let DropKind::Value { .. } = drop_kind { @@ -873,6 +878,73 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { success_block } + + // `match` arm scopes + // ================== + /// Unschedules any drops in the top scope. + /// + /// This is only needed for `match` arm scopes, because they have one + /// entrance per pattern, but only one exit. + pub fn clear_top_scope(&mut self, region_scope: region::Scope) { + let top_scope = self.scopes.last_mut().unwrap(); + + assert_eq!(top_scope.region_scope, region_scope); + + top_scope.drops.clear(); + top_scope.invalidate_cache(false, true); + } + + /// Drops the single variable provided + /// + /// * The scope must be the top scope. + /// * The variable must be in that scope. + /// * The variable must be at the top of that scope: it's the next thing + /// scheduled to drop. + /// * The drop must be of DropKind::Storage. + /// + /// This is used for the boolean holding the result of the match guard. We + /// do this because: + /// + /// * The boolean is different for each pattern + /// * There is only one exit for the arm scope + /// * The guard expression scope is too short, it ends just before the + /// boolean is tested. + pub fn pop_variable( + &mut self, + block: BasicBlock, + region_scope: region::Scope, + variable: Local, + ) { + let top_scope = self.scopes.last_mut().unwrap(); + + assert_eq!(top_scope.region_scope, region_scope); + + let top_drop_data = top_scope.drops.pop().unwrap(); + + match top_drop_data.kind { + DropKind::Value { .. } => { + bug!("Should not be calling pop_top_variable on non-copy type!") + } + DropKind::Storage => { + // Drop the storage for both value and storage drops. + // Only temps and vars need their storage dead. + match top_drop_data.location { + Place::Base(PlaceBase::Local(index)) => { + let source_info = top_scope.source_info(top_drop_data.span); + assert_eq!(index, variable); + self.cfg.push(block, Statement { + source_info, + kind: StatementKind::StorageDead(index) + }); + } + _ => unreachable!(), + } + } + } + + top_scope.invalidate_cache(true, true); + } + } /// Builds drops for pop_scope and exit_scope. diff --git a/src/librustc_mir/hair/cx/expr.rs b/src/librustc_mir/hair/cx/expr.rs index 50140880a368d..d623f149988c7 100644 --- a/src/librustc_mir/hair/cx/expr.rs +++ b/src/librustc_mir/hair/cx/expr.rs @@ -879,8 +879,12 @@ fn convert_arm<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, arm: &'tcx hir::Arm) _ => None, }, body: arm.body.to_ref(), - // BUG: fix this - lint_level: LintLevel::Inherited, + lint_level: LintLevel::Explicit(arm.hir_id), + scope: region::Scope { + id: arm.hir_id.local_id, + data: region::ScopeData::Node + }, + span: arm.span, } } diff --git a/src/test/mir-opt/match_false_edges.rs b/src/test/mir-opt/match_false_edges.rs index 7ac36a22274f3..0850c552536c5 100644 --- a/src/test/mir-opt/match_false_edges.rs +++ b/src/test/mir-opt/match_false_edges.rs @@ -45,13 +45,13 @@ fn main() { // _2 = std::option::Option::::Some(const 42i32,); // FakeRead(ForMatchedPlace, _2); // _3 = discriminant(_2); -// switchInt(move _3) -> [0isize: bb4, 1isize: bb2, otherwise: bb7]; +// switchInt(move _3) -> [0isize: bb4, 1isize: bb2, otherwise: bb6]; // } // bb1 (cleanup): { // resume; // } // bb2: { -// falseEdges -> [real: bb8, imaginary: bb3]; //pre_binding1 +// falseEdges -> [real: bb7, imaginary: bb3]; //pre_binding1 // } // bb3: { // falseEdges -> [real: bb11, imaginary: bb4]; //pre_binding2 @@ -62,48 +62,56 @@ fn main() { // bb5: { // unreachable; // } -// bb6: { // to pre_binding2 -// falseEdges -> [real: bb3, imaginary: bb3]; -// } -// bb7: { +// bb6: { // unreachable; // } -// bb8: { // binding1 and guard +// bb7: { // binding1 and guard // StorageLive(_6); // _6 = &(((promoted[0]: std::option::Option) as Some).0: i32); // _4 = &shallow _2; // StorageLive(_7); -// _7 = const guard() -> [return: bb9, unwind: bb1]; +// _7 = const guard() -> [return: bb8, unwind: bb1]; // } -// bb9: { +// bb8: { // end of guard // FakeRead(ForMatchGuard, _4); // FakeRead(ForGuardBinding, _6); -// switchInt(move _7) -> [false: bb6, otherwise: bb10]; +// switchInt(move _7) -> [false: bb10, otherwise: bb9]; // } -// bb10: { +// bb9: { // arm1 +// StorageDead(_7); // StorageLive(_5); // _5 = ((_2 as Some).0: i32); // StorageLive(_8); // _8 = _5; // _1 = (const 1i32, move _8); // StorageDead(_8); +// StorageDead(_5); +// StorageDead(_6); // goto -> bb13; // } -// bb11: { +// bb10: { // to pre_binding2 +// StorageDead(_7); +// StorageDead(_6); +// falseEdges -> [real: bb3, imaginary: bb3]; +// } +// bb11: { // arm2 // StorageLive(_9); // _9 = ((_2 as Some).0: i32); // StorageLive(_10); // _10 = _9; // _1 = (const 2i32, move _10); // StorageDead(_10); +// StorageDead(_9); // goto -> bb13; // } -// bb12: { +// bb12: { // arm3 // _1 = (const 3i32, const 3i32); // goto -> bb13; // } // bb13: { -// ... +// StorageDead(_1); +// StorageDead(_2); +// _0 = (); // return; // } // END rustc.full_tested_match.QualifyAndPromoteConstants.after.mir @@ -114,13 +122,13 @@ fn main() { // _2 = std::option::Option::::Some(const 42i32,); // FakeRead(ForMatchedPlace, _2); // _3 = discriminant(_2); -// switchInt(move _3) -> [0isize: bb3, 1isize: bb2, otherwise: bb7]; +// switchInt(move _3) -> [0isize: bb3, 1isize: bb2, otherwise: bb6]; // } // bb1 (cleanup): { // resume; // } // bb2: { -// falseEdges -> [real: bb8, imaginary: bb3]; +// falseEdges -> [real: bb7, imaginary: bb3]; // } // bb3: { // falseEdges -> [real: bb11, imaginary: bb4]; @@ -131,33 +139,38 @@ fn main() { // bb5: { // unreachable; // } -// bb6: { // to pre_binding3 (can skip 2 since this is `Some`) -// falseEdges -> [real: bb4, imaginary: bb3]; -// } -// bb7: { +// bb6: { // unreachable; // } -// bb8: { // binding1 and guard +// bb7: { // binding1 and guard // StorageLive(_6); // _6 = &((_2 as Some).0: i32); // _4 = &shallow _2; // StorageLive(_7); -// _7 = const guard() -> [return: bb9, unwind: bb1]; +// _7 = const guard() -> [return: bb8, unwind: bb1]; // } -// bb9: { // end of guard +// bb8: { // end of guard // FakeRead(ForMatchGuard, _4); // FakeRead(ForGuardBinding, _6); -// switchInt(move _7) -> [false: bb6, otherwise: bb10]; +// switchInt(move _7) -> [false: bb10, otherwise: bb9]; // } -// bb10: { // arm1 +// bb9: { // arm1 +// StorageDead(_7); // StorageLive(_5); // _5 = ((_2 as Some).0: i32); // StorageLive(_8); // _8 = _5; // _1 = (const 1i32, move _8); // StorageDead(_8); +// StorageDead(_5); +// StorageDead(_6); // goto -> bb13; // } +// bb10: { // to pre_binding3 (can skip 2 since this is `Some`) +// StorageDead(_7); +// StorageDead(_6); +// falseEdges -> [real: bb4, imaginary: bb3]; +// } // bb11: { // arm2 // _1 = (const 3i32, const 3i32); // goto -> bb13; @@ -169,16 +182,19 @@ fn main() { // _10 = _9; // _1 = (const 2i32, move _10); // StorageDead(_10); +// StorageDead(_9); // goto -> bb13; // } // bb13: { -// ... +// StorageDead(_1); +// StorageDead(_2); +// _0 = (); // return; // } // END rustc.full_tested_match2.QualifyAndPromoteConstants.before.mir // // START rustc.main.QualifyAndPromoteConstants.before.mir -// bb0: { +// bb0: { // ... // _2 = std::option::Option::::Some(const 1i32,); // FakeRead(ForMatchedPlace, _2); @@ -189,13 +205,13 @@ fn main() { // resume; // } // bb2: { -// falseEdges -> [real: bb9, imaginary: bb3]; +// falseEdges -> [real: bb7, imaginary: bb3]; // } // bb3: { -// falseEdges -> [real: bb12, imaginary: bb4]; +// falseEdges -> [real: bb11, imaginary: bb4]; // } // bb4: { -// falseEdges -> [real: bb13, imaginary: bb5]; +// falseEdges -> [real: bb12, imaginary: bb5]; // } // bb5: { // falseEdges -> [real: bb16, imaginary: bb6]; @@ -203,65 +219,79 @@ fn main() { // bb6: { // unreachable; // } -// bb7: { -// falseEdges -> [real: bb3, imaginary: bb3]; -// } -// bb8: { -// falseEdges -> [real: bb5, imaginary: bb5]; -// } -// bb9: { // binding1: Some(w) if guard() +// bb7: { // binding1: Some(w) if guard() // StorageLive(_7); // _7 = &((_2 as Some).0: i32); // _5 = &shallow _2; // StorageLive(_8); -// _8 = const guard() -> [return: bb10, unwind: bb1]; +// _8 = const guard() -> [return: bb8, unwind: bb1]; // } -// bb10: { //end of guard +// bb8: { //end of guard1 // FakeRead(ForMatchGuard, _5); // FakeRead(ForGuardBinding, _7); -// switchInt(move _8) -> [false: bb7, otherwise: bb11]; +// switchInt(move _8) -> [false: bb10, otherwise: bb9]; // } -// bb11: { // set up bindings for arm1 +// bb9: { +// StorageDead(_8); // StorageLive(_6); // _6 = ((_2 as Some).0: i32); // _1 = const 1i32; +// StorageDead(_6); +// StorageDead(_7); // goto -> bb17; // } -// bb12: { // binding2 & arm2 +// bb10: { +// StorageDead(_8); +// StorageDead(_7); +// falseEdges -> [real: bb3, imaginary: bb3]; +// } +// bb11: { // binding2 & arm2 // StorageLive(_9); // _9 = _2; // _1 = const 2i32; +// StorageDead(_9); // goto -> bb17; // } -// bb13: { // binding3: Some(y) if guard2(y) +// bb12: { // binding3: Some(y) if guard2(y) // StorageLive(_11); // _11 = &((_2 as Some).0: i32); // _5 = &shallow _2; // StorageLive(_12); // StorageLive(_13); // _13 = (*_11); -// _12 = const guard2(move _13) -> [return: bb14, unwind: bb1]; +// _12 = const guard2(move _13) -> [return: bb13, unwind: bb1]; // } -// bb14: { // end of guard2 +// bb13: { // end of guard2 // StorageDead(_13); // FakeRead(ForMatchGuard, _5); // FakeRead(ForGuardBinding, _11); -// switchInt(move _12) -> [false: bb8, otherwise: bb15]; +// switchInt(move _12) -> [false: bb15, otherwise: bb14]; // } -// bb15: { // binding4 & arm4 +// bb14: { // binding4 & arm4 +// StorageDead(_12); // StorageLive(_10); // _10 = ((_2 as Some).0: i32); // _1 = const 3i32; +// StorageDead(_10); +// StorageDead(_11); // goto -> bb17; // } +// bb15: { +// StorageDead(_12); +// StorageDead(_11); +// falseEdges -> [real: bb5, imaginary: bb5]; +// } // bb16: { // StorageLive(_14); // _14 = _2; // _1 = const 4i32; +// StorageDead(_14); // goto -> bb17; // } // bb17: { -// ... +// StorageDead(_1); +// StorageDead(_2); +// _0 = (); // return; // } // END rustc.main.QualifyAndPromoteConstants.before.mir diff --git a/src/test/mir-opt/match_test.rs b/src/test/mir-opt/match_test.rs index a5317f98ef188..2ef9520c12c63 100644 --- a/src/test/mir-opt/match_test.rs +++ b/src/test/mir-opt/match_test.rs @@ -20,10 +20,10 @@ fn main() { // START rustc.main.SimplifyCfg-initial.after.mir // bb0: { // ... -// switchInt(move _4) -> [false: bb7, otherwise: bb8]; +// switchInt(move _4) -> [false: bb6, otherwise: bb7]; // } // bb1: { -// falseEdges -> [real: bb12, imaginary: bb2]; +// falseEdges -> [real: bb10, imaginary: bb2]; // } // bb2: { // falseEdges -> [real: bb13, imaginary: bb3]; @@ -38,33 +38,35 @@ fn main() { // unreachable; // } // bb6: { -// falseEdges -> [real: bb4, imaginary: bb2]; +// _6 = Le(const 10i32, _1); +// switchInt(move _6) -> [false: bb8, otherwise: bb9]; // } // bb7: { -// _6 = Le(const 10i32, _1); -// switchInt(move _6) -> [false: bb9, otherwise: bb10]; +// _5 = Lt(_1, const 10i32); +// switchInt(move _5) -> [false: bb6, otherwise: bb1]; // } // bb8: { -// _5 = Lt(_1, const 10i32); -// switchInt(move _5) -> [false: bb7, otherwise: bb1]; +// switchInt(_1) -> [-1i32: bb3, otherwise: bb4]; // } // bb9: { -// switchInt(_1) -> [-1i32: bb3, otherwise: bb4]; +// _7 = Le(_1, const 20i32); +// switchInt(move _7) -> [false: bb8, otherwise: bb2]; // } // bb10: { -// _7 = Le(_1, const 20i32); -// switchInt(move _7) -> [false: bb9, otherwise: bb2]; +// _8 = &shallow _1; +// StorageLive(_9); +// _9 = _2; +// FakeRead(ForMatchGuard, _8); +// switchInt(move _9) -> [false: bb12, otherwise: bb11]; // } // bb11: { +// StorageDead(_9); // _3 = const 0i32; // goto -> bb16; // } // bb12: { -// _8 = &shallow _1; -// StorageLive(_9); -// _9 = _2; -// FakeRead(ForMatchGuard, _8); -// switchInt(move _9) -> [false: bb6, otherwise: bb11]; +// StorageDead(_9); +// falseEdges -> [real: bb4, imaginary: bb2]; // } // bb13: { // _3 = const 1i32; @@ -79,7 +81,6 @@ fn main() { // goto -> bb16; // } // bb16: { -// StorageDead(_9); // _0 = (); // StorageDead(_2); // StorageDead(_1); diff --git a/src/test/mir-opt/remove_fake_borrows.rs b/src/test/mir-opt/remove_fake_borrows.rs index 8348f9a774678..6ac9cee79f53a 100644 --- a/src/test/mir-opt/remove_fake_borrows.rs +++ b/src/test/mir-opt/remove_fake_borrows.rs @@ -19,10 +19,10 @@ fn main() { // bb0: { // FakeRead(ForMatchedPlace, _1); // _3 = discriminant(_1); -// switchInt(move _3) -> [1isize: bb5, otherwise: bb2]; +// switchInt(move _3) -> [1isize: bb4, otherwise: bb2]; // } // bb1: { -// goto -> bb7; +// goto -> bb5; // } // bb2: { // goto -> bb8; @@ -31,16 +31,9 @@ fn main() { // unreachable; // } // bb4: { -// goto -> bb2; -// } -// bb5: { // switchInt((*(*((_1 as Some).0: &' &' i32)))) -> [0i32: bb1, otherwise: bb2]; // } -// bb6: { -// _0 = const 0i32; -// goto -> bb9; -// } -// bb7: { +// bb5: { // _4 = &shallow _1; // _5 = &shallow ((_1 as Some).0: &' &' i32); // _6 = &shallow (*((_1 as Some).0: &' &' i32)); @@ -51,14 +44,22 @@ fn main() { // FakeRead(ForMatchGuard, _5); // FakeRead(ForMatchGuard, _6); // FakeRead(ForMatchGuard, _7); -// switchInt(move _8) -> [false: bb4, otherwise: bb6]; +// switchInt(move _8) -> [false: bb7, otherwise: bb6]; +// } +// bb6: { +// StorageDead(_8); +// _0 = const 0i32; +// goto -> bb9; +// } +// bb7: { +// StorageDead(_8); +// goto -> bb2; // } // bb8: { // _0 = const 1i32; // goto -> bb9; // } // bb9: { -// StorageDead(_8); // return; // } // bb10 (cleanup): { @@ -70,10 +71,10 @@ fn main() { // bb0: { // nop; // _3 = discriminant(_1); -// switchInt(move _3) -> [1isize: bb5, otherwise: bb2]; +// switchInt(move _3) -> [1isize: bb4, otherwise: bb2]; // } // bb1: { -// goto -> bb7; +// goto -> bb5; // } // bb2: { // goto -> bb8; @@ -82,16 +83,9 @@ fn main() { // unreachable; // } // bb4: { -// goto -> bb2; -// } -// bb5: { // switchInt((*(*((_1 as Some).0: &' &' i32)))) -> [0i32: bb1, otherwise: bb2]; // } -// bb6: { -// _0 = const 0i32; -// goto -> bb9; -// } -// bb7: { +// bb5: { // nop; // nop; // nop; @@ -102,14 +96,22 @@ fn main() { // nop; // nop; // nop; -// switchInt(move _8) -> [false: bb4, otherwise: bb6]; +// switchInt(move _8) -> [false: bb7, otherwise: bb6]; +// } +// bb6: { +// StorageDead(_8); +// _0 = const 0i32; +// goto -> bb9; +// } +// bb7: { +// StorageDead(_8); +// goto -> bb2; // } // bb8: { // _0 = const 1i32; // goto -> bb9; // } // bb9: { -// StorageDead(_8); // return; // } // bb10 (cleanup): { diff --git a/src/test/ui/lint/lint-unused-mut-variables.rs b/src/test/ui/lint/lint-unused-mut-variables.rs index da0236ac45070..78609a6e24b5e 100644 --- a/src/test/ui/lint/lint-unused-mut-variables.rs +++ b/src/test/ui/lint/lint-unused-mut-variables.rs @@ -105,6 +105,14 @@ fn main() { _ => {} } + // Attribute should be respected on match arms + match 0 { + #[allow(unused_mut)] + mut x => { + let mut y = 1; + }, + } + let x = |mut y: isize| y = 32; fn nothing(mut foo: isize) { foo = 37; } diff --git a/src/test/ui/lint/lint-unused-mut-variables.stderr b/src/test/ui/lint/lint-unused-mut-variables.stderr index e41d8f8ac7408..1a175c9683ec7 100644 --- a/src/test/ui/lint/lint-unused-mut-variables.stderr +++ b/src/test/ui/lint/lint-unused-mut-variables.stderr @@ -133,7 +133,7 @@ LL | fn mut_ref_arg(mut arg : &mut [u8]) -> &mut [u8] { | help: remove this `mut` error: variable does not need to be mutable - --> $DIR/lint-unused-mut-variables.rs:130:9 + --> $DIR/lint-unused-mut-variables.rs:138:9 | LL | let mut b = vec![2]; | ----^ @@ -141,7 +141,7 @@ LL | let mut b = vec![2]; | help: remove this `mut` | note: lint level defined here - --> $DIR/lint-unused-mut-variables.rs:126:8 + --> $DIR/lint-unused-mut-variables.rs:134:8 | LL | #[deny(unused_mut)] | ^^^^^^^^^^ From 0f507246e732c302fbfde83296c03c69fafdf4ad Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 3 Apr 2019 19:30:18 +0100 Subject: [PATCH 06/11] Remove MIR borrowck hack for old match scopes --- src/librustc_mir/dataflow/impls/mod.rs | 39 ++++++-------------------- 1 file changed, 8 insertions(+), 31 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index 4dcfb3f1a7fc3..03d55b84f32e2 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -11,8 +11,7 @@ use super::MoveDataParamEnv; use crate::util::elaborate_drops::DropFlagState; -use super::move_paths::{HasMoveData, MoveData, MovePathIndex, InitIndex}; -use super::move_paths::{LookupResult, InitKind}; +use super::move_paths::{HasMoveData, MoveData, MovePathIndex, InitIndex, InitKind}; use super::{BitDenotation, BlockSets, InitialFlow}; use super::drop_flag_effects_for_function_entry; @@ -470,35 +469,13 @@ impl<'a, 'gcx, 'tcx> BitDenotation<'tcx> for EverInitializedPlaces<'a, 'gcx, 'tc sets.gen_all(&init_loc_map[location]); match stmt.kind { - mir::StatementKind::StorageDead(local) | - mir::StatementKind::StorageLive(local) => { - // End inits for StorageDead and StorageLive, so that an immutable - // variable can be reinitialized on the next iteration of the loop. - // - // FIXME(#46525): We *need* to do this for StorageLive as well as - // StorageDead, because lifetimes of match bindings with guards are - // weird - i.e., this code - // - // ``` - // fn main() { - // match 0 { - // a | a - // if { println!("a={}", a); false } => {} - // _ => {} - // } - // } - // ``` - // - // runs the guard twice, using the same binding for `a`, and only - // storagedeads after everything ends, so if we don't regard the - // storagelive as killing storage, we would have a multiple assignment - // to immutable data error. - if let LookupResult::Exact(mpi) = - rev_lookup.find(&mir::Place::Base(mir::PlaceBase::Local(local))) { - debug!("stmt {:?} at loc {:?} clears the ever initialized status of {:?}", - stmt, location, &init_path_map[mpi]); - sets.kill_all(&init_path_map[mpi]); - } + mir::StatementKind::StorageDead(local) => { + // End inits for StorageDead, so that an immutable variable can + // be reinitialized on the next iteration of the loop. + let move_path_index = rev_lookup.find_local(local); + debug!("stmt {:?} at loc {:?} clears the ever initialized status of {:?}", + stmt, location, &init_path_map[move_path_index]); + sets.kill_all(&init_path_map[move_path_index]); } _ => {} } From b5643f1a490d6aa5f90fc45fd92ac34d6e8a05d8 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 3 Apr 2019 20:54:23 +0100 Subject: [PATCH 07/11] Emit fake borrows for all tests I was incorrectly under the impression that this would only lead to duplicates. See `mir-opt/match-arm-scope.rs` (upcomming commit) for a case where we didn't emit a fake borrow of `items.1`. --- src/librustc_mir/build/matches/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 091e39630d6cb..58ca35abcb123 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -870,7 +870,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { span, untested_candidates, join_block, - &mut None, + fake_borrows, ) } From abab9efbdb82b76f418115462311f8852600c9a2 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 3 Apr 2019 21:13:51 +0100 Subject: [PATCH 08/11] Schedule storage-dead of temporaries sooner This ensures that we will correctly generate a storage-dead if the initializing expression diverges. --- src/librustc_mir/build/expr/as_temp.rs | 40 +++++++++++++------ src/test/mir-opt/issue-49232.rs | 1 - src/test/mir-opt/match_false_edges.rs | 6 +-- .../mir-opt/storage_live_dead_in_statics.rs | 2 +- 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/librustc_mir/build/expr/as_temp.rs b/src/librustc_mir/build/expr/as_temp.rs index c60e197010067..ac70bf30e457b 100644 --- a/src/librustc_mir/build/expr/as_temp.rs +++ b/src/librustc_mir/build/expr/as_temp.rs @@ -1,6 +1,7 @@ //! See docs in build/expr/mod.rs use crate::build::{BlockAnd, BlockAndExtension, Builder}; +use crate::build::scope::{CachedBlock, DropKind}; use crate::hair::*; use rustc::middle::region; use rustc::mir::*; @@ -63,6 +64,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } this.local_decls.push(local_decl) }; + let temp_place = &Place::Base(PlaceBase::Local(temp)); + if !expr_ty.is_never() { this.cfg.push( block, @@ -71,25 +74,38 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { kind: StatementKind::StorageLive(temp), }, ); + + // In constants, temp_lifetime is None for temporaries that live for the + // 'static lifetime. Thus we do not drop these temporaries and simply leak them. + // This is equivalent to what `let x = &foo();` does in functions. The temporary + // is lifted to their surrounding scope. In a function that means the temporary lives + // until just before the function returns. In constants that means it outlives the + // constant's initialization value computation. Anything outliving a constant + // must have the `'static` lifetime and live forever. + // Anything with a shorter lifetime (e.g the `&foo()` in `bar(&foo())` or anything + // within a block will keep the regular drops just like runtime code. + if let Some(temp_lifetime) = temp_lifetime { + this.schedule_drop( + expr_span, + temp_lifetime, + temp_place, + expr_ty, + DropKind::Storage, + ); + } } - unpack!(block = this.into(&Place::Base(PlaceBase::Local(temp)), block, expr)); + unpack!(block = this.into(temp_place, block, expr)); - // In constants, temp_lifetime is None for temporaries that live for the - // 'static lifetime. Thus we do not drop these temporaries and simply leak them. - // This is equivalent to what `let x = &foo();` does in functions. The temporary - // is lifted to their surrounding scope. In a function that means the temporary lives - // until just before the function returns. In constants that means it outlives the - // constant's initialization value computation. Anything outliving a constant - // must have the `'static` lifetime and live forever. - // Anything with a shorter lifetime (e.g the `&foo()` in `bar(&foo())` or anything - // within a block will keep the regular drops just like runtime code. if let Some(temp_lifetime) = temp_lifetime { - this.schedule_drop_storage_and_value( + this.schedule_drop( expr_span, temp_lifetime, - &Place::Base(PlaceBase::Local(temp)), + temp_place, expr_ty, + DropKind::Value { + cached_block: CachedBlock::default(), + }, ); } diff --git a/src/test/mir-opt/issue-49232.rs b/src/test/mir-opt/issue-49232.rs index 3910183dee789..bf22f00b50552 100644 --- a/src/test/mir-opt/issue-49232.rs +++ b/src/test/mir-opt/issue-49232.rs @@ -86,7 +86,6 @@ fn main() { // unreachable; // } // bb17: { -// StorageDead(_4); // goto -> bb18; // } // bb18: { diff --git a/src/test/mir-opt/match_false_edges.rs b/src/test/mir-opt/match_false_edges.rs index 0850c552536c5..6979924c8cd90 100644 --- a/src/test/mir-opt/match_false_edges.rs +++ b/src/test/mir-opt/match_false_edges.rs @@ -109,8 +109,8 @@ fn main() { // goto -> bb13; // } // bb13: { -// StorageDead(_1); // StorageDead(_2); +// StorageDead(_1); // _0 = (); // return; // } @@ -186,8 +186,8 @@ fn main() { // goto -> bb13; // } // bb13: { -// StorageDead(_1); // StorageDead(_2); +// StorageDead(_1); // _0 = (); // return; // } @@ -289,8 +289,8 @@ fn main() { // goto -> bb17; // } // bb17: { -// StorageDead(_1); // StorageDead(_2); +// StorageDead(_1); // _0 = (); // return; // } diff --git a/src/test/mir-opt/storage_live_dead_in_statics.rs b/src/test/mir-opt/storage_live_dead_in_statics.rs index 10f00cf8b0c32..2ed34ecfad2c6 100644 --- a/src/test/mir-opt/storage_live_dead_in_statics.rs +++ b/src/test/mir-opt/storage_live_dead_in_statics.rs @@ -182,8 +182,8 @@ fn main() { // _2 = Foo { tup: const "hi", data: move _3 }; // _1 = &_2; // _0 = &(*_1); -// StorageDead(_1); // StorageDead(_5); +// StorageDead(_1); // return; // } //} From 2420d82a7c55432071a2094dac29dfe96f303dfe Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 3 Apr 2019 21:21:53 +0100 Subject: [PATCH 09/11] Add a test for match scopes --- src/test/mir-opt/match-arm-scopes.rs | 245 +++++++++++++++++++++++++++ 1 file changed, 245 insertions(+) create mode 100644 src/test/mir-opt/match-arm-scopes.rs diff --git a/src/test/mir-opt/match-arm-scopes.rs b/src/test/mir-opt/match-arm-scopes.rs new file mode 100644 index 0000000000000..0f026b8a08dfa --- /dev/null +++ b/src/test/mir-opt/match-arm-scopes.rs @@ -0,0 +1,245 @@ +// Test that StorageDead and Drops are generated properly for bindings in +// matches: +// * The MIR should only contain a single drop of `s` and `t`: at the end +// of their respective arms. +// * StorageDead and StorageLive statements are correctly matched up on +// non-unwind paths. +// * The visibility scopes of the match arms should be disjoint, and contain. +// all of the bindings for that scope. +// * No drop flags are used. + +#![feature(nll, bind_by_move_pattern_guards)] + +fn complicated_match(cond: bool, items: (bool, bool, String)) -> i32 { + match items { + (false, a, s) | (a, false, s) if if cond { return 3 } else { a } => 1, + (true, b, t) | (false, b, t) => 2, + } +} + +const CASES: &[(bool, bool, bool, i32)] = &[ + (false, false, false, 2), + (false, false, true, 1), + (false, true, false, 1), + (false, true, true, 2), + (true, false, false, 3), + (true, false, true, 3), + (true, true, false, 3), + (true, true, true, 2), +]; + +fn main() { + for &(cond, items_1, items_2, result) in CASES { + assert_eq!( + complicated_match(cond, (items_1, items_2, String::new())), + result, + ); + } +} + +// END RUST SOURCE +// START rustc.complicated_match.SimplifyCfg-initial.after.mir +// let mut _0: i32; +// let mut _3: &bool; // Temp for fake borrow of `items.0` +// let mut _4: &bool; // Temp for fake borrow of `items.1` +// let _5: bool; // `a` in arm +// let _6: &bool; // `a` in guard +// let _7: std::string::String; // `s` in arm +// let _8: &std::string::String; // `s` in guard +// let mut _9: bool; // `if cond { return 3 } else { a }` +// let mut _10: bool; // `cond` +// let mut _11: !; // `return 3` +// let mut _12: bool; // `if cond { return 3 } else { a }` +// let mut _13: bool; // `cond` +// let mut _14: !; // `return 3` +// let _15: bool; // `b` +// let _16: std::string::String; // `t` +// scope 1 { +// } +// scope 2 { +// } +// bb0: { +// FakeRead(ForMatchedPlace, _2); +// switchInt((_2.0: bool)) -> [false: bb2, otherwise: bb7]; +// } +// bb1 (cleanup): { +// resume; +// } +// bb2: { +// falseEdges -> [real: bb10, imaginary: bb3]; +// } +// bb3: { +// falseEdges -> [real: bb21, imaginary: bb4]; +// } +// bb4: { +// falseEdges -> [real: bb31, imaginary: bb5]; +// } +// bb5: { +// falseEdges -> [real: bb32, imaginary: bb6]; +// } +// bb6: { +// unreachable; +// } +// bb7: { +// switchInt((_2.1: bool)) -> [false: bb3, otherwise: bb8]; +// } +// bb8: { +// switchInt((_2.0: bool)) -> [false: bb5, otherwise: bb4]; +// } +// bb9: { // arm 1 +// _0 = const 1i32; +// drop(_7) -> [return: bb29, unwind: bb16]; +// } +// bb10: { // guard - first time +// StorageLive(_6); +// _6 = &(_2.1: bool); +// StorageLive(_8); +// _8 = &(_2.2: std::string::String); +// _3 = &shallow (_2.0: bool); +// _4 = &shallow (_2.1: bool); +// StorageLive(_9); +// StorageLive(_10); +// _10 = _1; +// FakeRead(ForMatchedPlace, _10); +// switchInt(_10) -> [false: bb12, otherwise: bb11]; +// } +// bb11: { +// falseEdges -> [real: bb14, imaginary: bb12]; +// } +// bb12: { +// falseEdges -> [real: bb18, imaginary: bb13]; +// } +// bb13: { +// unreachable; +// } +// bb14: { // `return 3` - first time +// _0 = const 3i32; +// StorageDead(_10); +// StorageDead(_9); +// StorageDead(_8); +// StorageDead(_6); +// goto -> bb17; +// } +// bb15: { +// return; +// } +// bb16 (cleanup): { +// drop(_2) -> bb1; +// } +// bb17: { +// drop(_2) -> [return: bb15, unwind: bb1]; +// } +// bb18: { // `else` block - first time +// _9 = (*_6); +// StorageDead(_10); +// FakeRead(ForMatchGuard, _3); +// FakeRead(ForMatchGuard, _4); +// FakeRead(ForGuardBinding, _6); +// FakeRead(ForGuardBinding, _8); +// switchInt(move _9) -> [false: bb20, otherwise: bb19]; +// } +// bb19: { +// StorageDead(_9); +// StorageLive(_5); +// _5 = (_2.1: bool); +// StorageLive(_7); +// _7 = move (_2.2: std::string::String); +// goto -> bb9; +// } +// bb20: { // guard otherwise case - first time +// StorageDead(_9); +// StorageDead(_8); +// StorageDead(_6); +// falseEdges -> [real: bb7, imaginary: bb3]; +// } +// bb21: { // guard - second time +// StorageLive(_6); +// _6 = &(_2.0: bool); +// StorageLive(_8); +// _8 = &(_2.2: std::string::String); +// _3 = &shallow (_2.0: bool); +// _4 = &shallow (_2.1: bool); +// StorageLive(_12); +// StorageLive(_13); +// _13 = _1; +// FakeRead(ForMatchedPlace, _13); +// switchInt(_13) -> [false: bb23, otherwise: bb22]; +// } +// bb22: { +// falseEdges -> [real: bb25, imaginary: bb23]; +// } +// bb23: { +// falseEdges -> [real: bb26, imaginary: bb24]; +// } +// bb24: { +// unreachable; +// } +// bb25: { // `return 3` - second time +// _0 = const 3i32; +// StorageDead(_13); +// StorageDead(_12); +// StorageDead(_8); +// StorageDead(_6); +// goto -> bb17; +// } +// bb26: { // `else` block - second time +// _12 = (*_6); +// StorageDead(_13); +// FakeRead(ForMatchGuard, _3); +// FakeRead(ForMatchGuard, _4); +// FakeRead(ForGuardBinding, _6); +// FakeRead(ForGuardBinding, _8); +// switchInt(move _12) -> [false: bb28, otherwise: bb27]; +// } +// bb27: { // Guard otherwise case - second time +// StorageDead(_12); +// StorageLive(_5); +// _5 = (_2.0: bool); +// StorageLive(_7); +// _7 = move (_2.2: std::string::String); +// goto -> bb9; +// } +// bb28: { // rest of arm 1 +// StorageDead(_12); +// StorageDead(_8); +// StorageDead(_6); +// falseEdges -> [real: bb8, imaginary: bb4]; +// } +// bb29: { +// StorageDead(_7); +// StorageDead(_5); +// StorageDead(_8); +// StorageDead(_6); +// goto -> bb34; +// } +// bb30: { // arm 2 +// _0 = const 2i32; +// drop(_16) -> [return: bb33, unwind: bb16]; +// } +// bb31: { // bindings for arm 2 - first pattern +// StorageLive(_15); +// _15 = (_2.1: bool); +// StorageLive(_16); +// _16 = move (_2.2: std::string::String); +// goto -> bb30; +// } +// bb32: { // bindings for arm 2 - first pattern +// StorageLive(_15); +// _15 = (_2.1: bool); +// StorageLive(_16); +// _16 = move (_2.2: std::string::String); +// goto -> bb30; +// } +// bb33: { // rest of arm 2 +// StorageDead(_16); +// StorageDead(_15); +// goto -> bb34; +// } +// bb34: { // end of match +// drop(_2) -> [return: bb15, unwind: bb1]; +// } +// END rustc.complicated_match.SimplifyCfg-initial.after.mir +// START rustc.complicated_match.ElaborateDrops.after.mir +// let _16: std::string::String; // No drop flags, which would come after this. +// scope 1 { +// END rustc.complicated_match.ElaborateDrops.after.mir From 015a45156f7ae9c1c0af86795409d892832aaa71 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Mon, 22 Apr 2019 20:35:00 +0200 Subject: [PATCH 10/11] Comment style fixes Co-Authored-By: matthewjasper --- src/librustc_mir/build/expr/as_temp.rs | 4 ++-- src/librustc_mir/build/scope.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/build/expr/as_temp.rs b/src/librustc_mir/build/expr/as_temp.rs index ac70bf30e457b..cffd8fb2892f5 100644 --- a/src/librustc_mir/build/expr/as_temp.rs +++ b/src/librustc_mir/build/expr/as_temp.rs @@ -75,8 +75,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { }, ); - // In constants, temp_lifetime is None for temporaries that live for the - // 'static lifetime. Thus we do not drop these temporaries and simply leak them. + // In constants, `temp_lifetime` is `None` for temporaries that live for the + // `'static` lifetime. Thus we do not drop these temporaries and simply leak them. // This is equivalent to what `let x = &foo();` does in functions. The temporary // is lifted to their surrounding scope. In a function that means the temporary lives // until just before the function returns. In constants that means it outlives the diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 0d1d40a8af633..34df40ae180af 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -900,7 +900,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { /// * The variable must be in that scope. /// * The variable must be at the top of that scope: it's the next thing /// scheduled to drop. - /// * The drop must be of DropKind::Storage. + /// * The drop must be of `DropKind::Storage`. /// /// This is used for the boolean holding the result of the match guard. We /// do this because: From 0835048ea0fc724163b5032112df3c7555a2073b Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 5 May 2019 17:32:52 +0100 Subject: [PATCH 11/11] Fix match ergonomics suggestion --- src/librustc_typeck/check/_match.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index 99b350b833274..e4b431e6e68f1 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/src/librustc_typeck/check/_match.rs @@ -540,7 +540,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { err.help(&format!("did you mean `{}: &{}`?", snippet, expected)); } } - hir::Node::Expr(hir::Expr { node: hir::ExprKind::Match(..), .. }) | + hir::Node::Arm(_) | hir::Node::Pat(_) => { // rely on match ergonomics or it might be nested `&&pat` if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(inner.span) {