Auto merge of #127543 - carbotaniuman:more_unsafe_attr_verification, r=estebank,traviscross

More unsafe attr verification

This code denies unsafe on attributes such as `#[test]` and `#[ignore]`, while also changing the `MetaItem` parsing so `unsafe` in args like `#[allow(unsafe(dead_code))]` is not accidentally allowed.

Tracking:

- https://github.com/rust-lang/rust/issues/123757

3 files changed, 110 insertions, 60 deletions

diff --git a/compiler/rustc_parse/src/errors.rs b/compiler/rustc_parse/src/errors.rs
index c5e7e0df631..99f2f26b8cb 100644
--- a/compiler/rustc_parse/src/errors.rs
+++ b/compiler/rustc_parse/src/errors.rs
@@ -3183,6 +3183,7 @@ pub(crate) struct DotDotRangeAttribute {
 #[note]
 pub struct InvalidAttrUnsafe {
     #[primary_span]
+    #[label]
     pub span: Span,
     pub name: Path,
 }
diff --git a/compiler/rustc_parse/src/parser/attr.rs b/compiler/rustc_parse/src/parser/attr.rs
index 12b9414d1f7..f34ef071e21 100644
--- a/compiler/rustc_parse/src/parser/attr.rs
+++ b/compiler/rustc_parse/src/parser/attr.rs
@@ -31,6 +31,12 @@ enum OuterAttributeType {
     Attribute,
 }
 
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub enum AllowLeadingUnsafe {
+    Yes,
+    No,
+}
+
 impl<'a> Parser<'a> {
     /// Parses attributes that appear before an item.
     pub(super) fn parse_outer_attributes(&mut self) -> PResult<'a, AttrWrapper> {
@@ -332,7 +338,7 @@ impl<'a> Parser<'a> {
 
     /// Parses `cfg_attr(pred, attr_item_list)` where `attr_item_list` is comma-delimited.
     pub fn parse_cfg_attr(&mut self) -> PResult<'a, (ast::MetaItem, Vec<(ast::AttrItem, Span)>)> {
-        let cfg_predicate = self.parse_meta_item()?;
+        let cfg_predicate = self.parse_meta_item(AllowLeadingUnsafe::No)?;
         self.expect(&token::Comma)?;
 
         // Presumably, the majority of the time there will only be one attr.
@@ -368,7 +374,10 @@ impl<'a> Parser<'a> {
     /// MetaItem = SimplePath ( '=' UNSUFFIXED_LIT | '(' MetaSeq? ')' )? ;
     /// MetaSeq = MetaItemInner (',' MetaItemInner)* ','? ;
     /// ```
-    pub fn parse_meta_item(&mut self) -> PResult<'a, ast::MetaItem> {
+    pub fn parse_meta_item(
+        &mut self,
+        unsafe_allowed: AllowLeadingUnsafe,
+    ) -> PResult<'a, ast::MetaItem> {
         // We can't use `maybe_whole` here because it would bump in the `None`
         // case, which we don't want.
         if let token::Interpolated(nt) = &self.token.kind
@@ -384,7 +393,11 @@ impl<'a> Parser<'a> {
         }
 
         let lo = self.token.span;
-        let is_unsafe = self.eat_keyword(kw::Unsafe);
+        let is_unsafe = if unsafe_allowed == AllowLeadingUnsafe::Yes {
+            self.eat_keyword(kw::Unsafe)
+        } else {
+            false
+        };
         let unsafety = if is_unsafe {
             let unsafe_span = self.prev_token.span;
             self.psess.gated_spans.gate(sym::unsafe_attributes, unsafe_span);
@@ -427,7 +440,7 @@ impl<'a> Parser<'a> {
             Err(err) => err.cancel(), // we provide a better error below
         }
 
-        match self.parse_meta_item() {
+        match self.parse_meta_item(AllowLeadingUnsafe::No) {
             Ok(mi) => return Ok(ast::NestedMetaItem::MetaItem(mi)),
             Err(err) => err.cancel(), // we provide a better error below
         }
diff --git a/compiler/rustc_parse/src/validate_attr.rs b/compiler/rustc_parse/src/validate_attr.rs
index 356bc9a410d..a64c00f3b6c 100644
--- a/compiler/rustc_parse/src/validate_attr.rs
+++ b/compiler/rustc_parse/src/validate_attr.rs
@@ -26,76 +26,35 @@ pub fn check_attr(features: &Features, psess: &ParseSess, attr: &Attribute) {
     let attr_info = attr.ident().and_then(|ident| BUILTIN_ATTRIBUTE_MAP.get(&ident.name));
     let attr_item = attr.get_normal_item();
 
-    let is_unsafe_attr = attr_info.is_some_and(|attr| attr.safety == AttributeSafety::Unsafe);
-
-    if features.unsafe_attributes {
-        if is_unsafe_attr {
-            if let ast::Safety::Default = attr_item.unsafety {
-                let path_span = attr_item.path.span;
-
-                // If the `attr_item`'s span is not from a macro, then just suggest
-                // wrapping it in `unsafe(...)`. Otherwise, we suggest putting the
-                // `unsafe(`, `)` right after and right before the opening and closing
-                // square bracket respectively.
-                let diag_span = if attr_item.span().can_be_used_for_suggestions() {
-                    attr_item.span()
-                } else {
-                    attr.span
-                        .with_lo(attr.span.lo() + BytePos(2))
-                        .with_hi(attr.span.hi() - BytePos(1))
-                };
-
-                if attr.span.at_least_rust_2024() {
-                    psess.dcx().emit_err(errors::UnsafeAttrOutsideUnsafe {
-                        span: path_span,
-                        suggestion: errors::UnsafeAttrOutsideUnsafeSuggestion {
-                            left: diag_span.shrink_to_lo(),
-                            right: diag_span.shrink_to_hi(),
-                        },
-                    });
-                } else {
-                    psess.buffer_lint(
-                        UNSAFE_ATTR_OUTSIDE_UNSAFE,
-                        path_span,
-                        ast::CRATE_NODE_ID,
-                        BuiltinLintDiag::UnsafeAttrOutsideUnsafe {
-                            attribute_name_span: path_span,
-                            sugg_spans: (diag_span.shrink_to_lo(), diag_span.shrink_to_hi()),
-                        },
-                    );
-                }
-            }
-        } else {
-            if let Safety::Unsafe(unsafe_span) = attr_item.unsafety {
-                psess.dcx().emit_err(errors::InvalidAttrUnsafe {
-                    span: unsafe_span,
-                    name: attr_item.path.clone(),
-                });
-            }
-        }
-    }
+    // All non-builtin attributes are considered safe
+    let safety = attr_info.map(|x| x.safety).unwrap_or(AttributeSafety::Normal);
+    check_attribute_safety(features, psess, safety, attr);
 
     // Check input tokens for built-in and key-value attributes.
     match attr_info {
         // `rustc_dummy` doesn't have any restrictions specific to built-in attributes.
         Some(BuiltinAttribute { name, template, .. }) if *name != sym::rustc_dummy => {
             match parse_meta(psess, attr) {
-                Ok(meta) => check_builtin_meta_item(psess, &meta, attr.style, *name, *template),
+                // Don't check safety again, we just did that
+                Ok(meta) => check_builtin_meta_item(
+                    features, psess, &meta, attr.style, *name, *template, false,
+                ),
                 Err(err) => {
                     err.emit();
                 }
             }
         }
-        _ if let AttrArgs::Eq(..) = attr_item.args => {
-            // All key-value attributes are restricted to meta-item syntax.
-            match parse_meta(psess, attr) {
-                Ok(_) => {}
-                Err(err) => {
-                    err.emit();
+        _ => {
+            if let AttrArgs::Eq(..) = attr_item.args {
+                // All key-value attributes are restricted to meta-item syntax.
+                match parse_meta(psess, attr) {
+                    Ok(_) => {}
+                    Err(err) => {
+                        err.emit();
+                    }
                 }
             }
         }
-        _ => {}
     }
 }
 
@@ -198,12 +157,85 @@ fn is_attr_template_compatible(template: &AttributeTemplate, meta: &ast::MetaIte
     }
 }
 
+pub fn check_attribute_safety(
+    features: &Features,
+    psess: &ParseSess,
+    safety: AttributeSafety,
+    attr: &Attribute,
+) {
+    if !features.unsafe_attributes {
+        return;
+    }
+
+    let attr_item = attr.get_normal_item();
+
+    if safety == AttributeSafety::Unsafe {
+        if let ast::Safety::Default = attr_item.unsafety {
+            let path_span = attr_item.path.span;
+
+            // If the `attr_item`'s span is not from a macro, then just suggest
+            // wrapping it in `unsafe(...)`. Otherwise, we suggest putting the
+            // `unsafe(`, `)` right after and right before the opening and closing
+            // square bracket respectively.
+            let diag_span = if attr_item.span().can_be_used_for_suggestions() {
+                attr_item.span()
+            } else {
+                attr.span.with_lo(attr.span.lo() + BytePos(2)).with_hi(attr.span.hi() - BytePos(1))
+            };
+
+            if attr.span.at_least_rust_2024() {
+                psess.dcx().emit_err(errors::UnsafeAttrOutsideUnsafe {
+                    span: path_span,
+                    suggestion: errors::UnsafeAttrOutsideUnsafeSuggestion {
+                        left: diag_span.shrink_to_lo(),
+                        right: diag_span.shrink_to_hi(),
+                    },
+                });
+            } else {
+                psess.buffer_lint(
+                    UNSAFE_ATTR_OUTSIDE_UNSAFE,
+                    path_span,
+                    ast::CRATE_NODE_ID,
+                    BuiltinLintDiag::UnsafeAttrOutsideUnsafe {
+                        attribute_name_span: path_span,
+                        sugg_spans: (diag_span.shrink_to_lo(), diag_span.shrink_to_hi()),
+                    },
+                );
+            }
+        }
+    } else {
+        if let Safety::Unsafe(unsafe_span) = attr_item.unsafety {
+            psess.dcx().emit_err(errors::InvalidAttrUnsafe {
+                span: unsafe_span,
+                name: attr_item.path.clone(),
+            });
+        }
+    }
+}
+
+// Called by `check_builtin_meta_item` and code that manually denies
+// `unsafe(...)` in `cfg`
+pub fn deny_builtin_meta_unsafety(features: &Features, psess: &ParseSess, meta: &MetaItem) {
+    // This only supports denying unsafety right now - making builtin attributes
+    // support unsafety will requite us to thread the actual `Attribute` through
+    // for the nice diagnostics.
+    if features.unsafe_attributes {
+        if let Safety::Unsafe(unsafe_span) = meta.unsafety {
+            psess
+                .dcx()
+                .emit_err(errors::InvalidAttrUnsafe { span: unsafe_span, name: meta.path.clone() });
+        }
+    }
+}
+
 pub fn check_builtin_meta_item(
+    features: &Features,
     psess: &ParseSess,
     meta: &MetaItem,
     style: ast::AttrStyle,
     name: Symbol,
     template: AttributeTemplate,
+    deny_unsafety: bool,
 ) {
     // Some special attributes like `cfg` must be checked
     // before the generic check, so we skip them here.
@@ -212,6 +244,10 @@ pub fn check_builtin_meta_item(
     if !should_skip(name) && !is_attr_template_compatible(&template, &meta.kind) {
         emit_malformed_attribute(psess, style, meta.span, name, template);
     }
+
+    if deny_unsafety {
+        deny_builtin_meta_unsafety(features, psess, meta);
+    }
 }
 
 fn emit_malformed_attribute(