Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

erickt
Copy link
Member

@erickt erickt commented Jun 23, 2016

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.

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.
@dtolnay
Copy link
Member

dtolnay commented Jun 23, 2016

Unfortunately this does mean that compiling serde_macros will now take a minute longer since it'll also be compiling syntex.

I predict this will make many people unhappy. Instead, how about a with-syntex feature for serde_macros that enables this? Or a feature that enables the old behavior.

@oli-obk
Copy link
Member

oli-obk commented Jun 23, 2016

I predict this will make many people unhappy. Instead, how about a with-syntex feature for serde_macros that enables this? Or a feature that enables the old behavior.

the way I read the changes, this is exactly how it's implemented.

oh wait... I got serde_macros and serde_codegen mixed up

@dtolnay
Copy link
Member

dtolnay commented Jun 23, 2016

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.

@oli-obk
Copy link
Member

oli-obk commented Jun 23, 2016

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.

@dtolnay
Copy link
Member

dtolnay commented Jun 23, 2016

How come the compile-fail tests failed?

@oli-obk
Copy link
Member

oli-obk commented Jun 29, 2016

So I tracked down the issue. Syntex's abort_if_errors does a regular panic, since it obviously doesn't know about rustc, so this is emitted an an ICE.

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

$ RUST_BACKTRACE=1 rustc tests/compile-fail/str_ref_deser.rs -L /tmp --target=x86_64-unknown-linux-gnu --error-format json -Z unstable-options -L /tmp/str_ref_deser.stage-id.compile-fail.libaux -C prefer-dynamic -o /tmp/str_ref_deser.stage-id -L target/debug/ -L target/debug/deps/
tests/compile-fail/str_ref_deser.rs:4:5: 4:15 error: Serde does not support deserializing fields of type &str; consider using String instead
tests/compile-fail/str_ref_deser.rs:4     s: &'a str,
                                          ^~~~~~~~~~
error: aborting due to previous error
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: run with `RUST_BACKTRACE=1` for a backtrace
thread 'rustc' panicked at 'Box<Any>', .multirust/toolchains/nightly/cargo/registry/src/github.com-1ecc6299db9ec823/syntex_syntax-0.36.0/src/errors/mod.rs:622
stack backtrace:
   1:     0x7fa210b2cdef - std::sys::backtrace::tracing::imp::write::h6528da8103c51ab9
   2:     0x7fa210b3addb - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hbe741a5cc3c49508
   3:     0x7fa210b3a978 - std::panicking::default_hook::he0146e6a74621cb4
   4:     0x7fa210b0067e - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
   5:     0x7fa205827da6 - std::panicking::begin_panic::hc84ce018c2abe42b
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:328
   6:     0x7fa205827774 - syntex_syntax::errors::Handler::abort_if_errors::h10e9d4d20b13eb89
                        at Projects/rust/serde/serde_macros/<std macros>:3
   7:     0x7fa205cc5f05 - syntex_syntax::ext::expand::expand_crate::h8e1f8cd6bc7e9b1a
                        at .multirust/toolchains/nightly/cargo/registry/src/github.com-1ecc6299db9ec823/syntex_syntax-0.36.0/src/ext/expand.rs:1177
   8:     0x7fa205797cca - syntex::Registry::expand_crate::h1c6c806dc4bb9075
                        at .multirust/toolchains/nightly/cargo/registry/src/github.com-1ecc6299db9ec823/syntex-0.36.0/src/lib.rs:215
   9:     0x7fa205797077 - syntex::Registry::expand_str::h23fc366b75858d74
                        at .multirust/toolchains/nightly/cargo/registry/src/github.com-1ecc6299db9ec823/syntex-0.36.0/src/lib.rs:189
  10:     0x7fa205500ea7 - serde_macros::expand::ha8125bcd9b2455b1
                        at Projects/rust/serde/serde_macros/src/lib.rs:81
  11:     0x7fa20550090c - serde_macros::expand_derive_deserialize::hfe8a0d81395b8960
                        at Projects/rust/serde/serde_macros/src/lib.rs:42
  12:     0x7fa20551046e - fn(&mut syntax..ext..base..ExtCtxt<'_>, syntax..codemap..Span, &syntax..codemap..Spanned<syntax..ast..MetaItemKind>, &syntax..ext..base..Annotatable, &mut std..ops..FnMut(syntax..ext..base..Annotatable)) $u7b$expand_derive_deserialize$u7d$::fn_pointer_shim.21895::hf4518da46620d136
  13:     0x7fa20551043d - _<F as syntax..ext..base..MultiItemDecorator>::expand::h7d6258fd3697d196
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libsyntax/ext/base.rs:131
  14:     0x7fa20c189cd0 - syntax::ext::expand::expand_decorators::hc82ea6a8744982f5
  15:     0x7fa20c189655 - syntax::ext::expand::decorate::hdcef6d2e36a4f9b4
  16:     0x7fa20c18921f - _<core..iter..FlatMap<I, U, F> as core..iter..iterator..Iterator>::next::hcae9464b75a27f16
  17:     0x7fa20c188ada - _<syntax..util..small_vector..SmallVector<T> as core..iter..traits..FromIterator<T>>::from_iter::h9b58f417adb12cf3
  18:     0x7fa20c183a0f - syntax::ext::expand::expand_multi_modified::h1278d11b5e015efd
  19:     0x7fa20c1410d2 - syntax::ext::expand::expand_annotatable::h05daf8dbfd45385d
  20:     0x7fa20c18e977 - _<core..iter..FlatMap<I, U, F> as core..iter..iterator..Iterator>::next::hb87f500b699cbf5e
  21:     0x7fa20c18e2aa - _<syntax..util..small_vector..SmallVector<T> as core..iter..traits..FromIterator<T>>::from_iter::hd28b5874498e39df
  22:     0x7fa20c141343 - syntax::ext::expand::expand_annotatable::h05daf8dbfd45385d
  23:     0x7fa20c13f974 - syntax::ext::expand::expand_item::h0c304e718d09cf1e
  24:     0x7fa20c149481 - _<syntax..ext..expand..MacroExpander<'a, 'b> as syntax..fold..Folder>::fold_item::h7e3a9cc89287eb31
  25:     0x7fa20c14913f - syntax::fold::noop_fold_mod::h9b639661453b4e4a
  26:     0x7fa20c14263d - syntax::ext::expand::expand_item_kind::h77370e172ebc6967
  27:     0x7fa20c187ef3 - syntax::fold::Folder::fold_item_simple::h389274292ee655e4
  28:     0x7fa20c187b1a - _<syntax..ptr..P<T>>::map::h0f976487c864d039
  29:     0x7fa20c181d00 - syntax::ext::expand::expand_multi_modified::h1278d11b5e015efd
  30:     0x7fa20c1410d2 - syntax::ext::expand::expand_annotatable::h05daf8dbfd45385d
  31:     0x7fa20c13f974 - syntax::ext::expand::expand_item::h0c304e718d09cf1e
  32:     0x7fa20c149664 - _<syntax..ext..expand..MacroExpander<'a, 'b> as syntax..fold..Folder>::fold_item::h7e3a9cc89287eb31
  33:     0x7fa20c19df72 - _<syntax..ext..expand..MacroExpander<'a, 'b> as syntax..fold..Folder>::fold_crate::hc04a1c2ade6e09bb
  34:     0x7fa20c19f2c9 - syntax::ext::expand::expand_crate::h751c1b1de09c42dd
  35:     0x7fa2110a37f3 - rustc_driver::driver::phase_2_configure_and_expand::_$u7b$$u7b$closure$u7d$$u7d$::hfdc4ab61e58580ac
  36:     0x7fa211057133 - rustc_driver::driver::phase_2_configure_and_expand::h7aa0bb3346817b72
  37:     0x7fa211029b26 - rustc_driver::driver::compile_input::hfd60b020f6d0208d
  38:     0x7fa21101ae74 - rustc_driver::run_compiler::h884d01d12eb76bbb
  39:     0x7fa211017f4e - std::panicking::try::call::hd72cf79141f67e60
  40:     0x7fa210b4929b - __rust_try
  41:     0x7fa210b4923e - __rust_maybe_catch_panic
  42:     0x7fa211018a34 - _<F as alloc..boxed..FnBox<A>>::call_box::h589d2091babf223a
  43:     0x7fa210b38f04 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  44:     0x7fa2086a6453 - start_thread
  45:     0x7fa210785eec - clone
  46:                0x0 - <unknown>

@erickt
Copy link
Member Author

erickt commented Jun 30, 2016

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:

  • Instead of using source, convert the libsyntax ast into tokens, then map those tokens into syntex tokens, and then pass things into the syntex parser. Once finished expanding, we invert the process. The assumption here is that tokens are less frequently changed, so there'd be less frequent breakages. This would also prototype the new libmacro process that @nrc and @cgswords are implementing.
  • Cheat, and just remap all spans produced by syntex into the item's span. This could probably work well for serde, but wouldn't be generally applicable to all macros.
  • Figure out how to inform syntex that this block of code is from a file from line X, rather than from line 1.

@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 :)

@dtolnay dtolnay added the wip label Jul 3, 2016
@dtolnay
Copy link
Member

dtolnay commented Sep 1, 2016

Closing in favor of #530.

@dtolnay dtolnay closed this Sep 1, 2016
@dtolnay dtolnay removed the wip label Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants