i128 and u128 integers missing Deserialize impls #1136

PlasmaPower · 2018-01-07T05:20:45Z

They should behind the unstable flag.

The text was updated successfully, but these errors were encountered:

PlasmaPower · 2018-03-24T22:18:30Z

u128 and i128 are going to be stabilized very soon. However, from what I can tell, adding support to serde will be a breaking change. Is serde 2.0 planned?

seunlanlege · 2018-04-08T23:03:47Z

They've been stabilized 🎊, will this be included in the next release?

oli-obk · 2018-04-09T07:07:49Z

We can't add them directly, because that would break all users of older rustc stable versions

We could add it under a (stable) feature flag

cgm616 · 2018-04-23T02:41:41Z

So I'm trying to add this behind a feature flag in cgm616/serde, but I come across a super weird error when attempting to compile. Can someone check this out for me?

I'm running

> cargo build --all-features

and getting this output:

   Compiling serde_test v1.0.41 (file:///Users/cgm616/Documents/Developing/rust/serde/serde_test)
error[E0046]: not all trait items implemented, missing: `deserialize_i128`, `deserialize_u128`
   --> serde_test/src/de.rs:126:1
    |
126 | impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `deserialize_i128`, `deserialize_u128` in implementation
    |
    = note: `deserialize_i128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`
    = note: `deserialize_u128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`

error[E0046]: not all trait items implemented, missing: `deserialize_i128`, `deserialize_u128`
   --> serde_test/src/de.rs:655:1
    |
655 | impl<'de> de::Deserializer<'de> for BytesDeserializer {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `deserialize_i128`, `deserialize_u128` in implementation
    |
    = note: `deserialize_i128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`
    = note: `deserialize_u128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`

error[E0046]: not all trait items implemented, missing: `serialize_i128`, `serialize_u128`
  --> serde_test/src/ser.rs:85:1
   |
85 | impl<'s, 'a> ser::Serializer for &'s mut Serializer<'a> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `serialize_i128`, `serialize_u128` in implementation
   |
   = note: `serialize_i128` from trait: `fn(Self, i128) -> std::result::Result<<Self as serde::Serializer>::Ok, <Self as serde::Serializer>::Error>`
   = note: `serialize_u128` from trait: `fn(Self, u128) -> std::result::Result<<Self as serde::Serializer>::Ok, <Self as serde::Serializer>::Error>`

error[E0046]: not all trait items implemented, missing: `serialize_i128`, `serialize_u128`
   --> serde_test/src/configure.rs:185:9
    |
185 | /         impl<S> Serializer for $wrapper<S>
186 | |         where
187 | |             S: Serializer,
188 | |         {
...   |
327 | |             }
328 | |         }
    | |_________^ missing `serialize_i128`, `serialize_u128` in implementation
...
476 |   impl_serializer!(Readable, true);
    |   --------------------------------- in this macro invocation
    |
    = note: `serialize_i128` from trait: `fn(Self, i128) -> std::result::Result<<Self as serde::Serializer>::Ok, <Self as serde::Serializer>::Error>`
    = note: `serialize_u128` from trait: `fn(Self, u128) -> std::result::Result<<Self as serde::Serializer>::Ok, <Self as serde::Serializer>::Error>`

error[E0046]: not all trait items implemented, missing: `serialize_i128`, `serialize_u128`
   --> serde_test/src/configure.rs:185:9
    |
185 | /         impl<S> Serializer for $wrapper<S>
186 | |         where
187 | |             S: Serializer,
188 | |         {
...   |
327 | |             }
328 | |         }
    | |_________^ missing `serialize_i128`, `serialize_u128` in implementation
...
477 |   impl_serializer!(Compact, false);
    |   --------------------------------- in this macro invocation
    |
    = note: `serialize_i128` from trait: `fn(Self, i128) -> std::result::Result<<Self as serde::Serializer>::Ok, <Self as serde::Serializer>::Error>`
    = note: `serialize_u128` from trait: `fn(Self, u128) -> std::result::Result<<Self as serde::Serializer>::Ok, <Self as serde::Serializer>::Error>`

error[E0046]: not all trait items implemented, missing: `deserialize_i128`, `deserialize_u128`
   --> serde_test/src/configure.rs:493:9
    |
493 | /         impl<'de, D> Deserializer<'de> for $wrapper<D>
494 | |         where
495 | |             D: Deserializer<'de>,
496 | |         {
...   |
622 | |             }
623 | |         }
    | |_________^ missing `deserialize_i128`, `deserialize_u128` in implementation
...
887 |   impl_deserializer!(Readable, true);
    |   ----------------------------------- in this macro invocation
    |
    = note: `deserialize_i128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`
    = note: `deserialize_u128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`

error[E0046]: not all trait items implemented, missing: `deserialize_i128`, `deserialize_u128`
   --> serde_test/src/configure.rs:493:9
    |
493 | /         impl<'de, D> Deserializer<'de> for $wrapper<D>
494 | |         where
495 | |             D: Deserializer<'de>,
496 | |         {
...   |
622 | |             }
623 | |         }
    | |_________^ missing `deserialize_i128`, `deserialize_u128` in implementation
...
888 |   impl_deserializer!(Compact, false);
    |   ----------------------------------- in this macro invocation
    |
    = note: `deserialize_i128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`
    = note: `deserialize_u128` from trait: `fn(Self, V) -> std::result::Result<<V as serde::de::Visitor<'de>>::Value, <Self as serde::Deserializer<'de>>::Error>`

error: aborting due to 7 previous errors

For more information about this error, try `rustc --explain E0046`.
error: Could not compile `serde_test`.

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

I really can't figure out what's going on. If anyone could help me that would be great. I've never tried to use feature flags and I've also never worked on serde (or a project of this size) before, so I'm probably pretty misguided in my changes.

PlasmaPower · 2018-04-23T02:43:05Z

Adding i128 and u128 is a breaking change, as I said before. You're running into some breakage from the change.

cgm616 · 2018-04-23T02:44:40Z

Breaking as in for consumers of the library? Or breaking as in it requires a redesign of the library's internals? I thought you meant the former, and that's what the feature flag is for.

cgm616 · 2018-04-23T02:45:43Z

To add u128 and i128, I literally searched everywhere i64 and u64 were used in the project and evaluated if the new types needed to be added in some way there. If they did, I put it behind a feature flag and moved on.

PlasmaPower · 2018-04-23T02:47:08Z

It is breaking for people using the library, but that includes the library itself. In those cases the library implements Deserializer or Serializer, such as BytesDeserializer, but there's no implementation for the [de]serialize_i128 functions which are now required.

cgm616 · 2018-04-23T02:51:23Z

This is what I've done in the case of BytesDeserializer, which seems to work for the other types that need functions:

impl<'de, 'a, E> Deserializer<'de> for BytesDeserializer<'a, E>
where
    E: Error,
{
    type Error = E;

    fn deserialize_any<V>(self, visitor: V) -> Result<V::Value, Self::Error>
    where
        V: Visitor<'de>,
    {
        visitor.visit_bytes(self.value)
    }

    forward_to_deserialize_any! {
        bool i8 i16 i32 i64 u8 u16 u32 u64 f32 f64 char str string bytes
        byte_buf option unit unit_struct newtype_struct seq tuple tuple_struct
        map struct enum identifier ignored_any
    }

    #[cfg(feature = "128")]
    forward_to_deserialize_any! {
        i128 u128
    }
}

At first I did it all without the feature flag, and it all compiled and tests passed (including new tests I added for i128 and u128). Now I'm heading back through and changing it all to use the flag, but I can't get past this weird error.

PlasmaPower · 2018-04-23T03:02:12Z

You could try using cargo rustc -- --pretty=expanded to show what the code looks like after the macros are expanded.

cgm616 · 2018-04-23T03:09:12Z

Okay, hmmm. When using that in each individual crate, the methods are present. In addition, those crates compile fine.

It's only when compiling at the workspace level that it doesn't seem to work. Is it possible that there's a problem with the feature passing on?

PlasmaPower · 2018-04-23T03:12:18Z

Oh yeah, that'll be a problem...

As far as I know, there's no good way to enable a dependency's feature conditional on a feature of the root crate: rust-lang/cargo#2524

There may be a crazy way to resolve this with additional crates, IDK.

cgm616 · 2018-04-23T03:25:15Z

So in effect there's no way to add i128 and u128 support until serde increases its minimum supported rustc version to the first version that includes these types on stable, which hasn't even come out yet.

That's a bit disappointing... I'm going to keep seeing what I can do.

hcpl · 2018-04-23T03:38:27Z

I think using separate #[cfg]ed traits like UnstableSerialize: Serialize and UnstableDeserialize: Deserialize could do the trick without breaking any existing code at all.

cgm616 · 2018-04-23T03:44:14Z

Oh, that's a good idea. I'll try that and see what I can come up with. It'll require a lot of duplicated code... but let's see.

cgm616 · 2018-04-23T06:01:24Z

Does it break consumer code if I add the following impls?

impl Serialize for u128 {}
impl Serialize for i128 {}

impl Deserialize for u128 {}
impl Deserialize for i128 {}

I'm guessing it does... but what if those are all gated?

PlasmaPower · 2018-04-23T12:04:52Z

I'm guessing those impls aren't possible if we're using an extension to the Serializer trait. The Serialize trait requires fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error>, i.e. it must work for all Serializers. We'll probably have to make another Serialize-like trait that is restricted to UnstableSerializer (or whatever we end up calling the Serializer trait - personally I don't think "unstable" is a good term since the feature has been stabilized).

cgm616 · 2018-04-26T04:22:57Z

So the problem here is that there's no way to really write the serialize impls with distinct traits like that. (Or I simply don't know how.) For example, say we have the following code:

impl BreakingSerializer: Serializer {
    fn serialize_i128(
        self,
        v: i128,
    ) -> Result<<Self as Serializer>::Ok, <Self as Serializer>::Error>;
}

impl Serialize for i128 {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: BreakingSerializer,
    {
        serializer.serialize_i128(*self)
    }
}

Compilation fails with the following error:

   Compiling serde v1.0.41 (file:///Users/cgm616/Documents/Developing/rust/serde/serde)
error[E0276]: impl has stricter requirements than trait
   --> serde/src/ser/impls.rs:36:5
    |
36  | /     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
37  | |     where
38  | |         S: BreakingSerializer,
39  | |     {
40  | |         serializer.serialize_i128(*self)
41  | |     }
    | |_____^ impl has extra requirement `S: ser::BreakingSerializer`
    | 
   ::: serde/src/ser/mod.rs:238:5
    |
238 | /     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
239 | |     where
240 | |         S: Serializer;
    | |______________________- definition of `serialize` from trait

error: aborting due to previous error

For more information about this error, try `rustc --explain E0276`.
error: Could not compile `serde`.

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

So to fix this, we need to add a BreakingSerialize trait, but that can't be added without messing up the entire way that Serialize works and reimplementing all of the functionality of Serialize. This is because this new trait can't simply be BreakingSerialize: Serialize because the very types that need to implement BreakingSerialize need to do so because they cannot implement Serialize.

I'm at a loss how to proceed here. Am I missing something?

gavofyork · 2018-05-14T15:41:46Z

@cgm616 Did you make any further progress on this? Does anyone know of a workaround in the meantime?

gavofyork · 2018-05-14T15:44:00Z

@dtolnay Since u128/i128 is a stable and elementary datatype, I'd argue that it makes more sense to think of this as a bug rather than an enhancement. A newcomer to Rust/Serde doesn't know the history of 128 bit types and would consider it broken if Serde worked fine with one size of basic integer but not another.

gavofyork · 2018-05-15T13:48:53Z

As a follow up to that last comment, the docs state:

Serde provides such impls for all of Rust's primitive types

Which is no longer true as of Rust 1.26, thus implying this be a bug (and one I'd love to see squashed).

SimonSapin · 2018-05-15T14:07:33Z

This is a bug, but whatever we say about its gravity doesn’t change the fact that (as far as I can tell) fixing it requires a breaking change. And publishing a new serde 2.x version that’s incompatible with 1.x would have a large impact on the ecosystem, particularly if there isn’t an easy way for libraries to support both for their public types.

dtolnay · 2018-05-15T18:44:33Z

I haven't read the whole discussion in this issue but this shouldn't be a breaking change beyond bumping the minimum supported rustc version from 1.13 to 1.26. Rather than exposing a Cargo cfg I would prefer to automatically detect when we are on a sufficiently new compiler and provide the impls without a cfg opt-in.

oli-obk · 2018-05-15T18:50:05Z

That would require some build script trickery, but should be doable.

SimonSapin · 2018-05-15T21:47:35Z

If the Serializer trait gains a new required method serialize_u128, isn’t that a breaking change for existing implementations of that trait?

dtolnay · 2018-05-15T22:15:41Z

@SimonSapin we would not make it a required method, it would have a default impl that always returns error.

dtolnay · 2018-05-17T21:01:10Z

Based on the discussion in rust-lang/rust#35118 it seems like we are going to need a whitelist of platforms that have reliable support for i128. Right?

cgm616 · 2018-05-18T14:40:36Z

I think the use of an internal config with a build script to enable it based on compiler/platform support is a good idea. I worry about future complications (docs only including certain methods depending on the compiler used to build them, a fracturing of the packages that can work together based on compiler version, stuff like that), but it seems like the best path forward.

cgm616 · 2018-05-18T14:46:30Z

Oh, but again, I'm not sure if this is even possible. See my earlier comment (and errors):

Okay, hmmm. When using that in each individual crate, the methods are present. In addition, those crates compile fine.
It's only when compiling at the workspace level that it doesn't seem to work. Is it possible that there's a problem with the feature passing on?

And then the response by @PlasmaPower :

Oh yeah, that'll be a problem...
As far as I know, there's no good way to enable a dependency's feature conditional on a feature of the root crate: rust-lang/cargo#2524
There may be a crazy way to resolve this with additional crates, IDK.

There's no current way to enable the features of a dependency depending on the features of the current crate, making it difficult to gate features for workspace crates. A separate build script per crate in the workspace might work... do build scripts run when building from workspace root?

dtolnay · 2018-05-19T22:56:46Z

@cgm616 I believe my implementation in #1263 avoids whatever problems you encountered in your earlier comments. Please check it out and let me know if you think there is still a problem.

cgm616 · 2018-05-21T14:20:18Z

Seems to work! Conditionally defining a macro is a good idea, I never thought to do that. I'm excited for this to land.

dtolnay · 2018-05-25T23:12:38Z

I published 1.0.60 with this feature.

dtolnay added the enhancement label Mar 3, 2018

gavofyork mentioned this issue May 15, 2018

Balance is 128-bit value paritytech/substrate#162

Merged

1 task

dtolnay mentioned this issue May 15, 2018

Build script that does nothing #1259

Merged

dtolnay mentioned this issue May 18, 2018

Add Serde impls for i128 and u128 #1263

Merged

dtolnay self-assigned this May 19, 2018

dtolnay closed this as completed in #1263 May 25, 2018

pickfire mentioned this issue Aug 29, 2018

Please add support for NonZeroU128 #1352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i128 and u128 integers missing Deserialize impls #1136

i128 and u128 integers missing Deserialize impls #1136

PlasmaPower commented Jan 7, 2018

PlasmaPower commented Mar 24, 2018

seunlanlege commented Apr 8, 2018 •

edited

Loading

oli-obk commented Apr 9, 2018

cgm616 commented Apr 23, 2018 •

edited

Loading

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018

cgm616 commented Apr 23, 2018

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018 •

edited

Loading

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018

hcpl commented Apr 23, 2018

cgm616 commented Apr 23, 2018

cgm616 commented Apr 23, 2018

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 26, 2018 •

edited

Loading

gavofyork commented May 14, 2018

gavofyork commented May 14, 2018 •

edited

Loading

gavofyork commented May 15, 2018

SimonSapin commented May 15, 2018

dtolnay commented May 15, 2018

oli-obk commented May 15, 2018

SimonSapin commented May 15, 2018

dtolnay commented May 15, 2018

dtolnay commented May 17, 2018

cgm616 commented May 18, 2018

cgm616 commented May 18, 2018 •

edited

Loading

dtolnay commented May 19, 2018

cgm616 commented May 21, 2018

dtolnay commented May 25, 2018

i128 and u128 integers missing Deserialize impls #1136

i128 and u128 integers missing Deserialize impls #1136

Comments

PlasmaPower commented Jan 7, 2018

PlasmaPower commented Mar 24, 2018

seunlanlege commented Apr 8, 2018 • edited Loading

oli-obk commented Apr 9, 2018

cgm616 commented Apr 23, 2018 • edited Loading

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018

cgm616 commented Apr 23, 2018

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018 • edited Loading

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 23, 2018

hcpl commented Apr 23, 2018

cgm616 commented Apr 23, 2018

cgm616 commented Apr 23, 2018

PlasmaPower commented Apr 23, 2018

cgm616 commented Apr 26, 2018 • edited Loading

gavofyork commented May 14, 2018

gavofyork commented May 14, 2018 • edited Loading

gavofyork commented May 15, 2018

SimonSapin commented May 15, 2018

dtolnay commented May 15, 2018

oli-obk commented May 15, 2018

SimonSapin commented May 15, 2018

dtolnay commented May 15, 2018

dtolnay commented May 17, 2018

cgm616 commented May 18, 2018

cgm616 commented May 18, 2018 • edited Loading

dtolnay commented May 19, 2018

cgm616 commented May 21, 2018

dtolnay commented May 25, 2018

seunlanlege commented Apr 8, 2018 •

edited

Loading

cgm616 commented Apr 23, 2018 •

edited

Loading

cgm616 commented Apr 23, 2018 •

edited

Loading

cgm616 commented Apr 26, 2018 •

edited

Loading

gavofyork commented May 14, 2018 •

edited

Loading

cgm616 commented May 18, 2018 •

edited

Loading