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: var declaration changes representation of its type (alias related) #66704

Closed
adonovan opened this issue Apr 5, 2024 · 8 comments
Closed
Assignees

Comments

@adonovan
Copy link
Member

adonovan commented Apr 5, 2024

This is a minimized repro of another gotypesalias=1 problem affecting the x/tools gcimporter.
The root cause is clearly in go/types (at b24ec88).

package types // "go/types"

func TestAliasProblem(t *testing.T) {
	t.Setenv("GODEBUG", "gotypesalias=1")
	const src = `package a

var x T[B] // <-- this causes the problem! deleting it causes type of B to be ok

type T[_ any] struct{}
type A T[B]
type B = T[A]
`
	fset := token.NewFileSet()
	f, err := parser.ParseFile(fset, "a.go", src, 0)
	if err != nil {
		t.Fatal(err)
	}
	pkg, err := new(Config).Check("a", fset, []*ast.File{f}, nil)
	if err != nil {
		t.Fatal(err)
	}

	B := pkg.Scope().Lookup("B")
	t.Error(Unalias(B.Type())) // "invalid type" if var x is present, "a.T[a.A]" otherwise
}
@adonovan adonovan self-assigned this Apr 5, 2024
@findleyr
Copy link
Contributor

findleyr commented Apr 5, 2024

FWIW, putting this in go/types/testdata/manual.go and running go test -v -run=Manual go/types , I see that when GODEBUG=gotypesalias=1, there are only 3 delayed calls to validType. With gotypesalias=0, there are 6.

@adonovan
Copy link
Member Author

adonovan commented Apr 5, 2024

Unalias is sticky. If called too soon on a type that's part of a cycle of declarations, it latches on to the invalid type which is destined to be updated, but too late. I guess that the var declaration calls Unalias on T[B].

@adonovan
Copy link
Member Author

adonovan commented Apr 5, 2024

Not memoizing Unalias if the actual type is Invalid makes the test pass. Not sure whether that's a fix or a hack.

@findleyr
Copy link
Contributor

findleyr commented Apr 5, 2024

Hmm, this is a tale as old as time, and gives me flashbacks to Named.Underlying and Named.Method, with generics. It took a lot of careful work to ensure that we don't touch the underlying or methods of instances until the type is fully set up. (but this type of problem didn't start with generics; I believe we had similar problems with method set completeness).

I suspect this may be very nontrivial to fix. Absent a formal partial ordering of type checking operations, the work tends to be whack-a-mole, delaying operations.

Not memoizing Unalias if the actual type is Invalid makes the test pass. Not sure whether that's a fix or a hack.

Sigh, we can change Alias.cleanup to say "really Unalias though".

@findleyr
Copy link
Contributor

findleyr commented Apr 5, 2024

Not memoizing Unalias if the actual type is Invalid makes the test pass. Not sure whether that's a fix or a hack.

That may actually be a reasonable solution, for now. Perhaps pre-existing code used typ == Typ[Invalid] to detect whether the type was still being set up. When that code was transposed to call Unalias, it may have inadvertently picked up this bug.

CC @griesemer

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/576975 mentions this issue: go/types: fix bug in premature Unalias of alias cycle

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/579015 mentions this issue: cmd/compile/internal/types2: port CL 576975 to types2

gopherbot pushed a commit that referenced this issue Apr 15, 2024
This CL ports to types2 the (passing) test from CL 576975,
which fixed a bug in go/types.

Updates #66704
Updates #65294

Change-Id: Icdf77e39ed177d9f9ecc435d5125f02f2ee4dd0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/579015
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/579075 mentions this issue: go/types, types2: simplify TestUnaliasTooSoonInCycle (cleanup)

gopherbot pushed a commit that referenced this issue Apr 15, 2024
Follow-up on CL 576975 and CL 579015.

Updates #66704
Updates #65294

Change-Id: Ied95386a346be38ccda86d332d09b2089a68c5e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/579075
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants