-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Expand Attributes on Statements and Expressions #49124
Conversation
This is the error that doesn't make any sense (others are breakages that I haven't fixed cause I've only been running the one test): https://travis-ci.org/rust-lang/rust/builds/354876674#L2899 The only attribute being expanded is |
I understand the problem now. |
Looks fine to me, but I'm somewhat out of touch with the compiler nowadays. |
Turns out #43988 isn't related to this bit of the code. |
1663d23
to
1214e58
Compare
@petrochenkov Squashed and ready for review. |
1214e58
to
ad6b7d2
Compare
I'll likely leave this until weekend. |
Also, Travis fails
|
Oh yeah that's because I changed the error message since attributes on statements are stable. |
ad6b7d2
to
24b3f31
Compare
@petrochenkov build is green |
Ping from triage @petrochenkov! This PR still needs your review. |
I'd like to see a couple more compile fail tests:
r=me after adding those tests |
24b3f31
to
607c866
Compare
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@TimNN suggestion: only send a message if the user doesn't push another commit within X hours. I'm watching these builds and I know how to interpret them so the bot's not really adding anything. |
@abonander: Thanks for the feedback, I'll look into implementing something like that. |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
let _ = #[no_output] "Hello, world!"; | ||
//~^ ERROR asdf | ||
|
||
let _ = #[duplicate] "Hello, world!"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrochenkov
https://travis-ci.org/rust-lang/rust/builds/360794617#L1905
25:13: 25:25: macro expansion ignores token
,
and any following
So it seems that the parser currently doesn't support directly expanding into >1 expressions.
use attr_stmt_expr::duplicate; | ||
|
||
fn main() { | ||
let _ = #[no_output] "Hello, world!"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't produce an error, which is concerning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it hits later than the other one so it didn't occur until I commented it out. However, the UX is pretty bad:
1:1: 1:1: expected expression, found `<eof>`
I reckon I should do the same checks that #[cfg]
does in this context and emit the same error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either that, or just re-span the parse error. That'd probably be easier.
Still needs a test for
+ squashing + green Travis. @bors delegate+ |
✌️ @abonander can now approve this pull request |
I'm at least gonna try to get it to point to the problematic invocation. The current error message has a dummy span. |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
0710682
to
e0c7271
Compare
Retains the `stmt_expr_attributes` feature requirement for attributes on expressions. closes rust-lang#41475 cc rust-lang#38356
e0c7271
to
7c0124d
Compare
@bors r+ |
📌 Commit 7c0124d has been approved by |
Expand Attributes on Statements and Expressions This enables attribute-macro expansion on statements and expressions while retaining the `stmt_expr_attributes` feature requirement for attributes on expressions. closes #41475 cc #38356 @petrochenkov @jseyfried r? @nrc
|
||
let _ = { | ||
#[no_output] | ||
"Hello, world!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, you can cfg-out a trailing expression and implicit ()
will be used instead? Fun.
At least this is consistent with existing behavior of #[cfg]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's just the behavior of an empty block, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's behavior of an empty block, yes, but it's not obvious that { #[cfg_like] trailing }
should expand into an empty block - it depends on interpretation.
If the attribute is applied to the statement, then 1 statement -> 0 statements
transformation is okay, but if the attribute is applied to the expression inside of that statement, then it supposedly should be an error, because you can't shrink an expression in that position into nothing (it's not a part of expression list).
As I understand the code in expand.rs
, attributes on expression statements are treated as expression attributes and not statement attributes, but the behavior follows the statement attribute interpretation.
Ok, now I'm actually not sure whether #[no_output]
is applied at all here.
We need to add a test making sure that this doesn't expand into let _ = { "Hello, world!" };
ignoring the attribute.
☀️ Test successful - status-appveyor, status-travis |
…etrochenkov run-pass/attr-stmt-expr: expand test cases Follow-up to rust-lang#49124 (comment) r? @petrochenkov
…etrochenkov run-pass/attr-stmt-expr: expand test cases Follow-up to rust-lang#49124 (comment) r? @petrochenkov
…etrochenkov run-pass/attr-stmt-expr: expand test cases Follow-up to rust-lang#49124 (comment) r? @petrochenkov
This enables attribute-macro expansion on statements and expressions while retaining the
stmt_expr_attributes
feature requirement for attributes on expressions.closes #41475
cc #38356 @petrochenkov @jseyfried
r? @nrc