Begin reorganization of proc_macro crate #134401

Aversefun · 2024-12-17T00:01:07Z

The tracking issue for this is #130856. This PR begins reorganizing the proc_macro crate as a prerequisite to beginning implementing the main portion of the linked issue.

r? @tgross35

rustbot · 2024-12-17T00:01:18Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

tgross35 · 2024-12-17T00:06:52Z

This PR may fail because https://github.com/rust-lang/rust/blob/6d9f6ae36ae1299d6126ba40c15191f7aa3b79d8/compiler/rustc_expand/src/proc_macro.rs uses proc_macro::bridge. Mybe bridge should remain a top-level module.

Edit: guess that already happened, see above.

tgross35 · 2024-12-17T00:20:26Z

To clarify some of my thoughts here: the reason for this refactor would be to give most of proc_macro (i.e. the things in diagnostic.rs and lib.rs) an interface that abstracts over what the actual backend is - rustc via RPC or the standalone backend. So for now, backend should pub use bridge::{Diagnostic, client::{TokenStream, Span, Symbol}} and then everything else in the crate should use backend::Foo rather than backend::bridge::Foo. Then once there is an actual standalone backend, backend will wrap this instead of just reexporting bridge.

^ That should be captured in the module docs for backend somehow.

For the above error, I think the cleanest way to fix this while keeping refactoring is to reexport the bridge code in lib.rs:

#[doc(hidden)]
pub mod bridge {
    pub use super::backend::bridge::*;
}

library/proc_macro/src/backend/mod.rs

tgross35 · 2024-12-17T01:56:52Z

After thinking about it a bit further, this naming convention seems like it would be more accurate:

"Backend": Everything related to how proc_macro is backed. Parent of everything else on this list.
"Client": The proc_macro side of a backend (definition unchanged)
"Server": Something abstract that communicates with the client
"Bridge": A specific server that communicates with rustc
"Standalone": A specific server that doesn't need rustc (does not yet exist)

So, suggestions for how this could be refactored a bit (each mod is an actual file):

// lib.rs

mod backend { // backend/mod.rs
    mod support { // backend/support/mod.rs
        //! All data structures that support backends but aren't interface-specific.
        // None of this should depend on `client`, `server`, or `bridge`.

        mod fxhash; // backend/support/fxhash.rs
        mod arena; // backend/support/arena.rs
        mod buffer; // backend/support/buffer.rs
        mod closure; // backend/support/closure.rs
        mod handle; // backend/support/handle.rs
        mod selfless_reify; // backend/support/selfless_reify.rs
    }

    /// Interfaces and types representing `proc_macro` as a client.
    pub mod client; // backend/client.rs

    /// Abstract interfaces for `proc_macro` servers.
    pub mod server; // backend/server.rs

    /// Serialization between a client and a server.
    pub mod rpc; // backend/rpc.rs

    /// Everything for a bridged backend to `rustc` or `rust-analyzer`.
    pub mod bridge; // backend/bridge.rs

    // Eventually there will be a `mod standalone` here

    mod symbol; // backend/symbol.rs

    pub use support::{fxhash::FxHashMap, arena::Arena, etc};
}

/// Public interfaces for use by the bridge backend
// This one can just be within lib.rs
#[doc(hidden)]
pub mod bridge {
    pub use super::backend::bridge::{
        DelimSpan, Diagnostic, ExpnGlobals, Group,
        Ident, LitKind, Literal, Punct, TokenTree,
    };

    pub use super::backend::{client, server};
}

Aversefun · 2024-12-17T02:11:27Z

One more thing that we should come up with a vocab for(not feeling very creative right now) is one of (bridge, standalone).

EDIT: Whoops, wasn't thinking, the name is "servers" 😅

Aversefun · 2024-12-17T02:12:21Z

Separate from that, I think that under the server/client modules we should have separate files for bridge v. standalone and they'd be conditionally compiled while implementing the same interface. Thoughts?

Aversefun · 2024-12-17T02:14:26Z

That might've been what you were saying, not sure.

Aversefun · 2024-12-17T02:19:17Z

Also, thoughts on adding a module common for things that both the server and client may need?

Aversefun · 2024-12-17T02:20:00Z

Annnd nevermind. I don't think that's actually necessary.

Aversefun · 2024-12-17T02:26:04Z

Sorry for all of the pings, but I'm trying to decide where bridge/mod.rs should go. It contains a good amount of code that appears as though it should be exported, should I just keep it as bridge.rs? Until further notice, that's what I'll do.

Aversefun · 2024-12-17T02:29:56Z

Aaand I do need to make a common directory. bridge/mod.rs, the client, and the server all need access to rpc.rs.

tgross35 · 2024-12-17T02:56:42Z

Separate from that, I think that under the server/client modules we should have separate files for bridge v. standalone and they'd be conditionally compiled while implementing the same interface. Thoughts?

We can't have any conditional compilation in the final product. Formerly I thought that using cfg to select a module would be easiest for development, but I'm not really thinking so anymore. Instead, once there is a standalone module, we just need to expose a way to register this (along the lines of what I mentioned here https://rust-lang.zulipchat.com/#narrow/channel/404510-wg-macros/topic/Design.20work.20for.20.60proc_macro.60.20outside.20of.20proc.20macro.20crates/near/489377716).

Also, thoughts on adding a module common for things that both the server and client may need?

The data structure-type things like fxhash.rs, arena.rs etc I would put in backend/support/.

Sorry for all of the pings,

Don't be, happy to work through this :)

but I'm trying to decide where bridge/mod.rs should go. It contains a good amount of code that appears as though it should be exported, should I just keep it as bridge.rs? Until further notice, that's what I'll do.

Just rename this one to backend/bridge.rs. Since everything moves out from under it, it shouldn't need to be a directory anymore.

There will wind up being two modules called bridge: one as an inline module mod bridge {...} in lib.rs, and this one at backend/bridge.rs. The inline module will just reexport specific things from backend::bridge, client, server, etc that are needed by rustc or rust-analyzer. (Specific reexports rather than the whole module or * is an improvement over what we have now, makes the API more clear and avoids exporting more than necessary).

Aaand I do need to make a common directory. bridge/mod.rs, the client, and the server all need access to rpc.rs.

I think this can just be at backend/rpc.rs since it is kind of glue between a client and server.

Aversefun · 2024-12-17T12:50:44Z

We can't have any conditional compilation in the final product. Formerly I thought that using cfg to select a module would be easiest for development, but I'm not really thinking so anymore. Instead, once there is a standalone module, we just need to expose a way to register this (along the lines of what I mentioned here rust-lang.zulipchat.com#narrow/channel/404510-wg-macros/topic/Design.20work.20for.20.60proc_macro.60.20outside.20of.20proc.20macro.20crates/near/489377716).

Okay I see, that would make sense. Actually, would we even need a standalone server? Since proc macros are meant to be compiled as a dynamic library, if we add a lint to prevent using proc macros as regular functions, then we should be able to just not have a standalone server as the proc macros would never get actually called.

Aversefun · 2024-12-17T13:15:19Z

Why does github not have the option to force-push in the online thing 😭

rust-log-analyzer · 2024-12-17T13:51:58Z

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#20 exporting to docker image format
#20 sending tarball 26.9s done
#20 DONE 39.7s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
To only update this specific test, also pass `--test-args proc-macro/bad-projection.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/proc-macro/bad-projection.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/proc-macro/bad-projection" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
--- stderr -------------------------------
error[E0277]: the trait bound `(): Project` is not satisfied
##[error]  --> /checkout/tests/ui/proc-macro/bad-projection.rs:14:17
   |
   |
LL | pub fn uwu() -> <() as Project>::Assoc {}
   |                 ^^^^^^^^^^^^^^^^^^^^^^ the trait `Project` is not implemented for `()`
help: this trait has no implementations, consider adding one
  --> /checkout/tests/ui/proc-macro/bad-projection.rs:9:1
   |
LL | trait Project {
LL | trait Project {
   | ^^^^^^^^^^^^^

error[E0593]: function is expected to take 1 argument, but it takes 0 arguments
##[error]  --> /checkout/tests/ui/proc-macro/bad-projection.rs:14:1
   |
LL | pub fn uwu() -> <() as Project>::Assoc {}
   | |
   | expected function that takes 1 argument
   | takes 0 arguments
   | required by a bound introduced by this call
   | required by a bound introduced by this call
   |
note: required by a bound in `ProcMacro::bang`
  --> /rustc/FAKE_PREFIX/library/proc_macro/src/backend/bridge/client.rs:409:5

error[E0277]: the trait bound `(): Project` is not satisfied
##[error]  --> /checkout/tests/ui/proc-macro/bad-projection.rs:14:1
   |
LL | pub fn uwu() -> <() as Project>::Assoc {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Project` is not implemented for `()`
help: this trait has no implementations, consider adding one
  --> /checkout/tests/ui/proc-macro/bad-projection.rs:9:1
   |
LL | trait Project {
LL | trait Project {
   | ^^^^^^^^^^^^^

error[E0277]: the trait bound `(): Project` is not satisfied
##[error]  --> /checkout/tests/ui/proc-macro/bad-projection.rs:14:1
   |
LL | pub fn uwu() -> <() as Project>::Assoc {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Project` is not implemented for `()`
help: this trait has no implementations, consider adding one
  --> /checkout/tests/ui/proc-macro/bad-projection.rs:9:1
   |
LL | trait Project {
LL | trait Project {
   | ^^^^^^^^^^^^^

error[E0277]: the trait bound `(): Project` is not satisfied
##[error]  --> /checkout/tests/ui/proc-macro/bad-projection.rs:14:40
   |
LL | pub fn uwu() -> <() as Project>::Assoc {}
   |                                        ^^ the trait `Project` is not implemented for `()`
help: this trait has no implementations, consider adding one
  --> /checkout/tests/ui/proc-macro/bad-projection.rs:9:1
   |
LL | trait Project {

tgross35 · 2024-12-17T20:35:11Z

We can't have any conditional compilation in the final product. Formerly I thought that using cfg to select a module would be easiest for development, but I'm not really thinking so anymore. Instead, once there is a standalone module, we just need to expose a way to register this (along the lines of what I mentioned here rust-lang.zulipchat.com#narrow/channel/404510-wg-macros/topic/Design.20work.20for.20.60proc_macro.60.20outside.20of.20proc.20macro.20crates/near/489377716).

Okay I see, that would make sense. Actually, would we even need a standalone server? Since proc macros are meant to be compiled as a dynamic library, if we add a lint to prevent using proc macros as regular functions, then we should be able to just not have a standalone server as the proc macros would never get actually called.

I am not sure I follow - the end goal is to make proc_macro useful when not actually built as a proc macro, how does linting against their use help us?

Why does github not have the option to force-push in the online thing 😭

Don't worry about the merge commit warning, I'll ask you to clean up history before merge.

tgross35 · 2024-12-17T20:50:07Z

@rustbot author for the things discussed in #134401 (comment) (just comment @rustbot review when this is ready for another look)

tgross35 · 2025-01-08T22:53:32Z

Hey @AverseABFun, I know you were on the fence about continued macro work but are you interested in finishing this up? Changing this to reflect #134401 (comment) is still needed, I don't think there is too much more needed for that.

Aversefun · 2025-01-09T02:17:31Z

Whoops, I completely forgot about this! I have some stuff I need to do first but I'll finish it when I can.

…

On Wed, Jan 8, 2025 at 4:53 PM Trevor Gross ***@***.***> wrote: Hey @AverseABFun <https://github.com/AverseABFun> are you interested in finishing this up? Changing this to reflect #134401 (comment) <#134401 (comment)> is still needed, I don't think there is too much more needed for that. — Reply to this email directly, view it on GitHub <#134401 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ATOOUFRQ7QH4A6DCOUYLM5L2JWUAFAVCNFSM6AAAAABTXHVFS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZYHAZTEOJRHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

alex-semenyuk · 2025-02-17T08:10:03Z

@AverseABFun
Thanks for your contribution
From wg-triage. Do you have any updates on this PR

Aversefun · 2025-02-17T14:50:52Z

Sorry! Can someone else pick this up? If not, I'll try to get around to this ASAP.

…

On Mon, Feb 17, 2025 at 2:10 AM alexey semenyuk ***@***.***> wrote: @AverseABFun <https://github.com/AverseABFun> Thanks for your contribution From wg-triage. Do you have any updates on this PR — Reply to this email directly, view it on GitHub <#134401 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ATOOUFTRHSWBW3BKAIE7TSL2QGKPHAVCNFSM6AAAAABTXHVFS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRSGM2DGMZTGU> . You are receiving this because you were mentioned.Message ID: ***@***.***> [image: alex-semenyuk]*alex-semenyuk* left a comment (rust-lang/rust#134401) <#134401 (comment)> @AverseABFun <https://github.com/AverseABFun> Thanks for your contribution From wg-triage. Do you have any updates on this PR — Reply to this email directly, view it on GitHub <#134401 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ATOOUFTRHSWBW3BKAIE7TSL2QGKPHAVCNFSM6AAAAABTXHVFS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRSGM2DGMZTGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

alex-semenyuk · 2025-05-27T08:33:14Z

@Aversefun
From wg-triage. Closed this PR due to inactivity. Feel free to reopen or raised new one. Thanks for your efforts.

lmmx · 2025-05-30T11:58:47Z

Aw no, looks like I just arrived a few days too late. This PR was mentioned in this video (at 9:18):

The proc macro API is not available to non-proc macro crates so if you want to be able to write unit tests, you need some sort of abstraction layer. There's a tracking issue to make the proc macro API available to non-proc macro crates with a PR that needs someone to adopt it.

Is there a concise TODO list of what is unfinished exactly? From reviewing the above I can see

this comment Dec 17th 2024

After thinking about it a bit further, this naming convention seems like it would be more accurate:

"Backend": Everything related to how proc_macro is backed. Parent of everything else on this list.

"Client": The proc_macro side of a backend (definition unchanged)

"Server": Something abstract that communicates with the client

"Bridge": A specific server that communicates with rustc

"Standalone": A specific server that doesn't need rustc (does not yet exist)

Create a backend/support/ directory for shared data structures
Properly separate client, server, and bridge components
Set up the module structure to eventually support both bridged and standalone backends

Fix the compilation errors

The PR was failing tests because some imports weren't properly updated

Create the abstraction layer

Eventually implement a way to switch between the rustc bridge backend and a standalone backend

tgross35 · 2025-05-30T15:07:15Z

@lmmx your description sounds accurate to me! If you are interested, feel free to put up a reorganization PR and request a review from me (either with this one as a base or from scratch). The reorg should be pretty easy, after that I think adding the abstractions / bridge should be straightforward.

Happy to help if you have any questions! We also have a chat for dev-related things at https://rust-lang.zulipchat.com/.

rustbot assigned tgross35 Dec 17, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2024