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

Add "const-generics" feature #1860

Closed
wants to merge 1 commit into from
Closed

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Jul 18, 2020

Brings an opt-in breaking change because T of [T; 0] Serialize/Deserialize implementation doesn't require T: Serialize/Deserialize bounds. This scenario has the same root cause of std Default implementation for [T; N].

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 18, 2020

If this change is desirable, I will fix and add more tests. If not, feel free to close this PR.

@MikailBag
Copy link

A way to avoid breakage is to use #[marker] traits and specialization, as in std PR switching Default to const generics. Unfortunately, these features will be likely stabilized much later that min_const_generics.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Aug 30, 2020

Tried to create a more robust array creation procedure based on the feedback of rust-lang/rust#75644 but looks like the new const N syntax is being rejected by older compiler versions even with #[cfg].

That said, it is just a matter of bumping the minimal compiler version and 1.31 (2018) should probably be a good choice.

@est31
Copy link
Contributor

est31 commented Sep 25, 2020

it is just a matter of bumping the minimal compiler version

serde's maintainer has gone to great lengths to avoid bumping the MSRV of serde proper. You could try moving the offending code into a separate module in a separate file, and put the mod foo statement behind a cfg. This should make sure the compiler never encounters it when the feature is turned off.

@c410-f3r
Copy link
Contributor Author

@est31 This module strategy is new to me. Thanks for the suggestion!

I am still not sure if dtolnay will accept these changes because there wasn't a feedback. He was quicker to respond on my last contributions to this project though 😄

@c410-f3r c410-f3r closed this Sep 27, 2020
@c410-f3r c410-f3r reopened this Sep 27, 2020
@MikailBag
Copy link

MikailBag commented Sep 27, 2020

With your PR as is, if any crate in compilation graph enables const-generics on serde other crates relying on e.g. impl<T> Serialize for [T; 0], will fail to compile. Since enabling some feature on some dependency is assumed to be semver-compatible, your PR in fact breaks all libraries that rely on 0 being special cased (because they can stop working after plain cargo update).

If I were you, I'd explore following approach (note it is pseudocode):

struct Helper<T>;
impl<T> Helper<T> {
    default fn serialize(val: T, ser: Serializer) {
       panic!()
     }
}

impl<T: Serialize> Helper<T> {
    fn serialize(val: T, ser: Serializer) {
        val.serialize(ser)
    }
}

impl<T> Default for Helper<T> where T: Serialize {...}

impl<T, const N> Serialize for [T; N] where [Helper<T>; N]: Default {
    fn serialize(self, ser: Serializer) {
         for item in self {
              Helper<T>::serialize(item);
         }
    }
}

It relies on rust-lang/rust#74254 merged first. However, if it works, no breakage occurs.

@c410-f3r
Copy link
Contributor Author

@MikailBag There are some projects that provide unstable features where stability guarantees aren't preserved and I though the same could be done with serde because specialization is something that probably won't be stabilized in the near future. Hopefully, constant generics will be ready for the 1.49 release.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Oct 4, 2020

@dtolnay Looks like strict semver is desirable and there is no way around the [T; 0] breaking change because it is something that can't be easily fixed. Please, just keep in mind that if min_const_generics is indeed going to be stabilized in the upcoming months, then more and more people will eventually want to use this feature with serde.

@MikailBag
Copy link

MikailBag commented Oct 5, 2020

As far as I understand, min_specialization isn't too far from stable too. (But I'm not in lang team so my understanding may be wrong. Can anyone clarify?)

@est31
Copy link
Contributor

est31 commented Oct 5, 2020

From my understanding, marker traits are also needed, not just specialization.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll close this and suggest that people use https://github.com/est31/serde-big-array for now.

I would like to follow along in rust-lang/rust#61415 + rust-lang/rust#76560 + rust-lang/rust#44580 a while longer before setting what approach to land in this crate.

Thanks anyway!

@dtolnay dtolnay closed this Aug 21, 2021
@c410-f3r
Copy link
Contributor Author

Thanks for the prompt response

@serde-rs serde-rs locked and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants