search

smithy-lang / smithy-rs

Code generation for the AWS SDK for Rust, as well as server and generic smithy client generation.
Apache License 2.0
498 stars 187 forks source link

Should we implement @range on float and double shapes? #2007

Open LukeMathWalker opened 1 year ago

LukeMathWalker commented 1 year ago

The smithy spec seems to suggest that @range is allowed on float and double shapes.

We should clarify if that is correct and how it should be implemented (e.g. should range allow to specify a tolerance threshold for comparisons?).

crisidev commented 1 year ago

Shouldn't this issue be in the awslabs/smithy repo? The spec "seems to suggest", but it not really clear right?

LukeMathWalker commented 1 year ago

Yes, this is for tracking on our side/to give visibility to users of smithy-rs who might be interested. @david-perez is going to follow up with the Smithy maintainers to get clarity (and if we have a public issue we'll link it here).

david-perez commented 1 year ago

I don't want to implement @range on floating point shapes because comparing floating point numbers without a tolerance threshold is almost always not what you want.

Consider a client performing floating point arithmetic and sending the result to a service:

$ python3 -c "print((0.1 + 0.2) > 0.3)"
True

Having the server framework compare floating point numbers generically can have devastating consequences in certain application domains. Depending on the hardware, FPU, compiler and compiler flags, an operation like 0.1 * 10 may yield different results, and as such may or may not satisfy @range(min: 1) in all target language implementations of Smithy.

This is not even getting into the problems that arise from negative and positive infinities, NaN breaking total ordering (the reason why Rust only implements PartialOrd and not Ord), or the total ordering imposed by IEEE-754.

I think that there's no easy way for Smithy to support float comparisons as-is interoperably across languages and architectures without prescribing an epsilon threshold.

This is also one of the reasons why Smithy dropped support for floating point shapes from set shapes (which ultimately got deprecated in favor of @uniqueItems in IDL v2), and the sole reason why @uniqueItems cannot apply to a list shape that transitively contains floats, doubles, or documents. Unfortunately, there's currently several uses of @range as-is on floating point shapes in existing AWS models that Smithy has limited options to address this.

However, smithy-rs, especially server-side development, is usually greenfield, and so we have a chance here to warn service owners. I therefore think it's best that this is not implemented, and instead each smithy-rs application, depending on their needs, enforce the check themselves as they require.