servo / app_units

https://crates.io/crates/app_units
3 stars 17 forks source link

Round-trip serialize/deserialize of `Au` is broken #50

Closed cillian64 closed 1 year ago

cillian64 commented 1 year ago

When originally written, Au used a custom serializer/deserializer which formatted it as the contained i32. In d60d1426, Au was updated to use the default derive-Serializer but still keeps its custom deserializer. With the RON serializer, the derive-serializer formats Au(3) as (3). But the custom deserializer expects it to be serialized as 3. The following example demonstrates the problem:

use app_units::Au;

fn main() {
    let foo = Au(123);
    let ronned = ron::ser::to_string(&foo).unwrap();

    println!("ronned: {:?}", ronned);

    let de: Au = ron::de::from_str(&ronned).unwrap();
    println!("de: {:?}", de);
}

When run with app_units 0.7.2 and ron 0.8.0 this gives the following output:

ronned: "(123)"
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: SpannedError { code: ExpectedInteger, position: Position { line: 1, col: 1 } }', src/main.rs:9:45
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Using derive(Deserialize) fixes this problem but doesn't include the clamp in the custom deserializer. I assume we could just update the custom deserializer to expect the parenthesis but I haven't tried implementing this. Another option would be to reinstate the custom serializer so we don't include the parenthesis, but I don't know if there's a preference for not changing the format of serialized webrender captures (I discovered this issue when trying to load a webrender capture in wrench, I'm a little confused why nobody else has hit the same problem).

gw3583 commented 1 year ago

I ran into this issue today as well, and confirm a local patch with auto derive works around the problem I was seeing.

I'm not entirely sure why there is a clamp in the custom impl, I would have thought there shouldn't be any logic that can change a value there. If we don't need the clamp, then removing the custom impl and auto-deriving it seems like that's probably the best fix.

@Florian-Schoenherr @jdm Do you have any ideas / insight in to why there is a clamp in that impl?

cillian64 commented 1 year ago

Fixed by #51, thanks!