Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cuonglm opened this issue Oct 9, 2020 · 8 comments
Open

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

cuonglm opened this issue Oct 9, 2020 · 8 comments

Comments

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Oct 9, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 9, 2020

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

@bcmills bcmills added this to the Go1.16 milestone Oct 9, 2020
@mdempsky 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
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 9, 2020

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:

  1. 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.

  2. 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
Copy link
Contributor Author

@cuonglm cuonglm commented Oct 9, 2020

@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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 9, 2020

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
Copy link
Contributor Author

@cuonglm cuonglm commented Oct 10, 2020

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
Copy link
Member

@mdempsky mdempsky commented Oct 16, 2020

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
Copy link

@gopherbot gopherbot commented Oct 17, 2020

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

@cuonglm
Copy link
Contributor Author

@cuonglm cuonglm commented Oct 17, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.