-
-
Notifications
You must be signed in to change notification settings - Fork 793
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 serde_macros from most unstable libsyntax changes #406
Conversation
This PR rewrites `serde_macros` to decouple it from most libsyntax changes by instead having the annotated item be rendered into text then processed by syntex and serde_codegen. This means that `serde_macros` users will only be broken by a nightly upgrade if a very small selection of the API is changed. Unfortunately this does mean that compiling `serde_macros` will now take a minute longer since it'll also be compiling syntex. Interestingly enough, this is not a breaking change since none of the serde_macros API is publicly exposed.
I predict this will make many people unhappy. Instead, how about a |
oh wait... I got |
No, all the with-syntex stuff is currently in serde_codegen not serde_macros. The plugin_registrar registers expand_derive_serialize which calls expand which uses Syntex. |
Yea, I got confused. I think having a fallback is a good idea, especially since code containing new rust-syntax won't work if syntex doesn't support the new rust-syntax yet. |
How come the compile-fail tests failed? |
So I tracked down the issue. Syntex's The second issue is that syntex doesn't support or get informed about "--error-format json", and so compiletest_rs 2.0 doesn't work
|
I think this might also be failing because the error span information is wrong. This effectively moves the annotated item to the first line, which probably confuses compiletest's error annotation. I have some ideas on how to fix this:
@dtolnay: Yeah, a feature for the old behavior might make sense. However, one of the biggest complaints that I've heard is that serde breaks all the time, because of this underlying issue with nightly. I'd much rather have people complain about how slow serde_macros can be to compile, which, in my opinion, is a much healthier place to be in. We at least can then start to figure out how we can speed up syntex's compilation. Besides, in our glorious future once we have a stable libmacro, users of serde_macros would have to use something like syntex to parse items anyway, so our users can't really escape these compile time issues. Also, one thing @Manishearth pointed out to me, at least in regards to Servo, is that they're transitively compiling syntex through some of their dependencies anyway. So I think that for a sufficiently large project, the odds of compiling syntex may eventually converge to 100%, so there might not actually be a real impact here. Finally, for our mental sanity here, if we give people the old behavior through a feature flag, we'll still have to do the frantic syntex upgrade dance whenever libsyntax changes out from under us. By just using this new approach, we could recover those hours back for other things :) |
Closing in favor of #530. |
This PR rewrites
serde_macros
to decouple it from most libsyntax changes by instead having the annotated item be rendered into text then processed by syntex and serde_codegen. This means thatserde_macros
users will only be broken by a nightly upgrade if a very small selection of the API is changed. Unfortunately this does mean that compilingserde_macros
will now take a minute longer since it'll also be compiling syntex.Interestingly enough, this is not a breaking change since none of the
serde_macros
API is publicly exposed.