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_codegen does not generate code for derive in cfg_attr #148

Closed
codyps opened this issue Aug 28, 2015 · 8 comments
Closed

serde_codegen does not generate code for derive in cfg_attr #148

codyps opened this issue Aug 28, 2015 · 8 comments
Assignees

Comments

@codyps
Copy link
Contributor

codyps commented Aug 28, 2015

For example, this doesn't work:

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
struct Foo {
 a: u32,
};

It appears that the codegen simply doesn't know what to do about cfg_attr.

I suppose it's possible that not enough info is available when codegen occurs to actually resolve this. If so, feel free to close.

@dtolnay
Copy link
Member

dtolnay commented Apr 14, 2016

@jmesmon I just tried this and it seems to work. Can you try again and report back? Here is the code I used:

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

#[cfg(feature = "serde")]
extern crate serde;

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
struct Foo {
    a: u32,
}

#[cfg(feature = "serde")]
fn main() {
    println!("with serde");

    fn assert<T: serde::Serialize + serde::Deserialize>(_: T) {}
    assert(Foo { a: 0 });
}

#[cfg(not(feature = "serde"))]
fn main() {
    println!("without serde");
}

@codyps
Copy link
Contributor Author

codyps commented Apr 14, 2016

@dtolnay you're using serde_macros (plugin). This bug is talking about serde_codegen (generates code with a build script that you then include!).

I'm still able to reproduce this with serde_codegen.

Here's a project that reproduces: https://github.com/jmesmon/serde-readme

@dtolnay
Copy link
Member

dtolnay commented Apr 19, 2016

I suppose it's possible that not enough info is available when codegen occurs to actually resolve this. If so, feel free to close.

I suspect this is the case, but we can fix it anyway, at least for the common cases. We need to transform this:

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
struct Foo { ... }

... into this:

#[cfg(feature = "serde")]
impl Serialize for Foo { ... }
#[cfg(feature = "serde")]
impl Deserialize for Foo { ... }
struct Foo { ... }

@dtolnay
Copy link
Member

dtolnay commented Jun 12, 2016

For those not watching the Syntex repo - check out the PR at serde-deprecated/syntex#71.

homu added a commit to serde-deprecated/syntex that referenced this issue Jun 16, 2016
Reimplement derive to work with cfg_attr

Fixes serde-rs/serde#148.

The implementation expands `cfg_attr` and `derive` in-line during decorator expansion rather than as pre/post-expansion passes. I think this is a simpler and more correct design, but at the expense of being more invasive into the libsyntax code.
@erickt erickt reopened this Jun 16, 2016
@erickt
Copy link
Member

erickt commented Jun 16, 2016

bad homu, this isn't implemented in serde yet.

@dtolnay
Copy link
Member

dtolnay commented Jun 16, 2016

Actually it is. 😄 That's all it took.

@dtolnay dtolnay closed this as completed Jun 16, 2016
homu added a commit that referenced this issue Jun 23, 2016
Add test for expansion inside of cfg_attr

Test for #148. Requires serde-deprecated/syntex#71 + Syntex release.
@nox
Copy link
Contributor

nox commented Jun 25, 2016

I have a similar problem with:

#[cfg(target_os = "macos")]
pub type NativeFontHandle = CGFont;

/// Native fonts are not used on Linux; all fonts are raw.
#[cfg(not(target_os = "macos"))]
#[derive(Clone, Serialize, Deserialize)]
pub struct NativeFontHandle;

Is it the same bug? Should I file another one?

@dtolnay
Copy link
Member

dtolnay commented Jun 25, 2016

Different bug unfortunately. I filed it as serde-deprecated/syntex#81.

rubdos pushed a commit to rubdos/serde that referenced this issue Jun 20, 2017
Add formatting options for Complex

This adds LowerExp and UpperExp traits for Complex, taking precision into account. Fixes serde-rs#148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants