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

Always invoke statement attributes on the statement itself #79326

Merged
merged 3 commits into from
Nov 25, 2020

Conversation

Aaron1011
Copy link
Member

This is preparation for PR #78296, which will require us to handle
statement items in addition to normal items.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 23, 2020
@petrochenkov
Copy link
Contributor

In #78296 (comment) I meant including the rustc_expand changes (and therefore rustc_ast changes as well.)

I've left the tests in this PR since they otherwise produce no useful output (we get a parse error before the macros are invoked)

I think they should parse if the #[attr] $stmt bits are removed?
(I'm just interested in looking at the test diff that the parsing and token collection changes bring.)

@petrochenkov petrochenkov 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 Nov 23, 2020
This is preparation for PR rust-lang#78296, which will require us to handle
statement items in addition to normal items.
@Aaron1011
Copy link
Member Author

@petrochenkov: Splitting out just the rustc_expand changes without the token collection causes us to lose spans for attribute macros in item statements (e.g. fn foo() { #[my_attr] fn bar() {} }) - we now invoke #[my_attr] on a statement, but we still have the buggy statement token collection.

I could try to add a workaround, but it would be immediately reverted by #78296

@Aaron1011 Aaron1011 force-pushed the fix/builtin-macro-stmt branch 2 times, most recently from 49c980b to 0850c65 Compare November 24, 2020 20:05
@Aaron1011
Copy link
Member Author

@petrochenkov: I've added the rustc_expand changes, together with modified versions of the tests. I needed to add a hack to preserve spans for the already-stable case of attributes on statement items, but it turned out to be a small change.

span: $DIR/allowed-attr-stmt-expr.rs:1:1: 1:1 (#0),
},
]
PRINT-ATTR INPUT (DISPLAY): #[rustc_dummy] struct Other { }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behavior that we want - the trailing semicolon is not considered part of the statement. I'll make sure that this behavior is preserved in #78296

@Aaron1011 Aaron1011 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 Nov 24, 2020
@petrochenkov
Copy link
Contributor

Splitting out just the rustc_expand changes without the token collection causes us to lose spans for attribute macros in item statements (e.g. fn foo() { #[my_attr] fn bar() {} }) - we now invoke #[my_attr] on a statement, but we still have the buggy statement token collection.

I see, I assumed that you could use tokens from the item (and looks like that's what the updated PR does).

@petrochenkov
Copy link
Contributor

r=me with #79326 (comment) addressed.

@petrochenkov petrochenkov 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 Nov 24, 2020
@Aaron1011 Aaron1011 changed the title Handle Annotatable::Stmt in some builtin macros Always invoke statement attributes on the statement itself Nov 24, 2020
@Aaron1011
Copy link
Member Author

I updated the PR description to better reflect the changes

@Aaron1011
Copy link
Member Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Nov 24, 2020

📌 Commit 9c9f406 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2020
@bors
Copy link
Contributor

bors commented Nov 25, 2020

⌛ Testing commit 9c9f406 with merge 22f0c711908e699e465a9a34980861cbce4902dd...

@bors
Copy link
Contributor

bors commented Nov 25, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 25, 2020
@Aaron1011
Copy link
Member Author

Failed due an old version of procedural-masquerade being in use, which is too strict about checking the pretty-printed output. I'm not sure why this only started failing now.

We need to bump to a version of procedural-masquerade that includes servo/rust-cssparser#274

@Aaron1011
Copy link
Member Author

@SimonSapin: It looks like cargotest is pulling in an old version of cssparser due to the servo dependency. What's the easiest way for us to make cargotest use a version of procedural-masquerade that includes servo/rust-cssparser#274?

@SimonSapin
Copy link
Contributor

The easiest is probably to use a more recent servo/servo commit. This is the latest:

diff --git src/tools/cargotest/main.rs src/tools/cargotest/main.rs
index 8aabe077cf1..dc09e7ffc1d 100644
--- src/tools/cargotest/main.rs
+++ src/tools/cargotest/main.rs
@@ -43,7 +43,7 @@ struct Test {
     Test {
         name: "servo",
         repo: "https://github.com/servo/servo",
-        sha: "caac107ae8145ef2fd20365e2b8fadaf09c2eb3b",
+        sha: "90e8e19f5e53fe35ab15e372c359ba681c8bc843",
         lock: None,
         // Only test Stylo a.k.a. Quantum CSS, the parts of Servo going into Firefox.
         // This takes much less time to build than all of Servo and supports stable Rust.

@petrochenkov petrochenkov 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 Nov 25, 2020
@Aaron1011
Copy link
Member Author

@petrochenkov: I've adjusted the pretty-print hack, which makes cssparser compile.

@Aaron1011 Aaron1011 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 Nov 25, 2020
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2020

📌 Commit baefba8 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2020
@bors
Copy link
Contributor

bors commented Nov 25, 2020

⌛ Testing commit baefba8 with merge 192c7db...

@bors
Copy link
Contributor

bors commented Nov 25, 2020

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 192c7db to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants