Apply suggestions from code review

rust-lang · RalfJung · May 11, 2020 · Apr 13, 2020 · May 1, 2020 · May 1, 2020
commit bb6791502846b62e752c99396953e4db7739f28c
diff --git a/src/librustc_lint/levels.rs b/src/librustc_lint/levels.rs
@@ -17,8 +17,8 @@ use rustc_middle::ty::TyCtxt;
 use rustc_session::lint::{builtin, Level, Lint};
 use rustc_session::parse::feature_err;
 use rustc_session::Session;
-use rustc_span::source_map::MultiSpan;
 use rustc_span::symbol::{sym, Symbol};
+use rustc_span::{source_map::MultiSpan, Span, DUMMY_SP};
 
 use std::cmp;
 
@@ -80,6 +80,8 @@ impl<'s> LintLevelsBuilder<'s> {
             let level = cmp::min(level, self.sets.lint_cap);
 
             let lint_flag_val = Symbol::intern(lint_name);
+            self.check_gated_lint(lint_flag_val, DUMMY_SP);
+
             let ids = match store.find_lints(&lint_name) {
                 Ok(ids) => ids,
                 Err(_) => continue, // errors handled in check_lint_name_cmdline above
@@ -211,6 +213,7 @@ impl<'s> LintLevelsBuilder<'s> {
                 let name = meta_item.path.segments.last().expect("empty lint name").ident.name;
                 match store.check_lint_name(&name.as_str(), tool_name) {
                     CheckLintNameResult::Ok(ids) => {
+                        self.check_gated_lint(name, attr.span);
                         let src = LintSource::Node(name, li.span(), reason);
                         for id in ids {
                             specs.insert(*id, (level, src));
@@ -383,6 +386,20 @@ impl<'s> LintLevelsBuilder<'s> {
         BuilderPush { prev, changed: prev != self.cur }
     }
 
+    fn check_gated_lint(&self, name: Symbol, span: Span) {
+        if name.as_str() == "unsafe_op_in_unsafe_fn"
+            && !self.sess.features_untracked().unsafe_block_in_unsafe_fn
+        {
+            feature_err(
+                &self.sess.parse_sess,
+                sym::unsafe_block_in_unsafe_fn,
+                span,
+                "the `unsafe_op_in_unsafe_fn` lint is unstable",
+            )
+            .emit();
+        }
+    }
+
     /// Called after `push` when the scope of a set of attributes are exited.
     pub fn pop(&mut self, push: BuilderPush) {
         self.cur = push.prev;

diff --git a/src/librustc_middle/mir/query.rs b/src/librustc_middle/mir/query.rs
@@ -21,16 +21,17 @@ pub enum UnsafetyViolationKind {
     GeneralAndConstFn,
     /// Borrow of packed field.
     /// Has to be handled as a lint for backwards compatibility.
-    BorrowPacked(hir::HirId),
+    BorrowPacked,
     /// Unsafe operation in an `unsafe fn` but outside an `unsafe` block.
     /// Has to be handled as a lint for backwards compatibility.
     /// Should stay gated under `#![feature(unsafe_block_in_unsafe_fn)]`.
-    UnsafeFn(hir::HirId),
+    UnsafeFn,
 }
 
 #[derive(Copy, Clone, PartialEq, RustcEncodable, RustcDecodable, HashStable)]
 pub struct UnsafetyViolation {
     pub source_info: SourceInfo,
+    pub lint_root: hir::HirId,
     pub description: Symbol,
     pub details: Symbol,
     pub kind: UnsafetyViolationKind,

diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs
@@ -2,6 +2,7 @@ use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::struct_span_err;
 use rustc_hir as hir;
 use rustc_hir::def_id::{DefId, LocalDefId};
+use rustc_hir::hir_id::HirId;
 use rustc_hir::intravisit;
 use rustc_hir::Node;
 use rustc_middle::mir::visit::{MutatingUseContext, PlaceContext, Visitor};
@@ -10,6 +11,7 @@ use rustc_middle::ty::cast::CastTy;
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::{self, TyCtxt};
 use rustc_session::lint::builtin::{SAFE_PACKED_BORROWS, UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
+use rustc_session::lint::Level;
 use rustc_span::symbol::{sym, Symbol};
 
 use std::ops::Bound;
@@ -220,7 +222,18 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
 
         for (i, elem) in place.projection.iter().enumerate() {
             let proj_base = &place.projection[..i];
-            let old_source_info = self.source_info;
+            if context.is_borrow() {
+                if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
+                    self.require_unsafe(
+                        "borrow of packed field",
+                        "fields of packed structs might be misaligned: dereferencing a \
+                        misaligned pointer or even just creating a misaligned reference \
+                        is undefined behavior",
+                        UnsafetyViolationKind::BorrowPacked,
+                    );
+                }
+            }
+            let source_info = self.source_info;
             if let [] = proj_base {
                 let decl = &self.body.local_decls[place.local];
                 if decl.internal {
@@ -301,7 +314,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                 }
                 _ => {}
             }
-            self.source_info = old_source_info;
+            self.source_info = source_info;
         }
     }
 }
@@ -314,9 +327,15 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
         kind: UnsafetyViolationKind,
     ) {
         let source_info = self.source_info;
+        let lint_root = self.body.source_scopes[self.source_info.scope]
+            .local_data
+            .as_ref()
+            .assert_crate_local()
+            .lint_root;
         self.register_violations(
             &[UnsafetyViolation {
                 source_info,
+                lint_root,
                 description: Symbol::intern(description),
                 details: Symbol::intern(details),
                 kind,
@@ -343,15 +362,15 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
                     match violation.kind {
                         UnsafetyViolationKind::GeneralAndConstFn
                         | UnsafetyViolationKind::General => {}
-                        UnsafetyViolationKind::BorrowPacked(_) => {
+                        UnsafetyViolationKind::BorrowPacked => {
                             if self.min_const_fn {
                                 // const fns don't need to be backwards compatible and can
                                 // emit these violations as a hard error instead of a backwards
                                 // compat lint
                                 violation.kind = UnsafetyViolationKind::General;
                             }
                         }
-                        UnsafetyViolationKind::UnsafeFn(_) => {
+                        UnsafetyViolationKind::UnsafeFn => {
                             bug!("`UnsafetyViolationKind::UnsafeFn` in an `Safe` context")
                         }
                     }
@@ -365,14 +384,9 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
             Safety::FnUnsafe if self.tcx.features().unsafe_block_in_unsafe_fn => {
                 for violation in violations {
                     let mut violation = *violation;
-                    let lint_root = self.body.source_scopes[self.source_info.scope]
-                        .local_data
-                        .as_ref()
-                        .assert_crate_local()
-                        .lint_root;
 
                     // FIXME(LeSeulArtichaut): what to do with `UnsafetyViolationKind::BorrowPacked`?
-                    violation.kind = UnsafetyViolationKind::UnsafeFn(lint_root);
+                    violation.kind = UnsafetyViolationKind::UnsafeFn;
                     if !self.violations.contains(&violation) {
                         self.violations.push(violation)
                     }
@@ -394,7 +408,7 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
                             UnsafetyViolationKind::GeneralAndConstFn => {}
                             // these things are forbidden in const fns
                             UnsafetyViolationKind::General
-                            | UnsafetyViolationKind::BorrowPacked(_) => {
+                            | UnsafetyViolationKind::BorrowPacked => {
                                 let mut violation = *violation;
                                 // const fns don't need to be backwards compatible and can
                                 // emit these violations as a hard error instead of a backwards
@@ -404,7 +418,7 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
                                     self.violations.push(violation)
                                 }
                             }
-                            UnsafetyViolationKind::UnsafeFn(_) => bug!(
+                            UnsafetyViolationKind::UnsafeFn => bug!(
                                 "`UnsafetyViolationKind::UnsafeFn` in an `ExplicitUnsafe` context"
                             ),
                         }
@@ -657,47 +671,50 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: DefId) {
     let UnsafetyCheckResult { violations, unsafe_blocks } =
         tcx.unsafety_check_result(def_id.expect_local());
 
-    let or_block_msg = if tcx.features().unsafe_block_in_unsafe_fn { "" } else { " or block" };
-
-    for &UnsafetyViolation { source_info, description, details, kind } in violations.iter() {
+    for &UnsafetyViolation { source_info, lint_root, description, details, kind } in
+        violations.iter()
+    {
         // Report an error.
+        let unsafe_fn_msg =
+            if unsafe_op_in_unsafe_fn_allowed(tcx, lint_root) { "" } else { " function or" };
+
         match kind {
             UnsafetyViolationKind::GeneralAndConstFn | UnsafetyViolationKind::General => {
                 // once
                 struct_span_err!(
                     tcx.sess,
                     source_info.span,
                     E0133,
-                    "{} is unsafe and requires unsafe function{}",
+                    "{} is unsafe and requires unsafe{} block",
                     description,
-                    or_block_msg,
+                    unsafe_fn_msg,
                 )
                 .span_label(source_info.span, &*description.as_str())
                 .note(&details.as_str())
                 .emit();
             }
-            UnsafetyViolationKind::BorrowPacked(lint_hir_id) => {
+            UnsafetyViolationKind::BorrowPacked => {
                 if let Some(impl_def_id) = builtin_derive_def_id(tcx, def_id) {
                     tcx.ensure().unsafe_derive_on_repr_packed(impl_def_id);
                 } else {
                     tcx.struct_span_lint_hir(
                         SAFE_PACKED_BORROWS,
-                        lint_hir_id,
+                        lint_root,
                         source_info.span,
                         |lint| {
                             lint.build(&format!(
-                                "{} is unsafe and requires unsafe function{} (error E0133)",
-                                description, or_block_msg,
+                                "{} is unsafe and requires unsafe{} block (error E0133)",
+                                description, unsafe_fn_msg,
                             ))
                             .note(&details.as_str())
                             .emit()
                         },
                     )
                 }
             }
-            UnsafetyViolationKind::UnsafeFn(lint_hir_id) => tcx.struct_span_lint_hir(
+            UnsafetyViolationKind::UnsafeFn => tcx.struct_span_lint_hir(
                 UNSAFE_OP_IN_UNSAFE_FN,
-                lint_hir_id,
+                lint_root,
                 source_info.span,
                 |lint| {
                     lint.build(&format!(
@@ -728,3 +745,7 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: DefId) {
         report_unused_unsafe(tcx, &unsafe_used, block_id);
     }
 }
+
+fn unsafe_op_in_unsafe_fn_allowed(tcx: TyCtxt<'_>, id: HirId) -> bool {
+    tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, id).0 == Level::Allow
+}
diff --git a/src/librustc_mir_build/build/block.rs b/src/librustc_mir_build/build/block.rs
@@ -4,6 +4,8 @@ use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
 use crate::hair::*;
 use rustc_hir as hir;
 use rustc_middle::mir::*;
+use rustc_session::lint::builtin::UNSAFE_OP_IN_UNSAFE_FN;
+use rustc_session::lint::Level;
 use rustc_span::Span;
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
@@ -218,7 +220,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 match self.unpushed_unsafe {
                     Safety::Safe => {}
                     // no longer treat `unsafe fn`s as `unsafe` contexts (see RFC #2585)
-                    Safety::FnUnsafe if self.hir.tcx().features().unsafe_block_in_unsafe_fn => {}
+                    Safety::FnUnsafe
+                        if self.hir.tcx().lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, hir_id).0
+                            != Level::Allow => {}
                     _ => return,
                 }
                 self.unpushed_unsafe = Safety::ExplicitUnsafe(hir_id);
@@ -233,7 +237,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     .push_unsafe_count
                     .checked_sub(1)
                     .unwrap_or_else(|| span_bug!(span, "unsafe count underflow"));
-                if self.push_unsafe_count == 0 { Some(self.unpushed_unsafe) } else { None }
+                if self.push_unsafe_count == 0 {
+                    Some(self.unpushed_unsafe)
+                } else {
+                    None
+                }
             }
         };
 

diff --git a/src/test/ui/feature-gates/feature-gate-unsafe_block_in_unsafe_fn.rs b/src/test/ui/feature-gates/feature-gate-unsafe_block_in_unsafe_fn.rs
@@ -1,11 +1,4 @@
-#![deny(unused_unsafe)]
-
-unsafe fn unsf() {}
-
-unsafe fn foo() {
-    unsafe { //~ ERROR unnecessary `unsafe` block
-        unsf()
-    }
-}
+#![deny(unsafe_op_in_unsafe_fn)]
+//~^ ERROR the `unsafe_op_in_unsafe_fn` lint is unstable
 
 fn main() {}
diff --git a/src/test/ui/feature-gates/feature-gate-unsafe_block_in_unsafe_fn.stderr b/src/test/ui/feature-gates/feature-gate-unsafe_block_in_unsafe_fn.stderr
@@ -1,16 +1,30 @@
-error: unnecessary `unsafe` block
-  --> $DIR/feature-gate-unsafe_block_in_unsafe_fn.rs:6:5
+error[E0658]: the `unsafe_op_in_unsafe_fn` lint is unstable
+  --> $DIR/feature-gate-unsafe_block_in_unsafe_fn.rs:1:1
    |
-LL | unsafe fn foo() {
-   | --------------- because it's nested under this `unsafe` fn
-LL |     unsafe {
-   |     ^^^^^^ unnecessary `unsafe` block
+LL | #![deny(unsafe_op_in_unsafe_fn)]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the lint level is defined here
-  --> $DIR/feature-gate-unsafe_block_in_unsafe_fn.rs:1:9
+   = note: see issue #71668 <https://github.com/rust-lang/rust/issues/71668> for more information
+   = help: add `#![feature(unsafe_block_in_unsafe_fn)]` to the crate attributes to enable
+
+error[E0658]: the `unsafe_op_in_unsafe_fn` lint is unstable
+  --> $DIR/feature-gate-unsafe_block_in_unsafe_fn.rs:1:1
+   |
+LL | #![deny(unsafe_op_in_unsafe_fn)]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: see issue #71668 <https://github.com/rust-lang/rust/issues/71668> for more information
+   = help: add `#![feature(unsafe_block_in_unsafe_fn)]` to the crate attributes to enable
+
+error[E0658]: the `unsafe_op_in_unsafe_fn` lint is unstable
+  --> $DIR/feature-gate-unsafe_block_in_unsafe_fn.rs:1:1
+   |
+LL | #![deny(unsafe_op_in_unsafe_fn)]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-LL | #![deny(unused_unsafe)]
-   |         ^^^^^^^^^^^^^
+   = note: see issue #71668 <https://github.com/rust-lang/rust/issues/71668> for more information
+   = help: add `#![feature(unsafe_block_in_unsafe_fn)]` to the crate attributes to enable
 
-error: aborting due to previous error
+error: aborting due to 3 previous errors
 
+For more information about this error, try `rustc --explain E0658`.
diff --git a/src/test/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.rs b/src/test/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.rs
@@ -21,6 +21,12 @@ unsafe fn baz() {
 #[allow(unsafe_op_in_unsafe_fn)]
 unsafe fn qux() {
     unsf(); // no error
+
+    unsafe { unsf() }
+    //~^ ERROR unnecessary `unsafe` block
 }
 
-fn main() {}
+fn main() {
+    unsf()
+    //~^ ERROR call to unsafe function is unsafe and requires unsafe function or block
+}
diff --git a/src/test/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.stderr b/src/test/ui/unsafe/rfc-2585-unsafe_op_in_unsafe_fn.stderr
@@ -25,5 +25,20 @@ note: the lint level is defined here
 LL | #![deny(unused_unsafe)]
    |         ^^^^^^^^^^^^^
 
-error: aborting due to previous error; 1 warning emitted
+error: unnecessary `unsafe` block
+  --> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:25:5
+   |
+LL |     unsafe { unsf() }
+   |     ^^^^^^ unnecessary `unsafe` block
+
+error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
+  --> $DIR/rfc-2585-unsafe_op_in_unsafe_fn.rs:30:5
+   |
+LL |     unsf()
+   |     ^^^^^^ call to unsafe function
+   |
+   = note: consult the function's documentation for information on how to avoid undefined behavior
+
+error: aborting due to 3 previous errors; 1 warning emitted
 
+For more information about this error, try `rustc --explain E0133`.