Rollup merge of rust-lang#71862 - LeSeulArtichaut:unsafe-block-in-unsafe-fn, r=nikomatsakis

Implement RFC 2585: unsafe blocks in unsafe fn

Tracking issue: rust-lang#71668
r? @RalfJung cc @nikomatsakis

Author: RalfJung

src/librustc_feature/active.rs

@@ -571,6 +571,9 @@ declare_features! (
     /// Allows the use of `#[ffi_const]` on foreign functions.
     (active, ffi_const, "1.45.0", Some(58328), None),
 
+    /// No longer treat an unsafe function as an unsafe block.
+    (active, unsafe_block_in_unsafe_fn, "1.45.0", Some(71668), None),
+
     // -------------------------------------------------------------------------
     // feature-group-end: actual feature gates
     // -------------------------------------------------------------------------

src/librustc_lint/levels.rs

@@ -14,11 +14,11 @@ use rustc_middle::lint::LintDiagnosticBuilder;
 use rustc_middle::lint::{struct_lint_level, LintLevelMap, LintLevelSets, LintSet, LintSource};
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::TyCtxt;
-use rustc_session::lint::{builtin, Level, Lint};
+use rustc_session::lint::{builtin, Level, Lint, LintId};
 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,11 +80,13 @@ impl<'s> LintLevelsBuilder<'s> {
             let level = cmp::min(level, self.sets.lint_cap);
 
             let lint_flag_val = Symbol::intern(lint_name);
+
             let ids = match store.find_lints(&lint_name) {
                 Ok(ids) => ids,
                 Err(_) => continue, // errors handled in check_lint_name_cmdline above
             };
             for id in ids {
+                self.check_gated_lint(id, DUMMY_SP);
                 let src = LintSource::CommandLine(lint_flag_val);
                 specs.insert(id, (level, src));
             }
@@ -213,6 +215,7 @@ impl<'s> LintLevelsBuilder<'s> {
                     CheckLintNameResult::Ok(ids) => {
                         let src = LintSource::Node(name, li.span(), reason);
                         for id in ids {
+                            self.check_gated_lint(*id, attr.span);
                             specs.insert(*id, (level, src));
                         }
                     }
@@ -383,6 +386,20 @@ impl<'s> LintLevelsBuilder<'s> {
         BuilderPush { prev, changed: prev != self.cur }
     }
 
+    fn check_gated_lint(&self, id: LintId, span: Span) {
+        if id == LintId::of(builtin::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;

src/librustc_middle/mir/mod.rs

@@ -408,7 +408,7 @@ impl<'tcx> Body<'tcx> {
     }
 }
 
-#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable, HashStable)]
+#[derive(Copy, Clone, PartialEq, Eq, Debug, RustcEncodable, RustcDecodable, HashStable)]
 pub enum Safety {
     Safe,
     /// Unsafe because of a PushUnsafeBlock

src/librustc_middle/mir/query.rs

@@ -15,15 +15,27 @@ use super::{Field, SourceInfo};
 
 #[derive(Copy, Clone, PartialEq, RustcEncodable, RustcDecodable, HashStable)]
 pub enum UnsafetyViolationKind {
+    /// Only permitted in regular `fn`s, prohibitted in `const fn`s.
     General,
     /// Permitted both in `const fn`s and regular `fn`s.
     GeneralAndConstFn,
-    BorrowPacked(hir::HirId),
+    /// Borrow of packed field.
+    /// Has to be handled as a lint for backwards compatibility.
+    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,
+    /// Borrow of packed field 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)]`.
+    UnsafeFnBorrowPacked,
 }
 
 #[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,

src/librustc_mir/transform/check_unsafety.rs

@@ -2,14 +2,16 @@ 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};
 use rustc_middle::mir::*;
 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, UNUSED_UNSAFE};
+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;
@@ -202,25 +204,30 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
 
         if context.is_borrow() {
             if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
-                let source_info = self.source_info;
-                let lint_root = self.body.source_scopes[source_info.scope]
-                    .local_data
-                    .as_ref()
-                    .assert_crate_local()
-                    .lint_root;
                 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(lint_root),
+                    UnsafetyViolationKind::BorrowPacked,
                 );
             }
         }
 
         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 +308,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                 }
                 _ => {}
             }
-            self.source_info = old_source_info;
+            self.source_info = source_info;
         }
     }
 }
@@ -314,9 +321,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,22 +356,42 @@ 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::UnsafeFnBorrowPacked => {
+                            bug!("`UnsafetyViolationKind::UnsafeFn` in an `Safe` context")
+                        }
+                    }
+                    if !self.violations.contains(&violation) {
+                        self.violations.push(violation)
+                    }
+                }
+                false
+            }
+            // With the RFC 2585, no longer allow `unsafe` operations in `unsafe fn`s
+            Safety::FnUnsafe if self.tcx.features().unsafe_block_in_unsafe_fn => {
+                for violation in violations {
+                    let mut violation = *violation;
+
+                    if violation.kind == UnsafetyViolationKind::BorrowPacked {
+                        violation.kind = UnsafetyViolationKind::UnsafeFnBorrowPacked;
+                    } else {
+                        violation.kind = UnsafetyViolationKind::UnsafeFn;
                     }
                     if !self.violations.contains(&violation) {
                         self.violations.push(violation)
                     }
                 }
                 false
             }
-            // `unsafe` function bodies allow unsafe without additional unsafe blocks
+            // `unsafe` function bodies allow unsafe without additional unsafe blocks (before RFC 2585)
             Safety::BuiltinUnsafe | Safety::FnUnsafe => true,
             Safety::ExplicitUnsafe(hir_id) => {
                 // mark unsafe block as used if there are any unsafe operations inside
@@ -373,7 +406,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
@@ -383,6 +416,10 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
                                     self.violations.push(violation)
                                 }
                             }
+                            UnsafetyViolationKind::UnsafeFn
+                            | UnsafetyViolationKind::UnsafeFnBorrowPacked => bug!(
+                                "`UnsafetyViolationKind::UnsafeFn` in an `ExplicitUnsafe` context"
+                            ),
                         }
                     }
                 }
@@ -575,9 +612,12 @@ fn is_enclosed(
             kind: hir::ItemKind::Fn(ref sig, _, _), ..
         })) = tcx.hir().find(parent_id)
         {
-            match sig.header.unsafety {
-                hir::Unsafety::Unsafe => Some(("fn".to_string(), parent_id)),
-                hir::Unsafety::Normal => None,
+            if sig.header.unsafety == hir::Unsafety::Unsafe
+                && !tcx.features().unsafe_block_in_unsafe_fn
+            {
+                Some(("fn".to_string(), parent_id))
+            } else {
+                None
             }
         } else {
             is_enclosed(tcx, used_unsafe, parent_id)
@@ -630,40 +670,90 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: DefId) {
     let UnsafetyCheckResult { violations, unsafe_blocks } =
         tcx.unsafety_check_result(def_id.expect_local());
 
-    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) { " function or" } else { "" };
+
         match kind {
             UnsafetyViolationKind::GeneralAndConstFn | UnsafetyViolationKind::General => {
+                // once
                 struct_span_err!(
                     tcx.sess,
                     source_info.span,
                     E0133,
-                    "{} is unsafe and requires unsafe function or block",
-                    description
+                    "{} is unsafe and requires unsafe{} block",
+                    description,
+                    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 or block (error E0133)",
-                                description
+                                "{} is unsafe and requires unsafe{} block (error E0133)",
+                                description, unsafe_fn_msg,
                             ))
                             .note(&details.as_str())
                             .emit()
                         },
                     )
                 }
             }
+            UnsafetyViolationKind::UnsafeFn => tcx.struct_span_lint_hir(
+                UNSAFE_OP_IN_UNSAFE_FN,
+                lint_root,
+                source_info.span,
+                |lint| {
+                    lint.build(&format!(
+                        "{} is unsafe and requires unsafe block (error E0133)",
+                        description,
+                    ))
+                    .span_label(source_info.span, &*description.as_str())
+                    .note(&details.as_str())
+                    .emit();
+                },
+            ),
+            UnsafetyViolationKind::UnsafeFnBorrowPacked => {
+                // When `unsafe_op_in_unsafe_fn` is disallowed, the behavior of safe and unsafe functions
+                // should be the same in terms of warnings and errors. Therefore, with `#[warn(safe_packed_borrows)]`,
+                // a safe packed borrow should emit a warning *but not an error* in an unsafe function,
+                // just like in a safe function, even if `unsafe_op_in_unsafe_fn` is `deny`.
+                //
+                // Also, `#[warn(unsafe_op_in_unsafe_fn)]` can't cause any new errors. Therefore, with
+                // `#[deny(safe_packed_borrows)]` and `#[warn(unsafe_op_in_unsafe_fn)]`, a packed borrow
+                // should only issue a warning for the sake of backwards compatibility.
+                //
+                // The solution those 2 expectations is to always take the minimum of both lints.
+                // This prevent any new errors (unless both lints are explicitely set to `deny`).
+                let lint = if tcx.lint_level_at_node(SAFE_PACKED_BORROWS, lint_root).0
+                    <= tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, lint_root).0
+                {
+                    SAFE_PACKED_BORROWS
+                } else {
+                    UNSAFE_OP_IN_UNSAFE_FN
+                };
+                tcx.struct_span_lint_hir(&lint, lint_root, source_info.span, |lint| {
+                    lint.build(&format!(
+                        "{} is unsafe and requires unsafe block (error E0133)",
+                        description,
+                    ))
+                    .span_label(source_info.span, &*description.as_str())
+                    .note(&details.as_str())
+                    .emit();
+                })
+            }
         }
     }
 
@@ -683,3 +773,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
+}