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

Conditional compilation ignores serde_as(as = X) #355

Closed
philher opened this issue Aug 31, 2021 · 4 comments · Fixed by #356
Closed

Conditional compilation ignores serde_as(as = X) #355

philher opened this issue Aug 31, 2021 · 4 comments · Fixed by #356
Labels
bug Something isn't working documentation Improvements or additions to documentation wontfix This will not be worked on

Comments

@philher
Copy link

philher commented Aug 31, 2021

Using conditional compilation appears to break the use of serde_as. Using a struct such as StructC below, I would expect it to be serialized to JSON without issue, as happens without using non conditional compilation.
However, the attribute appears to have no effect, instead failing with the error Error("key must be a string", line: 0, column: 0).

Is this really a bug, or is there something I am not doing right ? I have also given a full example with tests that showcase the problem and what works.

#[cfg_attr(feature="serde_test" ,serde_as)]
#[derive(Default, Debug, Serialize, Deserialize)]
struct StructC {
    #[cfg_attr(feature="serde_test" ,serde_as(as = "Vec<(_, _)>"))]
    map :  HashMap<(i32,i32), i32>
}
lib.rs
use serde::{Deserialize, Serialize};
use serde_with::serde_as;
use std::collections::HashMap;

/// Works
#[serde_as]
#[derive(Default, Debug, Serialize, Deserialize)]
struct StructA {
    #[serde_as(as = "Vec<(_, _)>")]
    map: HashMap<(i32, i32), i32>,
}

/// Fails as expected
#[derive(Default, Debug, Serialize, Deserialize)]
struct StructB {
    map: HashMap<(i32, i32), i32>,
}

/// Should work
#[cfg_attr(feature = "serde_test", serde_as)]
#[derive(Default, Debug, Serialize, Deserialize)]
struct StructC {
    #[cfg_attr(feature = "serde_test", serde_as(as = "Vec<(_, _)>"))]
    map: HashMap<(i32, i32), i32>,
}

/// Should work or not ?
#[serde_as]
#[derive(Default, Debug, Serialize, Deserialize)]
struct StructD {
    #[cfg_attr(feature = "serde_test", serde_as(as = "Vec<(_, _)>"))]
    map: HashMap<(i32, i32), i32>,
}

/// Shows that other attributes work
#[derive(Default, Debug, Serialize, Deserialize)]
struct StructE {
    #[cfg_attr(feature = "serde_test", serde(rename = "renamed"))]
    map: HashMap<String, i32>,
}

#[cfg(test)]
mod tests {
    use crate::{StructA, StructB, StructC, StructD, StructE};

    #[test]
    fn test_struct_a() {
        assert!(cfg!(feature = "serde_test"));
        let mut struct_a = StructA::default();
        struct_a.map.insert((1, -10), -1);

        let a_str = serde_json::to_string(&struct_a);
        println!("{:?}", a_str);
        assert_eq!(a_str.unwrap(), r#"{"map":[[[1,-10],-1]]}"#.to_string());
    }

    #[test]
    fn test_struct_b() {
        assert!(cfg!(feature = "serde_test"));
        let mut struct_b = StructB::default();
        struct_b.map.insert((1, -10), -1);

        let b_str = serde_json::to_string(&struct_b);
        println!("{:?}", b_str);
        assert!(b_str.is_err());
    }

    #[test]
    fn test_struct_c() {
        assert!(cfg!(feature = "serde_test"));
        let mut struct_c = StructC::default();
        struct_c.map.insert((1, -10), -1);

        let c_str = serde_json::to_string(&struct_c);
        println!("{:?}", c_str);
        assert_eq!(c_str.unwrap(), r#"{"map":[[[1,-10],-1]]}"#.to_string());
    }
    #[test]
    fn test_struct_d() {
        assert!(cfg!(feature = "serde_test"));
        let mut struct_d = StructD::default();
        struct_d.map.insert((1, -10), -1);

        let d_str = serde_json::to_string(&struct_d);
        println!("{:?}", d_str);
        assert_eq!(d_str.unwrap(), r#"{"map":[[[1,-10],-1]]}"#.to_string());
    }
    #[test]
    fn test_struct_e() {
        assert!(cfg!(feature = "serde_test"));
        let mut struct_e = StructE::default();
        struct_e.map.insert("a_key".to_string(), -1);

        let e_str = serde_json::to_string(&struct_e);
        println!("{:?}", e_str);
        assert_eq!(e_str.unwrap(), r#"{"renamed":{"a_key":-1}}"#.to_string());
    }
}
cargo.toml
[package]
name = "mwe_issue"
version = "0.1.0"
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
serde = "1.0.130"
serde_with = "1.9.4"
serde_json = "1.0.67"


[features]
serde_test = []
@jonasbb
Copy link
Owner

jonasbb commented Aug 31, 2021

EDIT(2023):

Thanks to the cfg_eval-crate this can be done with a single extra attribute. Details about this are in #610.

#[cfg_attr(feature="serde_test", cfg_eval::cfg_eval, serde_as)]
#[derive(Default, Debug, Serialize, Deserialize)]
struct StructC {
    #[cfg_attr(feature="serde_test", serde_as(as = "Vec<(_, _)>"))]
    map :  HashMap<(i32,i32), i32>
}

The rest of the answer remains valid. It is more complicated but avoid the extra dependency.


That is currently not supported, and I don't know of a good solution until support is added in Rust.

The problem is that the outer #[serde_as] proc-macro is only looking for #[serde_as(...)] attributes, but does not find any. The proc-macro runs before cfg-evaluation, so it only sees a #[cfg_attr(...)] which it ignores.
A proper solution is #[cfg_eval] (rust-lang/rust#82679, rust-lang/rust#87221). With it, you can force the evaluation of the cfg attributes, such that the outer #[serde_as] proc-macro will see another #[serde_as(...)] attribute.
serde does not have this problem, since derive macros already run after cfg evaluation.

As a workaround, you can apply the transformation steps of serde_as manually. You have the same expressiveness, but loose the nicer syntax.

use serde_with::{As, Same};

#[cfg_attr(feature="serde_test" ,serde(with = "As::<Vec<(Same, Same)>>"))]

Edit: To expand on the answer a bit: It would be possible to "understand" these cfg-attributes and process them inside the serde_as macro. However, parsing them is complicated, since multiple cfg can be nested inside each other, and other macros, like the rustversion would also need support. Therefore, I think it is better to wait for proper Rust support.

@philher
Copy link
Author

philher commented Sep 1, 2021

Thank you for the detailed answer, I expected that there was some proc-macro magic that I did not properly understand !

The workaround is a satisfying solution for my use case, until proper support is implemented.
Maybe this use case of the "manual implementation" could be added to the documentation, so that other users that are not very familiar with proc-macro may easily find it ?

@jonasbb
Copy link
Owner

jonasbb commented Sep 1, 2021

Glad I could help. I agree, this could be better documented.

One variation you didn't mention, but which I think could be supported quite easily, is this: Having a conditional implementation of the serde traits and always use serde_as. This way, whenever the traits are derived, they use the serde_as transformations. But the serde_with crate unconditionally depends on serde with the derive feature, so you wouldn't be saving much in terms of stripping compile time dependencies.

#[serde_as]
#[cfg_attr(feature = "...", derive(Serialize, Deserialize))]
struct StructD {
    #[serde_as(as = "Vec<(_, _)>")]
    map: HashMap<(i32, i32), i32>,
}

@jonasbb jonasbb added bug Something isn't working documentation Improvements or additions to documentation wontfix This will not be worked on labels Sep 1, 2021
@philher
Copy link
Author

philher commented Sep 1, 2021

I tried this variation when I was investigating the issue, but did not mention because indeed one of my goals was to limit dependencies.

If you implement support for it, i think it might be useful to put it in the documentation as an alternative, as it would still save compilation time.

jonasbb added a commit that referenced this issue Sep 4, 2021
* Move the list of transformations into its own file.
* Make better use of multiple heading levels.
* Explain that conditional compilation with serde_as is currently not
  working. Explain workaround.

Closes #355
bors bot added a commit that referenced this issue Sep 4, 2021
356: Restructure the serde_as documentation r=jonasbb a=jonasbb

* Move the list of transformations into its own file.
* Make better use of multiple heading levels.
* Explain that conditional compilation with serde_as is currently not
  working. Explain workaround.

Closes #355

Co-authored-by: Jonas Bushart <jonas@bushart.org>
@bors bors bot closed this as completed in 9ebccbf Sep 4, 2021
adamflott added a commit to adamflott/statgrab-rs that referenced this issue Jun 27, 2023
jonasbb added a commit that referenced this issue Jul 3, 2023
…utes

So far the story for cfg-gating serde_with attributes has not been
great, as shown in #331 or #355.
This adds a test showing how cfg_eval can be used to gate serde_with.

Part of #607
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants