search

ruma / js_int

JavaScript-interoperable integer types for Rust
MIT License
16 stars 8 forks source link

Bugfix: lax_deserialize currently accepts deserializing from NaN #25

Closed FSMaxB closed 2 years ago

FSMaxB commented 2 years ago

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 https://github.com/serde-rs/json/pull/516 obsolete as well ...).

jplatte commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

jplatte commented 2 years ago

Maybe it would be good for this use case though.

FSMaxB commented 2 years ago

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 :grimacing:

jplatte commented 2 years ago

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 commented 2 years ago

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 :grin: .