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.

[austinvalle]

Hi folks 👋🏻, reading through all the comments I think it's been explained well what's going on (appreciate the insight @apparentlymart!), but I will attempt to add some useful context from the maintaining team.

---

Generally speaking, you're both correct that the main goal of the reflection logic in Framework is to be consistent with what Terraform sends us in the protocol (i.e. we do our best to ensure we don't silently lose precision), as a good number of use-cases for retrieving/setting data via reflection are managed resources producing plan and state objects. One of the bigger concerns we have is just ensuring config values stay the exact same precision wise, which is why Framework now stores float64 types as big.Float under the hood -> #817 .

---

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.

That works fine, another option would be to continue using Float64Parameter and just use the types.Float64 struct to retrieve the argument, as that will successfully retain the precision initially, which you can then discard (if you don't care about the accuracy) by just calling (types.Float64).ValueFloat64()

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

// ---> inputNum.ValueFloat64() will return a float64, ignoring the accuracy return field

You also could just use *big.Float with reflection and a Float64Parameter, since that's what framework stores under the hood:

var inputNum *big.Float
resp.Error = req.Arguments.Get(ctx, &inputNum)

f64, acc := inputNum.Float64()

---

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

As for this, what @apparentlymart mentioned would work fine, you'll just need to create the data exactly as terraform-plugin-go does, which would look like this:

// This mimics HCL's division operation
arg1, _, _ := big.ParseFloat("100", 10, 512, big.ToNearestEven)
arg2, _, _ := big.ParseFloat("3", 10, 512, big.ToNearestEven)
result := new(big.Float).Quo(arg1, arg2)

testFloat64, _ := types.Float64Type.ValueFromTerraform(
	ctx,
	tftypes.NewValue(tftypes.Number, result),
) // This types.Float64 will have the precision you're looking to test for

argData := function.NewArgumentsData([]attr.Value{testFloat64})

// You can then pass argData into your provider function `Run` method

[mschuchard]

I appreciate both of those workaround suggestions, but I would feel more comfortable working with math/big type conversions for the receiver (first), and would also feel more comfortably reflecting a Number into a *big.Float than a Float64 since those are roughly analogous (second). I am (of course) not as much into this space as you guys, and I know it is not actually reflection, but I cynically wonder if impl Into<T> or impl TryFrom<T> in Rust would solve something like this outright. Anyway...

i also appreciate that snippet, but my lazy route was:

function.NewArgumentsData([]attr.Value{types.NumberValue(func() *big.Float { f := new(big.Float); f.SetString("1e+310"); return f }())}),

for rough equivalence of reproducing the reflection issue, and yes: Claude did convert it into a lambda for me so I could just one-line it and be done. I am not taking credit for that one.

All that being said: what is the plan moving forward for the plugin framework for situations like this? If the update is merely more accurate error logging then I think that would be fine (and Mart did say something similar in the last paragraph of the first comment).

[apparentlymart]

Regarding just the sidebar about Rust's Into<T>, TryFrom<T>, etc traits:

Unfortunately Go's design for generics and interfaces cannot support a pattern like that because interface methods are matched by the method names and so it's not possible for a type to implement multiple different instantiations of a generic interface at once if the function signature includes one of the type parameters.

For example, if you wrote an interface like this:

type TryInto<T> interface {
    TryConvertInto() (T, error)
}

...then any specific type could implement this for at most one T, because each type can only have one method named TryConvertInto, regardless of the signature of that method. This pattern works in Rust because trait implementations are explicit and so there's no problem with a single type implementing multiple traits (or multiple instantiations of the same generic trait) that all require methods of the same name, as long as callers disambiguate at the callsite which of the variants they are intending to call.

This sort of thing is more typically handled in Go using runtime reflection, which is the approach the plugin framework is already following.

[austinvalle]

All that being said: what is the plan moving forward for the plugin framework for situations like this? If the update is merely more accurate error logging then I think that would be fine (and Mart did say something similar in the last paragraph of the first comment).

I'm not opposed to producing a different error message with more information for some of the different permutations of rounding errors that are produced, although I'm not really sure what would be a good recommendation 🤔.

1. Should we be instructing the practitioner to modify their configuration so that the precision lost is explicitly performed prior to any provider defined logic? In this scenario it looks like the outcome was modifying the provider code to handle the precision loss, would the preference have been the practitioner was told to just use format and no bug report filed?
2. Is the problem that the error message points to the provider code itself as the problem? Our error message skews towards reporting to provider developers first so that practitioners don't report this to Terraform core (don't shoot the messenger 😆).
3. Is just more information about the exact rounding problem enough? Like "cannot convert to float64 without losing precision"

I'm open to suggestions 😄

[mschuchard]

My whimsical comment was more about some sort of alternate reality where the Terraform ecosystem was developed in Rust instead of Go, but still an interesting explanation, and thank you for that. I think I actually slammed into something similar to what you described when I tried a similar approach for implementing Rust-like enums and trait type conversions of those enums in Go, and then gave up and developed something that was something like 90% similar to the normal design pattern for "enum" in Go anyway.

[mschuchard]

I would personally prefer option 3 because the debugging and resolution process here was inhibited by the error message. The reporter/plugin user saw an error message that claimed there was an issue with the plugin (which there is the vast majority of the time when the reflection fails), and followed the instructions. I then had to scrutinize my code and file an issue here because I initially had no idea why a Float64Parameter was unable to reflect into a Go float64 as that seemed bizarre to me before Mart's explanation that the / operator returns a *big.Float, and then also shared the relevant source code.

Option 3 seems like it would have significantly decreased the debugging, root cause analysis, and resolution process. It also seems like "low-hanging fruit" from the perspective of the maintainers. Additionally I think it maintains the integrity of the current code for and in modules, plugins, and Terraform ecosystem Go modules.

[austinvalle]

Fair enough, I think that sounds like a reasonable way forward. I'll raise a PR to add that information to our error messaging 👍🏻