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

Decouple proc_macro from the rest of the compiler. #49219

Merged
merged 16 commits into from Nov 30, 2018

Conversation

@eddyb
Member

eddyb commented Mar 20, 2018

This PR removes all dependencies of proc_macro on compiler crates and allows multiple copies of proc_macro, built even by different compilers (but from the same source), to interoperate.

Practically, it allows:

  • running proc macro tests at stage1 (I moved most from -fulldeps to the regular suites)
  • using proc macros in the compiler itself (may require some rustbuild trickery)

On the server (i.e. compiler front-end) side:

  • server::* traits are implemented to provide the concrete types and methods
    • the concrete types are completely separated from the proc_macro public API
    • the only use of the type implementing Server is to be passed to Client::run

On the client (i.e. proc macro) side (potentially using a different proc_macro instance!):

  • client::Client wraps around client-side (expansion) function pointers
    • it encapsulates the proc_macro instance used by the client
    • its run method can be called by a server, to execute the client-side function
      • the client instance is bridged to the provided server, while it runs
      • currently a thread is spawned, could use process isolation in the future
        (not the case anymore, see #56058)
  • proc macro crates get a generated static holding a &[ProcMacro]
    • this describes all derives/attr/bang proc macros, replacing the "registrar" function
    • each variant of ProcMacro contains an appropriately typed Client<fn(...) -> ...>

proc_macro public APIs call into the server via an internal "bridge":

  • only a currently running proc macro Client can interact with those APIs
    • server code might not be able to (if it uses a different proc_macro instance)
      • however, it can always create and run its own Client, but that may be inefficient
  • the bridge uses serialization, C ABI and integer handles to avoid Rust ABI instability
  • each invocation of a proc macro results in disjoint integers in its proc_macro handles
    • this prevents using values of those types across invocations (if they even can be kept)

r? @alexcrichton cc @jseyfried @nikomatsakis @Zoxc @thepowersgang

@eddyb

This comment has been minimized.

Member

eddyb commented Mar 20, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Mar 20, 2018

🔒 Merge conflict

@eddyb eddyb force-pushed the eddyb:proc-macro-decouple branch from 67f317e to 6e63e8d Mar 20, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Mar 20, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Mar 20, 2018

⌛️ Trying commit 6e63e8d with merge c4a77e1...

bors added a commit that referenced this pull request Mar 20, 2018

Auto merge of #49219 - eddyb:proc-macro-decouple, r=<try>
Decouple proc_macro from the rest of the compiler.

This PR removes all dependencies of `proc_macro` on compiler crates and allows multiple copies of `proc_macro`, built even by different compilers (but from the same source), to interoperate.

On the compiler (aka "frontend") side:
* `Registry` is implemented (as until now)
  * instead of function pointers, the `Expand{1,2}` wrapper types are received
* `FrontendInterface` is implemented to provide the concrete type and methods
  * the concrete types are completely separated from the `proc_macro` public API
  * the only use of the implementer is to be passed to `Expand{1,2}::run`

On the proc macro side (potentially using a different `proc_macro` instance):
* `&mut Registry` is passed to the registrar (as until now)
* `Expand{1,2}` wrappers are created to be passed to the `Registry`
  * they encapsulate the `proc_macro` instance used by the macro crate
  * their `run` method will set up the "current frontend" of that instance

`proc_macro` public APIs call into the "current frontend" via the internal `bridge`:
* only a currently running proc macro can interact with those APIs
  * the frontend itself might not be able to (if it uses a different `proc_macro` instance)
* the `bridge` uses C ABI to avoid Rust ABI instability and "generation tagging" for safety
* each invocation of a proc macro results in a different "generation"
  * specifically, opaque `proc_macro` types wrapping concrete frontend types are tagged
  * this prevents using values of those types across invocations (even if they can be kept)
* an ABI mismatch between two versions of `proc_macro` is only possible during stage1
  * specifically, rustc compiled by stage0 but proc macros compiled by stage1
  * can only affect running tests at stage1 or the compiler using proc macros
  * defficiencies in the `bridge` can be addressed without waiting for a release cycle

r? @alexcrichton cc @jseyfried @nikomatsakis @Zoxc @thepowersgang
@eddyb

This comment has been minimized.

Member

eddyb commented Mar 20, 2018

cc @rust-lang/infra We shouldn't merge this without a crater run, although you might want to wait on a review as well (then again, the results could inform the review, so whichever comes first).

@bors

This comment has been minimized.

Contributor

bors commented Mar 21, 2018

☀️ Test successful - status-travis
State: approved= try=True

#[unstable(feature = "proc_macro_internals", issue = "27812")]
#[doc(hidden)]
pub contents: Term,

This comment has been minimized.

@Zoxc

Zoxc Mar 21, 2018

Contributor

Do we want to commit to using interning for literals? I'm not sure interning it terrible useful for that.

This comment has been minimized.

@eddyb

eddyb Mar 21, 2018

Member

Wait, I can make these fields private again. Definitely not considering stabilizing any of this.

@@ -11,3 +11,6 @@ crate-type = ["dylib", "rlib"]
[dependencies]
getopts = "0.2"
term = { path = "../libterm" }
# not actually used but needed to always have proc_macro in the sysroot
proc_macro = { path = "../libproc_macro" }

This comment has been minimized.

@Zoxc

Zoxc Mar 21, 2018

Contributor

This should probably be a rustbuild change.

@bjorn3

This comment has been minimized.

Contributor

bjorn3 commented Mar 21, 2018

Could you use something like bincode for data bridging, instead of 800 lines of own code?

@eddyb

This comment has been minimized.

Member

eddyb commented Mar 22, 2018

@bjorn3 I'm not serializing the data (but rather using handles), and bincode (like any other serialization) is annoying to use without derived impls (which you can't have because this is proc_macro itself - or at least I don't want to try to make that work, for now).

I am going to look into simplifying the bridge code, removing most unsafe, and making it more amenable to message-passing, however.

@bjorn3

This comment has been minimized.

Contributor

bjorn3 commented Mar 22, 2018

I get it.

@alexcrichton

This comment was marked as outdated.

Member

alexcrichton commented Mar 22, 2018

Some awesome progress! The "dream" of deleting libserialize is coming ever closer...

I'm personally having a lot of trouble following what's going on here though. Would it be possible to add a summary comment to the bridge.rs module explaining what's going on in some more depth? The PR description currently references a lot of new types/traits that I'm not sure what's going on after reading the first pass.

Perhaps you could detail an example of what happens when a small/example procedural macro is invoked?

I'm also a little worried about all the FIXME or todo annotations about ABI problems. For example it looks like this will require the ABI of PathBuf to never change, right? (the risk being that if we change the ABI we can't bootstrap the compiler?) There were other things like trait objects which I was also fairly suspicious about. (as well as fmt::Formatter)

One perhaps larger-than-intended change here as well is dealing with the standard library itself. Today because proc_macro links to libsyntax it dynamically links to the standard library, which means that crates linking to the proc_macro crate automatically link dynamically (for better or worse) to the standard library as well. Running around with two copies of the standard library (one in the plugin that we load and one in the compiler itself) can wreak havoc with panic hooks and the like. (or any other form of global state). Do we know how this'll get mitigated?

@eddyb eddyb changed the title from Decouple proc_macro from the rest of the compiler. to [WIP] Decouple proc_macro from the rest of the compiler. Mar 22, 2018

@eddyb eddyb force-pushed the eddyb:proc-macro-decouple branch from 6e63e8d to cd9ac21 Mar 22, 2018

@eddyb

This comment was marked as outdated.

Member

eddyb commented Mar 22, 2018

the risk being that if we change the ABI we can't bootstrap the compiler

I tried to have a reassuring comment in the PR description - we can always bootstrap the compiler, it'd just require fixing the ABI incompatibility within the in-tree libproc_macro.
That is, we don't have to wait 6 weeks to get such a fix in.

I will try to address most if not all such issues before merging the PR - see the TODO bit at the end of the description, I've been convinced, since opening the PR, that I should redo some parts of it.

Perhaps you could detail an example of what happens when a small/example procedural macro is invoked?

Heh, I tried this and ended up with the weird lists in the PR description, I'll try again once I've mad the code changes I plan to (for now I've also added [WIP] to the PR title).

Running around with two copies of the standard library (one in the plugin that we load and one in the compiler itself) can wreak havoc with panic hooks and the like. (or any other form of global state). Do we know how this'll get mitigated?

Did you see the commit where I moved the panic silencing hook from the compiler to proc_macro?
Even if we're still single-process for now, we could say that the proc macro ("the client") might run in a different process (thus use a different libstd) than the compiler frontend ("the server").
So cross-process solutions (such as making proc_macro state self-contained and serializing to plain bytes, or using the C ABI as-if it were serializing) should also deal with multiple libstd in-process.

@Mark-Simulacrum

This comment has been minimized.

@eddyb

This comment has been minimized.

Member

eddyb commented Mar 23, 2018

@Mark-Simulacrum It doesn't look bad, that's for sure, thanks! If we switch to spawning a thread (or even a process) per invocation it'd warrant another perf run.

@eddyb eddyb force-pushed the eddyb:proc-macro-decouple branch from cd9ac21 to 76590d8 Mar 23, 2018

Phlosioneer added a commit to Phlosioneer/rust that referenced this pull request Mar 25, 2018

Ignore proc macro stage 1 tests
A few tests related to proc-macros fail when performing stage-1
testing. This PR adds // ignore-stage1 to them, along with a FIXME.

Original issue: rust-lang#49352

This should (hopefully) be fixed when rust-lang#49219 is merged.
@aidanhs

This comment has been minimized.

Member

aidanhs commented Mar 26, 2018

Crater run started

@aidanhs

This comment has been minimized.

Member

aidanhs commented Mar 31, 2018

Hi @eddyb (crater requester), @Zoxc (PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49219/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

@eddyb eddyb force-pushed the eddyb:proc-macro-decouple branch from 329e4b8 to 3a04d44 Nov 30, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 30, 2018

@bors r=alexcrichton

@bors

This comment has been minimized.

Contributor

bors commented Nov 30, 2018

📌 Commit 3a04d44 has been approved by alexcrichton

@bors

This comment has been minimized.

Contributor

bors commented Nov 30, 2018

⌛️ Testing commit 3a04d44 with merge d48ab69...

bors added a commit that referenced this pull request Nov 30, 2018

Auto merge of #49219 - eddyb:proc-macro-decouple, r=alexcrichton
Decouple proc_macro from the rest of the compiler.

This PR removes all dependencies of `proc_macro` on compiler crates and allows multiple copies of `proc_macro`, built even by different compilers (but from the same source), to interoperate.

Practically, it allows:
* running proc macro tests at stage1 (I moved most from `-fulldeps` to the regular suites)
* using proc macros in the compiler itself (may require some rustbuild trickery)

On the server (i.e. compiler front-end) side:
* `server::*` traits are implemented to provide the concrete types and methods
  * the concrete types are completely separated from the `proc_macro` public API
  * the only use of the type implementing `Server` is to be passed to `Client::run`

On the client (i.e. proc macro) side (potentially using a different `proc_macro` instance!):
* `client::Client` wraps around client-side (expansion) function pointers
  * it encapsulates the `proc_macro` instance used by the client
  * its `run` method can be called by a server, to execute the client-side function
    * the client instance is bridged to the provided server, while it runs
    * ~~currently a thread is spawned, could use process isolation in the future~~
(not the case anymore, see #56058)
* proc macro crates get a generated `static` holding a `&[ProcMacro]`
  * this describes all derives/attr/bang proc macros, replacing the "registrar" function
  * each variant of `ProcMacro` contains an appropriately typed `Client<fn(...) -> ...>`

`proc_macro` public APIs call into the server via an internal "bridge":
* only a currently running proc macro `Client` can interact with those APIs
  * server code might not be able to (if it uses a different `proc_macro` instance)
    * however, it can always create and `run` its own `Client`, but that may be inefficient
* the `bridge` uses serialization, C ABI and integer handles to avoid Rust ABI instability
* each invocation of a proc macro results in disjoint integers in its `proc_macro` handles
  * this prevents using values of those types across invocations (if they even can be kept)

r? @alexcrichton cc @jseyfried @nikomatsakis @Zoxc @thepowersgang
@bors

This comment has been minimized.

Contributor

bors commented Nov 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d48ab69 to master...

@bors bors merged commit 3a04d44 into rust-lang:master Nov 30, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 30, 2018

I can't believe you've done this!

@eddyb eddyb deleted the eddyb:proc-macro-decouple branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment