Deny unsafe on more builtin attributes

carbotaniuman · carbotaniuman · commit d8bc8761a5bb · 2024-07-29T21:00:09.000-05:00
diff --git a/compiler/rustc_builtin_macros/src/cfg_accessible.rs b/compiler/rustc_builtin_macros/src/cfg_accessible.rs
@@ -47,11 +47,13 @@ impl MultiItemModifier for Expander {
     ) -> ExpandResult<Vec<Annotatable>, Annotatable> {
         let template = AttributeTemplate { list: Some("path"), ..Default::default() };
         validate_attr::check_builtin_meta_item(
+            &ecx.ecfg.features,
             &ecx.sess.psess,
             meta_item,
             ast::AttrStyle::Outer,
             sym::cfg_accessible,
             template,
+            true,
         );
 
         let Some(path) = validate_input(ecx, meta_item) else {
diff --git a/compiler/rustc_builtin_macros/src/derive.rs b/compiler/rustc_builtin_macros/src/derive.rs
@@ -38,11 +38,13 @@ impl MultiItemModifier for Expander {
                 let template =
                     AttributeTemplate { list: Some("Trait1, Trait2, ..."), ..Default::default() };
                 validate_attr::check_builtin_meta_item(
+                    features,
                     &sess.psess,
                     meta_item,
                     ast::AttrStyle::Outer,
                     sym::derive,
                     template,
+                    true,
                 );
 
                 let mut resolutions = match &meta_item.kind {
diff --git a/compiler/rustc_builtin_macros/src/util.rs b/compiler/rustc_builtin_macros/src/util.rs
@@ -17,11 +17,13 @@ pub(crate) fn check_builtin_macro_attribute(ecx: &ExtCtxt<'_>, meta_item: &MetaI
     // All the built-in macro attributes are "words" at the moment.
     let template = AttributeTemplate { word: true, ..Default::default() };
     validate_attr::check_builtin_meta_item(
+        &ecx.ecfg.features,
         &ecx.sess.psess,
         meta_item,
         AttrStyle::Outer,
         name,
         template,
+        true,
     );
 }
 
diff --git a/compiler/rustc_expand/src/config.rs b/compiler/rustc_expand/src/config.rs
@@ -8,7 +8,7 @@ use rustc_ast::tokenstream::{
 use rustc_ast::{self as ast, AttrStyle, Attribute, HasAttrs, HasTokens, MetaItem, NodeId};
 use rustc_attr as attr;
 use rustc_data_structures::flat_map_in_place::FlatMapInPlace;
-use rustc_feature::{Features, ACCEPTED_FEATURES, REMOVED_FEATURES, UNSTABLE_FEATURES};
+use rustc_feature::{AttributeSafety, Features, ACCEPTED_FEATURES, REMOVED_FEATURES, UNSTABLE_FEATURES};
 use rustc_lint_defs::BuiltinLintDiag;
 use rustc_parse::validate_attr;
 use rustc_session::parse::feature_err;
@@ -263,6 +263,13 @@ impl<'a> StripUnconfigured<'a> {
     /// is in the original source file. Gives a compiler error if the syntax of
     /// the attribute is incorrect.
     pub(crate) fn expand_cfg_attr(&self, cfg_attr: &Attribute, recursive: bool) -> Vec<Attribute> {
+        validate_attr::check_attribute_safety(
+            self.features.unwrap_or(&Features::default()),
+            &self.sess.psess,
+            AttributeSafety::Normal,
+            &cfg_attr,
+        );
+
         let Some((cfg_predicate, expanded_attrs)) =
             rustc_parse::parse_cfg_attr(cfg_attr, &self.sess.psess)
         else {
@@ -385,6 +392,13 @@ impl<'a> StripUnconfigured<'a> {
                 return (true, None);
             }
         };
+
+        validate_attr::deny_builtin_meta_unsafety(
+            self.features.unwrap_or(&Features::default()),
+            &self.sess.psess,
+            &meta_item,
+        );
+
         (
             parse_cfg(&meta_item, self.sess).map_or(true, |meta_item| {
                 attr::cfg_matches(meta_item, &self.sess, self.lint_node_id, self.features)
diff --git 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 {
+        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(
diff --git a/tests/ui/attributes/unsafe/derive-unsafe-attributes.rs b/tests/ui/attributes/unsafe/derive-unsafe-attributes.rs
@@ -3,4 +3,7 @@
 #[derive(unsafe(Debug))] //~ ERROR: traits in `#[derive(...)]` don't accept `unsafe(...)`
 struct Foo;
 
+#[unsafe(derive(Debug))] //~ ERROR: is not an unsafe attribute
+struct Bar;
+
 fn main() {}
diff --git a/tests/ui/attributes/unsafe/derive-unsafe-attributes.stderr b/tests/ui/attributes/unsafe/derive-unsafe-attributes.stderr
@@ -4,5 +4,13 @@ error: traits in `#[derive(...)]` don't accept `unsafe(...)`
 LL | #[derive(unsafe(Debug))]
    |          ^^^^^^
 
-error: aborting due to 1 previous error
+error: `derive` is not an unsafe attribute
+  --> $DIR/derive-unsafe-attributes.rs:6:3
+   |
+LL | #[unsafe(derive(Debug))]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: aborting due to 2 previous errors
 
diff --git a/tests/ui/attributes/unsafe/extraneous-unsafe-attributes.rs b/tests/ui/attributes/unsafe/extraneous-unsafe-attributes.rs
@@ -0,0 +1,31 @@
+//@ edition: 2024
+//@ compile-flags: -Zunstable-options
+#![feature(unsafe_attributes)]
+
+#[unsafe(cfg(any()))] //~ ERROR: is not an unsafe attribute
+fn a() {}
+
+#[unsafe(cfg_attr(any(), allow(dead_code)))] //~ ERROR: is not an unsafe attribute
+fn b() {}
+
+#[unsafe(test)] //~ ERROR: is not an unsafe attribute
+fn aa() {}
+
+#[unsafe(ignore = "test")] //~ ERROR: is not an unsafe attribute
+fn bb() {}
+
+#[unsafe(should_panic(expected = "test"))] //~ ERROR: is not an unsafe attribute
+fn cc() {}
+
+#[unsafe(macro_use)] //~ ERROR: is not an unsafe attribute
+mod inner {
+    #[unsafe(macro_export)] //~ ERROR: is not an unsafe attribute
+    macro_rules! m {
+        () => {};
+    }
+}
+
+#[unsafe(used)] //~ ERROR: is not an unsafe attribute
+static FOO: usize = 0;
+
+fn main() {}
diff --git a/tests/ui/attributes/unsafe/extraneous-unsafe-attributes.stderr b/tests/ui/attributes/unsafe/extraneous-unsafe-attributes.stderr
@@ -0,0 +1,66 @@
+error: `cfg` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:5:3
+   |
+LL | #[unsafe(cfg(any()))]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: `cfg_attr` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:8:3
+   |
+LL | #[unsafe(cfg_attr(any(), allow(dead_code)))]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: `test` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:11:3
+   |
+LL | #[unsafe(test)]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: `ignore` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:14:3
+   |
+LL | #[unsafe(ignore = "test")]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: `should_panic` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:17:3
+   |
+LL | #[unsafe(should_panic(expected = "test"))]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: `macro_use` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:20:3
+   |
+LL | #[unsafe(macro_use)]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: `macro_export` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:22:7
+   |
+LL |     #[unsafe(macro_export)]
+   |       ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: `used` is not an unsafe attribute
+  --> $DIR/extraneous-unsafe-attributes.rs:28:3
+   |
+LL | #[unsafe(used)]
+   |   ^^^^^^
+   |
+   = note: extraneous unsafe is not allowed in attributes
+
+error: aborting due to 8 previous errors
+
diff --git a/tests/ui/attributes/unsafe/proc-unsafe-attributes.rs b/tests/ui/attributes/unsafe/proc-unsafe-attributes.rs
diff --git a/tests/ui/attributes/unsafe/proc-unsafe-attributes.stderr b/tests/ui/attributes/unsafe/proc-unsafe-attributes.stderr
