-
Notifications
You must be signed in to change notification settings - Fork 17.5k
-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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: guarantee that a finite number of distinct instances are reachable via the API #52728
Comments
My apologies, I meant to hit the "subscribe" button, not the "close" button! |
Just to add a bit more context: Laziness serves at least three purposes:
Of these:
|
With regard to point 3 it would be interesting to think about how many clients of the type-checker end up having to expand generic types, anyway. Since #52715 spawned this conversation, I'm also curious how the type-checker, which itself needs to know method sets, can avoid expanding types? |
Yes, this is a good point. I would guess that in some clients (such as gopls) most types get expanded anyway.
The type-checker expands types whenever necessary. It just so happens that the method set is not needed in the example in question: |
CC @timothy-king as well. The more I think about this, the more I think it was just a mistake not to guarantee that reachable types are always finite. Because of instantiation we can't guarantee that instances are canonical, but we can guarantee that they are finite. I think there's actually a way to achieve this lazily, without pinning the It occurred to me that we can do something similar during lazy expansion, even if we don't have a In other words, we introduce a new invariant. Exactly one of the following hold for a named type
As a result of this change, we would guarantee that any instance reachable from
In other words, each instance is expanded in a shared |
This seems like it could work. Worth a try. |
Change https://go.dev/cl/404885 mentions this issue: |
This seemed to work out well in the associated stack of CLs, but of course these sorts of changes require great care. It is possible that I will land this stack on Monday (within the parameters of the freeze grace period, since the stack was mailed before the freeze), but even if I don't I think this is a sufficiently severe problem that it warrants being made a release blocker for 1.19. I am not sure if it should be back-ported to 1.18, since the fix is not small and the problem with NewMethodSet and LookupFieldOrMethod has been avoided. |
Change https://go.dev/cl/404874 mentions this issue: |
The resolved status of a Named type should be owned by its API, and callers should access resolved data via methods. Remove several instances where Named.resolve is explicitly invoked, only to be followed by a method that also resolves. Also make two minor cleanups: - Remove the tparams parameter to Checker.newNamed, as it was unused. - Include position information when assertions fail, so that one doesn't need to go digging in the panicking stack to find the assertion location. Updates #52728 Change-Id: Icbe8c89e9cfe02d60af7d9ba907eaebe1f00193e Reviewed-on: https://go-review.googlesource.com/c/go/+/404874 Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Robert Griesemer <gri@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Change https://go.dev/cl/404875 mentions this issue: |
Change https://go.dev/cl/404884 mentions this issue: |
Change https://go.dev/cl/404883 mentions this issue: |
Checking in on this as it's currently blocking beta 1. Any updates? Thanks. |
This has been implemented. The number of changes is significant, so we're not sure yet whether this should go in or not. |
We're planning to make a decision tomorrow on whether to land this. |
Update: We will try to land this by Monday (pending availability of @findleyr). |
Change https://go.dev/cl/410416 mentions this issue: |
Introduce a monotonic state variable to track the lifecycle of a named type, replacing the existing sync.Once. Having a single guard for the state of underlying and methods will allow for cleaning-up when the type is fully expanded. In the future, this state may also be used for detecting access to information such as underlying or methods before the type is fully set-up, though that will require rethinking our type-checking of invalid cyclic types. Also remove support for type-type inference. If we ever support this feature in the future (inference of missing type arguments for named type instances), it will likely involve additional machinery that does not yet exist. Remove the current partial support to simplify our internal APIs. In particular, this means that Named.resolver is only used for lazy loading. As a result, we can revert the lazy loader signature to its previous form. A lot of exposition is added for how Named types work. Along the way, the terminology we use to describe them is refined. Some microbenchmarks are added that were useful in evaluating the tradeoffs between synchronization mechanisms. Updates #52728 Change-Id: I4e147360bc6e5d8cd4f37e32e86fece0530a6480 Reviewed-on: https://go-review.googlesource.com/c/go/+/404875 Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Robert Griesemer <gri@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
In order to clean up context after fully expanding a type (in subsequent CLs), we must use a common mutex. Eliminate the lazy methodList type, which keeps a sync.Once per method, in favor of Named.mu. Updates #52728 Change-Id: I2d13319276df1330ee53046ef1823b0167a258d8 Reviewed-on: https://go-review.googlesource.com/c/go/+/404883 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Robert Griesemer <gri@google.com>
Separate instance information into an instance struct, to reduce memory footprint for non-instance Named types. This may induce a sense of deja-vu: we had a similar construct in the past that was removed as unnecessary. With additional new fields being added that only apply to instances, having a separate struct makes sense again. Updates #52728 Change-Id: I0bb5982d71c27e6b574bfb4f7b886a6aeb9c5390 Reviewed-on: https://go-review.googlesource.com/c/go/+/404884 Reviewed-by: Robert Griesemer <gri@google.com> Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
In CL 404885, we avoid infinite expansion of type instances by sharing a context between the expanding type and new instances created during expansion. This ensures that we do not create an infinite number of identical but distinct instances in the presence of reference cycles. This pins additional memory to the new instance, but no more (approximately) than would be pinned by the original expanding instance. However, we can do better: since type cycles are only possible within a single package, we only need to share the local context if the two types are in the same package. This reduces the scope of the shared local context, and in particular can avoid pinning the package of the expanding type to the package of the newly created instance. Updates #52728 Change-Id: Iad2c85f4ecf60125f1da0ba22a7fdec7423e0338 Reviewed-on: https://go-review.googlesource.com/c/go/+/410416 Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Robert Griesemer <gri@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
In #52715, we see an example of unexpanded types escaping type-checking, that result in an infinite number of reachable types when interrogated via
Underlying
(though only a finite number of identical types).It would be nice if we could avoid such infinite expansion, as it is bound to cause problems for our users (see e.g. https://go.dev/cl/404335). One way to achieve this would be to provide wrappers for
Underlying
andMethod
that accept aContext
. Another option would be to enforce that unexpanded types do not escape the API, though this can result in a lot of unnecessary work when underlying types or instance methods are not needed.CC @griesemer @adonovan @mdempsky
The text was updated successfully, but these errors were encountered: