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

Generate bounds on type parameters only #456

Merged
merged 2 commits into from
Jul 22, 2016
Merged

Generate bounds on type parameters only #456

merged 2 commits into from
Jul 22, 2016

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jul 22, 2016

Fixes #435 and fixes #436 and fixes #441 and fixes #443.

@dtolnay
Copy link
Member Author

dtolnay commented Jul 22, 2016

#[derive(Serialize)]
struct S<'a, A: 'a, B, C> {
    a: Option<&'a A>,
    b: B,
    #[serde(skip_serializing)]
    c: C,
    i: i32,
}

Before:

impl<'a, A: 'a, B, C> Serialize for S<'a, A, B, C>
    where Option<&'a A>: Serialize,
          B: Serialize,
          i32: Serialize { /* ... */ }

After:

impl<'a, A: 'a, B, C> Serialize for S<'a, A, B, C>
    where A: Serialize,
          B: Serialize { /* ... */ }

@dtolnay dtolnay added this to the v0.8.0 milestone Jul 22, 2016
@dtolnay
Copy link
Member Author

dtolnay commented Jul 22, 2016

Hmm I have to figure out how to make the hygiene work on nightly.

@dtolnay
Copy link
Member Author

dtolnay commented Jul 22, 2016

I am done working on this for today. @oli-obk see if you can figure out a way that works with serde_macros.

@oli-obk
Copy link
Member

oli-obk commented Jul 22, 2016

we have some pretty funky spans:

tests/../../testing/tests/test_gen.rs:42:19: 1:1 error: the trait bound T: serde::Serialize is not satisfied [E0277]

@dtolnay
Copy link
Member Author

dtolnay commented Jul 22, 2016

Yet serde_codegen is fine with it. I think serde_macros is seeing two different Ts, one in the bound and a different one in the field type.

@oli-obk
Copy link
Member

oli-obk commented Jul 22, 2016

It must be more complex... If I compile test_gen.rs directly, it works fine, if I do it through the test.rs.in, everything goes haywire.

@oli-obk
Copy link
Member

oli-obk commented Jul 22, 2016

fun fact: include! is the "culprit". If i do the following, the tests run through fine:

#[macro_use]
#[path = "../../testing/tests/macros.rs"]
mod macros;

#[path = "../../testing/tests/test_annotations.rs"]
mod test_annotations;
#[path = "../../testing/tests/test_bytes.rs"]
mod test_bytes;
#[path = "../../testing/tests/test_de.rs"]
mod test_de;
#[path = "../../testing/tests/test_gen.rs"]
mod test_gen;
#[path = "../../testing/tests/test_macros.rs"]
mod test_macros;
#[path = "../../testing/tests/test_ser.rs"]
mod test_ser;

@oli-obk
Copy link
Member

oli-obk commented Jul 22, 2016

pretty=expanded code of include! version:

#[automatically_derived]
        impl <T> _serde::ser::Serialize for With<T> {
            fn serialize<__S>(&self, _serializer: &mut __S)
             -> ::std::result::Result<(), __S::Error> where
             __S: _serde::ser::Serializer {
                {
                    let mut state =
                        match _serializer.serialize_struct("With", 0 + 1) {
                            ::std::result::Result::Ok(val) => val,
                            ::std::result::Result::Err(err) => {
                                return ::std::result::Result::Err(::std::convert::From::from(err))
                            }
                        };
                    if !false {
                        match _serializer.serialize_struct_elt(&mut state,
                                                               "t", &self.t) {
                            ::std::result::Result::Ok(val) => val,
                            ::std::result::Result::Err(err) => {
                                return ::std::result::Result::Err(::std::convert::From::from(err))
                            }
                        };
                    }
                    _serializer.serialize_struct_end(state)
                }
            }
        }

obviously the bound is missing. If I use mod ... instead of include! I get the correct code:

#[automatically_derived]
            impl <T> _serde::ser::Serialize for With<T> where
             T: _serde::ser::Serialize {
                fn serialize<__S>(&self, _serializer: &mut __S)
                 -> ::std::result::Result<(), __S::Error> where
                 __S: _serde::ser::Serializer {
                    {
                        let mut state =
                            match _serializer.serialize_struct("With", 0 + 1)
                                {
                                ::std::result::Result::Ok(val) => val,
                                ::std::result::Result::Err(err) => {
                                    return ::std::result::Result::Err(::std::convert::From::from(err))
                                }
                            };
                        if !false {
                            match _serializer.serialize_struct_elt(&mut state,
                                                                   "t",
                                                                   &self.t) {
                                ::std::result::Result::Ok(val) => val,
                                ::std::result::Result::Err(err) => {
                                    return ::std::result::Result::Err(::std::convert::From::from(err))
                                }
                            };
                        }
                        _serializer.serialize_struct_end(state)
                    }
                }
            }

include! completely messes up the identifiers' expansion info
@dtolnay
Copy link
Member Author

dtolnay commented Jul 22, 2016

Thanks for the fix 😃. I did not think the solution would be less hygiene instead of more hygiene.

@dtolnay dtolnay merged commit 8577272 into master Jul 22, 2016
@dtolnay dtolnay deleted the generic branch July 22, 2016 14:59
@eddyb
Copy link

eddyb commented Jul 22, 2016

@dtolnay I expected Option<&'a A>: Serialize, not A: Serialize, TBQH. That is, bounds are kept if they refer to type parameters, instead of just putting bounds on type parameters.

@dtolnay
Copy link
Member Author

dtolnay commented Jul 22, 2016

That breaks this case:

#[derive(Serialize, Deserialize)]
struct RecursiveGenericA<T> {
    t: T,
    b: Box<RecursiveGenericB<T>>,
}

#[derive(Serialize, Deserialize)]
enum RecursiveGenericB<T> {
    T(T),
    A(RecursiveGenericA<T>),
}

@eddyb
Copy link

eddyb commented Jul 22, 2016

@dtolnay Ah, right. We can't have nice things 😞.

@dtolnay
Copy link
Member Author

dtolnay commented Jul 22, 2016

One thing I still want to do is add a hardcoded exception for PhantomData. The following should not put a bound on T:

struct S<T> {
    phantom: PhantomData<T>
}

bors-servo pushed a commit to servo/servo that referenced this pull request Aug 13, 2016
Remove #[serde(bound = "")] attributes

These were fixed in serde_codegen 0.8.0 by serde-rs/serde#456.

cc @nox

- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes do not require tests because: the generated code continues to compile

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12834)
<!-- Reviewable:end -->
samuknet pushed a commit to samuknet/servo that referenced this pull request Sep 6, 2016
These were fixed in serde_codegen 0.8.0 by serde-rs/serde#456.
jyc added a commit to jyc/servo that referenced this pull request Jun 15, 2017
…Css deriver.

Add a new `#[omit_to_css_bounds]` attribute on variants in an enum with
\`#[derive(ToCss)]`. A variant `V(F1, F2, ...)` normally generates bounds on the
ToCss implementation `where F1: ToCss, F2: ToCss, ...`

This is problematic in following case:

    // Sort of what we will have for type custom CSS variable values.
    #[derive(ToCss)]
    pub enum E {
        V(Vec<E>)
    }

    // Sort of what we have in style_traits/values.rs.
    impl<T> ToCss for Vec<T> where T: ToCss {
        ...
    }

The generated implementation of `ToCss` for `E` looks like

    impl ToCss for E where Vec<E>: ToCss {
        ...
    }

... which creates a cycle, and the compiler exits with error E0257.

The `Vec<E>: ToCss` bound is not actually necessary, so with this commit we can
omit its generation with `#[omit_to_css_bounds]`. It would be cool if in the
future we could decide when to omit such bounds automatically.

For reference, what serde did is generate bounds for type parameters used in
fields only. See serde-rs/serde#456 . I tried that and
it didn't seem to work with style::values::generics::BasicShape, however, maybe
because Circle in that module doesn't implement ToCss.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…olnay:bound); r=nox

These were fixed in serde_codegen 0.8.0 by serde-rs/serde#456.

cc nox

- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes do not require tests because: the generated code continues to compile

Source-Repo: https://github.com/servo/servo
Source-Revision: 11b853fbf1916fb71b49fefbf2ebeaccaec50ee2

UltraBlame original commit: 14f16d3626e039515989bb5b69d38f0403b822c7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…olnay:bound); r=nox

These were fixed in serde_codegen 0.8.0 by serde-rs/serde#456.

cc nox

- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes do not require tests because: the generated code continues to compile

Source-Repo: https://github.com/servo/servo
Source-Revision: 11b853fbf1916fb71b49fefbf2ebeaccaec50ee2

UltraBlame original commit: 14f16d3626e039515989bb5b69d38f0403b822c7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…olnay:bound); r=nox

These were fixed in serde_codegen 0.8.0 by serde-rs/serde#456.

cc nox

- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes do not require tests because: the generated code continues to compile

Source-Repo: https://github.com/servo/servo
Source-Revision: 11b853fbf1916fb71b49fefbf2ebeaccaec50ee2

UltraBlame original commit: 14f16d3626e039515989bb5b69d38f0403b822c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment