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

reflect: add SliceAt function #61308

Closed
ianlancetaylor opened this issue Jul 11, 2023 · 23 comments
Closed

reflect: add SliceAt function #61308

ianlancetaylor opened this issue Jul 11, 2023 · 23 comments

Comments

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Jul 11, 2023

In Go 1.21 we are introducing some new functions in the unsafe package: unsafe.Slice, unsafe.SliceData, unsafe.String, unsafe.StringData. It should be possible to access this functionality through the reflect package as well.

The value returned by unsafe.SliceData is already available by calling the reflect.Value.Pointer method.

I propose that we extend reflect.Value.Pointer so that if it is called on a value of kind String, it returns the equivalent of unsafe.StringData.

The equivalent to unsafe.String is simply reflect.ValueOf(unsafe.String((*byte)(vp.UnsafePointer()), n)), so I don't think we need to add anything for that.

I propose one new function:

package reflect

// UnsafeSlice returns a Value representing a slice whose underlying data starts at vp,
// with length and capacity equal to n. vp.Kind() must be Pointer.
// The returned slice will have element type vp.Type().Elem().
// This is like unsafe.Slice.
func UnsafeSlice(vp Value, n int) Value

(edited to change function name from Slice to UnsafeSlice`)

@ianlancetaylor ianlancetaylor added this to the Proposal milestone Jul 11, 2023
@bcmills
Copy link
Contributor

bcmills commented Jul 12, 2023

It seems to me that the new functionality is to MakeSlice as NewAt is to New. So I suggest:

func SliceAt(typ Type, p unsafe.Pointer, len int) Value

That has the additional advantage of passing through the type unsafe.Pointer, which clarifies that it is in fact unsafe.

@rittneje
Copy link

Why does the proposed reflect.Slice (or reflect.SliceAt) require the length and capacity to be the same instead of having two separate parameters like reflect.MakeSlice?

@DeedleFake
Copy link

Those functions were introduced starting in 1.17 and then more in 1.20.

The equivalent to unsafe.String is simply reflect.ValueOf(unsafe.String(vp.Pointer(), n)), so I don't think we need to add anything for that.

That won't work. unsafe.String() requires a *byte, not a uintptr. The conversion requires first converting through an unsafe.Pointer, so I think having an equivalent top-level function makes sense for that one, too. It's not really any different from a slice otherwise except for the type always being the same:

reflect.ValueOf(unsafe.String((*byte)(unsafe.Pointer(v.Pointer())), v.Len()))
reflect.ValueOf(unsafe.Slice((*T)(unsafe.Pointer(v.Pointer())), v.Len()))

@bcmills
Copy link
Contributor

bcmills commented Jul 12, 2023

Why does the proposed reflect.Slice (or reflect.SliceAt) require the length and capacity to be the same … ?

Because unsafe.Slice does too. Most of the time you do want them to be the same, and if you want to reduce the capacity that's easy enough to do as a separate operation (via SetCap).

@bcmills
Copy link
Contributor

bcmills commented Jul 12, 2023

For the string case,

reflect.ValueOf(unsafe.String(v.Interface().(*byte), v.Len()))

or

reflect.ValueOf(unsafe.String((*byte)(v.UnsafePointer()), v.Len()))

will often suffice.

@cuonglm
Copy link
Member

cuonglm commented Aug 8, 2023

I think the function name should be UnsafeSlice instead of Slice.

Slice is already taken as a Kind.

@gopherbot
Copy link

Change https://go.dev/cl/516597 mentions this issue: reflect: add UnsafeSlice

@gopherbot
Copy link

Change https://go.dev/cl/516596 mentions this issue: reflect: handle String kind in Value.Pointer

@rsc
Copy link
Contributor

rsc commented Feb 7, 2024

I think the function name should be UnsafeSlice instead of Slice.
Slice is already taken as a Kind.

Another reason is that these are in fact unsafe, so the name should begin with Unsafe to distinguish it from the normal, safe reflect API.

@rsc
Copy link
Contributor

rsc commented Feb 7, 2024

It seems like we should also update UnsafePointer to handle String (not just update Pointer).

@rsc rsc changed the title proposal: reflect: add functions corresponding to new unsafe functions proposal: reflect: add UnsafeSlice function Feb 8, 2024
@rsc
Copy link
Contributor

rsc commented Feb 9, 2024

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 Feb 14, 2024

Have all remaining concerns about this proposal been addressed?

Two parts to the proposal.

First, add func UnsafeSlice, mimicking unsafe.Slice.

Second, update Value.Pointer and Value.UnsafePointer to handle String.

@bcmills
Copy link
Contributor

bcmills commented Feb 15, 2024

I don't think the signature concern in #61308 (comment) has been addressed yet.

The other functions and methods in the reflect package that use the word Unsafe still explicitly require the use of the unsafe package, but

func Slice(vp Value, n int) Value

(as described in #61308 (comment)) does not have that property.

@ianlancetaylor
Copy link
Contributor Author

I'm not sure that's quite true of the existing reflect.Value.UnsafePointer method, which does return a value of type unsafe.Pointer but can be used without importing the unsafe package.

Note that the proposed function is now named UnsafeSlice rather than Slice.

@aclements
Copy link
Member

I also prefer @bcmills ' alternate signature. It has the clear advantage of using unsafe.Pointer, and I suspect in practice it will be more common that callers will have an unsafe.Pointer already, rather than a Value, so it will save the conversion (though they can convert either way).

@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

Have all remaining concerns about this proposal been addressed?

Two parts to the proposal.

First, add func SliceAt, analogous to NewAt (which probably should have been called ValueAt) but for slices:

func SliceAt(typ Type, p unsafe.Pointer, len int) Value

Second, update Value.Pointer and Value.UnsafePointer to handle String.

@rsc rsc changed the title proposal: reflect: add UnsafeSlice function proposal: reflect: add SliceAt function Mar 13, 2024
@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

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

Two parts to the proposal.

First, add func SliceAt, analogous to NewAt (which probably should have been called ValueAt) but for slices:

func SliceAt(typ Type, p unsafe.Pointer, len int) Value

Second, update Value.Pointer and Value.UnsafePointer to handle String.

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Mar 16, 2024
For golang#61308

Change-Id: Ib799bf536eef9df63bf2c56c28bd62c286960a69
@gopherbot
Copy link

Change https://go.dev/cl/572117 mentions this issue: reflect: Value.Pointer and Value.UnsafePointer handle String

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Mar 16, 2024
DO NOT SUBMIT

Fixes golang#61308

Change-Id: I707e788591aa4545ceef340aab424a9ef24dcc07
@gopherbot
Copy link

Change https://go.dev/cl/572118 mentions this issue: reflect: add SliceAt

@cuonglm
Copy link
Member

cuonglm commented Mar 20, 2024

Does reflect.SliceAt do all the runtime validations that unsafe.Slice do? For example, should this code panic?

_ = SliceAt(TypeOf(0), unsafe.Pointer((*int)(nil)), 1)

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

Yes, reflect.SliceAt should be no more unsafe than unsafe.Slice. So it should do the same validations.

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

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

Two parts to the proposal.

First, add func SliceAt, analogous to NewAt (which probably should have been called ValueAt) but for slices:

func SliceAt(typ Type, p unsafe.Pointer, len int) Value

Second, update Value.Pointer and Value.UnsafePointer to handle String.

@rsc rsc changed the title proposal: reflect: add SliceAt function reflect: add SliceAt function Mar 27, 2024
@rsc rsc modified the milestones: Proposal, Backlog Mar 27, 2024
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Mar 30, 2024
gopherbot pushed a commit that referenced this issue Apr 2, 2024
Updates #61308

Change-Id: I92d459383c520d137787ce5c8f135d205af74e5d
Reviewed-on: https://go-review.googlesource.com/c/go/+/516596
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/575956 mentions this issue: reflect: add missing String case in Value.UnsafePointer doc

gopherbot pushed a commit that referenced this issue Apr 3, 2024
CL 516596 changes Value.UnsafePointer to handle String case. However,
the method's doc comment is not updated to reflect this change.

Updates #61308

Change-Id: I84e02fd969ae0244184e1a2f05cac4651cdf7bff
Reviewed-on: https://go-review.googlesource.com/c/go/+/575956
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

9 participants