OptimizeReflectImplements(?) is causing behaviour differences

[dgryski]

~/go/src/github.com/dgryski/bug/anynil $ tinygo run main.go
false
~/go/src/github.com/dgryski/bug/anynil $ go run main.go
true
~/go/src/github.com/dgryski/bug/anynil $ cat main.go
package main

import (
	"encoding/json"
	"reflect"
)

type nilJSONMarshaler string

func (nm *nilJSONMarshaler) MarshalJSON() ([]byte, error) {
	// behavior differs from upstream Go
	return json.Marshal("0zenil0:" + string(*nm))

	// behaviour matches upstream Go
	// return nil, nil
}

var _ = json.Marshaler(nil)

type Marshaler interface {
	MarshalJSON() ([]byte, error)
}

var marshalerType = reflect.TypeOf((*Marshaler)(nil)).Elem()

func main() {
	v := struct{ M Marshaler }{(*nilJSONMarshaler)(nil)}
	f0 := reflect.TypeOf(v).Field(0).Type
	println(f0.Implements(marshalerType))
}

[dgryski]

Another way to make the test pass under tinygo: change the name of the interface method from MarshalJSON to XXMarshalJSON. Definitely something weird going on.

[dgryski]

@aykevl I need help with this one :(

[aykevl]

I suspect the bug here is that f0 is of interface type, while OptimizeReflectImplements expects concrete types.

[aykevl]

Yes, thinking a bit more about this I am pretty sure the OptimizeReflectImplements pass is currently fundamentally flawed. It works for concrete types, but not for interface types. Regular interface types only ever have a concrete type as the underlying type, while reflect.Value (or reflect.Type) can also have interface types.

...now I think of it, I guess the reflect Interface() call is also broken as it doesn't check whether the type is of interface type. no this is actually handled correctly: if it's an interface that interface is returned.

[aykevl]

One possible fix is to modify OptimizeReflectImplements as follows. Instead of just calling the type assert, do the following:

if someType.Kind() == reflect.Interface {
    implements := someType.Implements(interfaceType)
} else {
    _, implements := someType.(interfaceType) // someType is the type pointer extracted from the interface
}

This is doable, but kinda gnarly (easiest would probably be to pass a function pointer of the type assert to a function in the reflect package and write the above code as plain Go code).
How high of a priority is this? Asking because I have a long-term goal of rewriting the current code that does type asserts and method calls in a way that this would be much easier to support correctly (without hacks like OptimizeReflectImplements).

[aykevl]

Also see #4376 which would probably fix the bug (since the buggy code gets removed).