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: inconsistency between LookupFieldOrMethod and NewMethodSet #60634

Closed
adonovan opened this issue Jun 6, 2023 · 5 comments
Closed

go/types: inconsistency between LookupFieldOrMethod and NewMethodSet #60634

adonovan opened this issue Jun 6, 2023 · 5 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Jun 6, 2023

package p

type T *int

func (r *T) f() {} // error: "invalid receiver type T[A] (pointer or interface type)"

It is an error to declare a method on *T when T's underlying type is a pointer. Nonetheless, after reporting the error, the type checker must represent this situation. However, its representation is inconsistent: LookupFieldOrMethod fails to find the method, but NewMethodSet does report it. I think it should be an invariant that these two operations are consistent. (This bug was the cause of a crash in gopls.)

https://go.dev/play/p/uWoUFewDCj4:

	// Look up func T.f (with addressable T): it doesn't exist.
	T := pkg.Scope().Lookup("T").Type()
	obj, _, _ := types.LookupFieldOrMethod(T, true, pkg, "f")
	if obj != nil {
		log.Fatalf("LookupFieldOrMethod(T.f) = %v", obj)
	}

	// But ask for the method set of *T: it contains f!
	mset := types.NewMethodSet(types.NewPointer(T))
	if mset.Len() > 0 {
		log.Fatalf("NewMethodSet(*T) is non empty: %v", mset)
	}

Output:

 NewMethodSet(*T) is non empty: MethodSet {
	method (*p.T) f()
}

@findleyr @mdempsky

@griesemer
Copy link
Contributor

Side note: NewMethodSet is not used by the compiler (it doesn't exist in types2), so it's not surprising that there are inconsistencies.

@findleyr
Copy link
Contributor

findleyr commented Jun 6, 2023

Thanks for digging into this crash!

Yep, sounds like a bug in NewMethodSet. Since it has existed for a while, it's probably OK to defer the fix until 1.22 (but fixing it in 1.21 means that we can delete our workaround 6 months sooner...).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/501197 mentions this issue: internal/typeparams: work around LookupFieldOrMethod inconsistency

@griesemer
Copy link
Contributor

griesemer commented Jun 6, 2023

Note that LookupFieldOrMethod does find the same method if the same receiver (*T) is used:
https://go.dev/play/p/2Yhzg1lLICr
(but not if the receiver is T).

gopherbot pushed a commit to golang/tools that referenced this issue Jun 6, 2023
This change adds to x/tools a workaround for a bug in go/types
that causes LookupFieldOrMethod and NewTypeSet to be inconsistent
wrt an ill-typed method (*T).f where T itself is a pointer.

The workaround is that, if Lookup fails, we walk the MethodSet.

Updates golang/go#60634
Fixes golang/go#60628

Change-Id: I87caa2ae077e5cdfa40b65a2f52e261384c91167
Reviewed-on: https://go-review.googlesource.com/c/tools/+/501197
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/501299 mentions this issue: go/types: fix method set computation if receiver is a named pointer

CarlosRosuero pushed a commit to CarlosRosuero/typeparams that referenced this issue Jun 13, 2023
This change adds to x/tools a workaround for a bug in go/types
that causes LookupFieldOrMethod and NewTypeSet to be inconsistent
wrt an ill-typed method (*T).f where T itself is a pointer.

The workaround is that, if Lookup fails, we walk the MethodSet.

Updates golang/go#60634
Fixes golang/go#60628

Change-Id: I87caa2ae077e5cdfa40b65a2f52e261384c91167
Reviewed-on: https://go-review.googlesource.com/c/tools/+/501197
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants