-
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
slices: Use ~[]E throughout #60546
Comments
If I'm understanding your argument correctly, this sounds like one of those "make it a little bit complicated for everybody" versus "make it simple for most, but quite complicated for some rare cases" tradeoffs. And if that's the case, then put me down as an enthusiast of "simple for most" (that is, 👎 ). Go seems to be generally biased in this direction, doesn't it? I certainly prefer, for example, just For such a big up-front cost, I think we'd want to see a correspondingly big benefit, and I'm not seeing that in this proposal (but if you think there is one, then laying it out in more detail may be the most persuasive way to make the case). |
@bitfield Everything you say is basically correct. Note that anything you say applies to I think there is a case to be made that the standard library really should provide the most general version of these functions we can. For example, we also have I also don't feel the cost is that high TBH. I'd expect the types in pretty much every usage to be fully inferable. So this is really just a documentation issue and we can get used to that. But ultimately, I don't have very strong feelings about this. |
How does this interact with the new type inference in Go 1.21? |
CC @griesemer |
@carlmjohnson I don't see an immediate issue with the improved type inference. As @Merovius already pointed out, this is not so much about type inference but about the existing Go type system: if a function is passed/assigned to another function, the argument types must be identical. By changing the signatures as suggested, after instantiation we get (using the above example) |
By my count, there are 20 functions in the Leaving aside all consideration of passing Right now, if you pass the type parameter explicitly you need to know which form the function you're calling is using. This seems confusing, even if it is a confusion few people will encounter. Again, consistency seems simpler.
So my vote is to consistently use the two-type-parameter form throughout. |
@carlmjohnson I mention the new rules FWIW, in the last paragraph of "Cost". And note that the main example in the top post does not compile with Go 1.20, but only with tip, exactly because of that change. [edit] I mean, technically it compiles with neither, because of the "does not compile" line. But you get what I mean [/edit] |
I'm playing around with the package I wrote for my dayjob that I had in mind when filing this issue and thought it might be useful to make some observations. I published that package as well, now. Our main use for this is protobuf, which generates getters for all fields. This package can relatively conveniently (though inefficiently) compose those into chained comparators for complex message types. Two relevant functions from that package are cmp.Slice (equivalent to func SliceFunc[T any](c cmp.Cmp[T]) cmp.Cmp[[]T] {
return cmp.SliceFunc[[]T](c)
} When trying this in the playground, with Go 1.21 we can observe:
I believe this mostly confirms the arguments made above and makes them more concrete. It just stood out to me this morning when putting last touches on the API. |
Can someone with tagging ability label this release-blocker? It should be decided one way or the other before 1.21. |
Change https://go.dev/cl/502955 mentions this issue: |
@prattmic should this be kept a blocker until https://go.dev/cl/502955 lands? We want to include that CL in the release for sure |
Reopening to complete proposal process. |
Based on the discussion above, this proposal seems like a likely accept. |
CL is committed, removing release-blocker label. |
No change in consensus, so accepted. 🎉 |
Make all functions use a constraint S ~[]E even if they don't return the slice type. This makes explicitly instantiating the functions more consistent: you don't have to remember which take ~[]E and which do not. It also permits inferring the type when passing one of these functions to some other function that is using a named slice type. Fixes golang#60546 Change-Id: Ib3435255d0177fdbf03455ae527d08599b1ce012 Reviewed-on: https://go-review.googlesource.com/c/go/+/502955 Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@google.com> Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Axel Wagner <axel.wagner.hh@googlemail.com> Reviewed-by: Eli Bendersky <eliben@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@google.com>
Make all functions use a constraint S ~[]E even if they don't return the slice type. This makes explicitly instantiating the functions more consistent: you don't have to remember which take ~[]E and which do not. It also permits inferring the type when passing one of these functions to some other function that is using a named slice type. Fixes golang#60546 Change-Id: Ib3435255d0177fdbf03455ae527d08599b1ce012 Reviewed-on: https://go-review.googlesource.com/c/go/+/502955 Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@google.com> Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Axel Wagner <axel.wagner.hh@googlemail.com> Reviewed-by: Eli Bendersky <eliben@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@google.com>
Proposal
I propose to change the slices function signatures to always add an extra type parameter
S ~[]E
, even when the slice type does not appear as a return type. When a function has multiple slice arguments, it should have multiple type parameters, for full generality.Notably, this should happen before Go 1.21 is released, if at all, as I think it would be a breaking change otherwise.
Concretely, I propose to change these function signatures:
Rationale
On Twitter, @joncalhoun asked why the proposal for slices.Reverse uses the
~[]E
trick. Though notably the commited version does not. That conversation prompted this proposal, that maybe it should.The current approach is for the
slices
package to use the extraS ~[]E
whenever the slice appears in a return type. This allows a statement likes = slices.Compact(s)
to work with defined slice types. Functions where it does not appear in a return type use[]E
directly, as a value of a defined slice type is assignable to[]E
. Soslices.Reverse(s)
works fine, even ifs
is a defined slice type.However, there is still a case where it makes a difference: When instantiating
slices.Reverse
you can only get afunc([]int)
, not afunc(MySliceType)
. This matters when passing them to higher level functions:I'm not entirely certain the difference is important to cover, but I thought it's probably useful to talk about this while we could still change it (even though it's past the freeze).
Cost
The main cost of this proposal is visual clutter and complexity of the function signatures. When looking at them the first time, it might not be obvious if and why this is needed (as demonstrated by @joncalhoun asking that question, even though he is a Go veteran).
A secondary cost is that explicit instantiation of these functions becomes a tiny bit more cumbersome. Currently, you write
slices.Reverse[T]
, in the future you might have to writeslices.Reverse[[]T]
. The element type itself can always be infered, I think. But I might be missing a corner case, in which case the instantiation will beslices.Reverse[[]T, T]
. Either way, functions likeEqual
would definitely become more cumbersome, withslices.Equal[T]
vs.slices.Equal[[]T, []T]
.Notably #59338 specifically mitigated this second cost a lot, by at least applying better type-inference when passing them directly to higher-level functions. Further improvements to type-inference can reduce this cost further, by reducing the number of cases where explicit instantiation is necessary.
Alternatives considered
func([]int)
is not afunc(IntSlice)
, no matter what we infer. That is, given that you can't instantiateslices.Reverse
to get afunc(IntSlice)
, inference obviously can't help you.func
, afunc([]int)
could be made assignable tofunc(IntSlice)
(asIntSlice
is assignable to[]int
(and vice-versa)). Type-inference could then solve whatever friction is left. Personally, I'd honestly prefer this, but I'm not optimistic it would ever happen. And even if, that seems like a far more significant change.The text was updated successfully, but these errors were encountered: