cannot store 33.33333333 in float64

[mschuchard]

Module version

1.15.1

Relevant provider source code

// schema
Parameters: []function.Parameter{
    function.Float64Parameter{
        Name:        "number",
        Description: "Input number parameter for determining the rounding.",
    },
},
// go type reflection
var inputNum float64

resp.Error = req.Arguments.Get(ctx, &inputNum)

Terraform Configuration Files

Please see mschuchard/terraform-provider-stdlib#2

Debug Output

upon request

Expected Behavior

The return of 100 / 3 in HCL2 is valid as a float64 tftype.

I am unsure whether this is an issue in my provider, tftypes, terraform core, or go-cty, and so I am filing this issue here first for feedback since my suspicion is the issue is related to the return type of the / operator, and therefore most likely tftypes or go-cty in that order.

Actual Behavior

Please see mschuchard/terraform-provider-stdlib#2

Steps to Reproduce

n/a

References

linked above

[apparentlymart]

I suspect that this is a problem of inconsistent precision between float64 and big.Float, where the latter is how numbers are represented in memory before a caller attempts to convert them to a lower-range numeric type.

terraform-plugin-framework/internal/reflect/number.go

Lines 164 to 175 in 5310c68

	case reflect.Float64:
	floatResult, _ := result.Float64()
	bf := big.NewFloat(floatResult)
	if result.Text('f', -1) != bf.Text('f', -1) {
	diags.Append(roundingErrorDiag)
	return target, diags
	}
	return reflect.ValueOf(floatResult), diags

result.Text('f', -1) != bf.Text('f', -1) is tricky... both result and bf are big.Float, but bf was presumably created by the MessagePack decoding in terraform-plugin-go, and it probably went through the big.ParseFloat(rv, 10, 512, big.ToNearestEven) path that causes the result to have a precision of 512. result is created using big.NewFloat, which always returns with a precision of 53.

result.Text with the second argument set to -1 uses the precision of the value being formatted, and so the string representation of result and bf probably have a different number of decimal places and so the string doesn't match. This would be true for any number that cannot be represented exactly as a float value.

Unfortunately this is a pretty tricky problem without a completely-satisfying solution, and more just a bunch of tradeoffs. Truncation of certain values to a lower-precision floating point representation inherently loses information, and so I would consider code as trying to implement a heuristic for whether the float64 result is similar enough to the higher-precision number that was sent to the provider, though indeed it seems like the current form of that heuristic is too strict.

I can see in the history that this current code was already a compromise compared to an earlier version that was relying on the "accuracy" information returned by the big.Float conversion functions, in #919. FWIW, the closest equivalent of this operation in cty (which, based on commentary in the snippet I linked above, terraform-plugin-go seems to be mimicking here) is to silently accept loss of precision in such a conversion as long as the result isn't a floating point infinity. Admittedly the plugin framework is under an additional constraint here that the provider protocol requires responses in certain contexts to exactly match data in the request -- though that doesn't actually apply to function calls as in this case -- so probably this stricter interpretation is to help avoid errors like "provider produced inconsistent result in plan" etc when implementing managed resource types. 🤔

Even if the heuristic doesn't change, it doesn't seem correct for the framework to report this as a bug in the provider: this situation is an inherent consequence of converting to float64, and so the only way a provider could avoid encountering it is to not try to convert to float64 at all. If this is to be reported as an error then I think it should be presented as the module author having specified an out-of-range value, similar to how cty handles conversion to fixed-size integers, although in this case the module author could presumably fix it only by preemptively rounding the number e.g. using format with the %f verb's precision modifier, which would be a strange requirement for an argument to a function whose only job is to round a number. 😀

[mschuchard]

I may be oversimplifying the root cause here but could https://pkg.go.dev/math/big#Float.Float64 not solve this at least partially? I know there is no IsFloat64() to validate overflows like with BigInt's IsInt64, but it does have an accuracy parameter.

Alternatively, I believe that the Number type value is represented as a *big.Floats value according to the tftypes documentation, and so if I begin using the Number type for the schema instead of Float64 then this is fine with me even though it probably loses a very tiny amount of efficiency. Also alternatively what if I retain the schema as a Float64 but use a *big.Float for reflection (which is what I believe is implied by "not try to convert to float64 at all")? Please let me know on either of these workarounds.

Admittedly the plugin framework is under an additional constraint here that the provider protocol requires responses in certain contexts to exactly match data in the request -- though that doesn't actually apply to function calls as in this case -- so probably this stricter interpretation is to help avoid errors like "provider produced inconsistent result in plan" etc when implementing managed resource types.

i would assume this also because custom functions are subject to these kinds of restrictions even if it seems to succeed in a vacuum e.g. I enabled generic types for elements in collections for input parameters in custom functions such that it successfully passed unit tests (as long as the type is of course uniform throughout the collection), but then unsurprisingly it failed acceptance tests due to violating schema restrictions. Austin's explanation about why I could not enable it solely for custom functions confirms (to me anyway) custom functions need to adhere to the same "rules" as everything else supported in the plugin framework.

[apparentlymart]

My intention in the previous comment was to try to identify the problem from the framework's own perspective rather than to propose a specific workaround for you, but what you've inferred from what I shared seems reasonable to me.

In particular, having this specific function do its work entirely with big.Float instead of trying to convert to float64 first seems like it would avoid this problem, indeed. That's what the built-in functions that work with numbers tend to do, also.

I think the approach with big.Accuracy that you described is what this code was doing before the PR I linked to, but I think that has effectively the same problem: there is no way to convert this specific value to float64 without losing precision, and so the accuracy-based approach would still detect this situation as a loss of precision and so report it as an error. I think the only way to make this actually work is to tolerate the loss of precision, but that risks running into trouble with Terraform's consistency checks.

(I should note, in case any reader recognizes my username, that although I used to work on Terraform at HashiCorp that is no longer true and so nothing I've said here today is on behalf of any team at HashiCorp. I'm just sharing this context in the hope that it's helpful.)

[mschuchard]

Yes I at least personally understand you are "pinch hitting" here, but thanks for confirming the proposed workaround is worth pursuing in the short term at least. I would also be interested in what Austin et al would propose too since that would be the direction this module is headed.

[mschuchard]

I also really need a pointer on reproducing this specific behavior with tests.

types.Float64Value(100 / 3) produces the expected behavior and result. types.Float64Value(100) / types.Float64Value(3) errors because the / operator is not defined for the float64 tftype. types.Number fails because it is not an acceptable input type for the schema. I then tried what I assumed would definitely be the behavior:

repeater, _ := new(big.Float).Quo(types.NumberValue(big.NewFloat(100)).ValueBigFloat(), types.NumberValue(big.NewFloat(3)).ValueBigFloat()).Float64()

Arguments: function.NewArgumentsData([]attr.Value{types.Float64Value(repeater)}),

but that STILL succeeds.

Therefore, all of my attempts at reproducing with tests yield that either this should succeed, or that it should fail during what would be equivalent to a terraform validate subcommand. Neither of these results are the actual behavior where the plugin framework is failing (the go type reflection).

[apparentlymart]

If I'm right about the root cause -- that Terraform is choosing to serialize the value as a string to preserve the high precision of the source value from the configuration -- then I think you probably need to match how the MessagePack decoding transforms a string into a big.Float value (linked in my first comment above), and ensure there are no subsequent conversions to float64 that would therefore truncate the precision of the input value.

You can mimic how HCL would parse a numeric literal by using the same big.Float parsing as cty.ParseNumberVal, since that's the same function HCL itself uses. So if you start by parsing both "100" and "3" using that strategy, then you can use both of those with Quo to get the fractional value that caused the original problem, with the same high precision.

To be fully realistic you'd need to then send that through the MessagePack encoding and decoding codepaths, but I don't think that should be necessary in practice here because the MessagePack decoder parses the string representation of numbers in a similar enough way as the cty function does.

Caveat that I've not tested any of this and am working only in theory here, but hopefully this is a helpful starting point.

[mschuchard]

I took a shortcut and did not reproduce due to precision, but instead with the easier path forward of the input parameter 1e+310 which is above the float64 upper limit, and that is good enough for me (although of course not exactly the same scenario). Thanks for the push in the correct direction.

Using Number and *big.Float as suggested also seems to workaround the issue, but now I have to do additional checks on the float64 conversion, but those are also borderline trivial, and so it is all good.