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, types2: broken invariants with respect to generic receiver types #50619

Closed
findleyr opened this issue Jan 14, 2022 · 3 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Jan 14, 2022

Our handling of receiver type on generic methods in go/types and types2 is confusing, and arguably incorrect.

While type checking method declarations, the type checker checks the receiver expression T[P1,...PN], resulting in a receiver type that is instantiated with its receiver type parameters, and then assigns this receiver type to the signature being type-checked. This has two surprising consequences:

  1. The receiver type of methods on a generic type is not itself generic: it is instantiated with receiver type parameters.
  2. Because the receiver type is treated as an instance, it is recorded in the Info.Instances map, and its methods are subsequently re-substituted. This means that the defined method F (the thing in Info.Defs!) is not contained in the method set of F.Type().(*types.Signature).Recv().Type()!

Of these, I think (1) is a perhaps peculiar choice that we must stick with, and (2) is a bug -- a consequence of our choice to treat the receiver as an instance and a later decision to instantiate methods on instances. I think this is probably not that difficult to fix: we should avoid naively type checking the receiver parameter list. It is certainly a release blocker to decide what to do here.

@griesemer, do you agree?

CC @timothy-king, who encountered this while working on go/ssa.

@findleyr findleyr added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Jan 14, 2022
@findleyr findleyr added this to the Go1.18 milestone Jan 14, 2022
@griesemer
Copy link
Contributor

Per virtual in-person discussion, I agree that we need to address this. The receiver type handling is a historical accident: before generics, the most expedient way to type-check a method signature was to treat the receiver parameter list just like any other parameter list.

This didn't work anymore for parameterized method receiver types because they look like an instantiated type - yet they are really declaring their own type parameters. Rather than completely changing the original code, I added some pre-amble to extract and declare those type parameters beforehand, so that the receiver type could be type-checked as an instantiated type (and the rest of the code didn't need to change). That code was pretty simple originally, but over time has become more and more complex, with unintended side effects (like what we're observing now). In retrospect, it would have been better to treat receiver parameter lists specially from the start (at least if they have a generic receiver).

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/380498 mentions this issue: go/types: don't re-instantiate receiver methods

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/380499 mentions this issue: go/types: make method instantiation lazy at the individual method level

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
Method signatures can introduce a significant number of edges into the
type graph. One can imagine a generic type with many methods, each of
which may use other instantiated types, etc. For performance, when type
checking generic code, we should avoid unnecessary instantiation of
methods wherever possible.

This CL achieves this by making method instantiation lazy at the
individual method level. It abstracts method access into a methodList
type, which may be either eager or lazy. In the lazy case, methods are
only instantiated when they are accessed via the Named.Method,
MethodSet, or LookupFieldOrMethod APIs. Factoring out a methodList type
makes it easier to verify that we're not leaking the methods slice
anywhere, and as a side benefit reduces the size of *Named types in the
case where there are no methods. The effective memory footprint of Named
types with methods increases by a pointer (to hold the slice of guards),
and the footprint of instantiated named types increases additionally by
a sync.Once per method. We estimate that this memory increase is more
than offset by the reduction in the number of instantiated methods.

This also simplifies the code. Previously we had to work around the fact
that named type expansion could occur before all signatures were set-up,
by stashing the instantiated receiver into a partially filled-out *Func.
With fully lazy methods, we can rely on the invariant that any use of
methods in valid code can only occur after all signatures can be type
checked. This means that we can fully instantiate the *Func, and don't
need to deal with partially instantiated stubs.

Finally, this CL fixes a bug (issue golang#50619), where traversing
Method->Receiver Type->Method did not get us back where we started. This
is fixed by not instantiating a new method if t is already the receiver
base of the original method.

A test is added to explicitly verify the invariant above, and more test
cases are added for the behavior of Info with respect to generic code.

Fixes golang#50619

Change-Id: I5b6d2bdc4404c9f5dcb583a29cb64e8af9794c54
Reviewed-on: https://go-review.googlesource.com/c/go/+/380499
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants