prevent panic in Value.AsBigFloat()

[azr]

Howdy 🙂 !

this fixes the following panic: interface conversion: interface {} is big.Float, not *big.Float I had while using this Func.

For context: I have encoded some cty.Values using gob and this might be causing it.

[codecov]

Codecov Report

Merging #31 into master will decrease coverage by 0.07%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   75.92%   75.85%   -0.08%     
==========================================
  Files          74       74              
  Lines        5758     5765       +7     
==========================================
+ Hits         4372     4373       +1     
- Misses        996     1002       +6     
  Partials      390      390

Impacted Files	Coverage Δ
cty/value_ops.go	83.12% <55.55%> (-0.51%)	⬇️
cty/convert/compare_types.go	97.61% <0%> (-2.39%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f21e35a...7e360d5. Read the comment docs.

[apparentlymart]

Hmm... thanks for reporting this odd behavior, @azr!

My intent here was that a number value should always be a *big.Float, and never just big.Float, so it seems like there must be a bug somewhere else where a number value is being constructed improperly. While I can see that your change here would address the symptom of panicking, I expect there will be other situations that would panic for the same reason because other code in cty tends to assume that the v field of cty.Value will always be of the Go type associated with the cty.Type unless it's nil or the special "unknown" value.

That you are using gob here seemed like a good clue so I went looking around in the gob-related code and found this interesting snippet in Value.GobEncode:

go-cty/cty/gob.go

Lines 49 to 53 in f21e35a

	// big.Float seems to, for some reason, lose its "pointerness" when we
	// round-trip it, so we'll fix that here.
	if bf, ok := gv.V.(big.Float); ok {
	gv.V = &bf
	}

...so indeed it does seem that in the past I found that this problem would occur under gob encoding, but I'm now wondering why this mitigation isn't fixing it as intended.

Do you happen to have a small code snippet that illustrates the situation you were trying to fix here? I'd ideally like to try to debug it further to see if we can fix the root cause, rather than adding in another conditional branch to treat the symptoms.

[azr]

Hey Martin, gotcha ! 3e83fcc adds a tests that provokes the panic. I'll try to see if I can fix at the source 🙂.

[apparentlymart]

Thanks @azr! It sounds from your comment like you're planning to debug this some more yourself, so I'll leave you to do that for now but feel free to let me know if you'd like me to dig in here and do some debugging too.

[azr]

Hello @apparentlymart ! I have been struggling with this one; everything I did caused a panic or made something else panic so I decided to refactor the Number testing to sort of scope it down; which also caused a panic and I may be doing something wrong here 🤔 but it looks like TestValueEquals was having a variable shadowing issue.

I think the last commit I pushed (67fae68) explains it better. (I think probably only the last tests were passing)

Edit: if it's true for TestValueEquals then it's true for more tests; adding this to see.

Edit2: Looks like this could be very true because t.Run launches a goroutine: https://github.com/golang/go/blob/3922c006ad57f042238e48bb2cd13e5d88499a6c/src/testing/testing.go#L1000-L1005

[azr]

Martin, I require your help 🙂 ! Am I doing something wrong here ? Should I test my case somewhere else ?

[apparentlymart]

Sorry for the delay in responding, @azr. I'll take a closer look at this to see if I can understand what's going on.

[apparentlymart]

I have tracked down what is required to trigger the panic: it seems to occur when a number appears inside some other collection value, which makes sense because the Value.GobDecode function only directly visits the top-level value, with everything else inside represented directly as the Go types that cty uses internally for values.

I'm going to see what it will take to either understand why the big.Float pointers are being un-pointered in the gob round-trip in the first place, or failing that to try to generalize the special case so it can find nested big.Float values too.

[apparentlymart]

I think the underlying problem here is related to the fact that GobEncode is a method with a pointer receiver, and so big.Float.GobEncode can't tell in practice whether it's being called via a big.Float or a *big.Float value (it always ends up being *big.Float) and it seems like it's just assuming that we want to encode the value part, ignoring the pointer.

This feels like a bug in either encoding/gob or big.Float.GobEncode to me, but it seems like one that can't be fixed without violating the Go 1.0 compatibility promise, and so I'm therefore going to try to generalize the existing workaround to catch deeply-nested big.Float values too.

[apparentlymart]

My theory as to the cause seems indirectly confirmed by golang/go#32251, where a similar problem is observed on url.URL values due to MarshalBinary having a pointer receiver. It seems like generally-speaking MarshalBinary should never be implemented with a pointer receiver, but any existing implementors are constrained by backward compatibility to retain their bug.

[apparentlymart]

Hi again, @azr!

After a few different attempts I got something that seems to work over in #32. I'm going to land a few other pending PRs here and then tag a release of this module containing the fix.

Thanks for the work here to narrow down the problem! In particular your failing test case was very helpful to understand why the existing test TestGobabilty wasn't panicking in the same way.

[azr]

Ah nice thanks @apparentlymart, yes about the original issue I was under the impression that because big.Float{} was gob.Registered as a non pointer type :

go-cty/cty/types_to_register.go

Lines 32 to 41 in 14e23b1

	InternalTypesToRegister = []interface{}{
	primitiveType{},
	typeList{},
	typeMap{},
	typeObject{},
	typeSet{},
	setRules{},
	set.Set{},
	typeTuple{},
	big.Float{},

Then it could only bubbled up as this type; I was planning on going forward with this but was stuck with a bunch of panics.