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

reflect: methods are sorted #30688

Closed
bep opened this Issue Mar 8, 2019 · 8 comments

Comments

Projects
None yet
6 participants
@bep
Copy link

bep commented Mar 8, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/bep/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/bep/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/n6/s_85mm8d31j6yctssnmn_g1r0000gn/T/go-build199582095=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/c91WyY2NI2l

package main

import (
	"fmt"
	"reflect"
)

type I interface {
	Method2()
	Method3()
	Method1()
}

type T struct {
	Field2 string
	Field3 string
	Field1 string
}

func main() {
	fmt.Println("Methods:")
	t := reflect.TypeOf((*I)(nil)).Elem()
	for i := 0; i < t.NumMethod(); i++ {
		fmt.Println(i, ":", t.Method(i).Name)
	}

	fmt.Println("Fields:")
	t = reflect.TypeOf(T{})
	for i := 0; i < t.NumField(); i++ {
		fmt.Println(i, ":", t.Field(i).Name)
	}
}

What did you expect to see?

Order preserved for both fields and methods.

What did you see instead?

Sorted order for methods, preserved for fields:

Methods:
0 : Method1
1 : Method2
2 : Method3
Fields:
0 : Field2
1 : Field3
2 : Field1

I was writing a generator to marshal some interfaces to JSON. I suspect Go's JSON package uses reflection behind the scenes, and that the order is preserved because of the "fields only" restriction. This breaks once you try to add methods to the mix.

The documentation in both of the cases above is similar:

Method returns a function value corresponding to v's i'th method.

@dsnet

This comment has been minimized.

Copy link
Member

dsnet commented Mar 8, 2019

In Go, you’re allowed to declare methods on a type from various .go files. In such a situation, what is the ordering?

@bep

This comment has been minimized.

Copy link
Author

bep commented Mar 8, 2019

In such a situation, what is the ordering?

Not sure if this was a question for me, I have not written the spec for the compiler. I'm just saying that the current behaviour is surprising (to me, at least) and un/underdocumented. I assume the various .go files are compiled in some stable order, so it should be possible to preserve that.

Also, you cannot spread an interface declaration into several files, so my concrete case should not be problematic.

@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented Mar 8, 2019

Why does it matter for methods? The method set is just that, a set. Sets are unordered.

Fields occupy memory and the ordering may be important.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 8, 2019

The order of the method set does matter when using reflect.Type.Method, which takes an int index. The reflect package should document that methods are sorted lexicographically, and that only exported methods can be seen.

@bep

This comment has been minimized.

Copy link
Author

bep commented Mar 8, 2019

Why does it matter for methods? The method set is just that, a set. Sets are unordered.

If I carefully order and group the methods in my interfaces, it certainly matters to me. You may not care, and that is fine.

Note that I really didn't expect this issue to somehow change the way methods are ordered (at least not short term), but as it came as a surprise to me (even after reading the documentation), I assume others will also share my surprise.

But there is an argument here, that you may remember for Go 2, and that is that interfaces do not have fields, so Name becomes Name() -- same value, different wrapping.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 8, 2019

The fields in a struct have an obvious order, as the struct can only be written in one way. The methods of a type can appear in any order and the effect for everything other than the reflect package is exactly the same. So we should document what the reflect package does and move on.

(There is a good reason that the reflect package sorts methods by name: it permits faster determination of whether a type implements an interface, in (*itab).init in runtime/iface.go.)

@rogpeppe

This comment has been minimized.

Copy link
Contributor

rogpeppe commented Mar 10, 2019

To expand slightly on Ian's point, these two struct types are different;

 struct {A int; B int}
 struct {B int; A int}

but these two interface types are identical:

 interface{A() int; B() int}
 interface{B() int; A() int}

Given that the reflect package returns identical type.Type instances for identical types, it would not be possible to preserve the originally declared method order without leading to a contradiction.

For example:

https://play.golang.org/p/YfQBaFmE4Ls

If methods were not sorted, then the above program would be required to return two different values for the two calls to the methods function, but by the definition of interfaces, exactly the same type is passed in both cases, so that's not possible.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 27, 2019

Change https://golang.org/cl/169597 mentions this issue: reflect: document that method sets are lexicographically sorted

@gopherbot gopherbot closed this in f33b67b Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.