Bugfix: lax_deserialize currently accepts deserializing from NaN

[FSMaxB]

With the lax_deserialize feature, NaN currently deserializes to 0. I don't think this was intended.

Although this can't happen with serde_json because JSON doesn't support NaN etc., this can very well happen with other deserializers.

This pull request fixes this by declinining to deserialize NaN, Infinity and -Infinity.

Note that using IntoDeserializer, as I've done here, could be used for the other tests as well, thereby getting rid of the necessity to use serde_json if desired. (This, by extension, should make the tests work with no-std again, thereby making the workaround for serde-rs/json#516 obsolete as well ...).

[jplatte]

This pull request fixes this by declinining to deserialize NaN, Infinity and -Infinity.

To be clear, the infinities were already rejected before, right?

Note that using IntoDeserializer, as I've done here, could be used for the other tests as well, thereby getting rid of the necessity to use serde_json if desired. (This, by extension, should make the tests work with no-std again, thereby making the workaround for serde-rs/json#516 obsolete as well ...).

Sounds good!

[FSMaxB]

To be clear, the infinities were already rejected before, right?

You're right, I've missed that. I've changed the code to only check for is_nan.

Sounds good!

I'll do that then.

[FSMaxB]

Ok, this was a bit more complicated than anticipated because I totally forgot about the Serialize direction. But nothing a custom TestSerializer couldn't fix!

[jplatte]

There's also serde_test but I find its API to be a little annoying to use.

[jplatte]

Maybe it would be good for this use case though.

[FSMaxB]

Sorry about the force push, I still had expand.rs in there (I was looking at what #[derive(Serialize)] produces.

There's also serde_test but I find its API to be a little annoying to use.
Maybe it would be good for this use case though.

I'll look into that. Let's hope that it works with no-std 😬

[FSMaxB]

I was tempted to use #[track_caller] here but then I remembered that the MSRV is 1.35.

Using a macro would also be possible but I don't think it's really worth it.

[FSMaxB]

Writing out the NewTypeStruct serialization token I wonder: Is this behavior actually intended? Should a Serializer really know both the name of the struct and it's internal workings?

This relates to #16 because a manual Serialize implementation could avoid leaking such implementation details.

[jplatte]

That's a good point. Idk which serializers out there actually serialize sth. for NewtypeStruct, but I agree that we shouldn't emit it. If the derives aren't to be replaced entirely, #[serde(transparent)] could be used to get rid of it.

[jplatte]

I have merged the first two commits (as one squashed commit) for now. It looks like you will have to rebase, which is a bit weird to me since I would expect a rebase to go through w/o any manual work needed.

[FSMaxB]

I'll close this pull request then and create a new one for the testing changes. I expect that this requires an interactive rebase, but no worries, I have experience with that 😁 .