reflect: ArenaNew panic non-pointer type

[578559967]

What version of Go are you using (go version)?

$ go version
go version go1.20.4 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/r/Library/Caches/go-build"
GOENV="/Users/r/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOMODCACHE="/Users/r/go/pkg/mod"
GOOS="darwin"
GOPATH="/Users/r/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/r/sdk/go1.20.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/r/sdk/go1.20.4/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/j_/rq9ph2cd3h50w468sgv34lwh0000gn/T/go-build3924141508=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version go1.20.4 darwin/arm64
GOROOT/bin/go tool compile -V: compile version go1.20.4
uname -v: Darwin Kernel Version 22.4.0: Mon Mar  6 20:59:28 PST 2023; root:xnu-8796.101.5~3/RELEASE_ARM64_T6000
ProductName:		macOS
ProductVersion:		13.3.1
ProductVersionExtra:	(a)
BuildVersion:		22E772610a
lldb --version: lldb-1403.0.17.64
Apple Swift version 5.8 (swiftlang-5.8.0.124.2 clang-1403.0.22.11.100)

What did you do?

test.go:

package main

import (
	"arena"
	"fmt"
	"reflect"
)

func main() {
	a := arena.NewArena()
	i := 0
	v := reflect.ArenaNew(a, reflect.TypeOf(i))
	fmt.Println(v.Type(), *v.Interface().(*int))
	a.Free()
}

GOEXPERIMENT=arenas go1.20.4 run -a test.go

What did you expect to see?

print

*int 0

What did you see instead?

panic:

fatal error: arena_New: non-pointer type

goroutine 1 [running]:
runtime.throw({0x1025c532b?, 0x10254070c?})
        /Users/r/sdk/go1.20.4/src/runtime/panic.go:1047 +0x40 fp=0x140000a4e20 sp=0x140000a4df0 pc=0x102564dd0
arena.runtime_arena_arena_New(0x129cc1408?, {0x1025fa760, 0x1025eb8a0})
        /Users/r/sdk/go1.20.4/src/runtime/arena.go:113 +0x78 fp=0x140000a4e60 sp=0x140000a4e20 pc=0x10258d318
reflect.arena_New(0x140000a4ec8?, {0x1025fa760?, 0x1025eb8a0?})
        /Users/r/sdk/go1.20.4/src/arena/arena.go:89 +0x28 fp=0x140000a4e90 sp=0x140000a4e60 pc=0x1025952d8
reflect.ArenaNew(0x1025f06e0?, {0x1025ff038?, 0x1025eb8a0?})
        /Users/r/sdk/go1.20.4/src/reflect/arena.go:15 +0x2c fp=0x140000a4ed0 sp=0x140000a4e90 pc=0x1025a39fc
main.main()
        /Users/r/workspace/testcode/testarena/test.go:12 +0xa0 fp=0x140000a4f70 sp=0x140000a4ed0 pc=0x1025c1050
runtime.main()
        /Users/r/sdk/go1.20.4/src/runtime/proc.go:250 +0x248 fp=0x140000a4fd0 sp=0x140000a4f70 pc=0x1025673f8
runtime.goexit()
        /Users/r/sdk/go1.20.4/src/runtime/asm_arm64.s:1172 +0x4 fp=0x140000a4fd0 sp=0x140000a4fd0 pc=0x102592364

If I change the code to v := reflect.ArenaNew(a, reflect.TypeOf(&i)), it will pass.

But it conflicts with the comment of reflect.ArenaNew:

// ArenaNew returns a Value representing a pointer to a new zero value for the
// specified type, allocating storage for it in the provided arena. That is,
// the returned Value's Type is PointerTo(typ).

[mknyszek]

Thanks. I think the right thing to do in this circumstance is to fix the function itself, since the GOEXPERIMENT is not backed by the compatibility guarantee. The simple thing would be to just call PointerTo on the type before passing it down in src/arena/reflect.go.

[ianlancetaylor]

@mknyszek I don't agree. While this code is not covered by the compatibility guarantee, there's no particular reason to change it. I think we should just change the documentation to match the actual behavior.

[mknyszek]

@ianlancetaylor Yeah, sure. That's a good point and fine with me. I don't feel strongly either way.

[mknyszek]

I also wanted to clarify: the reason I wanted to change the implementation is that this is reflect function is supposed to be a reflection of (ha) arena.New, which takes a type argument T and returns a *T value, and so it's unfortunate that that doesn't really line up here in the implementation (the documentation is correct).

However, I still agree that not breaking people unnecessarily (even in a GOEXPERIMENT) makes more sense.

[578559967]

I agree with @mknyszek . This reflect function should behave like arena.New. It should be fixed as early as possible before moving from experiment to standard api. Or it will last forever and confuses people continuously. Don't let this inconsistency become a permanent scar. It's better to endure short-term pain than prolonged suffering.

[ianlancetaylor]

I don't think the argument that arena.New and reflect.ArenaNew should behave the same is very strong. They are very different functions.

That said, I admit that I found only one call to reflect.ArenaNew (https://github.com/huandu/go-clone/blob/fcc492e9d68d20fe88146b17e8e45b1c9d9e6b8e/generic/arena.go#L62) so maybe it's OK to change.

See also #56234.

[578559967]

https://github.com/huandu/go-clone/blob/fcc492e9d68d20fe88146b17e8e45b1c9d9e6b8e/generic/arena.go#L62
This piece of code appears to be fixing the issue precisely.

reflect.New also returns the Value with the Type of PointerTo(typ). It's better to keep the save behavior.

// New returns a Value representing a pointer to a new zero value
// for the specified type. That is, the returned Value's Type is PointerTo(typ).
func New(typ Type) Value

[randall77]

I think that that this is worth fixing even if it breaks existing uses.