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

go/types: guarantee sorting of named type methods #61298

Closed
findleyr opened this issue Jul 11, 2023 · 9 comments
Closed

go/types: guarantee sorting of named type methods #61298

findleyr opened this issue Jul 11, 2023 · 9 comments
Assignees
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@findleyr
Copy link
Contributor

Certain libraries such as x/tools/go/types/objectpath rely on a well-defined sorting of methods on Named types. To enforce this definition, the objectpath package sorts, but as seen in #58668 (comment) that can be quite expensive.

We can fix that particular issue outside of go/types, but it would be nice to commit to a fixed ordering of methods in the go/types API. IIRC the ordering is currently deterministic and in source order (as defined by the input file slice), but this is not documented. We should document and preserve this behavior, or pick a different stable sort.

CC @griesemer @adonovan

@findleyr findleyr added this to the Go1.22 milestone Jul 11, 2023
@adonovan
Copy link
Member

Strictly speaking, all that objectpath needs is that the type checker uses a deterministic ordering of Named.Methods, and that the export/import process preserves that. So either of source order and Id order would be fine, but we should at a minimum commit to determinism (in go/types) and order preservation (in gcexportdata).

@dominikh
Copy link
Member

Related: #44195

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 11, 2023
@griesemer
Copy link
Contributor

Maybe we need both, a "fast" version that doesn't guarantee an ordering, and a "stable" version that guarantees an ordering.

Right now we have sorting, but I don't think we rely on it in the type checker (and maybe we could use a map internally).

@griesemer griesemer modified the milestones: Go1.22, Go1.23 Dec 13, 2023
@griesemer griesemer self-assigned this Dec 13, 2023
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Dec 13, 2023
@gopherbot
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.23.
That time is now, so a friendly reminder to look at it again.

@griesemer
Copy link
Contributor

@findleyr Is this still something we'd like to have, and if so, what exactly should we guarantee?

@adonovan
Copy link
Member

adonovan commented Jan 31, 2024

Yes, I think this is valuable. The obvious order is the lexical order, and that appears to be what's currently implemented:

func Test(t *testing.T) {
	const src = `package p

type T int

func (T) B(){}
func (T) A(){}
`
	dir := t.TempDir()
	cfg := &packages.Config{
		Dir:  dir,
		Mode: packages.LoadSyntax,
		Overlay: map[string][]byte{
			filepath.Join(dir, "p.go"): []byte(src),
		},
	}
	initial, err := packages.Load(cfg, "./p.go")
	if err != nil {
		t.Fatal(err)
	}
	if packages.PrintErrors(initial) > 0 {
		t.Fatal("packages contain errors")
	}
	T := initial[0].Types.Scope().Lookup("T").Type().(*types.Named)
	t.Error(T.Method(0)) // func (command-line-arguments.T).B()
	t.Error(T.Method(1)) // func (command-line-arguments.T).A()
}

@findleyr
Copy link
Contributor Author

findleyr commented Feb 7, 2024

The only objection to lexical ordering, I think, would be that it depends on the ordering of the input file slice. But IIRC last time we discussed this it was pointed out that other things in the go/types API are affected by the ordering of the input slice (such as initialization order), and so it is reasonable that the ordering of methods also depends on input order.

Therefore, I think it would be fine to simply document the current behavior and close this issue. I think we don't even need to commit to lexical ordering: we can just say that the order is deterministic based on the inputs. That is all the objectpath library needs, so I don't see a reason to guarantee more than that.

Perhaps we can simply guarantee determinism in go/types on the whole?

@griesemer
Copy link
Contributor

@findleyr I would think that go/types is deterministic given deterministic (order of) inputs. The only place where non-determinism can come in is via maps, which are a) exported and so we know what we have, or b) if they are internal, we sort before we do any kind of access that might depend on an index order.

That said, I'll keep the documentation local to this issue for now.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562347 mentions this issue: go/types, types2: document deterministic method index order and add test

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Fixes golang#61298.

Change-Id: Ie2f930752867710884ace3990447866e785ebf1c
Reviewed-on: https://go-review.googlesource.com/c/go/+/562347
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants