Parse unsafe attributes #124214

carbotaniuman · 2024-04-21T00:33:32Z

Initial parse implementation for #123757

This is the initial work to parse unsafe attributes, which is represented as an extra unsafety field in MetaItem and AttrItem. There's two areas in the code where it appears that parsing is done manually and not using the parser stuff, and I'm not sure how I'm supposed to thread the change there.

rustbot · 2024-04-21T00:33:40Z

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
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

bors · 2024-04-21T01:33:09Z

☔ The latest upstream changes (presumably #124208) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-04-25T09:02:48Z

☔ The latest upstream changes (presumably #124360) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_feature/src/builtin_attrs.rs

compiler/rustc_feature/src/unstable.rs

michaelwoerister · 2024-04-25T12:25:45Z

Thanks for the PR, @carbotaniuman!

Would you mind also adding a test case where using unsafe(..) works as expected and one where it is used in a nested context, like #[cfg_attr(foo, unsafe(no_mangle)]?

bors · 2024-04-27T03:18:10Z

☔ The latest upstream changes (presumably #124424) made this pull request unmergeable. Please resolve the merge conflicts.

carbotaniuman · 2024-04-28T20:43:57Z

I've added stuff to handle the $meta matcher in macro_rules!. I'm not sure if it's needed, but it's technically breaking, and RFC 3531 suggests I do.

michaelwoerister · 2024-05-02T09:13:35Z

compiler/rustc_ast/src/token.rs

@@ -911,7 +912,7 @@ impl NonterminalKind {
            sym::ident => NonterminalKind::Ident,
            sym::lifetime => NonterminalKind::Lifetime,
            sym::literal => NonterminalKind::Literal,
-            sym::meta => NonterminalKind::Meta,
+            sym::meta => NonterminalKind::Meta2021,


Is this actually needed? The grammar doesn't change, right?

I'm not sure...

https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=5ccbe0db996ad629df3df4dc2119e2cd

#[cfg_attr(not(debug), unsafe(Foo))] struct Foo;

The above is a parse error right now as it expects a path and not a keyword. There were 2 implementations strategies I considered, one of which was defining a macro unsafe that created the Attribute with the proper unsafety, but given that I would still need syntax changes (to deal with the unsafe), I opted to instead change the core AttrItem itself to have an unsafety field. So I'm not really sure if a) this is the right way of doing things and b) if I need to make the macro changes.

michaelwoerister · 2024-05-02T09:52:07Z

compiler/rustc_parse/src/parser/attr.rs

+                ast::Unsafe::No
+            };
+
+            let path = this.parse_path(PathStyle::Mod)?;


I expect this to result in a slightly weird error message when doing something like #[unsafe(unsafe(no_mangle))]. Can we add a test case for that?

error: expected identifier, found keyword `unsafe` --> /home/quydx/code/rust/tests/ui/attributes/unsafe/double-unsafe-in-attribute.rs:1:10 | LL | #[unsafe(unsafe(no_mangle))] | ^^^^^^ expected identifier, found keyword | help: escape `unsafe` to use it as an identifier | LL | #[unsafe(r#unsafe(no_mangle))] | ++ error: cannot find attribute `r#unsafe` in this scope --> /home/quydx/code/rust/tests/ui/attributes/unsafe/double-unsafe-in-attribute.rs:1:10 | LL | #[unsafe(unsafe(no_mangle))] | ^^^^^^

The error is basically just what we have now. I can probably add a note saying that unsafe(...) doesn't nest.

compiler/rustc_parse/src/parser/nonterminal.rs

michaelwoerister · 2024-05-02T10:17:21Z

compiler/rustc_parse/src/parser/nonterminal.rs

@@ -170,6 +175,7 @@ impl<'a> Parser<'a> {
                NtPath(P(self.collect_tokens_no_attrs(|this| this.parse_path(PathStyle::Type))?))
            }
            NonterminalKind::Meta => NtMeta(P(self.parse_attr_item(true)?)),
+            NonterminalKind::Meta2021 => NtMeta(P(self.parse_attr_item_no_unsafe(true)?)),


Again, I'm not sure this is necessary because using unsafe(...) currently fails to parse (see playground example). So I think this should fall under the following paragraph from RFC 3531:

In cases where we’re adding new syntax and updating the grammar to include that new syntax, if we can update the corresponding fragment specifier simultaneously to match the new grammar in such a way that we do not risk changing the behavior of existing macros (i.e., because the new syntax previously would have failed parsing), then we will do that so as to prevent or minimize divergence between the fragment specifier and the new grammar.

I.e. the question is: Can the addition of unsafe(..) change the behavior of any currently valid macros / macro invocations?

michaelwoerister · 2024-05-02T10:53:36Z

I think we need some expert input. There are two main questions:

The PR's current implementation strategy for unsafe attributes is to add an unsafety field to AttrItem and MetaItem and handled that during parsing. I.e. #[unsafe(Something)] will end up as a flat Something AttrItem as opposed to an unsafe AttrItem with Something nested as an argument. Given that the RFC says that unsafe attributes cannot be nested or complex, that implementation strategy makes sense to me. But I'm no parsing/macros expert so it would be good to get additional opinions here.
The PR currently introduces a new meta_2021 fragment specifier to account for the unsafe attributes changes (see RFC 3531 - Macro matcher fragment specifiers edition policy). However, I think that is not necessary since #[unsafe(something)] does not currently parse (see playground example). Therefore starting to allow it for the existing meta fragment specifier should not be a breaking change and does not need to happen on an edition boundary. Does this reasoning overlook something?

@rustbot ping parser

rustbot · 2024-05-02T10:53:39Z

Error: This team (parser) cannot be pinged via this command; it may need to be added to triagebot.toml on the default branch.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

michaelwoerister · 2024-05-02T10:57:47Z

It looks like rustbot does not allow pinging adhoc groups, so pinging manually regarding the questions in the comment above: @compiler-errors @davidtwco @estebank @nnethercote @petrochenkov @spastorino

petrochenkov · 2024-05-02T11:49:05Z

The PR's current implementation strategy for unsafe attributes is to add an unsafety field to AttrItem and MetaItem and handled that during parsing. I.e. #[unsafe(Something)] will end up as a flat Something AttrItem as opposed to an unsafe AttrItem with Something nested as an argument.

This is the right strategy.
It would be more clear if the syntax was different, like #[unsafe my_attr].
The confusing parenthesized syntax was selected in the end, but it doesn't change the semantics.

petrochenkov · 2024-05-02T11:51:37Z

Macro matcher fragment specifiers edition policy](https://rust-lang.github.io/rfcs/3531-macro-fragment-policy.html)). However, I think that is not necessary since #[unsafe(something)] does not currently parse (see playground example).

It would be good if unsafe could fit into the existing meta matcher.

There may be some issues with ambiguities between macro arms in cases similar to

macro_rules! m {
    (unsafe) => { 1 }
    ($meta:meta) => { 2 }
}

m!(unsafe(no_mangle));

This needs some testing.

michaelwoerister · 2024-05-02T12:12:21Z

This is the right strategy.

Thanks for the feedback!

It would be more clear if the syntax was different, like #[unsafe my_attr].
The confusing parenthesized syntax was selected in the end, but it doesn't change the semantics.

Yes, I read through the RFC discussion thread today. I'm going to stay neutral regarding the choice 🙂
From a grammar point of view #[unsafe my_attr] would indeed be clearer.

craterbot · 2024-05-17T23:31:08Z

🎉 Experiment pr-124214 is completed!
📊 106 regressed and 118 fixed (447503 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

carbotaniuman · 2024-05-18T11:59:02Z

Looking at the crater results, it appears like there's no regressions from this change.

bors · 2024-05-20T20:54:01Z

☔ The latest upstream changes (presumably #125331) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-05-22T07:01:51Z

☔ The latest upstream changes (presumably #125326) made this pull request unmergeable. Please resolve the merge conflicts.

michaelwoerister · 2024-05-22T08:24:56Z

Looking at the crater results, it appears like there's no regressions from this change.

I agree. I can't find anything that looks related this PR.

michaelwoerister

The PR looks good to merge now. Please expand the comment mentioned below. After that I'm happy to approve.

michaelwoerister · 2024-05-22T08:26:22Z

compiler/rustc_ast/src/attr/mod.rs

@@ -360,7 +367,8 @@ impl MetaItem {
            _ => path.span.hi(),
        };
        let span = path.span.with_hi(hi);
-        Some(MetaItem { path, kind, span })
+        // FIX THIS LATER


Can you expand this comment to give more information on what needs to be fixed?

I've put a proper FIXME here. I expect I'll tackle this in a subsequent PR, so I'd prefer not having to block on this.

Thanks, yes that works me.

michaelwoerister · 2024-05-22T08:50:59Z

Actually, @carbotaniuman, let's also make #[ffi_const] and #[ffi_pure] unsafe too. Those definitely fit the bill. Not sure about #[naked] yet.

michaelwoerister · 2024-05-22T09:01:50Z

@traviscross, let me know if you'd like me to hold off on approving the PR until the lang-team has reviewed the set of attributes.

RalfJung · 2024-05-22T09:20:33Z

#[naked] seems very unsafe, doesn't it? Cc @Amanieu

michaelwoerister · 2024-05-22T09:32:35Z

#[naked] seems very unsafe, doesn't it? Cc @Amanieu

I agree but it might be a similar case to #[link_name] because either the naked function itself has to be unsafe or its entire body must be wrapped inside an unsafe block. But maybe that's a kind of unsafety of a different category?

pnkfelix · 2024-05-22T13:06:01Z

@traviscross, let me know if you'd like me to hold off on approving the PR until the lang-team has reviewed the set of attributes.

I'm doing that review now, thanks @michaelwoerister

click to see review table

attribute	already unsafe in PR #124214	should be unsafe	stay safe	removed from lang
`link`			covered by coupling with (`unsafe`) `extern` `{...}`
`link_ordinal`			covered by coupling with (`unsafe`) `extern` `{...}`
`link_args`				replace with `-C link-arg` or `#![feature(link_arg_attribute]`and `#[link(kind="link-arg", ...]` (thus see above)
`link_name`			covered by coupling with (`unsafe`) `extern` `{...}`
`repr`			believed safe (apart from packed; see below)
`repr(packed)`			checked for error at use sites
`export_name`	done
`link_section`	done
`no_mangle`	done
`used`			believed safe

`ffi_const`		suggested in mw review
`ffi_pure`		suggested in mw review
`naked`			covered by `unsafe` for naked function itself

michaelwoerister · 2024-05-28T09:02:36Z

@pnkfelix, can I interpret your message as "the current set of attributes is approved for this PR"?

rustbot assigned michaelwoerister Apr 21, 2024

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 Apr 21, 2024

This comment has been minimized.

Sign in to view

carbotaniuman force-pushed the parse_unsafe_attrs branch from 0702557 to 6ebb485 Compare April 21, 2024 04:55

This comment has been minimized.

Sign in to view

traviscross mentioned this pull request Apr 21, 2024

Tracking Issue for RFC 3325: unsafe attributes #123757

Open

7 tasks

michaelwoerister reviewed Apr 25, 2024

View reviewed changes

compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved

michaelwoerister reviewed Apr 25, 2024

View reviewed changes

compiler/rustc_feature/src/unstable.rs Show resolved Hide resolved

carbotaniuman force-pushed the parse_unsafe_attrs branch 2 times, most recently from c91ab4f to eb16919 Compare April 25, 2024 16:39

carbotaniuman force-pushed the parse_unsafe_attrs branch from eb16919 to 5562e99 Compare April 28, 2024 20:10

michaelwoerister reviewed May 2, 2024

View reviewed changes

compiler/rustc_parse/src/parser/nonterminal.rs Outdated Show resolved Hide resolved

michaelwoerister reviewed May 2, 2024

View reviewed changes

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 17, 2024

carbotaniuman force-pushed the parse_unsafe_attrs branch from 06b26dd to 892efac Compare May 21, 2024 08:09

This comment has been minimized.

Sign in to view

michaelwoerister reviewed May 22, 2024

View reviewed changes

carbotaniuman force-pushed the parse_unsafe_attrs branch from ccced81 to 0a252c8 Compare May 23, 2024 14:13

carbotaniuman added 9 commits May 27, 2024 11:42

Parse unsafe attributes

f3f7d41

Disallow unsafe in derive

a2f2541

Reorder unsafe macro cases

66329b1

Add tests

2a88f92

Error on unsafe on non-unsafe attribute

cadacf1

Fix doc comment

ed2eb81

Add double unsafe test

401bc05

Fix build

1149293

Change comment to FIXME

b09b33d

carbotaniuman force-pushed the parse_unsafe_attrs branch from 4a63fdb to b09b33d Compare May 27, 2024 16:44

carbotaniuman added 2 commits May 27, 2024 11:45

Fix formatting

d0e7da1

Add unsafe to ffi_pure and ffi_const

9fdd56f

Parse unsafe attributes #124214

Are you sure you want to change the base?

Parse unsafe attributes #124214

Conversation

carbotaniuman commented Apr 21, 2024

rustbot commented Apr 21, 2024

This comment has been minimized.

bors commented Apr 21, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Apr 25, 2024

michaelwoerister commented Apr 25, 2024

bors commented Apr 27, 2024

carbotaniuman commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented May 2, 2024

rustbot commented May 2, 2024

michaelwoerister commented May 2, 2024

petrochenkov commented May 2, 2024

petrochenkov commented May 2, 2024

michaelwoerister commented May 2, 2024

craterbot commented May 17, 2024

carbotaniuman commented May 18, 2024

bors commented May 20, 2024

This comment has been minimized.

This comment has been minimized.

bors commented May 22, 2024

michaelwoerister commented May 22, 2024

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented May 22, 2024

michaelwoerister commented May 22, 2024

RalfJung commented May 22, 2024

michaelwoerister commented May 22, 2024 • edited

pnkfelix commented May 22, 2024 • edited

michaelwoerister commented May 28, 2024 • edited

michaelwoerister commented May 22, 2024 •

edited

pnkfelix commented May 22, 2024 •

edited

michaelwoerister commented May 28, 2024 •

edited