From a8d6ab7e9814b22aca64559aad07403c1175fc82 Mon Sep 17 00:00:00 2001 From: bors Date: Fri, 10 Nov 2017 11:37:32 +0000 Subject: [PATCH 1/3] Backported merge of #45785 - arielb1:unsafe-fixes, r=eddyb fixes to MIR effectck r? @eddyb beta-nominating because regression (MIR effectck is new) --- src/librustc/dep_graph/dep_node.rs | 4 +- src/librustc/ich/impls_mir.rs | 1 + src/librustc/mir/mod.rs | 10 ++ src/librustc/ty/maps/mod.rs | 9 +- src/librustc/ty/maps/plumbing.rs | 4 +- src/librustc_mir/transform/check_unsafety.rs | 170 +++++++++++------- src/librustc_mir/transform/mod.rs | 12 +- .../issue-45087-unreachable-unsafe.rs | 15 ++ .../issue-45729-unsafe-in-generator.rs | 19 ++ ...sue-45107-unnecessary-unsafe-in-closure.rs | 35 ++++ ...45107-unnecessary-unsafe-in-closure.stderr | 71 ++++++++ src/test/ui/span/lint-unused-unsafe.stderr | 16 +- 12 files changed, 282 insertions(+), 84 deletions(-) create mode 100644 src/test/compile-fail/issue-45087-unreachable-unsafe.rs create mode 100644 src/test/compile-fail/issue-45729-unsafe-in-generator.rs create mode 100644 src/test/ui/issue-45107-unnecessary-unsafe-in-closure.rs create mode 100644 src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr diff --git a/src/librustc/dep_graph/dep_node.rs b/src/librustc/dep_graph/dep_node.rs index 1ecbc62225e0b..f6e8fd03cf06e 100644 --- a/src/librustc/dep_graph/dep_node.rs +++ b/src/librustc/dep_graph/dep_node.rs @@ -455,6 +455,7 @@ define_dep_nodes!( <'tcx> // Represents the MIR for a fn; also used as the task node for // things read/modify that MIR. [] MirConstQualif(DefId), + [] MirBuilt(DefId), [] MirConst(DefId), [] MirValidated(DefId), [] MirOptimized(DefId), @@ -463,7 +464,7 @@ define_dep_nodes!( <'tcx> [] BorrowCheckKrate, [] BorrowCheck(DefId), [] MirBorrowCheck(DefId), - [] UnsafetyViolations(DefId), + [] UnsafetyCheckResult(DefId), [] Reachability, [] MirKeys, @@ -781,4 +782,3 @@ impl WorkProductId { impl_stable_hash_for!(struct ::dep_graph::WorkProductId { hash }); - diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs index 4bda89690b7a9..6583f24421c36 100644 --- a/src/librustc/ich/impls_mir.rs +++ b/src/librustc/ich/impls_mir.rs @@ -34,6 +34,7 @@ impl_stable_hash_for!(struct mir::LocalDecl<'tcx> { impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, by_ref }); impl_stable_hash_for!(struct mir::BasicBlockData<'tcx> { statements, terminator, is_cleanup }); impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, lint_node_id }); +impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_blocks }); impl<'gcx> HashStable> for mir::Terminator<'gcx> { diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 075a629de04da..75efe6eb90419 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -33,6 +33,7 @@ use std::cell::Ref; use std::fmt::{self, Debug, Formatter, Write}; use std::{iter, u32}; use std::ops::{Index, IndexMut}; +use std::rc::Rc; use std::vec::IntoIter; use syntax::ast::{self, Name}; use syntax_pos::Span; @@ -1646,6 +1647,15 @@ pub struct UnsafetyViolation { pub lint_node_id: Option, } +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct UnsafetyCheckResult { + /// Violations that are propagated *upwards* from this function + pub violations: Rc<[UnsafetyViolation]>, + /// unsafe blocks in this function, along with whether they are used. This is + /// used for the "unused_unsafe" lint. + pub unsafe_blocks: Rc<[(ast::NodeId, bool)]>, +} + /// The layout of generator state #[derive(Clone, Debug, RustcEncodable, RustcDecodable)] pub struct GeneratorLayout<'tcx> { diff --git a/src/librustc/ty/maps/mod.rs b/src/librustc/ty/maps/mod.rs index 3dcbeda94bc01..156f8c3dbc9a4 100644 --- a/src/librustc/ty/maps/mod.rs +++ b/src/librustc/ty/maps/mod.rs @@ -143,6 +143,10 @@ define_maps! { <'tcx> /// the value isn't known except to the pass itself. [] fn mir_const_qualif: MirConstQualif(DefId) -> (u8, Rc>), + /// Fetch the MIR for a given def-id right after it's built - this includes + /// unreachable code. + [] fn mir_built: MirBuilt(DefId) -> &'tcx Steal>, + /// Fetch the MIR for a given def-id up till the point where it is /// ready for const evaluation. /// @@ -159,9 +163,8 @@ define_maps! { <'tcx> /// expression defining the closure. [] fn closure_kind: ClosureKind(DefId) -> ty::ClosureKind, - /// Unsafety violations for this def ID. - [] fn unsafety_violations: UnsafetyViolations(DefId) - -> Rc<[mir::UnsafetyViolation]>, + /// The result of unsafety-checking this def-id. + [] fn unsafety_check_result: UnsafetyCheckResult(DefId) -> mir::UnsafetyCheckResult, /// The signature of functions and closures. [] fn fn_sig: FnSignature(DefId) -> ty::PolyFnSig<'tcx>, diff --git a/src/librustc/ty/maps/plumbing.rs b/src/librustc/ty/maps/plumbing.rs index 88b619558d90b..a0cfa32213bd4 100644 --- a/src/librustc/ty/maps/plumbing.rs +++ b/src/librustc/ty/maps/plumbing.rs @@ -725,6 +725,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>, force!(crate_inherent_impls_overlap_check, LOCAL_CRATE) }, DepKind::PrivacyAccessLevels => { force!(privacy_access_levels, LOCAL_CRATE); } + DepKind::MirBuilt => { force!(mir_built, def_id!()); } DepKind::MirConstQualif => { force!(mir_const_qualif, def_id!()); } DepKind::MirConst => { force!(mir_const, def_id!()); } DepKind::MirValidated => { force!(mir_validated, def_id!()); } @@ -732,7 +733,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>, DepKind::BorrowCheck => { force!(borrowck, def_id!()); } DepKind::MirBorrowCheck => { force!(mir_borrowck, def_id!()); } - DepKind::UnsafetyViolations => { force!(unsafety_violations, def_id!()); } + DepKind::UnsafetyCheckResult => { force!(unsafety_check_result, def_id!()); } DepKind::Reachability => { force!(reachable_set, LOCAL_CRATE); } DepKind::MirKeys => { force!(mir_keys, LOCAL_CRATE); } DepKind::CrateVariances => { force!(crate_variances, LOCAL_CRATE); } @@ -862,4 +863,3 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>, true } - diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs index 49ce36223994b..f852b46cfbc7c 100644 --- a/src/librustc_mir/transform/check_unsafety.rs +++ b/src/librustc_mir/transform/check_unsafety.rs @@ -34,6 +34,7 @@ pub struct UnsafetyChecker<'a, 'tcx: 'a> { tcx: TyCtxt<'a, 'tcx, 'tcx>, param_env: ty::ParamEnv<'tcx>, used_unsafe: FxHashSet, + inherited_blocks: Vec<(ast::NodeId, bool)>, } impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> { @@ -52,6 +53,7 @@ impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> { tcx, param_env, used_unsafe: FxHashSet(), + inherited_blocks: vec![], } } } @@ -74,7 +76,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { TerminatorKind::Resume | TerminatorKind::Return | TerminatorKind::Unreachable => { - // safe (at least as emitted during MIR construction) + // safe (at least as emitted during MIR construction) } TerminatorKind::Call { ref func, .. } => { @@ -116,12 +118,20 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { rvalue: &Rvalue<'tcx>, location: Location) { - if let &Rvalue::Aggregate( - box AggregateKind::Closure(def_id, _), - _ - ) = rvalue { - let unsafety_violations = self.tcx.unsafety_violations(def_id); - self.register_violations(&unsafety_violations); + if let &Rvalue::Aggregate(box ref aggregate, _) = rvalue { + match aggregate { + &AggregateKind::Array(..) | + &AggregateKind::Tuple | + &AggregateKind::Adt(..) => {} + &AggregateKind::Closure(def_id, _) | + &AggregateKind::Generator(def_id, _, _) => { + let UnsafetyCheckResult { + violations, unsafe_blocks + } = self.tcx.unsafety_check_result(def_id); + self.inherited_blocks.extend(unsafe_blocks.iter().cloned()); + self.register_violations(&violations, &unsafe_blocks); + } + } } self.super_rvalue(rvalue, location); } @@ -188,7 +198,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { source_info, description: "use of extern static", lint_node_id: Some(lint_root) - }]); + }], &[]); } } } @@ -221,41 +231,49 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> { let source_info = self.source_info; self.register_violations(&[UnsafetyViolation { source_info, description, lint_node_id: None - }]); + }], &[]); } - fn register_violations(&mut self, violations: &[UnsafetyViolation]) { - match self.visibility_scope_info[self.source_info.scope].safety { + fn register_violations(&mut self, + violations: &[UnsafetyViolation], + unsafe_blocks: &[(ast::NodeId, bool)]) { + let within_unsafe = match self.visibility_scope_info[self.source_info.scope].safety { Safety::Safe => { for violation in violations { if !self.violations.contains(violation) { self.violations.push(violation.clone()) } } + + false } - Safety::BuiltinUnsafe | Safety::FnUnsafe => {} + Safety::BuiltinUnsafe | Safety::FnUnsafe => true, Safety::ExplicitUnsafe(node_id) => { if !violations.is_empty() { self.used_unsafe.insert(node_id); } + true } - } + }; + self.inherited_blocks.extend(unsafe_blocks.iter().map(|&(node_id, is_used)| { + (node_id, is_used && !within_unsafe) + })); } } pub(crate) fn provide(providers: &mut Providers) { *providers = Providers { - unsafety_violations, + unsafety_check_result, ..*providers }; } -struct UnusedUnsafeVisitor<'a, 'tcx: 'a> { - tcx: TyCtxt<'a, 'tcx, 'tcx>, - used_unsafe: FxHashSet +struct UnusedUnsafeVisitor<'a> { + used_unsafe: &'a FxHashSet, + unsafe_blocks: &'a mut Vec<(ast::NodeId, bool)>, } -impl<'a, 'tcx> hir::intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'a, 'tcx> { +impl<'a, 'tcx> hir::intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'a> { fn nested_visit_map<'this>(&'this mut self) -> hir::intravisit::NestedVisitorMap<'this, 'tcx> { @@ -266,50 +284,15 @@ impl<'a, 'tcx> hir::intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'a, 'tcx> hir::intravisit::walk_block(self, block); if let hir::UnsafeBlock(hir::UserProvided) = block.rules { - if !self.used_unsafe.contains(&block.id) { - self.report_unused_unsafe(block); - } - } - } -} - -impl<'a, 'tcx> UnusedUnsafeVisitor<'a, 'tcx> { - /// Return the NodeId for an enclosing scope that is also `unsafe` - fn is_enclosed(&self, id: ast::NodeId) -> Option<(String, ast::NodeId)> { - let parent_id = self.tcx.hir.get_parent_node(id); - if parent_id != id { - if self.used_unsafe.contains(&parent_id) { - Some(("block".to_string(), parent_id)) - } else if let Some(hir::map::NodeItem(&hir::Item { - node: hir::ItemFn(_, hir::Unsafety::Unsafe, _, _, _, _), - .. - })) = self.tcx.hir.find(parent_id) { - Some(("fn".to_string(), parent_id)) - } else { - self.is_enclosed(parent_id) - } - } else { - None - } - } - - fn report_unused_unsafe(&self, block: &'tcx hir::Block) { - let mut db = self.tcx.struct_span_lint_node(UNUSED_UNSAFE, - block.id, - block.span, - "unnecessary `unsafe` block"); - db.span_label(block.span, "unnecessary `unsafe` block"); - if let Some((kind, id)) = self.is_enclosed(block.id) { - db.span_note(self.tcx.hir.span(id), - &format!("because it's nested under this `unsafe` {}", kind)); + self.unsafe_blocks.push((block.id, self.used_unsafe.contains(&block.id))); } - db.emit(); } } fn check_unused_unsafe<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId, - used_unsafe: FxHashSet) + used_unsafe: &FxHashSet, + unsafe_blocks: &'a mut Vec<(ast::NodeId, bool)>) { let body_id = tcx.hir.as_local_node_id(def_id).and_then(|node_id| { @@ -327,25 +310,27 @@ fn check_unused_unsafe<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, debug!("check_unused_unsafe({:?}, body={:?}, used_unsafe={:?})", def_id, body, used_unsafe); - hir::intravisit::Visitor::visit_body( - &mut UnusedUnsafeVisitor { tcx, used_unsafe }, - body); + let mut visitor = UnusedUnsafeVisitor { used_unsafe, unsafe_blocks }; + hir::intravisit::Visitor::visit_body(&mut visitor, body); } -fn unsafety_violations<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> - Rc<[UnsafetyViolation]> +fn unsafety_check_result<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) + -> UnsafetyCheckResult { debug!("unsafety_violations({:?})", def_id); // NB: this borrow is valid because all the consumers of - // `mir_const` force this. - let mir = &tcx.mir_const(def_id).borrow(); + // `mir_built` force this. + let mir = &tcx.mir_built(def_id).borrow(); let visibility_scope_info = match mir.visibility_scope_info { ClearOnDecode::Set(ref data) => data, ClearOnDecode::Clear => { debug!("unsafety_violations: {:?} - remote, skipping", def_id); - return Rc::new([]) + return UnsafetyCheckResult { + violations: Rc::new([]), + unsafe_blocks: Rc::new([]) + } } }; @@ -354,8 +339,43 @@ fn unsafety_violations<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> mir, visibility_scope_info, tcx, param_env); checker.visit_mir(mir); - check_unused_unsafe(tcx, def_id, checker.used_unsafe); - checker.violations.into() + check_unused_unsafe(tcx, def_id, &checker.used_unsafe, &mut checker.inherited_blocks); + UnsafetyCheckResult { + violations: checker.violations.into(), + unsafe_blocks: checker.inherited_blocks.into() + } +} + +/// Return the NodeId for an enclosing scope that is also `unsafe` +fn is_enclosed(tcx: TyCtxt, + used_unsafe: &FxHashSet, + id: ast::NodeId) -> Option<(String, ast::NodeId)> { + let parent_id = tcx.hir.get_parent_node(id); + if parent_id != id { + if used_unsafe.contains(&parent_id) { + Some(("block".to_string(), parent_id)) + } else if let Some(hir::map::NodeItem(&hir::Item { + node: hir::ItemFn(_, hir::Unsafety::Unsafe, _, _, _, _), + .. + })) = tcx.hir.find(parent_id) { + Some(("fn".to_string(), parent_id)) + } else { + is_enclosed(tcx, used_unsafe, parent_id) + } + } else { + None + } +} + +fn report_unused_unsafe(tcx: TyCtxt, used_unsafe: &FxHashSet, id: ast::NodeId) { + let span = tcx.hir.span(id); + let mut db = tcx.struct_span_lint_node(UNUSED_UNSAFE, id, span, "unnecessary `unsafe` block"); + db.span_label(span, "unnecessary `unsafe` block"); + if let Some((kind, id)) = is_enclosed(tcx, used_unsafe, id) { + db.span_note(tcx.hir.span(id), + &format!("because it's nested under this `unsafe` {}", kind)); + } + db.emit(); } pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) { @@ -366,9 +386,14 @@ pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) { _ => {} }; + let UnsafetyCheckResult { + violations, + unsafe_blocks + } = tcx.unsafety_check_result(def_id); + for &UnsafetyViolation { source_info, description, lint_node_id - } in &*tcx.unsafety_violations(def_id) { + } in violations.iter() { // Report an error. if let Some(lint_node_id) = lint_node_id { tcx.lint_node(SAFE_EXTERN_STATICS, @@ -384,4 +409,15 @@ pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) { .emit(); } } + + let mut unsafe_blocks: Vec<_> = unsafe_blocks.into_iter().collect(); + unsafe_blocks.sort(); + let used_unsafe: FxHashSet<_> = unsafe_blocks.iter() + .flat_map(|&&(id, used)| if used { Some(id) } else { None }) + .collect(); + for &(block_id, is_used) in unsafe_blocks { + if !is_used { + report_unused_unsafe(tcx, &used_unsafe, block_id); + } + } } diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 322f46cf02b63..34cc3a289d189 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -50,6 +50,7 @@ pub(crate) fn provide(providers: &mut Providers) { self::check_unsafety::provide(providers); *providers = Providers { mir_keys, + mir_built, mir_const, mir_validated, optimized_mir, @@ -103,9 +104,17 @@ fn mir_keys<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, krate: CrateNum) Rc::new(set) } +fn mir_built<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> &'tcx Steal> { + let mir = build::mir_build(tcx, def_id); + tcx.alloc_steal_mir(mir) +} + fn mir_const<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> &'tcx Steal> { - let mut mir = build::mir_build(tcx, def_id); + // Unsafety check uses the raw mir, so make sure it is run + let _ = tcx.unsafety_check_result(def_id); + let source = MirSource::from_local_def_id(tcx, def_id); + let mut mir = tcx.mir_built(def_id).steal(); transform::run_suite(tcx, source, MIR_CONST, &mut mir); tcx.alloc_steal_mir(mir) } @@ -117,7 +126,6 @@ fn mir_validated<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> &'tcx // this point, before we steal the mir-const result. let _ = tcx.mir_const_qualif(def_id); } - let _ = tcx.unsafety_violations(def_id); let mut mir = tcx.mir_const(def_id).steal(); transform::run_suite(tcx, source, MIR_VALIDATED, &mut mir); diff --git a/src/test/compile-fail/issue-45087-unreachable-unsafe.rs b/src/test/compile-fail/issue-45087-unreachable-unsafe.rs new file mode 100644 index 0000000000000..eeb66fa0e2c3b --- /dev/null +++ b/src/test/compile-fail/issue-45087-unreachable-unsafe.rs @@ -0,0 +1,15 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn main() { + return; + *(1 as *mut u32) = 42; + //~^ ERROR dereference of raw pointer requires unsafe +} diff --git a/src/test/compile-fail/issue-45729-unsafe-in-generator.rs b/src/test/compile-fail/issue-45729-unsafe-in-generator.rs new file mode 100644 index 0000000000000..489e91797f3d7 --- /dev/null +++ b/src/test/compile-fail/issue-45729-unsafe-in-generator.rs @@ -0,0 +1,19 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(generators)] + +fn main() { + let _ = || { + *(1 as *mut u32) = 42; + //~^ ERROR dereference of raw pointer requires unsafe + yield; + }; +} diff --git a/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.rs b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.rs new file mode 100644 index 0000000000000..833fc2802a3a5 --- /dev/null +++ b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.rs @@ -0,0 +1,35 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#[deny(unused_unsafe)] +fn main() { + let mut v = Vec::::with_capacity(24); + + unsafe { + let f = |v: &mut Vec<_>| { + unsafe { + v.set_len(24); + |w: &mut Vec| { unsafe { + w.set_len(32); + } }; + } + |x: &mut Vec| { unsafe { + x.set_len(40); + } }; + }; + + v.set_len(0); + f(&mut v); + } + + |y: &mut Vec| { unsafe { + y.set_len(48); + } }; +} diff --git a/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr new file mode 100644 index 0000000000000..5c58b19c7fb35 --- /dev/null +++ b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr @@ -0,0 +1,71 @@ +error: unnecessary `unsafe` block + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:17:13 + | +17 | / unsafe { +18 | | v.set_len(24); +19 | | |w: &mut Vec| { unsafe { +20 | | w.set_len(32); +21 | | } }; +22 | | } + | |_____________^ unnecessary `unsafe` block + | +note: lint level defined here + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:11:8 + | +11 | #[deny(unused_unsafe)] + | ^^^^^^^^^^^^^ +note: because it's nested under this `unsafe` block + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:15:5 + | +15 | / unsafe { +16 | | let f = |v: &mut Vec<_>| { +17 | | unsafe { +18 | | v.set_len(24); +... | +29 | | f(&mut v); +30 | | } + | |_____^ + +error: unnecessary `unsafe` block + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:19:38 + | +19 | |w: &mut Vec| { unsafe { + | ______________________________________^ +20 | | w.set_len(32); +21 | | } }; + | |_________________^ unnecessary `unsafe` block + | +note: because it's nested under this `unsafe` block + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:17:13 + | +17 | / unsafe { +18 | | v.set_len(24); +19 | | |w: &mut Vec| { unsafe { +20 | | w.set_len(32); +21 | | } }; +22 | | } + | |_____________^ + +error: unnecessary `unsafe` block + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:23:34 + | +23 | |x: &mut Vec| { unsafe { + | __________________________________^ +24 | | x.set_len(40); +25 | | } }; + | |_____________^ unnecessary `unsafe` block + | +note: because it's nested under this `unsafe` block + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:15:5 + | +15 | / unsafe { +16 | | let f = |v: &mut Vec<_>| { +17 | | unsafe { +18 | | v.set_len(24); +... | +29 | | f(&mut v); +30 | | } + | |_____^ + +error: aborting due to 6 previous errors + diff --git a/src/test/ui/span/lint-unused-unsafe.stderr b/src/test/ui/span/lint-unused-unsafe.stderr index 1fa5f94aa4ca6..f4998e08907a3 100644 --- a/src/test/ui/span/lint-unused-unsafe.stderr +++ b/src/test/ui/span/lint-unused-unsafe.stderr @@ -65,12 +65,14 @@ note: because it's nested under this `unsafe` block | |_____^ error: unnecessary `unsafe` block - --> $DIR/lint-unused-unsafe.rs:40:9 + --> $DIR/lint-unused-unsafe.rs:39:5 | -40 | / unsafe { //~ ERROR: unnecessary `unsafe` block +39 | / unsafe { //~ ERROR: unnecessary `unsafe` block +40 | | unsafe { //~ ERROR: unnecessary `unsafe` block 41 | | unsf() 42 | | } - | |_________^ unnecessary `unsafe` block +43 | | } + | |_____^ unnecessary `unsafe` block | note: because it's nested under this `unsafe` fn --> $DIR/lint-unused-unsafe.rs:38:1 @@ -85,14 +87,12 @@ note: because it's nested under this `unsafe` fn | |_^ error: unnecessary `unsafe` block - --> $DIR/lint-unused-unsafe.rs:39:5 + --> $DIR/lint-unused-unsafe.rs:40:9 | -39 | / unsafe { //~ ERROR: unnecessary `unsafe` block -40 | | unsafe { //~ ERROR: unnecessary `unsafe` block +40 | / unsafe { //~ ERROR: unnecessary `unsafe` block 41 | | unsf() 42 | | } -43 | | } - | |_____^ unnecessary `unsafe` block + | |_________^ unnecessary `unsafe` block | note: because it's nested under this `unsafe` fn --> $DIR/lint-unused-unsafe.rs:38:1 From b23c76e1dfe9af3d427cf94f0dead6b8c8c93fc4 Mon Sep 17 00:00:00 2001 From: bors Date: Mon, 13 Nov 2017 17:42:13 +0000 Subject: [PATCH 2/3] Backported merge of #45890 - arielb1:self-first, r=eddyb check::method - unify receivers before normalizing method signatures Normalizing method signatures can unify inference variables, which can cause receiver unification to fail. Unify the receivers first to avoid that. Fixes #36701. Fixes #45801. Fixes #45855. r? @eddyb beta-nominating because #43880 made this ICE happen in more cases (the code in that issue ICEs post-#43880 only, but the unit test here ICEs on all versions). --- src/librustc_typeck/check/method/confirm.rs | 30 ++++++++++++------ src/test/compile-fail/issue-45801.rs | 35 +++++++++++++++++++++ 2 files changed, 56 insertions(+), 9 deletions(-) create mode 100644 src/test/compile-fail/issue-45801.rs diff --git a/src/librustc_typeck/check/method/confirm.rs b/src/librustc_typeck/check/method/confirm.rs index a9830dd5ddece..17ed0aaa30b02 100644 --- a/src/librustc_typeck/check/method/confirm.rs +++ b/src/librustc_typeck/check/method/confirm.rs @@ -16,6 +16,7 @@ use hir::def_id::DefId; use rustc::ty::subst::Substs; use rustc::traits; use rustc::ty::{self, LvaluePreference, NoPreference, PreferMutLvalue, Ty}; +use rustc::ty::subst::Subst; use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, OverloadedDeref}; use rustc::ty::fold::TypeFoldable; use rustc::infer::{self, InferOk}; @@ -84,9 +85,6 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { // Adjust the self expression the user provided and obtain the adjusted type. let self_ty = self.adjust_self_ty(unadjusted_self_ty, &pick); - // Make sure nobody calls `drop()` explicitly. - self.enforce_illegal_method_limitations(&pick); - // Create substitutions for the method's type parameters. let rcvr_substs = self.fresh_receiver_substs(self_ty, &pick); let all_substs = self.instantiate_method_substs(&pick, segment, rcvr_substs); @@ -96,6 +94,22 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { // Create the final signature for the method, replacing late-bound regions. let (method_sig, method_predicates) = self.instantiate_method_sig(&pick, all_substs); + // Unify the (adjusted) self type with what the method expects. + // + // SUBTLE: if we want good error messages, because of "guessing" while matching + // traits, no trait system method can be called before this point because they + // could alter our Self-type, except for normalizing the receiver from the + // signature (which is also done during probing). + let method_sig_rcvr = + self.normalize_associated_types_in(self.span, &method_sig.inputs()[0]); + self.unify_receivers(self_ty, method_sig_rcvr); + + let (method_sig, method_predicates) = + self.normalize_associated_types_in(self.span, &(method_sig, method_predicates)); + + // Make sure nobody calls `drop()` explicitly. + self.enforce_illegal_method_limitations(&pick); + // If there is a `Self: Sized` bound and `Self` is a trait object, it is possible that // something which derefs to `Self` actually implements the trait and the caller // wanted to make a static dispatch on it but forgot to import the trait. @@ -106,9 +120,6 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { // appropriate hint suggesting to import the trait. let illegal_sized_bound = self.predicates_require_illegal_sized_bound(&method_predicates); - // Unify the (adjusted) self type with what the method expects. - self.unify_receivers(self_ty, method_sig.inputs()[0]); - // Add any trait/regions obligations specified on the method's type parameters. // We won't add these if we encountered an illegal sized bound, so that we can use // a custom error in that case. @@ -338,6 +349,9 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { /////////////////////////////////////////////////////////////////////////// // + // NOTE: this returns the *unnormalized* predicates and method sig. Because of + // inference guessing, the predicates and method signature can't be normalized + // until we unify the `Self` type. fn instantiate_method_sig(&mut self, pick: &probe::Pick<'tcx>, all_substs: &'tcx Substs<'tcx>) @@ -352,8 +366,6 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { let def_id = pick.item.def_id; let method_predicates = self.tcx.predicates_of(def_id) .instantiate(self.tcx, all_substs); - let method_predicates = self.normalize_associated_types_in(self.span, - &method_predicates); debug!("method_predicates after subst = {:?}", method_predicates); @@ -369,7 +381,7 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { debug!("late-bound lifetimes from method instantiated, method_sig={:?}", method_sig); - let method_sig = self.instantiate_type_scheme(self.span, all_substs, &method_sig); + let method_sig = method_sig.subst(self.tcx, all_substs); debug!("type scheme substituted, method_sig={:?}", method_sig); (method_sig, method_predicates) diff --git a/src/test/compile-fail/issue-45801.rs b/src/test/compile-fail/issue-45801.rs new file mode 100644 index 0000000000000..7823a7d6ba8b5 --- /dev/null +++ b/src/test/compile-fail/issue-45801.rs @@ -0,0 +1,35 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +struct Params; + +pub trait Plugin { + type Error; +} + +pub trait Pluggable { + fn get_ref>(&mut self) -> Option { + None + } +} + +struct Foo; +impl Plugin for Params { + type Error = (); +} + +impl Pluggable for T {} + +fn handle(req: &mut i32) { + req.get_ref::(); + //~^ ERROR the trait bound `Params: Plugin` is not satisfied +} + +fn main() {} From 73bdf67059e5247e4d5f2861010528939ce15d28 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 14 Nov 2017 16:01:50 +0200 Subject: [PATCH 3/3] check_unsafety: fix unused unsafe block duplication The duplicate error message is later removed by error message deduplication, but it still appears on beta and is still a bug --- src/librustc_mir/transform/check_unsafety.rs | 1 - ...e-45107-unnecessary-unsafe-in-closure.stderr | 17 +++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs index f852b46cfbc7c..ac76c8e95d2b0 100644 --- a/src/librustc_mir/transform/check_unsafety.rs +++ b/src/librustc_mir/transform/check_unsafety.rs @@ -128,7 +128,6 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { let UnsafetyCheckResult { violations, unsafe_blocks } = self.tcx.unsafety_check_result(def_id); - self.inherited_blocks.extend(unsafe_blocks.iter().cloned()); self.register_violations(&violations, &unsafe_blocks); } } diff --git a/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr index 5c58b19c7fb35..be6b1e30a59da 100644 --- a/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr +++ b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr @@ -36,15 +36,16 @@ error: unnecessary `unsafe` block | |_________________^ unnecessary `unsafe` block | note: because it's nested under this `unsafe` block - --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:17:13 + --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:15:5 | -17 | / unsafe { +15 | / unsafe { +16 | | let f = |v: &mut Vec<_>| { +17 | | unsafe { 18 | | v.set_len(24); -19 | | |w: &mut Vec| { unsafe { -20 | | w.set_len(32); -21 | | } }; -22 | | } - | |_____________^ +... | +29 | | f(&mut v); +30 | | } + | |_____^ error: unnecessary `unsafe` block --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:23:34 @@ -67,5 +68,5 @@ note: because it's nested under this `unsafe` block 30 | | } | |_____^ -error: aborting due to 6 previous errors +error: aborting due to 3 previous errors