Yaml: Better error when unmarshaling into non-pointer value

[lgo]

Recently I ran into an issue which had me pulling my hair for a good bit, and so I believe the main problem that an unexpected panic is thrown with an obscure error as opposed to preventing the error or returning a verbose error. This was a big usability problem as I only found the source of the problem after trial and error for the YAML and struct fields and then going through go-yaml's source. The source of this error happening could either be developer error or if user YAML files are being parsed which are invalid. I personally ran into this because I believed a library struct was YAML parsable when it in fact wasn't, while the other structs in the library were.

The problem is when there is a field in the YAML that doesn't correspond to a field in the struct, all reflect.Set methods will cause a panic immediately. In order to prevent this, you can do reflect.CanSet to check the field beforehand. This could be used for two manners of error recovery:

1) Silently ignore the issue and don't set the field at all, as it doesn't correspond to proper struct field

2) Don't unmarshal the YAML and simply return an error

Example code and stacktrace

package main

import (
    "fmt"

    "gopkg.in/yaml.v2"
)

const problemYaml = `
name: GO_BUILDER
`

type config struct {
    Name string
}

func main() {
    var c = config{}
    _ = yaml.Unmarshal([]byte(problemYaml), c)
    fmt.Println("I will not reach here, and can't handle the error for invalid YAML input.")
}

panic: reflect: reflect.Value.SetString using unaddressable value [recovered]
    panic: reflect: reflect.Value.SetString using unaddressable value

goroutine 1 [running]:
panic(0x4df780, 0xc42000e610)
    /usr/lib/go/src/runtime/panic.go:500 +0x1a1
gopkg.in/yaml%2ev2.handleErr(0xc42003ff00)
    /home/joey/.go/src/gopkg.in/yaml.v2/yaml.go:153 +0xe7
panic(0x4df780, 0xc42000e610)
    /usr/lib/go/src/runtime/panic.go:458 +0x243
reflect.flag.mustBeAssignable(0x98)
    /usr/lib/go/src/reflect/value.go:228 +0x102
reflect.Value.SetString(0x4df780, 0xc42000e550, 0x98, 0xc42000e590, 0xa)
    /usr/lib/go/src/reflect/value.go:1511 +0x2b
gopkg.in/yaml%2ev2.(*decoder).scalar(0xc4200121c0, 0xc42005c240, 0x4df780, 0xc42000e550, 0x98, 0x4df780)
    /home/joey/.go/src/gopkg.in/yaml.v2/decode.go:371 +0x1187
gopkg.in/yaml%2ev2.(*decoder).unmarshal(0xc4200121c0, 0xc42005c240, 0x4df780, 0xc42000e550, 0x98, 0xc42000e550)
    /home/joey/.go/src/gopkg.in/yaml.v2/decode.go:290 +0x122
gopkg.in/yaml%2ev2.(*decoder).mappingStruct(0xc4200121c0, 0xc42005c180, 0x4ea360, 0xc42000e550, 0x99, 0xc42002c028)
    /home/joey/.go/src/gopkg.in/yaml.v2/decode.go:635 +0x641
gopkg.in/yaml%2ev2.(*decoder).mapping(0xc4200121c0, 0xc42005c180, 0x4ea360, 0xc42000e550, 0x99, 0x4ea360)
    /home/joey/.go/src/gopkg.in/yaml.v2/decode.go:513 +0xaad
gopkg.in/yaml%2ev2.(*decoder).unmarshal(0xc4200121c0, 0xc42005c180, 0x4ea360, 0xc42000e550, 0x99, 0xc42005c180)
    /home/joey/.go/src/gopkg.in/yaml.v2/decode.go:292 +0x216
gopkg.in/yaml%2ev2.(*decoder).document(0xc4200121c0, 0xc42005c120, 0x4ea360, 0xc42000e550, 0x99, 0x44a210)
    /home/joey/.go/src/gopkg.in/yaml.v2/decode.go:304 +0x84
gopkg.in/yaml%2ev2.(*decoder).unmarshal(0xc4200121c0, 0xc42005c120, 0x4ea360, 0xc42000e550, 0x99, 0x10)
    /home/joey/.go/src/gopkg.in/yaml.v2/decode.go:280 +0x268
gopkg.in/yaml%2ev2.Unmarshal(0xc42000a3c0, 0x12, 0x20, 0x4ea360, 0xc42000e550, 0x0, 0x0)
    /home/joey/.go/src/gopkg.in/yaml.v2/yaml.go:90 +0x2ba
main.main()
    /home/joey/code/gum/error.go:15 +0xcc
exit status 2

[mbndr]

Same problem here. It would be nice that if something fails, I will get it through the err return value.

[vschoener]

Would be nice to know which field is throwing this error, just got the same and I don't know why, need to use debug I think

[jan0022]

I was getting the same error until I passed struct by address "&c"
_ = yaml.Unmarshal([]byte(problemYaml), &c)

[niemeyer]

Yeah, the situation is indeed a panic because it's a coding mistake. The yaml provided is completely valid, and the struct is fine as well. But there's just no possible way the logic can work when c is not provided as a pointer.

I'll keep this issue open to improve the error message, though.

[milkdazombie]

Any progress on improving the error message? @niemeyer