all: revise reflect.Type/reflect.Value NumMethod usage after CL 259237 #41882

cuonglm · 2020-10-09T10:45:09Z

With CL 259237 merged, the bug in #22075 is fixed. But I found some of the usage in standard library, which relies on old (wrong) behavior of NumMethod() to detect empty interface.

Example, this program:

package main

import (
	"encoding/json"
	"fmt"
)

func main() {
	var i interface {
		f()
	}
	var jsonBlob = []byte(`[
	{"Name": "Platypus", "Order": "Monotremata"},
	{"Name": "Quoll",    "Order": "Dasyuromorphia"}
]`)
	err := json.Unmarshal(jsonBlob, &i)
	if err != nil {
		fmt.Println("error:", err)
	}
	fmt.Printf("%+v", i)
}

go1.14 and go1.15 will print:

error: json: cannot unmarshal array into Go value of type interface { main.f() }

While on tip:

panic: reflect.Set: value of type []interface {} is not assignable to type interface { main.f() }

goroutine 1 [running]:
reflect.Value.assignTo(0x4ca280, 0xc00000c030, 0x97, 0x4e90d7, 0xb, 0x4cfe00, 0xc000010240, 0x4ce620, 0xc000060001, 0xc00000c030)
	/home/cuonglm/sources/go/src/reflect/value.go:2443 +0x3bf
reflect.Value.Set(0x4cfe00, 0xc000010240, 0x194, 0x4ca280, 0xc00000c030, 0x97)
	/home/cuonglm/sources/go/src/reflect/value.go:1555 +0xbd
encoding/json.(*decodeState).array(0xc00010a000, 0x4c7880, 0xc000010240, 0x16, 0xc00010a028, 0x5b)
	/home/cuonglm/sources/go/src/encoding/json/decode.go:518 +0xa2d
encoding/json.(*decodeState).value(0xc00010a000, 0x4c7880, 0xc000010240, 0x16, 0xc000074e68, 0x4ba40b)
	/home/cuonglm/sources/go/src/encoding/json/decode.go:360 +0x105
encoding/json.(*decodeState).unmarshal(0xc00010a000, 0x4c7880, 0xc000010240, 0xc00010a028, 0x0)
	/home/cuonglm/sources/go/src/encoding/json/decode.go:180 +0x1ea
encoding/json.Unmarshal(0xc00005e070, 0x63, 0x63, 0x4c7880, 0xc000010240, 0xc000074f48, 0x44248a)
	/home/cuonglm/sources/go/src/encoding/json/decode.go:107 +0x112
main.main()
	/home/cuonglm/main.go:16 +0xc5
exit status 2

We should fix wrong checking NumMethod() == 0 to detect empty interface.

cc @mdempsky @cherrymui

The text was updated successfully, but these errors were encountered:

gopherbot · 2020-10-09T10:57:10Z

Change https://golang.org/cl/260857 mentions this issue: all: fix wrong usage of reflect.Type/Value Nummethod to detect empty interface

mdempsky · 2020-10-09T18:17:43Z

So as feared, it looks like some users are relying on the undocument behavior. In particular, they're using NumMethods() == 0 to test for empty interface types/values.

Google's internal Code Search for NumMethod().*\b0\b (and excluding Go toolchain sources) gives about 50 hits. Not all of these are actually testing for empty interface (e.g., some are trying to optimize away unnecessary interface-to-interface assertions), but a lot are.

There's a reasonable and backwards-compatible alternative using package reflect's public API like:

var eface = reflect.TypeOf((*interface{})(nil)).Elem()

func isEmptyInterface(typ reflect.Type) bool {
    return typ.Kind() == reflect.Interface && eface.Implements(typ)
}

I see two questions:

Are we okay with sticking with CL 259237 for Go 1.16? It fixes a bug and a significant inconsistency with reflect's public API, but it does break some user code. The Go 1 compat guarantee allows us to fix bugs though.
Do we want to provide isEmptyInterface as a helper function in package reflect or somewhere since it appears to be a somewhat common use case? Providing it in an x/ repo would have the advantage of making its use backwards compatible to older Go versions.

/cc @rsc @ianlancetaylor

cuonglm · 2020-10-09T18:31:17Z

@mdempsky For 1), I think it should be ok, since when Go 1 compat guidelines also states that user's code which rely on buggy behavior may break.

ianlancetaylor · 2020-10-09T23:54:46Z

That many cases makes this sound like a pretty problematic change. While it's true that the compatibility guarantee permits us to fix bugs, we still have to be careful about breaking code. The fact that some of the code we are breaking is in the standard library is a particular bad sign.

Also, we only changed reflect.Type.NumMethod for a non-interface type to return only the number of exported methods in 1.7. Before 1.7 we returned the number of all methods. This was changed for #15673 which is following up on some earlier changes. So there has been some confusion over just how this works over time.

If we're going to go ahead with this change I think we do need to add some sort of reflect.IsEmptyInterface so that there is a very simple fix for code that breaks. And that should go through proposal review.

Want to open a proposal issue? Thanks and sorry.

cuonglm · 2020-10-10T01:06:09Z

That many cases makes this sound like a pretty problematic change. While it's true that the compatibility guarantee permits us to fix bugs, we still have to be careful about breaking code. The fact that some of the code we are breaking is in the standard library is a particular bad sign.

Also, we only changed reflect.Type.NumMethod for a non-interface type to return only the number of exported methods in 1.7. Before 1.7 we returned the number of all methods. This was changed for #15673 which is following up on some earlier changes. So there has been some confusion over just how this works over time.

If we're going to go ahead with this change I think we do need to add some sort of reflect.IsEmptyInterface so that there is a very simple fix for code that breaks. And that should go through proposal review.

Want to open a proposal issue? Thanks and sorry.

Thanks, I filled #41896

mdempsky · 2020-10-16T20:03:33Z

While we wait for the proposal committee to review #41896 for the public facing API, maybe we should at least add an internal/reflectutil.IsEmptyInterface(reflect.Type) bool helper function so we can fix the standard library?

gopherbot · 2020-10-17T02:28:09Z

Change https://golang.org/cl/263078 mentions this issue: internal/reflectutil: add helper function to detect empty interface

cuonglm · 2020-10-17T02:30:59Z

While we wait for the proposal committee to review #41896 for the public facing API, maybe we should at least add an internal/reflectutil.IsEmptyInterface(reflect.Type) bool helper function so we can fix the standard library?

I added one in CL 263078

ianlancetaylor · 2020-10-21T21:05:02Z

For the record: if we roll back https://golang.org/cl/259237 as suggested in #42123, then we don't have to do anything here.

cuonglm · 2020-10-29T02:04:24Z

Closed as the change was rolled back.

bcmills added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Oct 9, 2020

bcmills added this to the Go1.16 milestone Oct 9, 2020

mdempsky changed the title ~~Revise reflect.Type/reflect.Value NumMethod usage after CL 259237~~ all: revise reflect.Type/reflect.Value NumMethod usage after CL 259237 Oct 9, 2020

cuonglm mentioned this issue Oct 10, 2020

proposal: reflect: add Type.IsEmptyInterface #41896

Closed

cuonglm mentioned this issue Oct 21, 2020

reflect: roll back NumMethod change for interfaces #42123

Closed

cuonglm closed this as completed Oct 29, 2020

golang locked and limited conversation to collaborators Oct 29, 2021

gopherbot added the FrozenDueToAge label Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: revise reflect.Type/reflect.Value NumMethod usage after CL 259237 #41882

all: revise reflect.Type/reflect.Value NumMethod usage after CL 259237 #41882

cuonglm commented Oct 9, 2020

gopherbot commented Oct 9, 2020

mdempsky commented Oct 9, 2020 •

edited

cuonglm commented Oct 9, 2020

ianlancetaylor commented Oct 9, 2020

cuonglm commented Oct 10, 2020

mdempsky commented Oct 16, 2020

gopherbot commented Oct 17, 2020

cuonglm commented Oct 17, 2020

ianlancetaylor commented Oct 21, 2020

cuonglm commented Oct 29, 2020

all: revise reflect.Type/reflect.Value NumMethod usage after CL 259237 #41882

all: revise reflect.Type/reflect.Value NumMethod usage after CL 259237 #41882

Comments

cuonglm commented Oct 9, 2020

gopherbot commented Oct 9, 2020

mdempsky commented Oct 9, 2020 • edited

cuonglm commented Oct 9, 2020

ianlancetaylor commented Oct 9, 2020

cuonglm commented Oct 10, 2020

mdempsky commented Oct 16, 2020

gopherbot commented Oct 17, 2020

cuonglm commented Oct 17, 2020

ianlancetaylor commented Oct 21, 2020

cuonglm commented Oct 29, 2020

mdempsky commented Oct 9, 2020 •

edited