Auto merge of rust-lang#133270 - ehuss:fix-attr-item-span, r=<try>

Fix span of unsafe attribute diagnostic

This fixes the span of the `unsafe_attr_outside_unsafe` diagnostic when the attribute uses `cfg_attr` and comes from a macro. Previously the span it was pointing to was in the wrong place (offset by 2 bytes in the start, and 1 byte in the end), causing a corrupt suggestion.

The problem is that the lint was trying to do manual byte manipulation of the `Attribute` span to get within the `#[` and `]` tokens. However, when the attribute comes from `cfg_attr`, that span starts from the attribute path (like `no_mangle`), not the `#[` of the `cfg_attr`.

The solution here is to store the span of the `AttrItem` while parsing, so that we know for sure that it covers the correct range (the path and all args). We could not use `AttrItem::span()` (which is removed in this PR), because that function did not correctly account for the path and arguments coming from separate expansion contexts. For example, in the macro expansion of `#[$p = $a]`, the span would be `$p = ` because you are not allowed to generate a span across expansion contexts.

Fixes rust-lang#132908

Author: bors

compiler/rustc_ast/src/ast.rs

@@ -2966,6 +2966,7 @@ impl NormalAttr {
                 path: Path::from_ident(ident),
                 args: AttrArgs::Empty,
                 tokens: None,
+                span: DUMMY_SP,
             },
             tokens: None,
         }
@@ -2979,6 +2980,10 @@ pub struct AttrItem {
     pub args: AttrArgs,
     // Tokens for the meta item, e.g. just the `foo` within `#[foo]` or `#![foo]`.
     pub tokens: Option<LazyAttrTokenStream>,
+    /// The span of the contents of the attribute.
+    ///
+    /// This is the span starting from the path and ending at the end of the args.
+    pub span: Span,
 }
 
 impl AttrItem {

compiler/rustc_ast/src/attr/mod.rs

@@ -241,10 +241,6 @@ impl Attribute {
 }
 
 impl AttrItem {
-    pub fn span(&self) -> Span {
-        self.args.span().map_or(self.path.span, |args_span| self.path.span.to(args_span))
-    }
-
     pub fn meta_item_list(&self) -> Option<ThinVec<MetaItemInner>> {
         match &self.args {
             AttrArgs::Delimited(args) if args.delim == Delimiter::Parenthesis => {
@@ -633,7 +629,7 @@ pub fn mk_attr(
     args: AttrArgs,
     span: Span,
 ) -> Attribute {
-    mk_attr_from_item(g, AttrItem { unsafety, path, args, tokens: None }, None, style, span)
+    mk_attr_from_item(g, AttrItem { unsafety, path, args, tokens: None, span }, None, style, span)
 }
 
 pub fn mk_attr_from_item(

compiler/rustc_ast/src/mut_visit.rs

@@ -696,7 +696,7 @@ fn walk_attribute<T: MutVisitor>(vis: &mut T, attr: &mut Attribute) {
     match kind {
         AttrKind::Normal(normal) => {
             let NormalAttr {
-                item: AttrItem { unsafety: _, path, args, tokens },
+                item: AttrItem { unsafety: _, path, args, tokens, span: _ },
                 tokens: attr_tokens,
             } = &mut **normal;
             vis.visit_path(path);
@@ -883,7 +883,7 @@ fn visit_nonterminal<T: MutVisitor>(vis: &mut T, nt: &mut token::Nonterminal) {
         token::NtTy(ty) => vis.visit_ty(ty),
         token::NtLiteral(expr) => vis.visit_expr(expr),
         token::NtMeta(item) => {
-            let AttrItem { unsafety: _, path, args, tokens } = item.deref_mut();
+            let AttrItem { unsafety: _, path, args, tokens, span: _ } = item.deref_mut();
             vis.visit_path(path);
             visit_attr_args(vis, args);
             visit_lazy_tts(vis, tokens);

compiler/rustc_ast/src/token.rs

@@ -1161,7 +1161,7 @@ impl Nonterminal {
             NtPat(pat) => pat.span,
             NtExpr(expr) | NtLiteral(expr) => expr.span,
             NtTy(ty) => ty.span,
-            NtMeta(attr_item) => attr_item.span(),
+            NtMeta(attr_item) => attr_item.span,
             NtPath(path) => path.span,
             NtVis(vis) => vis.span,
         }

compiler/rustc_ast/src/visit.rs

@@ -1250,7 +1250,7 @@ pub fn walk_attribute<'a, V: Visitor<'a>>(visitor: &mut V, attr: &'a Attribute)
     match kind {
         AttrKind::Normal(normal) => {
             let NormalAttr { item, tokens: _ } = &**normal;
-            let AttrItem { unsafety: _, path, args, tokens: _ } = item;
+            let AttrItem { unsafety: _, path, args, tokens: _, span: _ } = item;
             try_visit!(visitor.visit_path(path, DUMMY_NODE_ID));
             try_visit!(walk_attr_args(visitor, args));
         }

compiler/rustc_ast_lowering/src/lib.rs

@@ -884,6 +884,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                     path: normal.item.path.clone(),
                     args: self.lower_attr_args(&normal.item.args),
                     tokens: None,
+                    span: normal.item.span,
                 },
                 tokens: None,
             })),

compiler/rustc_builtin_macros/src/cmdline_attrs.rs

@@ -17,18 +17,16 @@ pub fn inject(krate: &mut ast::Crate, psess: &ParseSess, attrs: &[String]) {
             raw_attr.clone(),
         ));
 
-        let start_span = parser.token.span;
-        let AttrItem { unsafety, path, args, tokens: _ } =
+        let AttrItem { unsafety, path, args, tokens: _, span } =
             match parser.parse_attr_item(ForceCollect::No) {
                 Ok(ai) => ai,
                 Err(err) => {
                     err.emit();
                     continue;
                 }
             };
-        let end_span = parser.token.span;
         if parser.token != token::Eof {
-            psess.dcx().emit_err(errors::InvalidCrateAttr { span: start_span.to(end_span) });
+            psess.dcx().emit_err(errors::InvalidCrateAttr { span });
             continue;
         }
 
@@ -38,7 +36,7 @@ pub fn inject(krate: &mut ast::Crate, psess: &ParseSess, attrs: &[String]) {
             unsafety,
             path,
             args,
-            start_span.to(end_span),
+            span,
         ));
     }
 }

compiler/rustc_parse/src/parser/attr.rs

@@ -276,6 +276,7 @@ impl<'a> Parser<'a> {
 
         // Attr items don't have attributes.
         self.collect_tokens(None, AttrWrapper::empty(), force_collect, |this, _empty_attrs| {
+            let lo = this.token.span;
             let is_unsafe = this.eat_keyword(kw::Unsafe);
             let unsafety = if is_unsafe {
                 let unsafe_span = this.prev_token.span;
@@ -290,8 +291,9 @@ impl<'a> Parser<'a> {
             if is_unsafe {
                 this.expect(&token::CloseDelim(Delimiter::Parenthesis))?;
             }
+            let span = lo.to(this.prev_token.span);
             Ok((
-                ast::AttrItem { unsafety, path, args, tokens: None },
+                ast::AttrItem { unsafety, path, args, tokens: None, span },
                 Trailing::No,
                 UsePreAttrPos::No,
             ))

compiler/rustc_parse/src/validate_attr.rs

@@ -12,7 +12,7 @@ use rustc_session::errors::report_lit_error;
 use rustc_session::lint::BuiltinLintDiag;
 use rustc_session::lint::builtin::{ILL_FORMED_ATTRIBUTE_INPUT, UNSAFE_ATTR_OUTSIDE_UNSAFE};
 use rustc_session::parse::ParseSess;
-use rustc_span::{BytePos, Span, Symbol, sym};
+use rustc_span::{Span, Symbol, sym};
 
 use crate::{errors, parse_in};
 
@@ -161,17 +161,7 @@ pub fn check_attribute_safety(psess: &ParseSess, safety: AttributeSafety, attr:
     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))
-            };
-
+            let diag_span = attr_item.span;
             if attr.span.at_least_rust_2024() {
                 psess.dcx().emit_err(errors::UnsafeAttrOutsideUnsafe {
                     span: path_span,

tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.fixed

@@ -40,6 +40,15 @@ macro_rules! meta2 {
     }
 }
 
+macro_rules! with_cfg_attr {
+    () => {
+        #[cfg_attr(all(), unsafe(link_section = ".custom_section"))]
+        //~^ ERROR: unsafe attribute used without unsafe
+        //~| WARN this is accepted in the current edition
+        pub extern "C" fn abc() {}
+    };
+}
+
 tt!([unsafe(no_mangle)]);
 //~^ ERROR: unsafe attribute used without unsafe
 //~| WARN this is accepted in the current edition
@@ -52,6 +61,8 @@ meta2!(unsafe(export_name = "baw"));
 //~| WARN this is accepted in the current edition
 ident2!(export_name, "bars");
 
+with_cfg_attr!();
+
 #[unsafe(no_mangle)]
 //~^ ERROR: unsafe attribute used without unsafe
 //~| WARN this is accepted in the current edition

tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.rs

@@ -40,6 +40,15 @@ macro_rules! meta2 {
     }
 }
 
+macro_rules! with_cfg_attr {
+    () => {
+        #[cfg_attr(all(), link_section = ".custom_section")]
+        //~^ ERROR: unsafe attribute used without unsafe
+        //~| WARN this is accepted in the current edition
+        pub extern "C" fn abc() {}
+    };
+}
+
 tt!([no_mangle]);
 //~^ ERROR: unsafe attribute used without unsafe
 //~| WARN this is accepted in the current edition
@@ -52,6 +61,8 @@ meta2!(export_name = "baw");
 //~| WARN this is accepted in the current edition
 ident2!(export_name, "bars");
 
+with_cfg_attr!();
+
 #[no_mangle]
 //~^ ERROR: unsafe attribute used without unsafe
 //~| WARN this is accepted in the current edition

tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.stderr

@@ -1,5 +1,5 @@
 error: unsafe attribute used without unsafe
-  --> $DIR/unsafe-attributes-fix.rs:43:6
+  --> $DIR/unsafe-attributes-fix.rs:52:6
    |
 LL | tt!([no_mangle]);
    |      ^^^^^^^^^ usage of unsafe attribute
@@ -34,7 +34,7 @@ LL |         #[unsafe($e)]
    |           +++++++  +
 
 error: unsafe attribute used without unsafe
-  --> $DIR/unsafe-attributes-fix.rs:47:7
+  --> $DIR/unsafe-attributes-fix.rs:56:7
    |
 LL | meta!(no_mangle);
    |       ^^^^^^^^^ usage of unsafe attribute
@@ -47,7 +47,7 @@ LL | meta!(unsafe(no_mangle));
    |       +++++++         +
 
 error: unsafe attribute used without unsafe
-  --> $DIR/unsafe-attributes-fix.rs:50:8
+  --> $DIR/unsafe-attributes-fix.rs:59:8
    |
 LL | meta2!(export_name = "baw");
    |        ^^^^^^^^^^^ usage of unsafe attribute
@@ -77,7 +77,24 @@ LL |         #[unsafe($e = $l)]
    |           +++++++       +
 
 error: unsafe attribute used without unsafe
-  --> $DIR/unsafe-attributes-fix.rs:55:3
+  --> $DIR/unsafe-attributes-fix.rs:45:27
+   |
+LL |         #[cfg_attr(all(), link_section = ".custom_section")]
+   |                           ^^^^^^^^^^^^ usage of unsafe attribute
+...
+LL | with_cfg_attr!();
+   | ---------------- in this macro invocation
+   |
+   = warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2024!
+   = note: for more information, see issue #123757 <https://github.com/rust-lang/rust/issues/123757>
+   = note: this error originates in the macro `with_cfg_attr` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: wrap the attribute in `unsafe(...)`
+   |
+LL |         #[cfg_attr(all(), unsafe(link_section = ".custom_section"))]
+   |                           +++++++                                +
+
+error: unsafe attribute used without unsafe
+  --> $DIR/unsafe-attributes-fix.rs:66:3
    |
 LL | #[no_mangle]
    |   ^^^^^^^^^ usage of unsafe attribute
@@ -89,5 +106,5 @@ help: wrap the attribute in `unsafe(...)`
 LL | #[unsafe(no_mangle)]
    |   +++++++         +
 
-error: aborting due to 6 previous errors
+error: aborting due to 7 previous errors