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

spec: allow eliding interface{ } in constraint literals #48424

Closed
fzipp opened this issue Sep 16, 2021 · 37 comments
Closed

spec: allow eliding interface{ } in constraint literals #48424

fzipp opened this issue Sep 16, 2021 · 37 comments

Comments

@fzipp
Copy link
Contributor

fzipp commented Sep 16, 2021

I propose an additional rule for constraints in type parameter lists:

[T nonInterfaceType][T interface{~nonInterfaceType}]

Rationale:

All functions of the proposed maps package (#47649) and most functions of the slices package (#45955, #47203) currently use constraints from the constraints package (#45458) for maximum generality. A hypothetical chans package would probably be similar (there's already a proposal for constraints.{ReadOnlyChan|WriteOnlyChan}: #48366). Some excerpts:

func EqualFunc[M1 constraints.Map[K, V1], M2 constraints.Map[K, V2], K comparable, V1, V2 any](m1 M1, m2 M2, cmp func(V1, V2) bool) bool

func Clone[S constraints.Slice[T], T any](s S) S

With this proposal these would become:

func EqualFunc[M1 map[K]V1, M2 map[K]V2, K comparable, V1, V2 any](m1 M1, m2 M2, cmp func(V1, V2) bool) bool

func Clone[S []T, T any](s S) S

The benefit:

  • constraints.{Slice|Map|Chan|ReadOnlyChan|WriteOnlyChan} would not be necessary at all.
  • No visual incongruency between plain old Go types like map[K]V, []T, chan T, <-chan T, chan<- T and their accompanying constraints.

This proposal stems from the discussion under #47330 (reply in thread)

Summary:

  [M map[K]V, K comparable, V any]
≡ [M interface{~map[K]V}, K comparable, V any]
≡ [M constraints.Map[K, V], K comparable, V any]

  [S []T, T any]
≡ [S interface{~[]T}, T any]
≡ [S constraints.Slice[T], T any]

  [C <-chan T, T any]
≡ [C interface{~<-chan T}, T any]
≡ [C constraints.ReadOnlyChan[T], T any]

  [C chan<- T, T any]
≡ [C interface{~chan<- T}, T any]
≡ [C constraints.WriteOnlyChan[T], T any]

  [C chan T, T any]
≡ [C interface{~chan T}, T any]
≡ [C constraints.Chan[T], T any]
@gopherbot gopherbot added this to the Proposal milestone Sep 16, 2021
@ianlancetaylor
Copy link
Member

CC @griesemer

@griesemer
Copy link
Contributor

This is an interesting idea and we had (internally to the Go team) discussed essentially the same thing: writing a type expression X in constraint position as syntactic sugar for interface{X}. I think there's something to it, but we haven't quite figured out (not have spent the time on) to determine how such an idea would fit smoothly in to the rest of the type system.

In any case, the proposal would have to be generalized a bit. For instance, we would want to say int|string instead of interface{int|string}. Also, it's not clear exactly when the ~ can be omitted and why.

@ianlancetaylor
Copy link
Member

If we do this, I don't think we should automatically insert a ~. That seems like unnecessary magic. That would give us

func EqualFunc[M1 ~map[K]V1, M2 ~map[K]V2, K comparable, V1, V2 any](m1 M1, m2 M2, cmp func(V1, V2) bool) bool

func Clone[S ~[]T, T any](s S) S

@ianlancetaylor ianlancetaylor added the generics Issue is related to generics label Sep 16, 2021
@fzipp
Copy link
Contributor Author

fzipp commented Sep 17, 2021

In my imagination it's not "type set expression hoisted to type parameter list level", hence no | or ~. I intended to keep more complex and nuanced constraint definitions inside interfaces. I see it as "expand non-interface type into its sole useful constraint form". It is admittedly some amount of magic.

@fzipp
Copy link
Contributor Author

fzipp commented Sep 17, 2021

I know it's probably too late for syntax changes re. constraints, but I personally would have preferred this:

My preference                             Vs. current design

[T interface { M() }]                     [T interface { M() }]
[T nonInterfaceType]                      [T interface { ~nonInterfaceType }]
[T union {A|B|C}]                         [T interface { ~A|~B|~C }].               
[T interface { nonInterfaceType; M() }]   [T interface { ~nonInterfaceType; M() }] 
[T interface { union {A|B|C}; M() }]      [T interface { ~A|~B|~C; M() }] 

With union being a non-interface type as well. The rule would have been simple: non-interface types match approximately when used as constraints, and they match exactly when used as regular types:

var x nonInterfaceType
var x union { A|B|C }

No tilde, and types like map[K]V could act directly as constraints.Map[K, V] etc.

@ianlancetaylor
Copy link
Member

@fzipp If we want to make this change, let's keep this issue tightly focused on this specific change, not on a discussion of possible alternative syntaxes. Thanks.

@griesemer
Copy link
Contributor

We might be able to say that a constraint is simply a type set. A type set may be expressed through an interface (as we do now), but it may also be a type expression of the form ~T or A|B. Additionally, we could say that an ordinary type T represents the type set interface{T} if we expect a type set. Syntactically, we'd have to allow type expressions of the form ~T and A|B outside of an interface (in constraint position only).

Or, looking from a slightly different angle, we could say that writing T, ~T , or A|B in constraint position is a form of syntactic sugar for interface{T}, interface{~T} , or interface{A|B}.

@rsc rsc changed the title proposal: non-interface types as constraints to avoid constraints.{Slice|Map|Chan|ReadOnlyChan|WriteOnlyChan} proposal: spec: allow eliding interface{ } in constraint literals Sep 22, 2021
@rsc
Copy link
Contributor

rsc commented Sep 22, 2021

Agree with @ianlancetaylor about the ~ not being tied to the shortening.
At that point [C X] and [C interface{X}] are just the same.

It's already the case that [C interface{X}] and [C interface{interface{X}}] are the same,
so that makes the shortening safe 100% of the time.

And it would let us remove constraints.Map, constraints.Slice, etc.
Those just get spelled '~' now, as in [M ~map[K]V].

@griesemer will prototype and prepare a CL showing the effect on code.

@fzipp
Copy link
Contributor Author

fzipp commented Sep 22, 2021

I'm ok with it if this is the preferred variant.

@rsc
Copy link
Contributor

rsc commented Sep 22, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@benhoyt
Copy link
Contributor

benhoyt commented Sep 23, 2021

This is definitely a readability win: I couldn't understand why the constraints.Map[K, V] thing was necessary at first at all, not to mention it'd be annoying to have to import and it takes up a lot of space. As a (newbie) code reader, to understand why it wasn't just map[K]V, you'd have to go to the constraints package docs, find the Map type, look at its definition, and notice the ~. With this proposed syntax, you'd just have to understand what ~ means. (I do prefer it without the ~, but I know what it means and can see the desire to avoid magically implying it.)

@akavel
Copy link
Contributor

akavel commented Sep 25, 2021

@rsc I'm not tracking all the (apparently more numerous than I thought) proposals related to generics, so I'd like to ask, if that's ok: will the discussion around this proposal temporarily suspend introduction of the constraints package (until this one is decided)? I'm not sure what's the status of the latter, but it would feel weird if constraints.Map were e.g. to be introduced with Go 1.18, just to become insta-legacy if this one (#48424) doesn't make it to Go1.18 (or wherever generics are to be introduced) yet possibly lands in (say) Go1.19. (In fact, I'm kinda guessing that's probably one of the main reasons you got this one on such a fast track, still I'm curious if you'd be ok with some explicit statement on the relation between the two. Thanks!)

@ianlancetaylor
Copy link
Member

@akavel I'm confident that this proposal will be either accepted or declined before Go 1.18. If accepted, we should then consider removing constraints.Map, constraints.Slice, and constraints.Chan.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353133 mentions this issue: cmd/compile/internal/types2: allow eliding interface in constraint literals

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353139 mentions this issue: cmd/compile/internal/types2: accept constraint literals with elided interfaces

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353389 mentions this issue: cmd/compile: accept constraint literals with elided interfaces

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353395 mentions this issue: x/tools/go/internal/gcimporter: exclude a file from TestImportTypeparamTests

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353396 mentions this issue: cmd/compile/internal/types2: mark implicit interfaces as such

gopherbot pushed a commit to golang/tools that referenced this issue Oct 1, 2021
…amTests

Matching change for the corresponding file in CL 353389.
To make the builds pass for now.

For golang/go#48424.

Change-Id: Id66837957ffec4e4b19faa0337a7587ea8bd6125
Reviewed-on: https://go-review.googlesource.com/c/tools/+/353395
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
gopherbot pushed a commit that referenced this issue Oct 1, 2021
…terals

This CL permits an arbitrary type as well as the type sets  ~T and A|B
in constraint position, without the need of a surrrounding interface.
For instance, the type parameter list

	[P interface{ ~map[K]V }, K comparable, V interface{ ~string }]

may be written as

	[P ~map[K]V, K comparable, V ~string]

The feature must be enabled explicitly with the AllowTypeSets mode
and is only available if AllowGenerics is set as well.

For #48424.

Change-Id: Ic70bb97a49ff75e67e040853eac10e6aed0fef1a
Reviewed-on: https://go-review.googlesource.com/c/go/+/353133
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit that referenced this issue Oct 1, 2021
…nterfaces

When collecting type parameters, wrap constraint literals of the
form ~T or A|B into interfaces so the type checker doesn't have
to deal with these type set expressions syntactically anywhere
else but in interfaces (i.e., union types continue to appear
only as embedded elements in interfaces).

Since a type constraint doesn't need to be an interface anymore,
we can remove the respective restriction. Instead, when accessing
the constraint interface via TypeParam.iface, wrap non-interface
constraints at that point and update the constraint so it happens
only once. By computing the types sets of all type parameters at
before the end of type-checking, we ensure that type constraints
are in their final form when accessed through the API.

For #48424.

Change-Id: I3a47a644ad4ab20f91d93ee39fcf3214bb5a81f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/353139
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit that referenced this issue Oct 1, 2021
This change enables the relaxed syntax for constraint literals
as proposed in issue #48424 and adds a simple smoke test for
the compiler. (Most of the relevant changes are in the syntax
and types2 package which have more extensive tests for this.)

This makes it possible to experiment with the new syntax while
we contemplate the fate of #48424.

If #48424 is accepted, this change can remain. If #48424 is
not accepted, reverting this CL will remove this feature in
the compiler.

For #48424.

Change-Id: I624fbb37c2f616ee9ad692e17e4fc75c9d5b06e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/353389
Trust: Robert Griesemer <gri@golang.org>
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Dan Scales <danscales@google.com>
gopherbot pushed a commit that referenced this issue Oct 1, 2021
Provide an accessor for clients, and don't print the interface
around implicitly wrapped embedded types.

For #48424.

Change-Id: Ib2c76315508fc749ea4337d52e13d17de80e04da
Reviewed-on: https://go-review.googlesource.com/c/go/+/353396
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@griesemer
Copy link
Contributor

This is now implemented in the compiler in the master branch at tip so we can explore this in practice. Note that this proposal has not been accepted yet, so don't write code that you expect to remain valid a month from now...

Only the compiler understands this notation at the moment, tools such as gofmt, vet, etc. cannot handle this yet.

If the proposal is not accepted, we can quickly disable the feature by changing a flag in the compiler.

@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: spec: allow eliding interface{ } in constraint literals spec: allow eliding interface{ } in constraint literals Oct 13, 2021
@rsc rsc modified the milestones: Proposal, Backlog Oct 13, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/355709 mentions this issue: cmd/compile/internal/syntax: remove AllowTypeSets mode

gopherbot pushed a commit that referenced this issue Oct 14, 2021
The respective issue has been accepted, so we can always
accept constraint literals with omitted interfaces.

For #48424.

Change-Id: Ia3d325401252a5a22d5ffa98d2ae6af73178dec0
Reviewed-on: https://go-review.googlesource.com/c/go/+/355709
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@griesemer griesemer modified the milestones: Backlog, Go1.18 Oct 16, 2021
@griesemer griesemer self-assigned this Oct 16, 2021
@griesemer
Copy link
Contributor

This has been implemented in the compiler and go/types and go/print. @findleyr is there more to do on the std library side or can we close this (and track additional work separately)?

@findleyr
Copy link
Member

@griesemer I've filed #49040 to track adding the implicit bit to export data, so I think this issue can be closed.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357796 mentions this issue: go/types, types2: add the Interface.MarkImplicit method

gopherbot pushed a commit that referenced this issue Oct 21, 2021
Add a new interface method, MarkImplicit, to allow marking interfaces as
implicit from outside the type-checker. This is necessary so that we can
capture the implicit bit in export data, and use it from importers.

For #48424
For #49040

Change-Id: I999aba2a298f92432326d7ccbd87fe133a2e1a72
Reviewed-on: https://go-review.googlesource.com/c/go/+/357796
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/359258 mentions this issue: constraints: remove Slice/Map/Chan

gopherbot pushed a commit that referenced this issue Oct 27, 2021
Now that we permit arbitrary types as constraints, we no longer need them.

For #48424

Change-Id: I15fef26a563988074650cb0801895b002c44148a
Reviewed-on: https://go-review.googlesource.com/c/go/+/359258
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@tooolbox
Copy link

I first proposed this in the slice functions discussion but didn't get anywhere. Admittedly I was a little off-topic. Imagine my surprise just now when I find the official Generics Tutorial is using union syntax directly in type arguments, and then the Proposal is using ~[]E as well.

I had been resigned to using constraints.Slice and such. I just want to express my thanks to @fzipp for making an actual proposal and the Go Team members for accepting this change, I think it's a big improvement.

In terms of the philosophy mentioned by @griesemer :

We might be able to say that a constraint is simply a type set. A type set may be expressed through an interface (as we do now), but it may also be a type expression of the form ~T or A|B. Additionally, we could say that an ordinary type T represents the type set interface{T} if we expect a type set. Syntactically, we'd have to allow type expressions of the form ~T and A|B outside of an interface (in constraint position only).

Or, looking from a slightly different angle, we could say that writing T, ~T , or A|B in constraint position is a form of syntactic sugar for interface{T}, interface{~T} , or interface{A|B}.

Both work, my opinion is that the former is superior. A type is T, full stop. A type set is T or some expansion of the set via union or inclusion of descendants, or narrowing by requiring behavior in the form of methods.

The real litmus test is how well devs work with that mental model. I for one love it.

@jub0bs
Copy link

jub0bs commented Mar 28, 2022

Shouldn't there be a go vet check for type constraints consisting of a single non-generic concrete type? Here is a contrived example:

func foo[T int](t T) {
   fmt.Println(t)
}

Unless I'm missing something, the type parameter doesn't provide any benefit and the function could as well be declared as

func foo(i int) {
   fmt.Println(i)
}

@findleyr
Copy link
Member

@jub0bs please feel free to open a proposal for such a vet check.

Vet has three conditions for new checks:
https://go.dev/src/cmd/vet/README

I suspect that what you suggests meets the Correctness and Precision requirements, but may not meet the Frequency requirement. This could be discussed in a proposal.

@rsc rsc moved this to Accepted in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc removed this from Proposals Oct 19, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests