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.

Sign up for GitHub

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

Jump to bottom

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
@griesemer
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@adonovan
Copy link
Member

adonovan commented Jun 6, 2023 •
edited
Loading 

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
@adonovan adonovan mentioned this issue Jun 6, 2023
Closed
@adonovan adonovan added this to the Go1.21 milestone Jun 6, 2023
@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
Member

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 •
edited
Loading

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
@adonovan @findleyr
internal/typeparams: work around LookupFieldOrMethod inconsistency
a01290f 
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
@adonovan @findleyr
internal/typeparams: work around LookupFieldOrMethod inconsistency
3e7c812 
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
@golang golang locked and limited conversation to collaborators Dec 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@griesemer griesemer

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
5 participants
@dmitshur @adonovan @griesemer @gopherbot @findleyr