-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: permit multiple blank type parameters (fix export data) #50481
Comments
Change https://golang.org/cl/376058 mentions this issue: |
If we accept CL 376058 then this issue can be retitled accordingly and moved to 1.19 where we should remove the restriction introduced by this CL. |
Work-around for #50481: report an error for multiple blank type parameters. It's always possible to use non-blank names in those cases. We expect to lift this restriction for 1.19. For #50481. Change-Id: Ifdd2d91340aac1da3387f7d80d46e44f5997c2a8 Reviewed-on: https://go-review.googlesource.com/c/go/+/376058 Trust: Robert Griesemer <gri@golang.org> Run-TryBot: Robert Griesemer <gri@golang.org> Trust: Dan Scales <danscales@google.com> Reviewed-by: Dan Scales <danscales@google.com> Reviewed-by: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
cc: @mdempsky (for consideration when working on 1.19 export data) |
Change https://golang.org/cl/376216 mentions this issue: |
For #50481. Change-Id: I27e6c6499d6abfea6e215d8aedbdd5074ff88291 Reviewed-on: https://go-review.googlesource.com/c/go/+/376216 Trust: Robert Griesemer <gri@golang.org> Run-TryBot: Robert Griesemer <gri@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com>
If we're imposing this restriction for 1.18 be sure to note it in the release notes in the list under "The current generics implementation has the following limitations:" Thanks. |
FWIW, unified IR indexes type parameters by ordinal index, not by name. I'm interested if we have a test case that fails under the current scheme though. Because blank type parameters can't be referenced. Is the issue that the type parameters end up with the wrong constraints due to non-unique indexing? The other quick fix is to rename blank type parameters like we do blank and anonymous (value) parameters to ~rN and ~bN. |
Before the change to disallow this, it was possible to import a type parameter with incorrect constraint (only one constraint could be recorded for |
Change https://golang.org/cl/378580 mentions this issue: |
Unfortunately, it's not actually trivial to support multiple blanks on method receivers. The reason being that we need to substitute receiver type parameters in their constraints, as in the example below, and there is no such API exposed by the type checker:
In this example, the constraint on the second receiver type parameter should be Therefore, even though we may have an associated constraint for receiver type parameters, there is no way to make this constraint consistent with the rest of the declaration. This may not matter much in practice, but it's incorrect. It appears we need to either disallow multiple blanks in receiver type expressions, which would be annoying, or revisit whether or not to change the export data format. |
We've thought about this more, particularly in consideration of receiver type parameters (#50481 (comment)). We decided that it would be better to fix this now (as unobtrusively as possible), than to have users continually bump up against this limitation in 1.18. This hypothetical fix proposed in #50481 (comment) is probably the least invasive change we can make: we will change the indexed name of blank type parameters from To further smooth the roll-out of this change, we will ensure that our importers continue to support the old, ambiguous index name ( Moved this to the 1.18 milestone, and marked as a release-blocker. We are working on it now. CC @golang/release |
Change https://golang.org/cl/379855 mentions this issue: |
As described in golang/go#50481, in the existing export data schema blank type parameters do not have unique names, and therefore types with multiple blank (receiver) type parameters cannot be correctly imported. This CL implements the fix proposed in that issue, using the schema <prefix>.$<index> as the exported name of a blank type parameter, where <prefix> is the qualifying prefix and <index> is the index of the type parameter in its type parameter list. The importer is backwards compatible with the old schema: it will continue to import <prefix>._ as long as there are not multiple blank type parameters. I considered not making the exporter change simultaneously with the importer change, so that we interleave the corresponding changes in the standard library. However, that made it harder to test the importer, and updating both seems unlikely to cause problems. For golang/go#50481 Change-Id: Id24428c6ea2b256312156894f9f76fa8e9ee38d4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/379855 Trust: Robert Findley <rfindley@google.com> Run-TryBot: Robert Findley <rfindley@google.com> Trust: Dan Scales <danscales@google.com> Reviewed-by: Dan Scales <danscales@google.com> Reviewed-by: Robert Griesemer <gri@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Change https://golang.org/cl/379835 mentions this issue: |
Oops, CL https://go-review.googlesource.com/c/go/+/379194 fixes this bug and should have mentioned it in the commit. We can maybe wait a few days to close this one, to make sure the compiler and tools changes all settle smoothly. |
This reverts CL 376414. For #47694. For #50481. Change-Id: Ie73961046e52e6e5d3262ef0aeaa24bec7eaa937 Reviewed-on: https://go-review.googlesource.com/c/go/+/379835 Trust: Robert Griesemer <gri@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Builds all seem fine (including tools), closing this bug now. |
Work-around for golang#50481: report an error for multiple blank type parameters. It's always possible to use non-blank names in those cases. We expect to lift this restriction for 1.19. For golang#50481. Change-Id: Ifdd2d91340aac1da3387f7d80d46e44f5997c2a8 Reviewed-on: https://go-review.googlesource.com/c/go/+/376058 Trust: Robert Griesemer <gri@golang.org> Run-TryBot: Robert Griesemer <gri@golang.org> Trust: Dan Scales <danscales@google.com> Reviewed-by: Dan Scales <danscales@google.com> Reviewed-by: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
For golang#50481. Change-Id: I27e6c6499d6abfea6e215d8aedbdd5074ff88291 Reviewed-on: https://go-review.googlesource.com/c/go/+/376216 Trust: Robert Griesemer <gri@golang.org> Run-TryBot: Robert Griesemer <gri@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com>
This reverts CL 376414. For golang#47694. For golang#50481. Change-Id: Ie73961046e52e6e5d3262ef0aeaa24bec7eaa937 Reviewed-on: https://go-review.googlesource.com/c/go/+/379835 Trust: Robert Griesemer <gri@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
While investigating #50419, I observed a problem with the way we're indexing type parameters in export data. Consider the following type declaration:
Because we index type parameters along with other declarations in the package export data, we assign a unique name to them by prefixing with the name of the parameterized declaration, in this case, for example,
"T.P"
is the type parameter constrained byA
. However, this schema has no way to distinguish the type parameter constrained byB
and the type parameter constrained byC
: both would be indexed by"T._"
.Currently, our exporters are not failing, but rather choosing one of the blank type parameters to index.
Given the care required for changes to the export data, it is probably too late to fix this. Maybe we could change the indexed name for blank type parameters to something like
"<prefix>.$<index>"
, so that the blank type parameters in the example above would be indexed as"T.$1"
and"T.$2"
, but even that seems risky at this point.This is unfortunate, but also should not be a significant problem for our users, since blank type parameters can always be given names. Discussing with @griesemer, we think that for Go 1.18 we should note this limitation and make it a type checking error to have multiple blank type parameters in a type or function declaration, or perhaps an error to have ANY blank names for type parameters.
Note that this limitation should NOT apply to receiver type parameters:EDIT: I spoke too soon: see #50481 (comment)func (T[_, _, _]) m()
should be fine.At some point, perhaps 1.19, we plan to increment the export data version again for the unified export data format. At that point we can lift this restriction.
CC @danscales @griesemer @mdempsky @ianlancetaylor
The text was updated successfully, but these errors were encountered: