Skip to content
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.

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

Fix span of unsafe attribute diagnostic #133270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 21, 2024

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 #132908

@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 21, 2024
@@ -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

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.

/// 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad span in suggestion for unsafe_attr_outside_unsafe in macro expansion with cfg_attr
4 participants