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

serde_macros incompatible with extern crate serde not in toplevel module #159

Closed
whitequark opened this issue Sep 22, 2015 · 13 comments
Closed
Assignees
Milestone

Comments

@whitequark
Copy link

To reproduce:

#![plugin(serde_macros)]
#![feature(plugin, custom_derive)]

mod x {
    extern crate serde;
    #[derive(Serialize)] struct S;
}

fn main() {}
@erickt
Copy link
Member

erickt commented Sep 22, 2015

Unfortunately I'm not sure if there's much we can do in this circumstance. I'm not sure if it's possible to resolve the location of the serde crate, so the most reliable location to find it is in the toplevel crate namespace.

@whitequark
Copy link
Author

Hm, good point. This might be worth bringing up whenever custom derivers are being stabilized.

@erickt
Copy link
Member

erickt commented Sep 22, 2015

It's worse than that, I think we need name resolution to happen in order to find where the serde crate was imported, which may not get exposed to compiler plugins for a long time :(

@whitequark
Copy link
Author

What if you did something like expanding it to...

mod internal {
  extern crate serde;
  impl serde::whatever for dunno {}
}
use internal::*;

but cleaner? Basically, hygiene for crate imports.

@grncdr
Copy link

grncdr commented Oct 3, 2015

hm, am I correct in understanding that #[derive(Serialize)] essentially won't work anywhere outside of the top-level of a crate? I was trying to derive these traits in a child module (e.g. separate file) and it seemed impossible to convince the compiler that the implementation was there.

@dtolnay
Copy link
Member

dtolnay commented Apr 20, 2016

@grncdr

hm, am I correct in understanding that #[derive(Serialize)] essentially won't work anywhere outside of the top-level of a crate? I was trying to derive these traits in a child module (e.g. separate file) and it seemed impossible to convince the compiler that the implementation was there.

It should work as long as extern crate serde is at the top level of your crate. The reason is the generated code expects to be able to use absolute paths like ::serde::ser::Serialize.

The following works for me:

#![feature(plugin, custom_derive)]
#![plugin(serde_macros)]

extern crate serde;

mod m {
    #[derive(Serialize)]
    pub struct Success;
}

fn main() {
    fn assert<T: serde::Serialize>(_: T) {}
    assert(m::Success);
}

@grncdr
Copy link

grncdr commented Apr 20, 2016

Thanks @dtolnay

@dtolnay
Copy link
Member

dtolnay commented Apr 20, 2016

In line with @whitequark's idea, I started implementing this as:

mod m {
    struct A { ... }
    #[derive(Serialize)]
    struct B { a: A }
}

// -->

mod m {
    struct A { ... }
    struct B { a: A }
    mod B_Serialize {
        use /* ??? */;
        extern crate serde;
        impl serde::Serialize for B { ... }
    }
}

I ran into trouble on the line marked ???. Without a use declaration, A and B are not in scope. Ideally I think I would want to write use super::* but that only seems to bring in public items so it only works if B is pub.

It works if I do use super::{A, B} so I guess we need to traverse the definition of B to figure out the individual things to import into B_Serialize. This is going to be nasty so let me know if anyone has a better idea. In particular, we will need to differentiate among:

  1. Things we need to use, like A and B,
  2. Generic type parameters like T, do not use.
  3. Builtin types like isize, do not use.
  4. Prelude types like Result, may need to use if prelude is disabled.

As far as I know, all of these "look" identical during macro expansion so it will be tricky to differentiate.

@dtolnay
Copy link
Member

dtolnay commented Apr 24, 2016

I think I have a much more promising approach. Apparently this is allowed...

const _IMPL_DESERIALIZE_FOR_S: () = {
    extern crate serde as _serde;
    impl _serde::Deserialize for S {
        // ...
    }
};

and it is way nicer than using a module to declare the impl because it does not affect the scope of imports.

I will work on a PR today.

@dtolnay
Copy link
Member

dtolnay commented May 3, 2016

@whitequark @erickt @oli-obk this was addressed in #298 and can be closed.

@oli-obk
Copy link
Member

oli-obk commented May 3, 2016

thanks @dtolnay

@dtolnay
Copy link
Member

dtolnay commented May 14, 2016

Unintended consequence: truly awful compiler diagnostics:

the trait bound `uuid::Uuid: Book::model::_IMPL_DESERIALIZE_FOR_Book::_serde::Deserialize` is not satisfied

@oli-obk
Copy link
Member

oli-obk commented May 14, 2016

we could probably fix this (in rustc) by checking if a path goes into an external crate, and invent a new display syntax along the lines of <extern crate serde>::Deserialize

@dtolnay dtolnay added this to the v0.7.4 milestone May 17, 2016
@dtolnay dtolnay self-assigned this Jun 7, 2016
rubdos pushed a commit to rubdos/serde that referenced this issue Jun 20, 2017
Add Binary, Octal to BigUint. Add UpperHex, LowerHex, Binary, Octal to BigInt

Is the testing for these enough? Any suggestions to improve them?
danburkert added a commit to tokio-rs/prost that referenced this issue Jul 11, 2017
The 'dummy const' pattern was copied from SerDe[1], but it causes
rustdoc not to generate documentation due to a [known bug][2].
Apparently SerDe [considered][3] using a dummy module instead of a dummy
const, but decided against it due to private type issues. Since
everything's public in `prost`, we shouldn't have such an issue.

Also removes #[automatically_derived] annotations, since apparently the
no longer do anything[4].

Fixes #11

[1]: https://github.com/serde-rs/serde/blob/775e8154e7151eb1576d65df539c4ac1612595c6/serde_derive/src/ser.rs#L28
[2]: rust-lang/rust#36922
[3]: serde-rs/serde#159 (comment)
[4]: https://botbot.me/mozilla/rust/2017-07-11/?msg=88402326&page=3
danburkert added a commit to tokio-rs/prost that referenced this issue Jul 11, 2017
The 'dummy const' pattern was copied from SerDe[1], but it causes
rustdoc not to generate documentation due to a [known bug][2].
Apparently SerDe [considered][3] using a dummy module instead of a dummy
const, but decided against it due to private type issues. Since
everything's public in `prost`, we shouldn't have such an issue.

Also removes #[automatically_derived] annotations, since apparently the
no longer do anything[4].

Fixes #11

[1]: https://github.com/serde-rs/serde/blob/775e8154e7151eb1576d65df539c4ac1612595c6/serde_derive/src/ser.rs#L28
[2]: rust-lang/rust#36922
[3]: serde-rs/serde#159 (comment)
[4]: https://botbot.me/mozilla/rust/2017-07-11/?msg=88402326&page=3
danburkert added a commit to tokio-rs/prost that referenced this issue Jul 11, 2017
The 'dummy const' pattern was copied from SerDe[1], but it causes
rustdoc not to generate documentation due to a [known bug][2].
Apparently SerDe [considered][3] using a dummy module instead of a dummy
const, but decided against it due to private type issues. Since
everything's public in `prost`, we shouldn't have such an issue.

Also removes #[automatically_derived] annotations, since apparently the
no longer do anything[4].

Fixes #11

[1]: https://github.com/serde-rs/serde/blob/775e8154e7151eb1576d65df539c4ac1612595c6/serde_derive/src/ser.rs#L28
[2]: rust-lang/rust#36922
[3]: serde-rs/serde#159 (comment)
[4]: https://botbot.me/mozilla/rust/2017-07-11/?msg=88402326&page=3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants