jakobhellermann / cargo-watt

cargo subcommand for building proc-macro crates with web assembly
MIT License
34 stars 4 forks source link

Interoperability with proc-macro-error #3

Open CreepySkeleton opened 4 years ago

CreepySkeleton commented 4 years ago

Author of proc-macro-error here.

It's a shame that "replace proc_macro with proc_macro2" approach doesn't work with my crate. I suspect this is because #[proc_macro_error] has some proc_macro::TokenStream in its expansion, and the expansion is done after the grand replacement.

#[proc_macro_error]
fn my_macro(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
    // code
}

// after replacement:
#[proc_macro_error]
fn my_macro(input: proc_macro2::TokenStream) -> proc_macro2::TokenStream {
    // code
}
// after expansion:
fn my_macro(input: proc_macro2::TokenStream) -> proc_macro::TokenStream {
    // a little mangled code
}

Come to think of it, PME doesn't really care about the types there. It's just that proc macro facility expects the TokenStream to be from proc_macro, not from elsewhere, and my crate has to accommodate. But it's no longer the case with your crate.

I have a little compact hack in mind:

You crate searches for #[proc_macro_error] and replaces it with #[proc_macro_error(__cargo_watt_compat)]. On the other side, my crate detects it and uses proc_macro2 instead of proc_macro.

Would you be open to accept such a PR?

jakobhellermann commented 4 years ago

sure, that would be nice

jakobhellermann commented 4 years ago

Actually, I think a tweak in the macro will probably not be enough. Since cargo watt patches syn to use proc_macro2 instead of proc_macro, other crates (like proc_macro_error) are likely to break.

For procedural macros that just depend on syn and quote this is not a big deal, because they too have proc_macro replaced. But for those depending on synstructure, proc_macro_error or similar it's not so easy.

Another solution would be to detect if the crate uses proc_macro_error and patch it (and proc_macro_error_attr) to a compatible version, like this. That has the disadvantage that it would only support the newest (or a compatible) version of proc_macro_error.

CreepySkeleton commented 4 years ago

First of all, I've managed to tweak my crate to work with both proc_macro and proc_macro2 without any changes on your side.

But yeah, I can't just stop depending on syn, the dep would have to be patched, too. Actually, I depend on syn-mid, another proc macro helper, which also depends on syn.

If you want to continue relying on the dumb replacement, you have only two options:

And that's not to mention that some macros use unstable types from proc_macro on nigthly (proc-macro-error does) proc_macro2 lacks. Nightly builds will likely break.

jakobhellermann commented 4 years ago

I don't think it is possible to make any approach work flawlessly for every crate, at least not without wasm-macros being implemented as first-class citizens in rustc.

So I think I will just accept (and clarify in the README) that cargo watt will never work for all crates, because of its trial and error approach which comes to its limits pretty quickly.

Still, I think it may be feasible to either provide patches for popular macro helper libraries (like syn-mid, synstructure and proc_macro_error) or at least make it easy to add these patches yourself (--patch crate repo for example).

FWIW, I made proc_macro_error (not proc_macro_error_attr) work by running

sd 'proc_macro::' 'proc_macro2::' src/*.rs
# conflicting impl
sed -i '/impl DoubleSpanSingleSpan for proc_macro2::Span {/,+4d' src/lib.rs

so if the macro can be made to work with both TokenStreams, adding support for proc_macro_error should just be a matter of adding a patched git repo to a list of automatically applied patches.

Freax13 commented 4 years ago

it's possible to get proc_macro working outside a macro on nightly by implementing proc_macro::bridge::server::Server and using proc_macro::bridge::client::Client. wouldn't that solve most of the compatibility issues?

jakobhellermann commented 4 years ago

I didn't know about proc_macro::bridge. Where can I find documentation about it?

Freax13 commented 4 years ago

https://github.com/rust-lang/rust/blob/master/src/libproc_macro/bridge/mod.rs it's only meant to be used internally, but you can use it with #![feature(proc_macro_internals)]

Freax13 commented 4 years ago

it's not documented anywhere afaict, so you kinda have to read the source code

jakobhellermann commented 4 years ago

I wonder whether this could be used in watt directly? If watt could handle pub extern "C" fn the_macro(input: proc_macro::TokenStream) -> proc_macro::TokenStream (not proc_macro2), then the translation from regular macro to watt macro would be very easy.

Freax13 commented 4 years ago

probably

pksunkara commented 4 years ago

@CreepySkeleton, I was talking about this on Reddit but what about the approach of the watt feature in syn and proc_macro_error? I think that would solve all our issues.

We add the feature to syn first eliminating the need for the patch. Then, we add support for this feature to proc_macro_error, then we can simply enable these features and compile clap_derive with cargo watt build. I think the other creates synstructure etc.. can start propagating this feature and at the end all the crates should work with watt.

jakobhellermann commented 4 years ago

I looked into what it would take to support watt as a feature in syn, it doesn't seem to be that complicated: https://github.com/jakobhellermann/syn-watt/commits/watt-feature

pksunkara commented 4 years ago

Yup, I looked at your patch and did a little digging before suggesting this.

CreepySkeleton commented 4 years ago

A dedicated feature was actually the first thing I thought about but decided that it would be quite a lame solution.

You see, features are user facing, now more than ever since rustdoc has been working on explicit cfg tags. The first thing crossing user's mind when they see a feature is "How - and for what - can I use it?". In our case, the answer would be that they don't, it's just an implementation detail that was supposed to be hidden from users but wasn't. Also, such a feature in proc-macro-error, syn, whatever will become a marker of sorts which will divide proc macros to those that work with watt and those do not. This all sounds very unnecessary.

I like @Freax13 idea - orchestrate the macro expansion on our own, either by implementing Server or patching libproc_macro as whole and replacing thr default dynamic lib with our own via LD_PRELOAD or some other hack.

It looks like @Freax13 is working on it! https://github.com/dtolnay/watt/issues/42