Skip to content

Fix span of unsafe attribute diagnostic #133270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix span of unsafe attribute diagnostic #133270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions compiler/rustc_ast/src/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2966,6 +2966,7 @@ impl NormalAttr {
path: Path::from_ident(ident),
args: AttrArgs::Empty,
tokens: None,
span: DUMMY_SP,
},
tokens: None,
}
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increasing the size of AttrItem may be a performance sensitive change.

}

impl AttrItem {
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_ast/src/attr/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

@compiler-errors compiler-errors Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we can't save this by using one of the span ancestor functions like https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/span_encoding/struct.Span.html#method.find_ancestor_in_same_ctxt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another instance of the span combining problem really.
The x.to(y) operation should just work for things like #[$a = $b] if all metavar spans are recorded.
In fact the parser does exactly the same a.to(b) operation to save the span into the AST, it just may see different spans if nonterminals are involved.

The logic in validate_attrs may work or not work depending on how exactly parts of the attribute was passed (as tt, ident, meta or expr), previously it was fine-tuned for one scenarios, now it's fine tuned for others.

The only certain thing here is that the manual span arithmetic (+/- BytePos(1/2)) should not be performed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just remove the span arithmetic and not try to improve spans for macro scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a way to get find_ancestor_in_same_ctxt to work. The issue is that in the case of #[link_section = $s], the span for $s is not stored anywhere. The AttrItem stores the path as link_section (or $e in the other example, it is before expansion). The AttrArgs stores the eq_span for the = in the macro, and the value as "foo" from the invocation.

I think supporting proper suggestions in macros is pretty important, as this pattern comes up quite frequently. I realize this can only be tuned to support certain scenarios, and not be perfect in all situations. However, I think recording the span as implemented in this PR covers the vast majority of situations well.

Are you still concerned with the regressions here?

}

pub fn meta_item_list(&self) -> Option<ThinVec<MetaItemInner>> {
match &self.args {
AttrArgs::Delimited(args) if args.delim == Delimiter::Parenthesis => {
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_ast/src/mut_visit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,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);
Expand Down Expand Up @@ -827,7 +827,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);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,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,
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/visit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1242,7 +1242,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));
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_ast_lowering/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})),
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_builtin_macros/src/cmdline_attrs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -38,7 +36,7 @@ pub fn inject(krate: &mut ast::Crate, psess: &ParseSess, attrs: &[String]) {
unsafety,
path,
args,
start_span.to(end_span),
span,
));
}
}
4 changes: 3 additions & 1 deletion compiler/rustc_parse/src/parser/attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
))
Expand Down
14 changes: 2 additions & 12 deletions compiler/rustc_parse/src/validate_attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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,
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
27 changes: 22 additions & 5 deletions tests/ui/rust-2024/unsafe-attributes/unsafe-attributes-fix.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Loading