`float` value validation isn't right, accepts invalid values

[osa1]

edition = "2024";

message Foo {
  repeated float fs = 1;
}

import 'test.pb.dart';

void main() {
  final foo = Foo();
  foo.fs.add(0.123456789);
  print(foo);
  print(Foo.fromBuffer(foo.writeToBuffer()));
}

Prints:

fs: 0.123456789
fs: 0.12345679104328156

This is because the value being set isn't valid float, but the validation function isn't correct and accepts it:

protobuf.dart/protobuf/lib/src/protobuf/field_error.dart

Lines 140 to 143 in 27730db

	bool _isFloat32(double value) =>
	value.isNaN ||
	value.isInfinite ||
	(-3.4028234663852886E38 <= value) && (value <= 3.4028234663852886E38);

In this particular case, it doesn't handle the numbers between the min. and max. specified by this function, but are have more fractional digits that a 32-bit float can represent. There could be other bugs too.

Ideally you would convert the number to a float and back to a double, and check that you get the same number back, but we can't easily to this in Dart. Not sure if there's a performant way to do it, without writing the number to a Float32List or a ByteData and reading it back.

[osa1]

I think this could potentially be intentional, though not being able to roundtrip feels wrong. I'll ask the protobuf team about this.

[lrhn]

(If intentional, it can be written shorter as !value.isFinite || value.abs() <= 3.4028234663852886e+38.)

[rakudrama]

I suggest no validation (other than is double).

It would be unreasonable to require the user to pass in a double that happens to have an exact float32 representation.
Even if dart:math had fround, the user would have to defensively call it, e.g. foo.fs.add(math.fround(0.123456789)).

Given that requiring exact float32 values is unreasonable, and there are no double values that do not map to some float32 value, add could reasonable accept any double value.

If we convert, should we convert on assignment or only on serialization, and convert for all wire-formats or just binary ones, leaving the original 64-bit value in JSON.

Conversion on assignment would ensure that calling an out-of-process service call with a serialized message behaves the same as an in-process service call with a direct (unserialized) message.

There are some downsides to conversion-on-assignment:

• fround is likely to yield a different object to the input (but unlikely to be worse than any validation).
• When serializing to JSON, messages that use float32 for smaller wire-size we might get the opposite effect, emitting 0.12345679104328156 instead of 0.123456789.

@lrhn Can we add fround (perhaps renamed) to dart:math to ensure that on all platform conversion is not needlessly inefficient?

[osa1]

From the discussion here and also with the protobuf team internally, I'm inclined to leave this as-is:

• It's unclear how to change this. There are advantages and disadvantages to

  • Checking it in different places: when setting the fields vs. when encoding.
  • Checking and not checking: we could choose to accept all doubles and round when serializing, as today.

• If we want to check, it's unclear what to exactly check:

  • Checking that the values are the same when rounded to a float will reject too many values
  • OTOH, it's also unclear whether checking the min/max values as today is useful

• Because protobufs are used in JS, Python, Java, and other languages that also don't have the same int/float types (with different signs and sizes) as protobuf, this kind of thing is not too uncommon.

• This code has been here for at least 12 years, it was added with the "initial import" of this repo:

  protobuf.dart/lib/generated_message.dart

  Lines 18 to 19 in d6b1923

  	_isFloat32(double value) => value.isNaN || value.isInfinite ||
  	_inRange(-3.4028234663852886E38, value, 3.4028234663852886E38);

  I've been maintaining this library for the past ~3.5 years and I haven't seen anyone complain about the double values losing precision as you send them over via protobufs.

  (I've only noticed this behavior because I saw the check function and thought that it can't be right, and tested it. It didn't come up in a real use case.)