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

parser: Remove support for inner attributes on non-block expressions

Merged
merged 1 commit into from May 3, 2021

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Mar 20, 2021

Remove support for attributes like

fn attrs() {
    (#![print_target_and_args(fifth)] 1, 2);

    [#![print_target_and_args(sixth)] 1 , 2];
    [#![print_target_and_args(seventh)] true ; 5];


    match 0 {
        #![print_target_and_args(eighth)]
        _ => {}
    }

    MyStruct { #![print_target_and_args(ninth)] field: true };
}

They are

I still want to run crater on this to check whether the stability holes are exploited in practice, and whether such attributes are used at all.

@rust-highfive
Copy link
Collaborator

rust-highfive commented Mar 20, 2021

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review label Mar 20, 2021
@petrochenkov petrochenkov added T-lang S-waiting-on-crater and removed S-waiting-on-review labels Mar 20, 2021
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 20, 2021

@bors try

@bors
Copy link
Contributor

bors commented Mar 20, 2021

Trying commit 89895b0 with merge 32cb8dc...

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 20, 2021
parser: Remove support for inner attributes on non-block expressions

Remove support for attributes like
```rust
fn attrs() {
    (#![print_target_and_args(fifth)] 1, 2);

    [#![print_target_and_args(sixth)] 1 , 2];
    [#![print_target_and_args(seventh)] true ; 5];

    match 0 {
        #![print_target_and_args(eighth)]
        _ => {}
    }

    MyStruct { #![print_target_and_args(ninth)] field: true };
}
```
They are
- useless
- unstable (modulo holes like rust-lang#65860)
- pessimize compiler performance, namely token collection for macros (cc rust-lang#82608)

I still want to run crater on this to check whether the stability holes are exploited in practice, and whether such attributes are used at all.
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 20, 2021

☀️ Try build successful - checks-actions
Build commit: 32cb8dc (32cb8dc6fdc8140b91f4d17c4883e528db4e7e6d)

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 20, 2021

@craterbot check

@craterbot
Copy link
Collaborator

craterbot commented Mar 20, 2021

👌 Experiment pr-83312 created and queued.
🤖 Automatically detected try build 32cb8dc
⚠️ Try build based on commit f5f33ec, but latest commit is 89895b0. Did you forget to make a new try build?
🔍 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

@craterbot
Copy link
Collaborator

craterbot commented Mar 23, 2021

🚧 Experiment pr-83312 is now running

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

@craterbot
Copy link
Collaborator

craterbot commented Mar 29, 2021

🎉 Experiment pr-83312 is completed!
📊 17 regressed and 14 fixed (152046 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

@craterbot craterbot added S-waiting-on-review and removed S-waiting-on-crater labels Mar 29, 2021
@petrochenkov petrochenkov added S-waiting-on-author and removed S-waiting-on-review labels Mar 29, 2021
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 29, 2021

4 unique legitimate regressions:

  • match port { #![allow(clippy::cast_ptr_alignment)] ... }
  • match self.color { //! The `format!()` [macro] lets us create a `String` with a pattern, ... }
  • match *self { #![allow(unknown_lints,match_same_arms)] ... }
  • match mdid { #![allow(clippy::unreadable_literal)] }

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Apr 3, 2021

FCP report

I propose removing support for inner attributes on tuple, parentheses, array, struct and match expressions from the language.

Examples:

fn attrs() {
    (#![print_target_and_args(fifth)] 1, 2);

    [#![print_target_and_args(sixth)] 1 , 2];
    [#![print_target_and_args(seventh)] true ; 5];


    match 0 {
        #![print_target_and_args(eighth)]
        _ => {}
    }

    MyStruct { #![print_target_and_args(ninth)] field: true };
}

Such attributes are not used in practice and pose challenges to token collection performed during parsing for various macros to work correctly (#82608).
Expressions are the most common AST nodes and we have to collect tokens for most of them if we assume that they can potentially contain inner attributes, pessimizing the compiler significantly.

Support for these attributes is a part of #![feature(stmt_expr_attributes)].
Attributes on expressions and statements, including inner attributes, were introduced by RFC 16 https://github.com/rust-lang/rfcs/blob/master/text/0016-more-attributes.md.
The RFC, however says "Inner attributes can be placed at the top of blocks (and other structure incorporating a block) and apply to that block." (https://github.com/rust-lang/rfcs/blob/master/text/0016-more-attributes.md#inner-attributes), which doesn't apply to any cases removed in this PR.
The RFC gives an example with match though, which is not a block.
Others cases (tuples, etc.) were added in #29850 without any discussion.

So, removal of these cases also makes the rule for "where are inner attributes accepted" much simpler and aligns it with the original RFC.

Detailed design

Inner attributes are now accepted only on crate/module files and braced blocks ({ ... }) containing statements (or some subset of statements like items).
Support for inner attributes in other places is removed.

Issues

Feature gating for attributes on expressions and statements is exceptionally random and buggy.
As a result some of the inner attributes in question are also available on stable in some cases (the attribute must be a built-in, and the expression must be an expression statement).
Fortunately, we are saved here by the fact that these attributes are not useful in practice, crater found only 4 uses of such attributes, all on match (#83312 (comment), #83312 (comment)).

@petrochenkov petrochenkov added I-nominated S-waiting-on-team and removed S-waiting-on-author labels Apr 3, 2021
@bors
Copy link
Contributor

bors commented Apr 10, 2021

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

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 20, 2021

@rfcbot fcp merge

I'm going go ahead and kick off an FCP, based on @petrochenkov's report. This is also nominated for discussion in today's lang-team meeting.

@rfcbot
Copy link

rfcbot commented Apr 20, 2021

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period label Apr 20, 2021
@petrochenkov petrochenkov added S-waiting-on-review and removed S-waiting-on-team to-announce labels Apr 30, 2021
@Aaron1011
Copy link
Member

Aaron1011 commented May 1, 2021

r=me with the comment addressed

@petrochenkov petrochenkov added S-waiting-on-author and removed S-waiting-on-review labels May 1, 2021
@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 3, 2021

@bors r=Aaron1011

@bors
Copy link
Contributor

bors commented May 3, 2021

📌 Commit 1443c76 has been approved by Aaron1011

@bors bors added S-waiting-on-bors and removed S-waiting-on-author labels May 3, 2021
@bors
Copy link
Contributor

bors commented May 3, 2021

Testing commit 1443c76 with merge c825bc4...

@bors
Copy link
Contributor

bors commented May 3, 2021

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing c825bc4 to master...

@bors bors added the merged-by-bors label May 3, 2021
@bors bors merged commit c825bc4 into rust-lang:master May 3, 2021
11 checks passed
@rustbot rustbot added this to the 1.54.0 milestone May 3, 2021
@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 3, 2021

Uh, @rust-lang/lang, I kind of missed the boat here -- I had an action item to leave a concern about the backwards compatibility angle here, as we discussed at our last meeting, but I fell behind and didn't realize this was time sensitive.

I guess I will open an issue to discuss.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 3, 2021

Opened #84879

This was referenced May 4, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 22, 2021
…s-within-match, r=nikomatsakis

Re-add support for parsing (and pretty-printing) inner-attributes in match body

Re-add support for parsing (and pretty-printing) inner-attributes within body of a `match`.

In other words, we can do `match EXPR { #![inner_attr] ARM_1 ARM_2 ... }` again.

I believe this unbreaks the only four crates that crater flagged as broken by PR rust-lang#83312.

(I am putting this up so that the lang-team can check it out and decide whether it changes their mind about what to do regarding PR rust-lang#83312.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge finished-final-comment-period merged-by-bors S-waiting-on-bors T-lang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants