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: permit multiple blank type parameters (fix export data) #50481

Closed
findleyr opened this issue Jan 6, 2022 · 14 comments
Closed

cmd/compile: permit multiple blank type parameters (fix export data) #50481

findleyr opened this issue Jan 6, 2022 · 14 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Jan 6, 2022

While investigating #50419, I observed a problem with the way we're indexing type parameters in export data. Consider the following type declaration:

type T[P A, _ B, _ C] int

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 by A. However, this schema has no way to distinguish the type parameter constrained by B and the type parameter constrained by C: 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: func (T[_, _, _]) m() should be fine. EDIT: I spoke too soon: see #50481 (comment)

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

@findleyr findleyr changed the title cmd/compile: multiple blank type parameters cannot be encoded in export data cmd/compile: multiple blank type parameters cannot be indexed in export data Jan 6, 2022
@findleyr findleyr added this to the Go1.18 milestone Jan 6, 2022
@findleyr findleyr added release-blocker NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 6, 2022
@gopherbot
Copy link

gopherbot commented Jan 6, 2022

Change https://golang.org/cl/376058 mentions this issue: go/types, types2: disallow multiple blank type parameters

@griesemer
Copy link
Contributor

griesemer commented Jan 6, 2022

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.

gopherbot pushed a commit that referenced this issue Jan 7, 2022
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>
@griesemer griesemer changed the title cmd/compile: multiple blank type parameters cannot be indexed in export data cmd/compile: permit multiple blank type parameters (fix export data) Jan 7, 2022
@griesemer griesemer added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Jan 7, 2022
@griesemer griesemer modified the milestones: Go1.18, Go1.19 Jan 7, 2022
@griesemer
Copy link
Contributor

griesemer commented Jan 7, 2022

cc: @mdempsky (for consideration when working on 1.19 export data)

@gopherbot
Copy link

gopherbot commented Jan 7, 2022

Change https://golang.org/cl/376216 mentions this issue: test/typeparam: adjust test preamble (fix longtests)

gopherbot pushed a commit that referenced this issue Jan 7, 2022
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>
@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Jan 7, 2022

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.

@mdempsky
Copy link
Member

mdempsky commented Jan 10, 2022

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.

@findleyr
Copy link
Contributor Author

findleyr commented Jan 11, 2022

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?

Before the change to disallow this, it was possible to import a type parameter with incorrect constraint (only one constraint could be recorded for _). This could result in explicit instantiations that either succeed or fail where they shouldn't.

@gopherbot
Copy link

gopherbot commented Jan 14, 2022

Change https://golang.org/cl/378580 mentions this issue: go/internal/gcimporter: ensure the correct constraint for blank rparams

@findleyr
Copy link
Contributor Author

findleyr commented Jan 15, 2022

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:

type T[P any, _ *P] int

func (T[R, _]) m() {}

In this example, the constraint on the second receiver type parameter should be *R.

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.

@findleyr findleyr modified the milestones: Go1.19, Go1.18 Jan 19, 2022
@findleyr
Copy link
Contributor Author

findleyr commented Jan 19, 2022

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 <type/function>.<method?>._ to <type/function>.<method?>.$i, where i is the index of the blank type parameter in its type parameter list. Notably, this doesn't change the layout of the export data, and doesn't change the indexed names of anything other than blank type parameters. For this reason, while this is technically a breaking change we don't think it will cause many problems.

To further smooth the roll-out of this change, we will ensure that our importers continue to support the old, ambiguous index name (T.m._).

Moved this to the 1.18 milestone, and marked as a release-blocker. We are working on it now.

CC @golang/release

@gopherbot
Copy link

gopherbot commented Jan 20, 2022

Change https://golang.org/cl/379855 mentions this issue: go/internal/gcimporter: support unique naming for blank type parameters

gopherbot pushed a commit to golang/tools that referenced this issue Jan 20, 2022
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>
@gopherbot
Copy link

gopherbot commented Jan 21, 2022

Change https://golang.org/cl/379835 mentions this issue: Revert "doc/go1.18: document type parameter name restriction"

@danscales
Copy link
Contributor

danscales commented Jan 21, 2022

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.

gopherbot pushed a commit that referenced this issue Jan 21, 2022
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>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jan 21, 2022
@danscales
Copy link
Contributor

danscales commented Jan 24, 2022

Builds all seem fine (including tools), closing this bug now.

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
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>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
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>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants