Implements RFC 16, attributes on statements and expressions. #29850
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
cc @nikomatsakis |
|
Is there a feature gate? It doesn't look like it from the tests, but it should have one. |
|
Oh! Right, feature gate. Where do I add that? |
|
It might be good to do a few measurements of this along the lines of:
Thanks for the impl regardless @Kimundi! |
|
Here's the result of compiling rustc with stmt expr attrs:rustc without stmt expr attrs:
|
|
Question about macros: is Also, the trailing expression thing seems like it might be nice to get working, because |
|
Good question about the macro case! I'm guessing it counts as the latter if that macro syntax is legal today, I'd have to test it. And yeah, the more I think about it the more I think the trailing case should just be made to work as expected. (Though its indeed simulatable today) |
|
☔ The latest upstream changes (presumably #29903) made this pull request unmergeable. Please resolve the merge conflicts. |
Kimundi
force-pushed
the
attributes_that_make_a_statement
branch
from
88c1eca
to
9131502
Compare
|
Okay, I added the feature gate and rebased on upstream changes. @durka I tested the macro case, and it will accept both, depending only on order of the macro clauses. Not sure if that is an issue regarding macro stability... |
|
☔ The latest upstream changes (presumably #30015) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Kimundi I dunno about future-proofing, but I think that is the best outcome in terms of macro writing. Because by switching the order of macro arms, you can choose whether you want to match "any expr, with or without attrs" or catch the attr (in order to implement custom attrs or something). |
nodes in statement position. Extended #[cfg] folder to allow removal of statements, and of expressions in optional positions like expression lists and trailing block expressions. Extended lint checker to recognize lint levels on expressions and locals.
…hat is actually neccessary)
Kimundi
force-pushed
the
attributes_that_make_a_statement
branch
from
e2df4bc
to
49e9974
Compare
| // merge attributes into the inner expression. | ||
| return lower_expr(lctx, ex).map(|mut ex| { | ||
| ex.attrs.update(|attrs| { | ||
| attrs.prepend(e.attrs.clone()) |
There was a problem hiding this comment.
hmm, it might be good to have a test exercising the ordering enforced by this code.
(I did see a test with a generic #[attr], along the lines of #[attr] ( #[attr] expr ), but I didn't see anything that ensured that a prepend is occurring here rather than an append.)
Having said that, it isn't really that big a deal (especially since it may be non-trivial to actually make such a test... I guess one could do it by applying an #[allow(foo)] at the outer point followed by a #[deny(foo)] on the inner, and vice-versa, and making sure one gets the expected effect.
There was a problem hiding this comment.
Good idea about how to test that, added a test for it.
| // in case of a cfg attr. | ||
| // | ||
| // NB: This is intentionally not part of the fold_expr() function | ||
| // in order for fold_opt_expr() to be able to avoid this check |
There was a problem hiding this comment.
when I first read this comment, I interpreted the phrase "to be able to avoid this check" as some sort of efficiency concern (which admittedly seemed a little strange in this context).
But now I think I understand: fold_opt_expr must skip the check, because the check does not apply to optional expression contexts. Do I read that right?
(I'm not sure I can think of a better phrasing off-hand. It could be just me that strongly associates the phrase "avoid a check" with "micro-optimization"... :)
There was a problem hiding this comment.
That's correct. Maybe it could be better phrased, but I'm not sure how either, hm...
|
📌 Commit d06f480 has been approved by |
…kfelix See rust-lang/rfcs#16 and #15701 - Added syntax support for attributes on expressions and all syntax nodes in statement position. - Extended `#[cfg]` folder to allow removal of statements, and of expressions in optional positions like expression lists and trailing block expressions. - Extended lint checker to recognize lint levels on expressions and locals. - As per RFC, attributes are not yet accepted on `if` expressions. Examples: ```rust let x = y; { ... } assert_eq!((1, #[cfg(unset)] 2, 3), (1, 3)); let FOO = 0; ``` Implementation wise, there are a few rough corners and open questions: - The parser work ended up a bit ugly. - The pretty printer change was based mostly on guessing. - Similar to the `if` case, there are some places in the grammar where a new `Expr` node starts, but where it seemed weird to accept attributes and hence the parser doesn't. This includes: - const expressions in patterns - in the middle of an postfix operator chain (that is, after `.`, before indexing, before calls) - on range expressions, since `#[attr] x .. y` parses as `(#[attr] x) .. y`, which is inconsistent with `#[attr] .. y` which would parse as `#[attr] (.. y)` - Attributes are added as additional `Option<Box<Vec<Attribute>>>` fields in expressions and locals. - Memory impact has not been measured yet. - A cfg-away trailing expression in a block does not currently promote the previous `StmtExpr` in a block to a new trailing expr. That is to say, this won't work: ```rust let x = { #[cfg(foo)] Foo { data: x } #[cfg(not(foo))] Foo { data: y } }; ``` - One-element tuples can have their inner expression removed to become Unit, but just Parenthesis can't. Eg, `(#[cfg(unset)] x,) == ()` but `(#[cfg(unset)] x) == error`. This seemed reasonable to me since tuples and unit are type constructors, but could probably be argued either way. - Attributes on macro nodes are currently unconditionally dropped during macro expansion, which seemed fine since macro disappear at that point? - Attributes on `ast::ExprParens` will be prepend-ed to the inner expression in the hir folder. - The work on pretty printer tests for this did trigger, but not fix errors regarding macros: - expression `foo![]` prints as `foo!()` - expression `foo!{}` prints as `foo!()` - statement `foo![];` prints as `foo!();` - statement `foo!{};` prints as `foo!();` - statement `foo!{}` triggers a `None` unwrap ICE.
|
Holy shit, it broke 4 of my 5 pending PRs :D |
|
@petrochenkov yeah we debated this at the compiler team mtg; niko and I weren't sure changing libsyntax like this should be categorized as "breaking" the way other changes are. I guess I still might have injected the breaking-change text, I don't know, we didn't come up with a firm rule |
Attrs are now allowed in statement and expression contexts. This will of course break the build, as syntex_syntax will need to be updated as well. I also noticed that the test suite doesn't check nightly, only stable. Perhaps we should update to run both?
This will of course break the build on stable, as syntex_syntax will need to be updated as well. The suite now passes on nightly, however. Though the pull request upstream which broke this adds support for attributes in statement and expression contexts, I've opted not to actually support it here yet, as we presumably cannot handle that on stable for at least 2 more Rust versions. Fixes serde-deprecated#56
|
FTR, I think breaking changes to libsyntax should be marked |
|
This broke |
Fixes the cascade of dependency breakages introduced in rust-lang/rust#29850
See rust-lang/rfcs#16 and #15701
#[cfg]folder to allow removal of statements, andof expressions in optional positions like expression lists and trailing
block expressions.
locals.
ifexpressions.Examples:
Implementation wise, there are a few rough corners and open questions:
ifcase, there are some places in the grammar where a newExprnode starts,but where it seemed weird to accept attributes and hence the parser doesn't. This includes:
., before indexing, before calls)#[attr] x .. yparses as(#[attr] x) .. y, which is inconsistent with#[attr] .. ywhich would parse as#[attr] (.. y)Option<Box<Vec<Attribute>>>fields in expressions and locals.StmtExprin a block to a new trailing expr. That is to say, this won't work:(#[cfg(unset)] x,) == ()but(#[cfg(unset)] x) == error. This seemed reasonable to me since tuples and unit are type constructors, but could probably be argued either way.ast::ExprParenswill be prepend-ed to the inner expression in the hir folder.foo![]prints asfoo!()foo!{}prints asfoo!()foo![];prints asfoo!();foo!{};prints asfoo!();foo!{}triggers aNoneunwrap ICE.