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

Jump to bottom

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

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

mdempsky opened this issue Jun 14, 2021 · 14 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-FinalCommentPeriod release-blocker
Projects
Proposals (old)
Milestone
Go1.17

Comments

@mdempsky
Copy link
Member

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
Member 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
Member 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
Member 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.

Sign in to view
@mdempsky

This comment has been minimized.

Sign in to view
@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 rsc added this to Active in Proposals (old) 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 rsc mentioned this issue Jun 16, 2021
Open
@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!

@mdempsky mdempsky mentioned this issue Jun 25, 2021
Closed
@gopherbot
Copy link

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

@gopherbot
Copy link

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
@mdempsky
spec: change unsafe.Slice((*T)(nil), 0) to return []T(nil)
1519271 
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
@mdempsky
cmd/compile,runtime: change unsafe.Slice((*T)(nil), 0) to return []T(…
4bb0847 
…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
Member 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 rsc moved this from Active to Likely Accept in Proposals (old) Jul 14, 2021
@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 rsc moved this from Likely Accept to Accepted in Proposals (old) Jul 21, 2021
@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
Member Author

This is implemented.

@golang golang locked and limited conversation to collaborators Jul 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-FinalCommentPeriod release-blocker
Projects
No open projects
Proposals (old)
Accepted
Milestone
Go1.17
Development

No branches or pull requests
3 participants
@mdempsky @rsc @gopherbot