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 conversion from slice to array #46505

Open
bradfitz opened this issue Jun 1, 2021 · 71 comments
Open

spec: allow conversion from slice to array #46505

bradfitz opened this issue Jun 1, 2021 · 71 comments
Assignees
Labels
Milestone

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Jun 1, 2021

I recently tried to use the new #395 feature (for converting a slice to an array pointer: https://golang.org/cl/216424) in:

https://go-review.googlesource.com/c/go/+/322329

But in review, it was pointed out that it was a little ugly, as what I wanted to return was an array, which required a dereference:

	return *(*[Size224]byte)(sum[:Size224])

It would've been nicer if I could've just converted to an array instead:

	return ([Size224]byte)(sum[:Size224])

Talking to @ianlancetaylor and @griesemer, we couldn't remember great reasons for not also allowing this as part of #395. It does mean there's an subtle copy, but arguably the * dereference above is also a somewhat subtle copy.

Could we also add support for converting to the array?

/cc @katiehockman @josharian @rogpeppe @mdempsky

@gopherbot gopherbot added this to the Proposal milestone Jun 1, 2021
@randall77
Copy link
Contributor

randall77 commented Jun 1, 2021

Note the [:Size224] is unnecessary. There, I saved you more characters than the 2 *s you were worried about!

I think the decision to omit direct-to-array casts was just to keep the feature change minimal. We need the cast-to-pointer regardless to support aliasing to the same backing store, and cast-to-array is easily implemented using cast-to-pointer.

@go101
Copy link

go101 commented Jun 2, 2021

Some like #36890

@mdempsky
Copy link
Member

mdempsky commented Jun 2, 2021

I don't immediately see anything wrong with allowing slice to array conversions, but I feel like saving two asterisks in an already rather niche feature doesn't justify the extra complexity on compilers and tooling.

@bcmills
Copy link
Member

bcmills commented Jun 2, 2021

Since this conversion would do a copy anyway, would we also add a conversion from string to [N]byte for consistency?

@rsc rsc changed the title proposal: allow conversion from slice to array proposal: spec: allow conversion from slice to array Jun 2, 2021
@rsc rsc moved this from Incoming to Active in Proposals (old) Jun 2, 2021
@rsc
Copy link
Contributor

rsc commented Jun 2, 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

@rogpeppe
Copy link
Contributor

rogpeppe commented Jun 2, 2021

FWIW I didn't propose this originally because it could easily be implemented, even prior to #395, by using copy, albeit not quite so concisely.

I'm not entirely convinced that it's worth it tbh, especially given the possibility for the expression to panic.

@katiehockman
Copy link
Contributor

katiehockman commented Jun 2, 2021

Question: is it possible to make changes to copy() so that it is equally performant to the *(*array)(slice) conversion? Then we don't need to support this additional conversion at all, and it can just be common practice to use copy in such situations.

FWIW I'm a lot less concerned about a few extra characters in the line than I am about readability. I was pretty confused as a reader to see a conversion in the form *(*array)(slice). I read it as "Cast a slice to a pointer to an array, then dereference this array", which felt more like a hack than a best practice. But maybe I just need to get used to it.

@mdempsky
Copy link
Member

mdempsky commented Jun 2, 2021

Question: is it possible to make changes to copy() so that it is equally performant to the *(*array)(slice) conversion? Then we don't need to support this additional conversion at all, and it can just be common practice to use copy in such situations.

Probably. I don't think there's any fundamental reason they'd perform differently. Can you file an issue with examples of code that you think should perform equivalently, and we can look into it for Go 1.18?

I was pretty confused as a reader to see a conversion in the form *(*array)(slice). I read it as "Cast a slice to a pointer to an array, then dereference this array"

Assuming you meant "dereference this [pointer to] array", that description is pretty much how I think of it, though arguably it's more of a (checked) "assertion" (as in type assertions), as conversions generally can't fail. A slice is a pointer + dynamic length (and capacity). The conversion here is changing it to a pointer + static length, with a runtime check to ensure the lengths are compatible.

@katiehockman
Copy link
Contributor

katiehockman commented Jun 2, 2021

Can you file an issue with examples of code that you think should perform equivalently, and we can look into it for Go 1.18?

Brad wrote a little benchmark that demonstrated some of the performance differences which probably shouldn't be different: https://play.golang.org/p/BkYM9Gbi4t2. He got the following results:

BenchmarkNamed-8        201051339                6.263 ns/op
BenchmarkCopy-8         172203873                6.071 ns/op
BenchmarkPointer-8      1000000000               1.031 ns/op

You can also check out the Sum functions in https://go-review.googlesource.com/c/go/+/322329 which are good examples of code that should perform ~equivalently.

@mdempsky
Copy link
Member

mdempsky commented Jun 2, 2021

@katiehockman Thanks, filed #46529.

As noted in that issue, the first two functions have different semantics than the last function when len(b) < Size256. I suspect that doesn't matter for your use case, but the compiler would need some way to discern that can't happen before it can optimize them identically.

@kortschak
Copy link
Contributor

kortschak commented Jun 2, 2021

ISTM that the need for a dereference is a nice visual signal to an author that *(*[n]T)(s[:n]) is more costly than (*[n]T)(s[:n]). As it is it's reasonably common the users are surprised that an array expression results in a copy of the array (for example in a range over an array value).

@bcmills
Copy link
Member

bcmills commented Jun 3, 2021

ISTM that the need for a dereference is a nice visual signal to an author

I am personally unlikely to notice that signal among the line-noise of the rest of the expression.
Also note that ([]byte)(s) for a string s, or string(b) for a slice b, has no such visual signal — the destination type is in some sense “good enough”.

As it is it's reasonably common the users are surprised that an array expression results in a copy

Sure, but the ship has already sailed on that one. 😅 Given that we already have array values, it doesn't seem particularly more confusing to allow conversions to them.

@bcmills
Copy link
Member

bcmills commented Jun 3, 2021

I am personally unlikely to noticed that signal among the line-noise of the rest of the expression.

Looking at this again, I'm not sure why the slice part of the expression is needed at all.
@bradfitz, could you not have written that as

	return *(*[Size224]byte)(sum)

?

And then the direct-conversion alternative becomes

	return ([Size224]byte)(sum)

which reads a lot cleaner to me. (Without the redundant slice expression, those redundant * tokens are a larger fraction of the noise.)

@kortschak
Copy link
Contributor

kortschak commented Jun 3, 2021

Given that we already have array values, it doesn't seem particularly more confusing to allow conversions to them.

It's not a matter of confusion in this case, but signalling that there is more work being done; the pointer conversion is essentially free. WRT the signalling for string/[]byte, the difference there is that they are clearly distinct types, in this situation it is potentially a single character that makes the difference. between behaviours under the proposal.

@rsc
Copy link
Contributor

rsc commented Jun 9, 2021

Strictly speaking this is unnecessary. You can always write

*(*[10]int)(x)

instead of the proposed

[10]int(x)

That said, copying data out from a slice into a fixed-size array is pretty common when using fixed-size arrays (like checksums etc), so I don't really see the harm in making that less star-full.

/cc @robpike

@rsc
Copy link
Contributor

rsc commented Jul 14, 2021

Any objections to adding this?

@randall77
Copy link
Contributor

randall77 commented Jul 14, 2021

I'm leaning against. I don't think adding a new conversion case is worth saving two *s.

@mdempsky
Copy link
Member

mdempsky commented Jul 14, 2021

I'm also leaning against.

We've had a string of fixes to x/tools to update code for slice-to-array-pointer conversions. None of the CLs have been very involved, but just cases where we forgot to update it. I don't think this feature is going to be so frequently used to justify forcing tools authors to support both forms.

@rsc
Copy link
Contributor

rsc commented Jul 21, 2021

Does anyone want to make an argument in favor?

@Splizard
Copy link

Splizard commented Aug 4, 2021

Does anyone want to make an argument in favor?

I believe that this proposal would result in improved discoverability around this kind of type conversion, when I first started writing Go (~8 years ago) I recall at one point wondering why I couldn't convert a slice to an array just like this. This seemed like the obvious way to do it.

[10]int(x)

At the time (even if #395 existed), it would never have occurred to me that I could do something like this:

*(*[10]int)(x)

I had previously been using languages without pointers and I wasn't familiar with the semantics around dereferencing or how slices relate to them.

It's the kind of small thing as a beginner you don't even bother looking into when the compiler throws an error and you simply write it another way until you find out months or years later when you see it somewhere and/or learn more about pointer semantics and realise that this is possible.

IE. somebody who starts learning about pointer semantics in Go can infer that *(*[10]int)(x) is possible if they are already familiar with [10]int(x) but if this proposal is declined then beginners in Go are locked out from benefiting from #395 until they learn more about the language (and this is hindered further if they come from languages without pointers).

Instead of framing this proposal around "saving two asterisks", I would argue that it helps discoverability around #395 because it is an obvious way to convert from a slice to an array.

@griesemer griesemer added this to the Go1.20 milestone May 17, 2022
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label May 17, 2022
lberrymage added a commit to accrescent/apkstat that referenced this issue Jun 28, 2022
We can finally stop using those ugly slice to array conversions.
golang/go#46505
@gopherbot
Copy link

gopherbot commented Aug 12, 2022

This issue is currently labeled as early-in-cycle for Go 1.20.
That time is now, so a friendly reminder to look at it again.

@gopherbot
Copy link

gopherbot commented Sep 7, 2022

Change https://go.dev/cl/428938 mentions this issue: types2: implement slice-to-array conversions

@gopherbot
Copy link

gopherbot commented Sep 7, 2022

Change https://go.dev/cl/429295 mentions this issue: x/tools/go/ssa: disable slice-to-array test

gopherbot pushed a commit to golang/tools that referenced this issue Sep 7, 2022
CL 428938 implements slice to array conversions in the type checkers,
per accepted proposal issue golang/go#46505. The disabled test was
expected to fail, but it won't fail anymore for Go 1.20 and higher.
Disable it for now (and track it in golang/go#54822) so we can make
progress with CL 428938.

For golang/go#46505.
For golang/go#54822.

Change-Id: I87fc78dccaa2bf1517cf875fec97855e360d0f25
Reviewed-on: https://go-review.googlesource.com/c/tools/+/429295
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tim King <taking@google.com>
@gopherbot
Copy link

gopherbot commented Sep 7, 2022

Change https://go.dev/cl/429315 mentions this issue: spec: describe slice-to-array conversions

gopherbot pushed a commit that referenced this issue Sep 8, 2022
For #46505.

Change-Id: I9bc9da5dd4b76cb2d8ff41390e1567678e72d88d
Reviewed-on: https://go-review.googlesource.com/c/go/+/428938
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Sep 8, 2022
For #46505.

Change-Id: I1a30fd895496befd16626afb48717ac837ed5778
Reviewed-on: https://go-review.googlesource.com/c/go/+/429315
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
@griesemer griesemer assigned mdempsky and timothy-king and unassigned mdempsky and griesemer Sep 9, 2022
@gopherbot
Copy link

gopherbot commented Sep 12, 2022

Change https://go.dev/cl/430415 mentions this issue: cmd/compile: implement slice-to-array conversions

@mdempsky
Copy link
Member

mdempsky commented Sep 12, 2022

This still needs support in package reflect.

@cuonglm
Copy link
Member

cuonglm commented Sep 12, 2022

This still needs support in package reflect.

I can take a look tomorrow if no one beat me.

@mdempsky
Copy link
Member

mdempsky commented Sep 12, 2022

I can take a look tomorrow if no one beat me.

SGTM, thanks @cuonglm.

Tricky cases to handle:

  • Converting []T(nil) to [0]T needs to succeed.
  • Converting a slice to non-empty array needs to return a non-addressable copy of the original memory. In particular, subsequent mutations to the original slice should not be visible in the Value.Convert result.

@gopherbot
Copy link

gopherbot commented Sep 13, 2022

Change https://go.dev/cl/430475 mentions this issue: reflect: allow conversion from slice to array

@gopherbot
Copy link

gopherbot commented Sep 14, 2022

Change https://go.dev/cl/430835 mentions this issue: spec: describe an edge case of slice-to-array conversions

gopherbot pushed a commit that referenced this issue Sep 16, 2022
Updates #46505

Change-Id: Ib8f52d6ae199338f278731267c966da85dd0acdd
Reviewed-on: https://go-review.googlesource.com/c/go/+/430475
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@mdempsky
Copy link
Member

mdempsky commented Sep 19, 2022

I think x/tools/go/ssa still needs support for this. (/cc @timothy-king)

It can probably be represented as SliceToArrayPtr + UnOp/STAR.

@mdempsky mdempsky reopened this Sep 19, 2022
gopherbot pushed a commit that referenced this issue Sep 21, 2022
Converting from nil slice to zero element array is ok, so explicitly
describe the behavior in the spec.

For #46505

Change-Id: I68f432deb6c21a7549bf7e870185fc62504b37f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/430835
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@gopherbot
Copy link

gopherbot commented Sep 22, 2022

Change https://go.dev/cl/432735 mentions this issue: net/netip: use slice-to-array conversions

gopherbot pushed a commit that referenced this issue Sep 23, 2022
Use slice-to-array conversions in AddrFromSlice and
(*Addr).UnmarshalBinary. This allows allows to use AddrFrom16 and drop
the redundant ipv6Slice helper.

For #46505

Change-Id: I0e3a7d8825ad438115b7f23ee97cc74eec41a826
Reviewed-on: https://go-review.googlesource.com/c/go/+/432735
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@cuiweixie
Copy link
Contributor

cuiweixie commented Sep 24, 2022

Hi, masters. Please check this cl that add slice to array converstion to x/tools at go/ssa.

@gopherbot
Copy link

gopherbot commented Sep 24, 2022

Change https://go.dev/cl/433816 mentions this issue: go/ssa: add slice to array conversion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Accepted
Development

No branches or pull requests