Skip to content
This repository was archived by the owner on Dec 17, 2025. It is now read-only.

Switch public interface to always use proc-macro2#93

Closed
elichai wants to merge 4 commits into
bincode-org:trunkfrom
elichai:proc-macro2
Closed

Switch public interface to always use proc-macro2#93
elichai wants to merge 4 commits into
bincode-org:trunkfrom
elichai:proc-macro2

Conversation

@elichai

@elichai elichai commented Jun 23, 2025

Copy link
Copy Markdown
Contributor

This aligns more with how quote works:

Performs variable interpolation against the input and produces it as proc_macro2::TokenStream.

Note: for returning tokens to the compiler in a procedural macro, use .into() on the result to convert to proc_macro::TokenStream.

Also: https://github.com/dtolnay/quote/blob/62fd385a800f7398ab416c00100664479261a86e/Cargo.toml#L15-L26

In practice, I got to it because I encountered the following problem:
I have a workspace similiar to bincode/serde, where I have a workspace with crate A and a derive crate,
the derive uses virtue and contains a bunch of test vectors that are done by using proc_macro2 in the proc-macro unit tests.
crate A also depends on bincode(in tests/benchmarks) and uses its derive proc-macro.
This means that when I run cargo t --all, then the tests of crate A will use bincode-derive->virtue with the proc-macro2 feature enabled (as they are needed in the derive crate dev-depdencies).

This causes bincode-derive to fail with this error:

error[E0308]: mismatched types
   --> /Users/elichai2/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bincode_derive-2.0.1/src/lib.rs:10:25
    |
10  |     derive_encode_inner(input).unwrap_or_else(|e| e.into_token_stream())
    |     ------------------- ^^^^^ expected `virtue::prelude::TokenStream`, found `proc_macro::TokenStream`
    |     |
    |     arguments to this function are incorrect

The intrinsic problem here is that the current proc-macro2 feature is not strictly additive,
I also thought about removing TokenStream from the prelude of this crate, but decided to leave it as proc_macro2::TokenStream but I'm more than willing to just remove it and force users to explicitly import from the "right" place

@elichai
elichai force-pushed the proc-macro2 branch 2 times, most recently from 2bac2c6 to f7540c0 Compare June 23, 2025 15:07
@elichai

elichai commented Jun 23, 2025

Copy link
Copy Markdown
Contributor Author

The code coverage fails even after upgrading, I can try to re-write the whole action but I think this is mostly out of scope for this PR :)

@VictorKoenders

Copy link
Copy Markdown
Contributor

We specifically do not want to have proc-macro2 as a dependency. From the docs:

  • Zero dependencies, so fast compile times
  • No other dependencies needed

The only reason we have proc-macro2 is so we can test the builder structs to generate the expected output. Developers should use virtue as a standalone crate, and this should not pull in proc-macro2 because of virtue.

@VictorKoenders

Copy link
Copy Markdown
Contributor

I started with ripping out proc-macro2 and writing a custom shim in the feat/remove-proc-macro2 branch. Going to close this in favor of that solution

@elichai

elichai commented Jun 23, 2025

Copy link
Copy Markdown
Contributor Author

@VictorKoenders are you thinking about exposing this shim to downstream crates?
They also need to shim TokenStream in order to write unit tests

@VictorKoenders

Copy link
Copy Markdown
Contributor

No, as the shim will only be enabled in test mode so it should not be accessible to other crates.

If there's a desire for this you can always grab the code and put it in your own project. I recommend not exposing it as a library, because then you get the same shimming problems as you've had before

@elichai

elichai commented Jun 23, 2025

Copy link
Copy Markdown
Contributor Author

@VictorKoenders I have a derive crate using virtue, and I have tests for that crate, currently using proc-macro2,
Ideally I'd like to keep using virtue but also keep testing my code via regular unit tests,
I don't think I can use virtue in a non proc-macro(tests) if there's no shim

Does bincode-derive not have input-output tests using TokenStream?

@VictorKoenders

Copy link
Copy Markdown
Contributor

Bincode does behavioral testing, we test if the generated interfaces behave as expected.

One way I can think of to do this is have a ...Context type, e.g. ProcMacroContext, ProcMacro2Context and VirtueTestContext. The ProcMacro2Context would be behind a feature flag. We'd have to figure out how to make this api ergonomic to the user.

Maybe

let parse = Parse::new(input)?;

Can default to ProcMacroContext::parse(input), and the user can switch to ProcMacro2Context and VirtueTestContext if they want a different implementation

@elichai

elichai commented Jun 24, 2025

Copy link
Copy Markdown
Contributor Author

Bincode does behavioral testing, we test if the generated interfaces behave as expected.

One way I can think of to do this is have a ...Context type, e.g. ProcMacroContext, ProcMacro2Context and VirtueTestContext. The ProcMacro2Context would be behind a feature flag. We'd have to figure out how to make this api ergonomic to the user.

Maybe

let parse = Parse::new(input)?;

Can default to ProcMacroContext::parse(input), and the user can switch to ProcMacro2Context and VirtueTestContext if they want a different implementation

The only thing is that the features here should be additive, we can gate a separate Parse::new2(input) or Parse::test(input) that is feature gated and always uses the "right" type.

Personally I don't mind if it's ProcMacro2Context or VirtueTestContext as long as I can call TokenStream::to_string to test output against committed code.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants