From d8bc8761a5bb8456c0b7940434b3b3b4f0ed035c Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Tue, 2 Jul 2024 23:52:16 -0500 Subject: [PATCH] Deny unsafe on more builtin attributes --- .../src/cfg_accessible.rs | 2 + compiler/rustc_builtin_macros/src/derive.rs | 2 + compiler/rustc_builtin_macros/src/util.rs | 2 + compiler/rustc_expand/src/config.rs | 16 +- compiler/rustc_parse/src/validate_attr.rs | 148 +++++++++++------- .../unsafe/derive-unsafe-attributes.rs | 3 + .../unsafe/derive-unsafe-attributes.stderr | 10 +- .../unsafe/extraneous-unsafe-attributes.rs | 31 ++++ .../extraneous-unsafe-attributes.stderr | 66 ++++++++ .../unsafe/proc-unsafe-attributes.rs | 27 ++++ .../unsafe/proc-unsafe-attributes.stderr | 58 +++++++ 11 files changed, 307 insertions(+), 58 deletions(-) create mode 100644 tests/ui/attributes/unsafe/extraneous-unsafe-attributes.rs create mode 100644 tests/ui/attributes/unsafe/extraneous-unsafe-attributes.stderr create mode 100644 tests/ui/attributes/unsafe/proc-unsafe-attributes.rs create mode 100644 tests/ui/attributes/unsafe/proc-unsafe-attributes.stderr diff --git a/compiler/rustc_builtin_macros/src/cfg_accessible.rs b/compiler/rustc_builtin_macros/src/cfg_accessible.rs index 2d4a93776bb..006b6aa823f 100644 --- 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, 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 index 03970a48638..c540ff4e1de 100644 --- 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 index 65b50736c55..73cc8ff547d 100644 --- 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 index 756290be0a8..d46ec7a9ac0 100644 --- 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 { + 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 index 356bc9a410d..af2db171840 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 { + 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 index 774ce86c096..7f0d6a0cf8b 100644 --- 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 index fc0daf16790..22010c61b5f 100644 --- 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 new file mode 100644 index 00000000000..0181add843b --- /dev/null +++ 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 new file mode 100644 index 00000000000..a2e56087d16 --- /dev/null +++ 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 new file mode 100644 index 00000000000..caf6c6dc8ff --- /dev/null +++ b/tests/ui/attributes/unsafe/proc-unsafe-attributes.rs @@ -0,0 +1,27 @@ +#![feature(unsafe_attributes)] + +#[unsafe(proc_macro)] +//~^ ERROR: is not an unsafe attribute +//~| ERROR attribute is only usable with crates of the `proc-macro` crate type +pub fn a() {} + + +#[unsafe(proc_macro_derive(Foo))] +//~^ ERROR: is not an unsafe attribute +//~| ERROR attribute is only usable with crates of the `proc-macro` crate type +pub fn b() {} + +#[proc_macro_derive(unsafe(Foo))] +//~^ ERROR attribute is only usable with crates of the `proc-macro` crate type +pub fn c() {} + +#[unsafe(proc_macro_attribute)] +//~^ ERROR: is not an unsafe attribute +//~| ERROR attribute is only usable with crates of the `proc-macro` crate type +pub fn d() {} + +#[unsafe(allow(dead_code))] +//~^ ERROR: is not an unsafe attribute +pub fn e() {} + +fn main() {} diff --git a/tests/ui/attributes/unsafe/proc-unsafe-attributes.stderr b/tests/ui/attributes/unsafe/proc-unsafe-attributes.stderr new file mode 100644 index 00000000000..346d2c2e9ae --- /dev/null +++ b/tests/ui/attributes/unsafe/proc-unsafe-attributes.stderr @@ -0,0 +1,58 @@ +error: `proc_macro` is not an unsafe attribute + --> $DIR/proc-unsafe-attributes.rs:3:3 + | +LL | #[unsafe(proc_macro)] + | ^^^^^^ + | + = note: extraneous unsafe is not allowed in attributes + +error: `proc_macro_derive` is not an unsafe attribute + --> $DIR/proc-unsafe-attributes.rs:9:3 + | +LL | #[unsafe(proc_macro_derive(Foo))] + | ^^^^^^ + | + = note: extraneous unsafe is not allowed in attributes + +error: `proc_macro_attribute` is not an unsafe attribute + --> $DIR/proc-unsafe-attributes.rs:18:3 + | +LL | #[unsafe(proc_macro_attribute)] + | ^^^^^^ + | + = note: extraneous unsafe is not allowed in attributes + +error: `allow` is not an unsafe attribute + --> $DIR/proc-unsafe-attributes.rs:23:3 + | +LL | #[unsafe(allow(dead_code))] + | ^^^^^^ + | + = note: extraneous unsafe is not allowed in attributes + +error: the `#[proc_macro]` attribute is only usable with crates of the `proc-macro` crate type + --> $DIR/proc-unsafe-attributes.rs:3:1 + | +LL | #[unsafe(proc_macro)] + | ^^^^^^^^^^^^^^^^^^^^^ + +error: the `#[proc_macro_derive]` attribute is only usable with crates of the `proc-macro` crate type + --> $DIR/proc-unsafe-attributes.rs:9:1 + | +LL | #[unsafe(proc_macro_derive(Foo))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: the `#[proc_macro_derive]` attribute is only usable with crates of the `proc-macro` crate type + --> $DIR/proc-unsafe-attributes.rs:14:1 + | +LL | #[proc_macro_derive(unsafe(Foo))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: the `#[proc_macro_attribute]` attribute is only usable with crates of the `proc-macro` crate type + --> $DIR/proc-unsafe-attributes.rs:18:1 + | +LL | #[unsafe(proc_macro_attribute)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 8 previous errors +