runtime: pinner.Pin doesn't panic when it says it will [1.21 backport]

[bcmills]

The documentation for runtime.Pinner.Pin currently says:

The argument must be a pointer of any type or an unsafe.Pointer. It must be the result of calling new, taking the address of a composite literal, or taking the address of a local variable. If one of these conditions is not met, Pin will panic.

However, it does not always do so today.

A channel, map, or slice value can be constructed by calling make, which does not involve a call to new, nor a composite literal, nor the address of a local variable. Empirically, Pin does not panic when called on such an object (https://go.dev/play/p/q_iyNHJduRf).

	p := new(runtime.Pinner)
	c := make(chan struct{}, 1)
	p.Pin(reflect.ValueOf(c).UnsafePointer())

It isn't clear to me whether this is a mistake in the documentation (a valid case that was omitted from the doc comment), or a bug in the implementation (Pin failing to panic for an unsupported address).

At a higher level, it seems to me that it is basically always a mistake for a doc comment to state that something will panic for an unsupported input, since that makes it an incompatible change to ever add support for such an input. (It would be better for Pin not to promise anything about what happens for such an input, but to panic anyway as a debugging aid to the user.)

[bcmills]

This was prompted by the desire to pin channel values in https://go.dev/cl/528796.

Note that channels are, to my knowledge, the only Go type that cannot be allocated by either calling new or taking the address of a composite literal or local variable (compare #28366).

[dr2chase]

It reads like an oversight in the documentation, though it is also unclear to me what C code would do with a pinned channel or map.

[dr2chase]

CC @mknyszek especially.

[qiulaidongfeng]

See #62356 and CL 527156，After CL 527156, the documentation becomes:
The argument must be a pointer of any type or an unsafe.Pointer. It's safe to call Pin on non-Go pointers, in which case Pin will do nothing.

[bcmills]

Huh! Yeah, it looks like this is probably fixed at head as a result of #62356. 🙃

[bcmills]

In that case, I guess this is a bug affecting only Go 1.21. (Leaving open for the runtime folks to decide whether it's worth backporting some kind of fix for compatibility.)

[randall77]

We only usually backport fixes for regressions, so I'm inclined to leave this one be.
Pin will panic less in 1.22 than in 1.21, but that's not so bad. It isn't like Pin in 1.21 panics more than 1.20 (because Pin doesn't exist in 1.20).

[qiulaidongfeng]

Perhaps this is a go1.21 documention issue that should be written as

The argument must be a pointer of any type or an unsafe.Pointer.

[gopherbot]

Change https://go.dev/cl/553395 mentions this issue: [release-branch.go1.21] runtime: fix Pinner.Pin documention

[qiulaidongfeng]

See https://go.dev/wiki/MinorReleases, I think can repair go1.21 document.
I think the title of this issue is followed should by [1.21 backport] , don't have to open a new backport issue.

[qiulaidongfeng]

@bcmills Could you please add [1.21 backport] after the title of this issue?

[bcmills]

Done.

[mknyszek]

Sorry for not replying here. @bcmills I think you're right that we shouldn't explicitly say anything panics necessarily as part of the API. We should just drop the problematic part of the documentation. (EDIT: Which it looks like we do on tip, as noted above, and we should backport that too.)

I do also think we should consider:

• Adding a warning to the documentation that writing the code you wrote may not work correctly and is not supported.
• Redirect users to runtime/cgo.Handle if the only reason they want to pin something is to pass it back to Go.
• Explicitly allowing some common and fairly safe cases, like slices and strings. (Slices are especially egregious today, since you can just pass an interior pointer to the slice and that doesn't even need unsafe!)

I will file a proposal for the third bullet point. EDIT: Filed #65286. This should hopefully allow simplifying usage of the pinner API slightly, and should slightly simplify the cgo pointer passing rules.

[gopherbot]

Closed by merging 916e6cd to release-branch.go1.21.