overflow evaluating the requirement test_ser::Outer: test_annotations::serde::Serialize

[benashford]

I have noticed an issue in one of my projects, that I have been able to reduce to the following example:

#[derive(Serialize)]
struct Outer {
    inner: Box<Option<Inner>>
}

#[derive(Serialize)]
struct Inner {
    outer: Outer
}

This fails to compile with:

/Users/ben/oss/serde/testing/target/debug/build/serde_testing-5d7d99abec542b6e/out/test.rs:16645:18: 16645:40 error: overflow evaluating the requirement `test_ser::Outer: test_annotations::serde::Serialize` [E0275]
/Users/ben/oss/serde/testing/target/debug/build/serde_testing-5d7d99abec542b6e/out/test.rs:16645             impl _serde::ser::Serialize for Outer where
                                                                                                                  ^~~~~~~~~~~~~~~~~~~~~~
tests/test.rs:4:1: 4:48 note: in this expansion of include!
/Users/ben/oss/serde/testing/target/debug/build/serde_testing-5d7d99abec542b6e/out/test.rs:16645:18: 16645:40 help: run `rustc --explain E0275` to see a detailed explanation
/Users/ben/oss/serde/testing/target/debug/build/serde_testing-5d7d99abec542b6e/out/test.rs:16645:18: 16645:40 note: required by `test_annotations::serde::Serialize`
error: aborting due to previous error
error: Could not compile `serde_testing`.

To learn more, run the command again with --verbose.

These examples compiled in 0.7.11 and earlier, but has failed in 0.7.12 and all versions since.

[oli-obk]

This is due to your mutually recursive structure. It should probably be documented in a better way.

Sorry about the breaking in a minor version, but everything else was worse for the common case.

// We do not attempt to generate any bounds based on field types that are
// directly recursive, as in:
//
//    struct Test<D> {
//        next: Box<Test<D>>,
//    }
//
// This does not catch field types that are mutually recursive with some other
// type. For those, we require bounds to be specified by a `bound` attribute if
// the inferred ones are not correct.
//
//    struct Test<D> {
//        #[serde(bound="D: Serialize + Deserialize")]
//        next: Box<Other<D>>,
//    }
//    struct Other<D> {
//        #[serde(bound="D: Serialize + Deserialize")]
//        next: Box<Test<D>>,
//    }

[mitsuhiko]

I can confirm having the same issue with this case: https://gist.github.com/mitsuhiko/f0e5984755403cf785db389569b12043

[mitsuhiko]

I would propose yanking the release an making a new release with a higher version number if this is not going to be changed. From what I can tell this broke all my projects using JSON types.

[oli-obk]

actually the change I was thinking about happened in https://github.com/serde-rs/serde/releases/tag/v0.7.8 ...

0.7.12 was a pure syntex bump, meaning only nightly stuff should break.

[oli-obk]

ah, this was probably caused by #416 so this issue is a dupe of #436

[benashford]

@oli-obk cool, thanks for looking into this.

I've decided to write a custom Serialize implementation for the root struct, otherwise it was getting a bit confusing with optionals and renames and other annotations. But either way it has had the desired effect.

[adimarco]

This broke some stuff in our crate as well (rusoto/rusoto#302), though adding the #[serde(bound="")] annotation where struct members reference recursively defined types seems to fix the problem.

Seeing the commit above https://github.com/serde-rs/serde/pull/416/files that removes the generic type check is kind of confusing. Is the expectation now that crates that depend on serde will re-implement the logic that was pulled out?

We read a schema (botocore) and generate serde-annotated Rust code based on it, which is then passed through serde_codegen::expand() to generate more Rust code that actually gets built as our crate. I'm just wondering if we should go implement the logic that was removed in that pull request in our own first-pass of codegen, or if I'm missing something altogether.

[adimarco]

I guess maybe to rephrase - due to the way bounds were implemented before the change in #416, recursively defined types didn't cause explosions and they now do. Is it now expected that we'll annotate places where recursion occurs?

If this were hand-written code it'd be a little less painful, but since it's generated from a schema, to solve the generic case I'll need to put in some checks for type loops in case the schema changes under me.

[dtolnay]

No, #416 is only tangentially related.

---

Assume Recursive is some struct that references S.

struct S {
    r: Recursive
}

• in Serde 0.6: no bound at all
• in Serde 0.7 with filtering: no bound at all
• in Serde 0.7 without filtering: where Recursive: Serialize
• in Serde 0.8: no bound at all

struct S<T> {
    r: Recursive<T>
}

• in Serde 0.6: where T: Serialize
• in Serde 0.7 with filtering: where Recursive<T>: Serialize
• in Serde 0.7 without filtering: where Recursive<T>: Serialize
• in Serde 0.8: where T: Serialize

---

The filtering that was removed in #416 affects only the first case, but the second case requires a bound attribute in 0.7 with or without filtering.

The second case is interesting because neither bound is uniformly better. If the impl for Recursive looks like impl<T> Serialize for Recursive<T> then the 0.7 bound is correct and the 0.6 bound is incorrect. If the impl looks like impl<T> Serialize for Recursive<T> where T: Serialize then both bounds are correct. If the impl looks like impl<T> Serialize for Recursive<T> where S<T>: Serialize then the 0.6 bound is correct and the 0.7 bound is incorrect.

Basically in 0.7 we learned that recursive types are more common than bounds that behave like impl<T> Serialize for X<T>, and so it is more often better to go with the Serde 0.6 strategy for generating bounds. As tracked by this issue, for 0.8 we will be reverting to (a better version of) 0.6's strategy. This is a breaking change because some types that used to work without a bound attribute in 0.7 will require a bound attribute in 0.8. The compiler does not have enough information during codegen to generate correct bounds in all cases.

[dtolnay]

To clarify why I say that #416 is only tangentially related: from the first example in my previous comment, it looks like the solution is to revert #416 and put the filtering back in. But as you can see from the second example, that is not right. #416 is related only to the extent that it affects bounds generation.