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

syntax: Support modern attribute syntax in the `meta` matcher #63674

Open
wants to merge 2 commits into
base: master
from

Conversation

@petrochenkov
Copy link
Contributor

commented Aug 17, 2019

Where "modern" means #57367:

PATH
PATH `(` TOKEN_STREAM `)`
PATH `[` TOKEN_STREAM `]`
PATH `{` TOKEN_STREAM `}`

Unfortunately, meta wasn't future-proofed using the FOLLOW token set like other matchers (#34011), so code like $meta:meta { or $meta:meta [ may break, and we need a crater run to find out how often this happens in practice.

Closes #49629 (by fully supporting meta rather than removing it.)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2019

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

⌛️ Trying commit d73c85e with merge 81ebaa6...

bors added a commit that referenced this pull request Aug 17, 2019
Auto merge of #63674 - petrochenkov:meta2, r=<try>
syntax: Support modern attribute syntax in the `meta` matcher

Where "modern" means #57367:
```
PATH
PATH `(` TOKEN_STREAM `)`
PATH `[` TOKEN_STREAM `]`
PATH `{` TOKEN_STREAM `}`
```

Unfortunately, `meta` wasn't future-proofed using the `FOLLOW` token set like other matchers (#34011), so code like `$meta:meta {` or `$meta:meta [` may break, and we need a crater run to find out how often this happens in practice.

Closes #49629 (by fully supporting `meta` rather than removing it.)
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

☀️ Try build successful - checks-azure
Build commit: 81ebaa6

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Hah; the crater queue is getting crazy, oh well =P

@craterbot run mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 18, 2019

👌 Experiment pr-63674 created and queued.
🤖 Automatically detected try build 81ebaa6
🔍 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

@Centril Centril added this to the 1.39 milestone Aug 18, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

the crater queue is getting crazy

That's a problem with the number of crater instances first of all.

(Two was enough to keep the queue almost empty, but with one it looks like it's going to grow to infinity, so the average PR stream "density" should be somewhere in between.)

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

so code like $meta:meta { or $meta:meta [ may break, and we need a crater run to find out how often this happens in practice.

This sentiment concerns me. Crater can tell you if a breaking change does affect code, but it can't tell you that a breaking change doesn't affect code. It's one thing if we're talking about a bugfix or soundness change and introducing a necessary breaking change because of that, using crater to mitigate the impact. But this is clearly a new feature, and I don't think we should consider breaking any Rust code to support it acceptable, even if none of the code checked by crater is affected. Folks use Rust because it has strong stability guarantees, and we should be trying to uphold that.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

@sgrif For language team praxis, please see: #60932 (comment), #53668 (comment), #53668 (comment), and #53668 (comment).

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

I agree with

Seperately, I think it would be useful to open an RFC that makes this "practical definition" of breaking changes more explicit and tries to narrow down and layout the criteria when what is a "breaking change" (versus a "change that theoretically breaks") -- the rustc bug fix procedure does exactly this, but in a narrower context.

from one of those comments. Until then, please forgive me for pointing this out when I see it, as I continue to find it troubling <3

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

🚧 Experiment pr-63674 is now running on agent aws-1.

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

@pietroalbini

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@craterbot abort

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

🗑 Experiment pr-63674 deleted!

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

@pietroalbini

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@craterbot run mode=check-only p=1

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

👌 Experiment pr-63674 created and queued.
🤖 Automatically detected try build 81ebaa6
🔍 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

@pietroalbini

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@craterbot crates=full

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

📝 Configuration of the pr-63674 experiment changed.

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

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

🚧 Experiment pr-63674 is now running on agent aws-1.

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

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2019

🎉 Experiment pr-63674 is completed!
📊 0 regressed and 0 fixed (74234 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

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

This PR provides some cleanup both in the compiler and the language unifying the meta and attribute grammars (as in #57367) into one form as per the PR description. This makes for an overall more consistent UX using Rust with fewer surprises.

As crater (which has recently been extended with a lot more crates) is green, and our usual policy (#63674 (comment)) is to be practical rather than absolutist about breakage, I think that we should:

@rfcbot merge

@Centril Centril removed the needs-fcp label Sep 13, 2019

@rfcbot

This comment has been minimized.

Copy link

commented Sep 13, 2019

Team member @Centril 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.