automerge / automerge-classic

A JSON-like data structure (a CRDT) that can be modified concurrently by different users, and merged again automatically.
http://automerge.org/
MIT License
14.75k stars 466 forks source link

Explicitly distinguish integers and floating-point values #356

Closed ept closed 3 years ago

ept commented 3 years ago

When binary-encoding a change or a document, Automerge-JS uses the following logic to decide whether to encode a number as an integer or as floating point:

if (Number.isInteger(value) && value <= Number.MAX_SAFE_INTEGER && value >= Number.MIN_SAFE_INTEGER) {
  // integer
} else {
  // floating point
}

This is problematic since it forces JavaScript's idiosyncratic handling of numbers onto Rust, which does not conflate integers and floating-point numbers in the way JS does. One particular failure mode is that in Rust, you can create an Automerge document containing the floating-point value 1.0; when this document is decompressed in JS, that value is treated as an integer, so the reconstructed change history contains an integer rather than a floating-point value. Thus, the reconstructed change history is no longer bytewise identical to the original, which breaks hash chaining and causes document loading to fail.

I think the best way of handling this is for the decompressed change format in JS to explicitly tag every number as either an integer or floating-point. We can use the existing datatype property for this. That should ensure safe round-trip conversions between the compressed binary and uncompressed representations.

(Thanks @orionz for reporting this)

orionz commented 3 years ago

I want to put together a PR for this... how about we use the datatype values of int, uint, f32 and f64. distinguishing between f32 and f64 seems needed since either can be encoded in VALUE_TYPE_IEEE754

ept commented 3 years ago

Agree those are the four types we need. Thanks!

ept commented 3 years ago

Fixed by #371