Auto merge of #124482 - spastorino:unsafe-extern-blocks, r=oli-obk

Unsafe extern blocks

This implements RFC 3484.

Tracking issue #123743 and RFC rust-lang/rfcs#3484

This is better reviewed commit by commit.

compiler/rustc_ast/src/ast.rs

@@ -2501,6 +2501,8 @@ pub enum IsAuto {
 pub enum Safety {
     /// `unsafe` an item is explicitly marked as `unsafe`.
     Unsafe(Span),
+    /// `safe` an item is explicitly marked as `safe`.
+    Safe(Span),
     /// Default means no value was provided, it will take a default value given the context in
     /// which is used.
     Default,
@@ -3162,6 +3164,7 @@ pub struct DelegationMac {
 #[derive(Clone, Encodable, Decodable, Debug)]
 pub struct StaticItem {
     pub ty: P<Ty>,
+    pub safety: Safety,
     pub mutability: Mutability,
     pub expr: Option<P<Expr>>,
 }
@@ -3171,6 +3174,7 @@ pub struct StaticItem {
 #[derive(Clone, Encodable, Decodable, Debug)]
 pub struct StaticForeignItem {
     pub ty: P<Ty>,
+    pub safety: Safety,
     pub mutability: Mutability,
     pub expr: Option<P<Expr>>,
 }
@@ -3179,6 +3183,7 @@ impl From<StaticItem> for StaticForeignItem {
     fn from(static_item: StaticItem) -> StaticForeignItem {
         StaticForeignItem {
             ty: static_item.ty,
+            safety: static_item.safety,
             mutability: static_item.mutability,
             expr: static_item.expr,
         }
@@ -3189,6 +3194,7 @@ impl From<StaticForeignItem> for StaticItem {
     fn from(static_item: StaticForeignItem) -> StaticItem {
         StaticItem {
             ty: static_item.ty,
+            safety: static_item.safety,
             mutability: static_item.mutability,
             expr: static_item.expr,
         }

compiler/rustc_ast/src/mut_visit.rs

@@ -862,6 +862,7 @@ fn visit_defaultness<T: MutVisitor>(defaultness: &mut Defaultness, vis: &mut T)
 fn visit_safety<T: MutVisitor>(safety: &mut Safety, vis: &mut T) {
     match safety {
         Safety::Unsafe(span) => vis.visit_span(span),
+        Safety::Safe(span) => vis.visit_span(span),
         Safety::Default => {}
     }
 }
@@ -1079,7 +1080,7 @@ impl NoopVisitItemKind for ItemKind {
         match self {
             ItemKind::ExternCrate(_orig_name) => {}
             ItemKind::Use(use_tree) => vis.visit_use_tree(use_tree),
-            ItemKind::Static(box StaticItem { ty, mutability: _, expr }) => {
+            ItemKind::Static(box StaticItem { ty, safety: _, mutability: _, expr }) => {
                 vis.visit_ty(ty);
                 visit_opt(expr, |expr| vis.visit_expr(expr));
             }
@@ -1289,7 +1290,12 @@ pub fn noop_flat_map_item<K: NoopVisitItemKind>(
 impl NoopVisitItemKind for ForeignItemKind {
     fn noop_visit(&mut self, visitor: &mut impl MutVisitor) {
         match self {
-            ForeignItemKind::Static(box StaticForeignItem { ty, mutability: _, expr }) => {
+            ForeignItemKind::Static(box StaticForeignItem {
+                ty,
+                mutability: _,
+                expr,
+                safety: _,
+            }) => {
                 visitor.visit_ty(ty);
                 visit_opt(expr, |expr| visitor.visit_expr(expr));
             }

compiler/rustc_ast/src/token.rs

@@ -210,6 +210,7 @@ pub fn ident_can_begin_expr(name: Symbol, span: Span, is_raw: IdentIsRaw) -> boo
             kw::Unsafe,
             kw::While,
             kw::Yield,
+            kw::Safe,
             kw::Static,
         ]
         .contains(&name)
@@ -577,6 +578,7 @@ impl Token {
                 kw::Impl,
                 kw::Unsafe,
                 kw::Const,
+                kw::Safe,
                 kw::Static,
                 kw::Union,
                 kw::Macro,

compiler/rustc_ast/src/visit.rs

@@ -334,7 +334,7 @@ impl WalkItemKind for ItemKind {
         match self {
             ItemKind::ExternCrate(_) => {}
             ItemKind::Use(use_tree) => try_visit!(visitor.visit_use_tree(use_tree, item.id, false)),
-            ItemKind::Static(box StaticItem { ty, mutability: _, expr }) => {
+            ItemKind::Static(box StaticItem { ty, safety: _, mutability: _, expr }) => {
                 try_visit!(visitor.visit_ty(ty));
                 visit_opt!(visitor, visit_expr, expr);
             }
@@ -658,7 +658,12 @@ impl WalkItemKind for ForeignItemKind {
     ) -> V::Result {
         let &Item { id, span, ident, ref vis, .. } = item;
         match self {
-            ForeignItemKind::Static(box StaticForeignItem { ty, mutability: _, expr }) => {
+            ForeignItemKind::Static(box StaticForeignItem {
+                ty,
+                mutability: _,
+                expr,
+                safety: _,
+            }) => {
                 try_visit!(visitor.visit_ty(ty));
                 visit_opt!(visitor, visit_expr, expr);
             }

compiler/rustc_ast_lowering/src/item.rs

@@ -181,7 +181,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 
                 self.lower_use_tree(use_tree, &prefix, id, vis_span, ident, attrs)
             }
-            ItemKind::Static(box ast::StaticItem { ty: t, mutability: m, expr: e }) => {
+            ItemKind::Static(box ast::StaticItem { ty: t, safety: _, mutability: m, expr: e }) => {
                 let (ty, body_id) =
                     self.lower_const_item(t, span, e.as_deref(), ImplTraitPosition::StaticTy);
                 hir::ItemKind::Static(ty, *m, body_id)
@@ -388,7 +388,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
                     ImplPolarity::Negative(s) => ImplPolarity::Negative(self.lower_span(*s)),
                 };
                 hir::ItemKind::Impl(self.arena.alloc(hir::Impl {
-                    safety: self.lower_safety(*safety),
+                    safety: self.lower_safety(*safety, hir::Safety::Safe),
                     polarity,
                     defaultness,
                     defaultness_span,
@@ -418,7 +418,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
                         let items = this.arena.alloc_from_iter(
                             items.iter().map(|item| this.lower_trait_item_ref(item)),
                         );
-                        let safety = this.lower_safety(*safety);
+                        let safety = this.lower_safety(*safety, hir::Safety::Safe);
                         (safety, items, bounds)
                     },
                 );
@@ -660,13 +660,21 @@ impl<'hir> LoweringContext<'_, 'hir> {
                                 this.lower_fn_params_to_names(fdec),
                             )
                         });
+                    let safety = self.lower_safety(sig.header.safety, hir::Safety::Unsafe);
 
-                    hir::ForeignItemKind::Fn(fn_dec, fn_args, generics)
+                    hir::ForeignItemKind::Fn(fn_dec, fn_args, generics, safety)
                 }
-                ForeignItemKind::Static(box StaticForeignItem { ty, mutability, expr: _ }) => {
+                ForeignItemKind::Static(box StaticForeignItem {
+                    ty,
+                    mutability,
+                    expr: _,
+                    safety,
+                }) => {
                     let ty = self
                         .lower_ty(ty, ImplTraitContext::Disallowed(ImplTraitPosition::StaticTy));
-                    hir::ForeignItemKind::Static(ty, *mutability)
+                    let safety = self.lower_safety(*safety, hir::Safety::Unsafe);
+
+                    hir::ForeignItemKind::Static(ty, *mutability, safety)
                 }
                 ForeignItemKind::TyAlias(..) => hir::ForeignItemKind::Type,
                 ForeignItemKind::MacCall(_) => panic!("macro shouldn't exist here"),
@@ -1360,7 +1368,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
             hir::IsAsync::NotAsync
         };
         hir::FnHeader {
-            safety: self.lower_safety(h.safety),
+            safety: self.lower_safety(h.safety, hir::Safety::Safe),
             asyncness: asyncness,
             constness: self.lower_constness(h.constness),
             abi: self.lower_extern(h.ext),
@@ -1410,10 +1418,11 @@ impl<'hir> LoweringContext<'_, 'hir> {
         }
     }
 
-    pub(super) fn lower_safety(&mut self, s: Safety) -> hir::Safety {
+    pub(super) fn lower_safety(&mut self, s: Safety, default: hir::Safety) -> hir::Safety {
         match s {
             Safety::Unsafe(_) => hir::Safety::Unsafe,
-            Safety::Default => hir::Safety::Safe,
+            Safety::Default => default,
+            Safety::Safe(_) => hir::Safety::Safe,
         }
     }
 

compiler/rustc_ast_lowering/src/lib.rs

@@ -1321,7 +1321,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 let generic_params = self.lower_lifetime_binder(t.id, &f.generic_params);
                 hir::TyKind::BareFn(self.arena.alloc(hir::BareFnTy {
                     generic_params,
-                    safety: self.lower_safety(f.safety),
+                    safety: self.lower_safety(f.safety, hir::Safety::Safe),
                     abi: self.lower_extern(f.ext),
                     decl: self.lower_fn_decl(&f.decl, t.id, t.span, FnDeclKind::Pointer, None),
                     param_names: self.lower_fn_params_to_names(&f.decl),

compiler/rustc_ast_passes/messages.ftl

@@ -67,6 +67,9 @@ ast_passes_extern_fn_qualifiers = functions in `extern` blocks cannot have quali
     .label = in this `extern` block
     .suggestion = remove this qualifier
 
+ast_passes_extern_invalid_safety = items in unadorned `extern` blocks cannot have safety qualifiers
+    .suggestion = add unsafe to this `extern` block
+
 ast_passes_extern_item_ascii = items in `extern` blocks cannot use non-ascii identifiers
     .label = in this `extern` block
     .note = this limitation may be lifted in the future; see issue #83942 <https://github.com/rust-lang/rust/issues/83942> for more information
@@ -174,6 +177,8 @@ ast_passes_match_arm_with_no_body =
     `match` arm with no body
     .suggestion = add a body after the pattern
 
+ast_passes_missing_unsafe_on_extern = extern blocks must be unsafe
+
 ast_passes_module_nonascii = trying to load file for module `{$name}` with non-ascii identifier name
     .help = consider using the `#[path]` attribute to specify filesystem path
 

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -15,7 +15,8 @@ use rustc_data_structures::fx::FxIndexMap;
 use rustc_feature::Features;
 use rustc_parse::validate_attr;
 use rustc_session::lint::builtin::{
-    DEPRECATED_WHERE_CLAUSE_LOCATION, MISSING_ABI, PATTERNS_IN_FNS_WITHOUT_BODY,
+    DEPRECATED_WHERE_CLAUSE_LOCATION, MISSING_ABI, MISSING_UNSAFE_ON_EXTERN,
+    PATTERNS_IN_FNS_WITHOUT_BODY,
 };
 use rustc_session::lint::{BuiltinLintDiag, LintBuffer};
 use rustc_session::Session;
@@ -86,6 +87,9 @@ struct AstValidator<'a> {
     /// or `Foo::Bar<impl Trait>`
     is_impl_trait_banned: bool,
 
+    /// Used to ban explicit safety on foreign items when the extern block is not marked as unsafe.
+    extern_mod_safety: Option<Safety>,
+
     lint_buffer: &'a mut LintBuffer,
 }
 
@@ -116,6 +120,12 @@ impl<'a> AstValidator<'a> {
         self.outer_trait_or_trait_impl = old;
     }
 
+    fn with_in_extern_mod(&mut self, extern_mod_safety: Safety, f: impl FnOnce(&mut Self)) {
+        let old = mem::replace(&mut self.extern_mod_safety, Some(extern_mod_safety));
+        f(self);
+        self.extern_mod_safety = old;
+    }
+
     fn with_banned_impl_trait(&mut self, f: impl FnOnce(&mut Self)) {
         let old = mem::replace(&mut self.is_impl_trait_banned, true);
         f(self);
@@ -429,6 +439,18 @@ impl<'a> AstValidator<'a> {
         }
     }
 
+    fn check_foreign_item_safety(&self, item_span: Span, safety: Safety) {
+        if matches!(safety, Safety::Unsafe(_) | Safety::Safe(_))
+            && (self.extern_mod_safety == Some(Safety::Default)
+                || !self.features.unsafe_extern_blocks)
+        {
+            self.dcx().emit_err(errors::InvalidSafetyOnExtern {
+                item_span,
+                block: self.current_extern_span(),
+            });
+        }
+    }
+
     fn check_defaultness(&self, span: Span, defaultness: Defaultness) {
         if let Defaultness::Default(def_span) = defaultness {
             let span = self.session.source_map().guess_head_span(span);
@@ -518,18 +540,14 @@ impl<'a> AstValidator<'a> {
     fn check_foreign_fn_headerless(
         &self,
         // Deconstruct to ensure exhaustiveness
-        FnHeader { safety, coroutine_kind, constness, ext }: FnHeader,
+        FnHeader { safety: _, coroutine_kind, constness, ext }: FnHeader,
     ) {
         let report_err = |span| {
             self.dcx().emit_err(errors::FnQualifierInExtern {
                 span: span,
                 block: self.current_extern_span(),
             });
         };
-        match safety {
-            Safety::Unsafe(span) => report_err(span),
-            Safety::Default => (),
-        }
         match coroutine_kind {
             Some(knd) => report_err(knd.span()),
             None => (),
@@ -1017,19 +1035,39 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                 return; // Avoid visiting again.
             }
             ItemKind::ForeignMod(ForeignMod { abi, safety, .. }) => {
-                let old_item = mem::replace(&mut self.extern_mod, Some(item));
-                self.visibility_not_permitted(
-                    &item.vis,
-                    errors::VisibilityNotPermittedNote::IndividualForeignItems,
-                );
-                if let &Safety::Unsafe(span) = safety {
-                    self.dcx().emit_err(errors::UnsafeItem { span, kind: "extern block" });
-                }
-                if abi.is_none() {
-                    self.maybe_lint_missing_abi(item.span, item.id);
-                }
-                visit::walk_item(self, item);
-                self.extern_mod = old_item;
+                self.with_in_extern_mod(*safety, |this| {
+                    let old_item = mem::replace(&mut this.extern_mod, Some(item));
+                    this.visibility_not_permitted(
+                        &item.vis,
+                        errors::VisibilityNotPermittedNote::IndividualForeignItems,
+                    );
+
+                    if this.features.unsafe_extern_blocks {
+                        if &Safety::Default == safety {
+                            if item.span.at_least_rust_2024() {
+                                this.dcx()
+                                    .emit_err(errors::MissingUnsafeOnExtern { span: item.span });
+                            } else {
+                                this.lint_buffer.buffer_lint(
+                                    MISSING_UNSAFE_ON_EXTERN,
+                                    item.id,
+                                    item.span,
+                                    BuiltinLintDiag::MissingUnsafeOnExtern {
+                                        suggestion: item.span.shrink_to_lo(),
+                                    },
+                                );
+                            }
+                        }
+                    } else if let &Safety::Unsafe(span) = safety {
+                        this.dcx().emit_err(errors::UnsafeItem { span, kind: "extern block" });
+                    }
+
+                    if abi.is_none() {
+                        this.maybe_lint_missing_abi(item.span, item.id);
+                    }
+                    visit::walk_item(this, item);
+                    this.extern_mod = old_item;
+                });
                 return; // Avoid visiting again.
             }
             ItemKind::Enum(def, _) => {
@@ -1161,6 +1199,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
     fn visit_foreign_item(&mut self, fi: &'a ForeignItem) {
         match &fi.kind {
             ForeignItemKind::Fn(box Fn { defaultness, sig, body, .. }) => {
+                self.check_foreign_item_safety(fi.span, sig.header.safety);
                 self.check_defaultness(fi.span, *defaultness);
                 self.check_foreign_fn_bodyless(fi.ident, body.as_deref());
                 self.check_foreign_fn_headerless(sig.header);
@@ -1180,7 +1219,8 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                 self.check_foreign_ty_genericless(generics, where_clauses);
                 self.check_foreign_item_ascii_only(fi.ident);
             }
-            ForeignItemKind::Static(box StaticForeignItem { ty: _, mutability: _, expr }) => {
+            ForeignItemKind::Static(box StaticForeignItem { expr, safety, .. }) => {
+                self.check_foreign_item_safety(fi.span, *safety);
                 self.check_foreign_kind_bodyless(fi.ident, "static", expr.as_ref().map(|b| b.span));
                 self.check_foreign_item_ascii_only(fi.ident);
             }
@@ -1736,6 +1776,7 @@ pub fn check_crate(
         outer_impl_trait: None,
         disallow_tilde_const: Some(DisallowTildeConstContext::Item),
         is_impl_trait_banned: false,
+        extern_mod_safety: None,
         lint_buffer: lints,
     };
     visit::walk_crate(&mut validator, krate);

compiler/rustc_ast_passes/src/errors.rs

@@ -216,6 +216,15 @@ pub enum ExternBlockSuggestion {
     },
 }
 
+#[derive(Diagnostic)]
+#[diag(ast_passes_extern_invalid_safety)]
+pub struct InvalidSafetyOnExtern {
+    #[primary_span]
+    pub item_span: Span,
+    #[suggestion(code = "", applicability = "maybe-incorrect")]
+    pub block: Span,
+}
+
 #[derive(Diagnostic)]
 #[diag(ast_passes_bound_in_context)]
 pub struct BoundInContext<'a> {
@@ -485,6 +494,13 @@ pub struct UnsafeItem {
     pub kind: &'static str,
 }
 
+#[derive(Diagnostic)]
+#[diag(ast_passes_missing_unsafe_on_extern)]
+pub struct MissingUnsafeOnExtern {
+    #[primary_span]
+    pub span: Span,
+}
+
 #[derive(Diagnostic)]
 #[diag(ast_passes_fieldless_union)]
 pub struct FieldlessUnion {