golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.83k stars 17.65k forks source link

reflect: MakeFunc functions cannot use the nil error return idiom #6871

Closed gopherbot closed 9 years ago

gopherbot commented 10 years ago

by kin.wilson.za:

(This is not 1.2 specific, confirmed the same result on 1.1.2)

Since the (documented) type of an interface is <nil>, reflection crashes when
attempting to coerce a nil interface{}, or a more commonly used nil error.

See TestMadeFunc below.

I have also confirmed that Convert() also crashes.

See TestConvert below.

Could this be "special-cased" since returning a nil error a preferred idiom?

---

package a

import . "reflect"
import "testing"
import "errors"

func TestNormalFunc(t *testing.T) {
    f := func() (ret error) { return }
    r := f()
    t.Logf("%T %v\n", r, r)
}

// panic: runtime error: invalid memory address or nil pointer dereference [recovered]
//  panic: runtime error: invalid memory address or nil pointer dereference
// [signal 0xc0000005 code=0x0 addr=0x14 pc=0x489f31]
//
// goroutine 3 [running]:
// runtime.panic(0x4d1b40, 0x5f284f)
//  D:/Go1.2/src/pkg/runtime/panic.c:266 +0xa6
// testing.func·005()
//  D:/Go/src/pkg/testing/testing.go:385 +0xd9
// runtime.panic(0x4d1b40, 0x5f284f)
//  D:/Go1.2/src/pkg/runtime/panic.c:248 +0xe8
// reflect.callReflect(0x10d72190, 0x30e55584)
//  D:/Go1.2/src/pkg/reflect/value.go:547 +0x361
//
//  544 if v.typ != typ {
//  545     panic("reflect: function created by MakeFunc using " + funcName(f) +
//  546         " returned wrong type: have " +
//  547         out[i].typ.String() + " for " + typ.String())
//  548 }
//
// runtime: unknown argument frame size for reflect.makeFuncStub called from 0x420764
[runtime.call16]
// reflect.makeFuncStub()
//  D:/Go1.2/src/pkg/reflect/asm_386.s:15 +0x20
// reflect.Value.call(0x4b6420, 0x10d72190, 0x130, 0x4e2be8, 0x4, ...)
//  D:/Go1.2/src/pkg/reflect/value.go:474 +0xad7
// reflect.Value.Call(0x4b6420, 0x10d72190, 0x130, 0x30e55760, 0x0, ...)
//  D:/Go1.2/src/pkg/reflect/value.go:345 +0x73
// github.com/tHinqa/a.TestMadeFunc(0x10d961e0)
//  D:/myGo/src/github.com/tHinqa/a/a_test.go:52 +0xd5
// testing.tRunner(0x10d961e0, 0x5f132c)
//  D:/Go/src/pkg/testing/testing.go:391 +0x87
// created by testing.RunTests
//  D:/Go/src/pkg/testing/testing.go:471 +0x6d2
//
// goroutine 1 [chan receive]:
// testing.RunTests(0x514eb8, 0x5f1320, 0x3, 0x3, 0x1)
//  D:/Go/src/pkg/testing/testing.go:472 +0x6ed
// testing.Main(0x514eb8, 0x5f1320, 0x3, 0x3, 0x5f4da0, ...)
//  D:/Go/src/pkg/testing/testing.go:403 +0x6a
// main.main()
//  github.com/tHinqa/a/_test/_testmain.go:51 +0x82
func TestMadeFunc(t *testing.T) {
    var x func() error
    f := MakeFunc(ValueOf(x).Type(), func([]Value) []Value {
        var T error
        return []Value{ValueOf(T)}
    })
    f.Call([]Value{})
}

// panic: runtime error: invalid memory address or nil pointer dereference [recovered]
//  panic: runtime error: invalid memory address or nil pointer dereference
// [signal 0xc0000005 code=0x0 addr=0x84 pc=0x48db6c]
//
// goroutine 3 [running]:
// runtime.panic(0x4d0020, 0x5ed82f)
//  D:/Go1.2/src/pkg/runtime/panic.c:266 +0xa6
// testing.func·005()
//  D:/Go/src/pkg/testing/testing.go:385 +0xd9
// runtime.panic(0x4d0020, 0x5ed82f)
//  D:/Go1.2/src/pkg/runtime/panic.c:248 +0xe8
// reflect.convertOp(0x4cc640, 0x0, 0x10d602a0)
//  D:/Go1.2/src/pkg/reflect/value.go:2218 +0x26
// reflect.Value.Convert(0x0, 0x0, 0x0, 0x0, 0x0, ...)
//  D:/Go1.2/src/pkg/reflect/value.go:2208 +0x6c
//
//  2204 func (v Value) Convert(t Type) Value {
//  2205    if v.flag&flagMethod != 0 {
//  2206        v = makeMethodValue("Convert", v)
//  2207    }
//  2208    op := convertOp(t.common(), v.typ)
//  2209    if op == nil {
//  2210        panic("reflect.Value.Convert: value of type " + v.typ.String() +
" cannot be converted to type " + t.String())
//  2211    }
//  2212    return op(v, t)
//  2213 }
//  ...
//  2217 func convertOp(dst, src *rtype) func(Value, Type) Value {
//  2218    switch src.Kind() {
//  2219    case Int, Int8, Int16, Int32, Int64:
//
// github.com/tHinqa/a.TestConvert(0x10d861e0)
//  D:/myGo/src/github.com/tHinqa/a/a_test.go:66 +0x86
// testing.tRunner(0x10d861e0, 0x5eabec)
//  D:/Go/src/pkg/testing/testing.go:391 +0x87
// created by testing.RunTests
//  D:/Go/src/pkg/testing/testing.go:471 +0x6d2
//
// goroutine 1 [chan receive]:
// testing.RunTests(0x512750, 0x5eabe0, 0x2, 0x2, 0x1)
//  D:/Go/src/pkg/testing/testing.go:472 +0x6ed
// testing.Main(0x512750, 0x5eabe0, 0x2, 0x2, 0x5efd80, ...)
//  D:/Go/src/pkg/testing/testing.go:403 +0x6a
// main.main()
//  github.com/tHinqa/a/_test/_testmain.go:49 +0x82
func TestConvert(t *testing.T) {
    ValueOf(error(nil)).Convert(TypeOf(error(errors.New("error"))))
}
adg commented 10 years ago

Comment 1:

If my reading of

Labels changed: added priority-soon, go1.3, removed priority-triage, go1.3maybe.

Status changed to Accepted.

bradfitz commented 10 years ago

Comment 2:

http://play.golang.org/p/hX--Pf-sBe
package main
import "reflect"
var errorType = reflect.TypeOf(make([]error, 1)).Elem()
func main() {
    var x func() error
    f := reflect.MakeFunc(reflect.ValueOf(x).Type(), func([]reflect.Value) []reflect.Value {
        return []reflect.Value{reflect.Zero(errorType)}
    })
    f.Call([]reflect.Value{})
}
gopherbot commented 10 years ago

Comment 3 by kin.wilson.za:

Thanks Brad - All I need.
gopherbot commented 10 years ago

Comment 4 by kin.wilson.za:

Technique in #2 should work for any idirection in the reflect domain.
I used TypeOf(new(error)).Elem()
gopherbot commented 10 years ago

Comment 5 by kin.wilson.za:

Further generalized to
if v == ValueOf(interface{}(nil)) {
    return Zero(t)
}
return v.Convert(t)
rsc commented 10 years ago

Comment 6:

Labels changed: added release-go1.3.

rsc commented 10 years ago

Comment 7:

Labels changed: removed go1.3.

rsc commented 10 years ago

Comment 8:

Labels changed: added repo-main.

rsc commented 10 years ago

Comment 9:

reflect requires you to spell things out more than ordinary Go does.
There's not a bug here, just an inconvenience.

Status changed to Unfortunate.