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

unsafe: double-check specified semantics for Slice((*T)(nil), 0) for Go 1.17 #46742

Closed
mdempsky opened this issue Jun 14, 2021 · 14 comments
Closed

Comments

@mdempsky
Copy link
Contributor

For #19367, we ultimately specified that Slice(ptr, len) means the same as (*[len]T)(unsafe.Pointer(ptr))[:]. A consequence of this is that Slice((*T)(nil), 0) panics, because (*[0]T)(nil)[:] panics. However, users would probably prefer that it instead evaluated to []T(nil), and changing the semantics has been suggested in #19367.

In prior discussion on the issue, I had pointed out ptr == nil && len > 0 as a failure case to consider, which perhaps implied the expected behavior would be for Slice((*T)(nil), 0) to evaluate to []T(nil). But I don't see any other specific mention of "nil" in the discussions.

A few options I see:

  1. Leave the current semantics, and match (*[0]T)(nil)[:]. Changes in the future require conditioning on -lang (e.g., functions appearing in the source of packages compiled with -lang=go1.17 would panic, but would return nil when compiled with -lang=go1.18; I think this is an allowed use of -lang, but it would be the first backwards incompatible change tied to -lang).
  2. Leave the current semantics, but tweak the wording to say ptr must be non-nil, sidestepping the issue for Go 1.17 and letting us decide on semantics in the future. Probably continue panicking, but make -d=checkptr throw instead to emphasize it's really not specified.
  3. Change the semantics for Go 1.17 to return []T(nil), intentionally diverging from (*[0]T)(nil)[:] as a special case.

Filing as a release blocking issue to make sure we agree on an option for Go 1.17.

@mdempsky mdempsky added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Jun 14, 2021
@mdempsky mdempsky added this to the Go1.17 milestone Jun 14, 2021
@mdempsky
Copy link
Contributor Author

👍/👎: Option 1: Keep the current semantics for Go 1.17 (i.e., Slice((*T)(nil), 0) panics, like (*[0]T)(nil)[:]).

@mdempsky
Copy link
Contributor Author

👍/👎: Option 2: Change the semantics to be unspecified in Go 1.17 (i.e., Slice((*T)(nil), 0) may panic, or throw, or return nil in Go 1.17; and we can decide what's best for Go 1.18).

@mdempsky
Copy link
Contributor Author

👍/👎: Option 3: Change Slice((*T)(nil), 0) to return []T(nil) for Go 1.17 (i.e., intentionally diverging from (*[0]T)(nil)[:]).

@mdempsky

This comment has been minimized.

@mdempsky

This comment has been minimized.

@rsc
Copy link
Contributor

rsc commented Jun 16, 2021

[]int((*[0]int)(nil)) panics because []int((*[1]int)(nil)) panics. I think we agree that that's fine, or at least no one has complained in the decade it's been that way.

Defining unsafe.Slice in terms of this conversion was a nice shorthand. Right now the rule is "you can't use nil". The question is whether the rule should be "you can't use nil, except with zero you can". It does seem like the old conversion and the new one should be consistent at least. It seems a little late to be changing the old one though, although we could maybe consider it for Go 1.18 if people felt strongly.

On the other hand it is weird that unsafe.Slice is incapable of creating a slice == nil. It does seem like that's a significant hole.

Discussed with @griesemer and @iant who are okay with allowing unsafe.Slice((*T)(nil), 0) == []T(nil) for Go 1.17.

Does anyone object to this?

@rsc rsc changed the title unsafe: double-check specified semantics for Slice((*T)(nil), 0) for Go 1.17 proposal: unsafe: double-check specified semantics for Slice((*T)(nil), 0) for Go 1.17 Jun 16, 2021
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 16, 2021
@rsc
Copy link
Contributor

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

@rsc
Copy link
Contributor

rsc commented Jun 25, 2021

We are in the quiet weeks and not holding proposal review again until July 12.
Given the discussion and the fact that no one responded with objections to my comment above of June 16, the overwhelmingly likely outcome here is that this moves to likely accept on July 12 and accepted on July 19.

Given that
(1) this is much more likely than not going to be accepted, and
(2) we would like the release candidate to match the final Go 1.17 behavior,
I am going to make an exception to our usual procedure and approve committing this change ahead of approval. If for some unforeseen reason we decline the proposal, it will be easy to roll back. But the more likely case is we accept it, and making the change now makes the release candidate match the final release.

@mdempsky, please go ahead and make the change. If you run into any gotchas that we haven't considered, please let us know. Thanks!

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/331069 mentions this issue: spec: change unsafe.Slice((*T)(nil), 0) to return []T(nil)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/331070 mentions this issue: cmd/compile,runtime: change unsafe.Slice((*T)(nil), 0) to return []T(nil)

gopherbot pushed a commit that referenced this issue Jun 28, 2021
Updates #46742.

Change-Id: I044933a657cd1a5cdb29863e49751df5fe9c258a
Reviewed-on: https://go-review.googlesource.com/c/go/+/331069
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit that referenced this issue Jun 28, 2021
…nil)

This CL removes the unconditional OCHECKNIL check added in
walkUnsafeSlice by instead passing it as a pointer to
runtime.unsafeslice, and hiding the check behind a `len == 0` check.

While here, this CL also implements checkptr functionality for
unsafe.Slice and disallows use of unsafe.Slice with //go:notinheap
types.

Updates #46742.

Change-Id: I743a445ac124304a4d7322a7fe089c4a21b9a655
Reviewed-on: https://go-review.googlesource.com/c/go/+/331070
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
@mdempsky
Copy link
Contributor Author

The code changes are in now. I'm assuming this is technically still a release blocker to ratify the changes made, but that it shouldn't hinder releasing a release-candidate. /cc @toothrot

@rsc
Copy link
Contributor

rsc commented Jul 14, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jul 21, 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: unsafe: double-check specified semantics for Slice((*T)(nil), 0) for Go 1.17 unsafe: double-check specified semantics for Slice((*T)(nil), 0) for Go 1.17 Jul 21, 2021
@mdempsky
Copy link
Contributor Author

This is implemented.

@golang golang locked and limited conversation to collaborators Jul 21, 2022
@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
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

3 participants