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

Run decorators on expanded AST #34010

Merged
merged 2 commits into from
Jun 8, 2016
Merged

Conversation

jseyfried
Copy link
Contributor

Fixes #32950.
r? @nrc

@jseyfried
Copy link
Contributor Author

cc @durka

@nrc
Copy link
Member

nrc commented Jun 1, 2016

This is (I think) a breaking change since a decorator author could be using macros in the AST as input to their functionality.

I think I prefer the expansion order to be the same for all attributes, seems confusing to have difference expansion orders. Other than #32950, is there any reason to do this? re #32950, I think a better fix is to implement visit_mac and issue an error rather than panicking.

@durka
Copy link
Contributor

durka commented Jun 1, 2016

What if the decorator produces more decorators?

@durka
Copy link
Contributor

durka commented Jun 1, 2016

cc #33769, the previous attempt to fix this

@jseyfried
Copy link
Contributor Author

@durka decorator-generated items would still get expanded.

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 1, 2016

@nrc

This is (I think) a breaking change

Agreed, but I'm pretty sure it wouldn't break anything in practice.

seems confusing to have difference expansion orders

I agree that this would be a downside.

Other than #32950, is there any reason to do this?

I believe some use cases of decorators might benefit from having access to expanded AST, but I don't have any specific examples in mind (besides #32950, of course).

In lieu of specific examples, a thought experiment: if we allowed macros in field declaration positions (not saying this would be a good idea), for example

struct Foo {
    x: i32,
    mac!(), //< say `mac!()` expands to `y: i32`
}

then derive would need access to expanded AST.

@jseyfried
Copy link
Contributor Author

That being said, I only weakly support this PR -- @nrc if you still don't think this is a good idea I'll close.

@durka
Copy link
Contributor

durka commented Jun 1, 2016

Only a plugin-breaking-change, to be clear, since the only stable "decorator author" is rustc.

@durka
Copy link
Contributor

durka commented Jun 1, 2016

It does seem to make some intuitive sense that a decorator on an item would be the last thing to be expanded, after any macros which generate parts of the item. But what about mac! { #[derive(Foo)] struct Item { stuff: thing!() } }, what is the order there?

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 1, 2016

@durka

Only a plugin-breaking-change, to be clear

Agreed, although it could theoretically break code that uses (unstable) decorator plugins without breaking the plugins themselves (highly unlikely, imo).

what about mac! { #[derive(Foo)] struct Item { stuff: thing!() } }, what is the order there?

We would expand mac! first. Before expanding mac!, #[derive(Foo)] is just a part of a token tree -- we don't know if it will end up being a decorator or if it has some other meaning, for example:

macro_rules! mac {
    (#[derive($i:ident)] $it:item) => { println!(stringify!($i)); }
}
fn main() {
    mac! { #[derive(Foo)] struct Bar; } // prints "Foo"
}

After expanding mac!, say we end up with #[derive(Foo)] struct Item { stuff: thing!() }. Then, we would expand expand Item (i.e. we would expand thing!) and finally decorate Item.

@nrc
Copy link
Member

nrc commented Jun 7, 2016

Hmm, I'm still not super-happy about having different expansion orders, but this seems to be a fine solution other than that. I think it should probably land. We'll have an opportunity to change direction with the procedural macro overhaul in any case.

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit 635a82e has been approved by nrc

@bors
Copy link
Contributor

bors commented Jun 8, 2016

⌛ Testing commit 635a82e with merge ff13155...

bors added a commit that referenced this pull request Jun 8, 2016
Run decorators on expanded AST

Fixes #32950.
r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants