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

cmd/compile: cannot convert v (variable of type *Bar[T]) to type *Foo[T] #52529

Closed
b97tsk opened this issue Apr 24, 2022 · 8 comments
Closed

cmd/compile: cannot convert v (variable of type *Bar[T]) to type *Foo[T] #52529

b97tsk opened this issue Apr 24, 2022 · 8 comments
Labels
NeedsFix
Milestone

Comments

@b97tsk
Copy link

@b97tsk b97tsk commented Apr 24, 2022

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

$ go version
go version go1.18.1 windows/amd64

Does this issue reproduce with the latest release?

Yes.

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

Irrelevant.

What did you do?

package main

type Foo[T any] struct {
	_ *Bar[T]
}

type Bar[T any] Foo[T]

func (v *Bar[T]) M() {
	_ = (*Foo[T])(v)
}

func main() {}

What did you expect to see?

The code compiles.

What did you see instead?

main.go:10:16: cannot convert v (variable of type *Bar[T]) to type *Foo[T]

The code does compile if the compiler sees type Bar first.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 25, 2022

It does seem that the behavior changes based on the location of the line type Bar .... That can't be right.

CC @griesemer

@ianlancetaylor ianlancetaylor added the NeedsInvestigation label Apr 25, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Apr 25, 2022
@griesemer griesemer self-assigned this Apr 25, 2022
@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 25, 2022

Changing the M method to a function also makes it work:

// func (v *Bar[T]) M() {
// 	_ = (*Foo[T])(v)
// }

func _[T any](v *Bar[T]) {
	_ = (*Foo[T])(v)
}

Definitely a bug. Possibly some bad interaction with method type parameter instantiation/renaming.
cc @findleyr

@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 25, 2022

We tracked it down. The issue is that with this set-up, the call to under() in the sanity here is happening eagerly, when it should be lazy:
https://cs.opensource.google/go/go/+/master:src/go/types/decl.go;l=715;drc=f2cdc6d1672fb335ac56f9c7b824071f1e5ba545

This can be easily fixed by deferring this sanity check, but it does of course beg the question of how to make the type-checker less sensitive to accidental eager evaluation. For long-term maintainability, we must introduce invariants that allow us to detect or even prevent such bugs.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 25, 2022

@gopherbot, please open a backport issue for 1.18, this is a bug causing valid code not to compile.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 25, 2022

Backport issue(s) opened: #52558 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@findleyr findleyr self-assigned this Apr 25, 2022
@findleyr findleyr added the NeedsFix label Apr 25, 2022
@gopherbot gopherbot removed the NeedsInvestigation label Apr 25, 2022
@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 25, 2022

This turned out not to be entirely trivial, so I will fix it later this week.

The root cause of the problem is that when we collect a methods, we check that method names are unique among existing methods and fields before adding each method. As a result, we have an invariant that recorded method names are always unique among methods and fields. But enforcing this invariant results in us expanding underlying types too soon, before they are fully set-up.

@gopherbot
Copy link

@gopherbot gopherbot commented May 2, 2022

Change https://go.dev/cl/403455 mentions this issue: go/types: delay check for duplicate struct field names

@gopherbot
Copy link

@gopherbot gopherbot commented May 3, 2022

Change https://go.dev/cl/403754 mentions this issue: [release-branch.go1.18] go/types,types2: delay the check for conflicting struct field names

gopherbot pushed a commit that referenced this issue May 9, 2022
…ing struct field names

In #52529, we observed that checking types for duplicate fields and
methods during method collection can result in incorrect early expansion
of the base type. Fix this by delaying the check for duplicate fields.
Notably, we can't delay the check for duplicate methods as we must
preserve the invariant that added method names are unique.

After this change, it may be possible in the presence of errors to have
a type-checked type containing a method name that conflicts with a field
name. With the previous logic conflicting methods would have been
skipped. This is a change in behavior, but only for invalid code.
Preserving the existing behavior would likely require delaying method
collection, which could have more significant consequences.

As a result of this change, the compiler test fixedbugs/issue28268.go
started passing with types2, being previously marked as broken. The fix
was not actually related to the duplicate method error, but rather the
fact that we stopped reporting redundant errors on the calls to x.b()
and x.E(), because they are now (valid!) methods.

Updates #52529
Fixes #52558

Change-Id: I850ce85c6ba76d79544f46bfd3deb8538d8c7d00
Reviewed-on: https://go-review.googlesource.com/c/go/+/403455
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit b75e492)
Reviewed-on: https://go-review.googlesource.com/c/go/+/403754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

5 participants