zclconf / go-cty

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

panic with `AsString()` on result of json encoding of to_number conversion of null value #177

Closed johakoch closed 3 months ago

johakoch commented 3 months ago

See code in https://go.dev/play/p/Y3j39bAiRKq:

package main

import (
    "fmt"

    "github.com/zclconf/go-cty/cty"
    "github.com/zclconf/go-cty/cty/function/stdlib"
)

func main() {
    toNumberFn := stdlib.MakeToFunc(cty.Number)
    nullVal := cty.NullVal(cty.DynamicPseudoType)
    args := []cty.Value{nullVal}
    n, err := toNumberFn.Call(args)
    if err != nil {
        panic(err)
    }
    fmt.Printf("result of toNumber(): %#v\n", n)

    args = []cty.Value{n}
    // obj := cty.ObjectVal(map[string]cty.Value{"n": n})
    // args = []cty.Value{obj}
    j, err := stdlib.JSONEncodeFunc.Call(args)
    if err != nil {
        panic(err)
    }
    fmt.Printf("result of json encode: %#v\n", j)
    fmt.Printf("result of json encode as string: %#v\n", j.AsString())
}

If I pass a cty.NullVal(cty.DynamicPseudoType) as param to a stdlib.MakeToFunc(cty.Number) conversion function and then pass the result to stdlib.JSONEncodeFunc I get a result (cty.UnknownVal(cty.String).RefineNotNull()) which cannot be "stringified" with .AsString(). Instead AsString() panics with

panic: value is unknown

Is this the intended behaviour?

johakoch commented 3 months ago

The value returned from toNumberFn.Call(args) is

cty/function/function.go:

func (f Function) Call(args []cty.Value) (val cty.Value, err error) {
    expectedType, dynTypeArgs, err := f.returnTypeForValues(args)
    if err != nil {
        return cty.NilVal, err
    }
    if dynTypeArgs {
        // returnTypeForValues sets this if any argument was inexactly typed
        // and the corresponding parameter did not indicate it could deal with
        // that. In that case we also avoid calling the implementation function
        // because it will also typically not be ready to deal with that case.
        return cty.UnknownVal(expectedType), nil
               ^^^^^^^^^^^^^^
    }

because of

func (f Function) returnTypeForValues(args []cty.Value) (ty cty.Type, dynTypedArgs bool, err error) {
...
        if val.Type() == cty.DynamicPseudoType {
            if !spec.AllowDynamicType {
                return cty.DynamicPseudoType, true, nil
            }

If I add

                AllowDynamicType: true,

to the parameter specification in cty/function/stdlib/conversion.go

func MakeToFunc(wantTy cty.Type) function.Function {
    return function.New(&function.Spec{
        Description: fmt.Sprintf("Converts the given value to %s, or raises an error if that conversion is impossible.", wantTy.FriendlyName()),
        Params: []function.Parameter{
            {
                Name: "v",
                // We use DynamicPseudoType rather than wantTy here so that
                // all values will pass through the function API verbatim and
                // we can handle the conversion logic within the Type and
                // Impl functions. This allows us to customize the error
                // messages to be more appropriate for an explicit type
                // conversion, whereas the cty function system produces
                // messages aimed at _implicit_ type conversions.
                Type:             cty.DynamicPseudoType,
                AllowNull:        true,
            },
        },

there's no panic, because then the value returned from toNumberFn.Call(args) is a cty.NullVal(cty.Number).

johakoch commented 3 months ago

This occurs in an HCL-based config language with to_number() and json_encode() functions implemented by stdlib.MakeToFunc(cty.Number) and stdlib.JSONEncodeFunc when passing literal null: json_encode(to_number(null)).

So the cty.NullVal(cty.DynamicPseudoType) is the result of parsing a literal null using ParseHCL() on an *hclparse.Parser.

johakoch commented 3 months ago

@apparentlymart Do you have any thoughts on this?

apparentlymart commented 3 months ago

Thanks for reporting this, @johakoch!

You were right about the cause; I've fixed it in 4b76b751a5cb173d4776b530a3f58c8e851834a2 and will make a new patch release containing that fix soon.