search

zclconf / go-cty

A type system for dynamic values in Go applications
MIT License
338 stars 70 forks source link

stdlib: SetProductFunc doesn't seem to handle refinements quite right #157

Closed apparentlymart closed 1 year ago

apparentlymart commented 1 year ago

The Terraform team at HashiCorp accidentally upgraded to cty v1.13 and got refinements in a patch release. They plan to revert back to v1.12 for now but in the process they learned about https://github.com/hashicorp/terraform/issues/32853 which they suspect is a bug in SetProductFunc's handling of refinements.

I've not yet reproduced this to confirm, but if refinements are the cause then it seems like the function is trying to apply collection-type-specific refinements to a value that isn't of a collection type.

apparentlymart commented 1 year ago

This doesn't seem to be directly related to refinements because the following is the line blamed in the Terraform stack traces:

https://github.com/zclconf/go-cty/blob/0401e09da18c5a83582814b19f267b66c6704191/cty/function/stdlib/collection.go#L982

However, it does still seem like something's not quite right here, because all of the successful return paths in Type return collection types so it's not clear how a non-collection type would end up as retType in the Impl function. Further investigation required!

apparentlymart commented 1 year ago

This is now fixed in v1.13.1.

The problem wasn't refinements directly, but it arose because I weakened the constraints on the parameters to this function to allow it to handle unknown values itself, so that the Impl function could produce refined unknown results. That then exposed a previously-hidden bug where the function package was still calling Impl even after short-circuiting Type, causing this function to be asked to produce a cty.DynamicPseudoType result even though it had never promised it could.

The fix then was to make the function system respect its own rule that Impl will only be called if Type was called and it succeeded. Passing a cty.DynamicVal into SetProductFunc (or any of the other functions that got similar treatment in v1.13.0) will now immediately return cty.DynamicVal as before, rather than panicking after calling its Impl function with an unsupported type.