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

macros: expand items before their #[derive]s #41029

Closed
wants to merge 2 commits into from

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Apr 3, 2017

For example, after this PR the following are equivalent:

#[derive(Example)] // for any `#[proc_macro_derive]` or built-in derive `Example`
struct S(i32);

macro_rules! m { () => { i32 } }
#[derive(Example)]
struct S(m!());

Note that today, #[cfg]s in an item are processed before expanding #[derive]s. For example,

#[derive(Example)] // `Example` will see `struct S();`
struct S(#[cfg(any())] i32);

After this PR, we can consider #[cfg] to be a normal attribute macro (assuming attribute macros were allowed everywhere #[cfg] is allowed).

Since type macros and #[proc_macro_derive] are stable, this is a [breaking-change].

r? @nrc

@jseyfried
Copy link
Contributor Author

cc #38356
cc @keeperofdakeys @dtolnay @sgrif

@jseyfried jseyfried force-pushed the expansion_order branch 3 times, most recently from 536d411 to 370a837 Compare April 3, 2017 06:46
@nrc
Copy link
Member

nrc commented Apr 4, 2017

cc @alexcrichton @rust-lang/lang

@nrc
Copy link
Member

nrc commented Apr 4, 2017

Other than bringing cfg into line, what is the motivation for this change?

Could we Crater this?

@jseyfried
Copy link
Contributor Author

@nrc
Yeah, the motivation is to allow user defined cfg-like attribute procedural macros to be as powerful as cfg, as opposed to being "second-class".

Also, derives usually want expanded code, e.g.

@alexcrichton
Copy link
Member

Seems fine to me!

@nikomatsakis
Copy link
Contributor

Seems like we could detect some breakage here via a crater run, for sure.

@sgrif
Copy link
Contributor

sgrif commented Apr 5, 2017

To be clear, this change would mean that #[cfg(foo)] on a struct field would require custom code from every derive that could be affected by it? Big 👎 from me in that case, then. It would certainly break Diesel

@withoutboats
Copy link
Contributor

withoutboats commented Apr 5, 2017

@sgrif my understanding of this change is that cfg attributes will not change at all. Its that other macros inside a struct will be expanded in the same way that cfg is.

@jseyfried
Copy link
Contributor Author

@withoutboats exactly.

@sgrif
Copy link
Contributor

sgrif commented Apr 5, 2017

Ah. Big 👍 from me then. XD

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

r=me with the tests fixed and a comment on PartialExpansion. I think that in general the expansion code is getting pretty complex, if you get a chance it would be great if you could add some docs giving an overview of the process and data structures, and some detailed comments around the place to explain what is going on (in another PR, doesn't need to block this one).

@@ -156,16 +156,20 @@ impl<'a> base::Resolver for Resolver<'a> {
self.whitelisted_legacy_custom_derives.contains(&name)
}

fn visit_expansion(&mut self, mark: Mark, expansion: &Expansion, derives: &[Mark]) {
fn visit_expansion(&mut self, mark: Mark, expansion: &Expansion, derives: &[(Mark, Path)]) {
Copy link
Member

Choose a reason for hiding this comment

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

I would think about making this new tuple into a struct - it's OK for now, but I think it is a bit opaque to identify a derive by a (Mark, Path).

monotonic: bool, // c.f. `cx.monotonic_expander()`
}

struct PartialExpansion {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here please - it would be good to explain how partial expansions come about - are they just intermediate state or are they stored like this, in what circumstances is a macro partially rather than fully expanded, and are there invariants that can be assumed about partial expansions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. Partial expansion are macro expansions that have unexpanded macro invocations in them. That is, the macro itself has resolved and been expanded, but it created more macro invocations that have yet to be expanded. This is in contrast to "fully expanded" AST, which has no macro invocations left.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017
@carols10cents
Copy link
Member

Friendly ping to keep this PR on your radar @jseyfried! ❤️

@shepmaster
Copy link
Member

Looks like there's some minor tweaks requested, @jseyfried — do you think you will be able to address those soon?

@jseyfried
Copy link
Contributor Author

@carols10cents @shepmaster Yeah, this is still on my radar -- I plan on fixing the tweaks and r+ing this weekend (been super busy these last two weeks).

@alexcrichton
Copy link
Member

I'm going to close this out of inactivity for now, but @jseyfried you're of course welcome to resubmit at any time! (just trying to help clear out the queue)

@abonander
Copy link
Contributor

@jseyfried are you going to revive this PR?

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 5, 2018
…henkov

Expand items before their derives

continuation of rust-lang#41029

At this point all I've done is naively rebased on top of `master` and fixed the code enough to compile. I'm not even sure if it works because my local build stopped here:

```
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\VC\VCTargets\Microsoft.CppBuild.targets(317,5): error MSB3491: Could not write lines to file "emscripten
-optimizer.dir\Release\emscript.63209ED8.tlog\emscripten-optimizer.lastbuildstate". The specified path, file name, or both are too long. The fully qualified file name must be less
than 260 characters, and the directory name must be less than 248 characters. [C:\Users\cyber\Rust\rust\build\x86_64-pc-windows-msvc\stage0-rustc\x86_64-pc-windows-msvc\release\bui
ld\rustc_binaryen-aaf5e3b308543526\out\build\src\emscripten-optimizer\emscripten-optimizer.vcxproj]
```

So I'm going to be relying on Travis to run tests unless I can fix this error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants