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

michaelwoerister · 2024-04-25T12:14:02Z

compiler/rustc_feature/src/unstable.rs

@@ -628,6 +628,7 @@ declare_features! (
    /// Allows unnamed fields of struct and union type
    (incomplete, unnamed_fields, "1.74.0", Some(49804)),
    /// Allows unsized fn parameters.
+    (unstable, unsafe_attributes, "CURRENT_RUSTC_VERSION", Some(123757)),


The doc comment needs an update too.

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.

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.

michaelwoerister · 2024-05-02T12:29:43Z

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

Let's try to make that work then.

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.

To clarify, the above test case would be an example where the current and the new meta matcher could give different results?

Do you mean a case like the following where currently :meta does not match unsafe(no_mangle), so we get the second arm, but with the new behavior we would get the first one (playground)?:

fn main() {
    macro_rules! foo {
        ($l:meta) => { 1 };
        (unsafe($x:ident)) => { 2 };
    }

    let x = foo!(unsafe(no_mangle));
    println!("{x}");
}

carbotaniuman · 2024-05-04T00:47:54Z

Yeah, that foo macro is the main one I'm worried about. I think you could get away with it with some sort of migration lint, but I'm really not sure.

michaelwoerister · 2024-05-07T07:49:47Z

@carbotaniuman, can you bring this PR in a state where we don't introduce a separate meta_2021 matcher and just modify the existing meta matcher? Then we can do a crater run to see what the impact is.

@rustbot author

This reverts commit 61fe268.

This reverts commit 7e32657.

carbotaniuman · 2024-05-08T06:00:26Z

Alright, I've reverted those commits.

@rustbot ready

michaelwoerister · 2024-05-08T08:56:46Z

Thanks, @carbotaniuman!

Let's do a try build for the crater run.
@bors try

I guess we'll need a build-and-test run since the difference in parsing does not necessarily result in a compile time error.

…<try> Parse unsafe attributes Initial parse implementation for rust-lang#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.

bors · 2024-05-08T09:03:01Z

⌛ Trying commit aadc436 with merge 54bfda8...

michaelwoerister · 2024-05-08T09:15:49Z

A code search on github does not yield anything that looks problematic:
/#\[unsafe$.*$\]/ language:Rust
/#\[cfg_attr$.*,.*unsafe\(.*$\)\]/ language:Rust

bors · 2024-05-08T10:40:24Z

☀️ Try build successful - checks-actions
Build commit: 54bfda8 (54bfda8e7e7745c74b12d95d7a6cd614e83f888c)

michaelwoerister · 2024-05-08T20:15:04Z

@craterbot run mode=build-and-test

craterbot · 2024-05-08T20:15:13Z

👌 Experiment pr-124214 created and queued.
🤖 Automatically detected try build 54bfda8
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

traviscross · 2024-05-14T21:16:34Z

@rustbot labels +I-lang-nominated

Let's nominate to discuss and confirm that the only attributes that need to be marked as unsafe are:

no_mangle
link_section
export_name

There's been discussion about link and link_ordinal also.

cc @RalfJung

RalfJung · 2024-05-15T05:48:39Z

This is the best list I know. But I honestly don't have a good overview of which attributes we even have for controlling linker behavior, let alone which of them are unsafe.

Cc @bjorn3

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

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

carbotaniuman added 6 commits April 28, 2024 15:09

Parse unsafe attributes

31ab9cb

Fix rustdoc compile

94d3158

Fix it for real

9b8196b

Fix again

8638edd

Disallow unsafe in derive

e6d612e

Reorder unsafe macro cases

5562e99

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

carbotaniuman added 2 commits April 28, 2024 15:43

Properly handle macro_rules

7e32657

Add tests

8d5667d

michaelwoerister reviewed May 2, 2024

View reviewed changes

Remove extra Meta check

61fe268

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2024

carbotaniuman added 2 commits May 8, 2024 00:57

Revert "Remove extra Meta check"

b1c591d

This reverts commit 61fe268.

Revert "Properly handle macro_rules"

aadc436

This reverts commit 7e32657.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2024

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

rustbot added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse unsafe attributes #124214

Parse unsafe attributes #124214

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

michaelwoerister commented Apr 25, 2024

bors commented Apr 27, 2024

carbotaniuman commented Apr 28, 2024

michaelwoerister May 2, 2024

carbotaniuman May 2, 2024

michaelwoerister May 2, 2024

carbotaniuman May 4, 2024

michaelwoerister May 2, 2024

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

michaelwoerister commented May 2, 2024

carbotaniuman commented May 4, 2024

michaelwoerister commented May 7, 2024

carbotaniuman commented May 8, 2024

michaelwoerister commented May 8, 2024

bors commented May 8, 2024

michaelwoerister commented May 8, 2024

bors commented May 8, 2024

michaelwoerister commented May 8, 2024

craterbot commented May 8, 2024

traviscross commented May 14, 2024

RalfJung commented May 15, 2024

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

Choose a reason for hiding this comment

michaelwoerister commented Apr 25, 2024

bors commented Apr 27, 2024

carbotaniuman commented Apr 28, 2024

michaelwoerister May 2, 2024

Choose a reason for hiding this comment

carbotaniuman May 2, 2024

Choose a reason for hiding this comment

michaelwoerister May 2, 2024

Choose a reason for hiding this comment

carbotaniuman May 4, 2024

Choose a reason for hiding this comment

michaelwoerister May 2, 2024

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

michaelwoerister commented May 2, 2024

carbotaniuman commented May 4, 2024

michaelwoerister commented May 7, 2024

carbotaniuman commented May 8, 2024

michaelwoerister commented May 8, 2024

bors commented May 8, 2024

michaelwoerister commented May 8, 2024

bors commented May 8, 2024

michaelwoerister commented May 8, 2024

craterbot commented May 8, 2024

traviscross commented May 14, 2024

RalfJung commented May 15, 2024