Go version

go version go1.22.5 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/gopher/.cache/go-build'
GOENV='/home/gopher/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/gopher/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/gopher/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/snap/go/10660'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/snap/go/10660/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3287061668=/tmp/go-build -gno-record-gcc-switches'

What did you do?

See this playground.

package main

import (
    "fmt"
    "runtime"
    "slices"
)

func main() {
    fmt.Println("running for standard library")
    cloneWithFunc(slices.Clone[[]*int, *int])
    fmt.Println()
    fmt.Println("running for local func")
    cloneWithFunc(clone[[]*int, *int])
}

func cloneWithFunc(f func([]*int) []*int) {
    x := []*int{newInt(), newInt(), newInt()}
    x = f(x[:0])
    forceGC()
    fmt.Println("did the finalizer for this slice items run before this printed line?", x)
    forceGC()
}

func clone[S ~[]E, E any](s S) S {
    return append(S(nil), s...)
}

func forceGC() {
    runtime.GC()
    runtime.GC()
}

var count int

func newInt() *int {
    count++
    id := count
    x := new(int)
    runtime.SetFinalizer(x, func(*int) {
        fmt.Println("finalizer run for item:", id)
    })
    return x
}

What did you see happen?

running for standard library
did the finalizer for this slice items run before this printed line? []
finalizer run for item: 3
finalizer run for item: 2
finalizer run for item: 1

running for local func
finalizer run for item: 5
finalizer run for item: 4
finalizer run for item: 6
did the finalizer for this slice items run before this printed line? []

What did you expect to see?

running for standard library
finalizer run for item: 3
finalizer run for item: 2
finalizer run for item: 1
did the finalizer for this slice items run before this printed line? []

running for local func
finalizer run for item: 5
finalizer run for item: 4
finalizer run for item: 6
did the finalizer for this slice items run before this printed line? []

Comment From: diegommm

Open to contribute with a CL in case this can be confirmed.

Comment From: gabyhelp

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

Comment From: Jorropo

See this modified version: https://go.dev/play/p/uSY4HsNVTjQ

running for standard library
did the finalizer for this slice items run before this printed line? {0xc000010018 0 0}
finalizer run for item: 3
finalizer run for item: 2
finalizer run for item: 1

running for local func
finalizer run for item: 4
finalizer run for item: 6
finalizer run for item: 5
did the finalizer for this slice items run before this printed line? {<nil> 0 0}

As well as:

https://github.com/golang/go/blob/90c6558b6acef5a9b9fb8f3c35cff58423c8b00e/src/slices/slices.go#L348-L349

the behaviors are different.

I'll submit a fix thx for the report.

Comment From: diegommm

Jorropo changed the title slices: make Clone allow gc of non-copied items slices: Clone keeps source array alive when source slice is zero length

@Jorropo this also happens when the source slice is non-zero length. In cloneWithFunc, try changing x = f(x[:0]) for x = f(x[:1]). Starting from your version, I get:

running for standard library
finalizer run for item: 3
finalizer run for item: 2
did the finalizer for this slice items run before this printed line? {0xc00004a028 1 1}
finalizer run for item: 1

running for local func
finalizer run for item: 6
finalizer run for item: 5
did the finalizer for this slice items run before this printed line? {0xc00004a000 1 1}
finalizer run for item: 4

Comment From: Jorropo

@diegommm the arrays don't overlap when using x[:1] https://go.dev/play/p/GpymLHZOQFL Finalizers are janky (such as not being synchronous and reviving references) I don't think the output above can be interpreted because both the std bugged version and your fixed version give the same output.

Comment From: gopherbot

Change https://go.dev/cl/598875 mentions this issue: slices: prevent Clone keeping alive the array when cloning empty slices

Comment From: ianlancetaylor

I'm not convinced that this is worth fixing. Finalizers don't make any promises. Calling slices.Clone on an empty slice is an unusual action. This seems more like something to cover in a blog post somewhere.

Comment From: tdakkota

@ianlancetaylor

I would expect that slices.Clone always returns slice pointing to a new location. People might use it in a same way as strings.Clone: to make a copy that does not point to a larger slice.

Comment From: diegommm

@ianlancetaylor Could you clarify what do you mean by "worth fixing"? Do you see any downside of applying this fix?

Comment From: ianlancetaylor

On the CL @randall77 gave a good rationale for the change, and I'm OK with it.

Comment From: zigo101

There is not a perfect clone implementation in Go.

I agree that simplicity is not important for the implementation of the slices.Clone function.

Comment From: zigo101

On the other hand, I think Go runtime should specially handle zero-capacity slicing results, along with taking addresses of zero-size values, to make the data pointers point to a global unique address within each program run. Doing this will remove many surprises in Go programming.

[edit] Doing this will benefit the cases shown in this issue and in https://go.dev/play/p/ENRf3aUWhV2 and https://go.dev/play/p/kBhZIEk77Dz

Comment From: diegommm

@zigo101 would it make sense instead to just use a nil pointer instead of using a global unique address for all zero-size values?

Comment From: zigo101

In Go, SliceType{} and SliceType(nil) are different. Similarly, a pointer to a zero-size value is not a nil pointer.

Comment From: muktihari

I was asking this kind of question in golang-nuts since I didn't know this issue is already exist, thanks @randall77 for pointing me to this issue. There is already CL (https://go-review.googlesource.com/c/go/+/598875) for this, thanks for that.

I just want to share a real world use case that I come across when I try implementing Garmin's FIT Protocol encoding in Go, FIT SDK for Go, in case this may add another rationale why the fix is matter. Here is the context:

In a nutshell, FIT file is a binary file format that consist of multiple messages, and each message has fields: layout: message, len(fields), fields...

record,4,timestamp,distance,latitude,longitude
record,2,timestamp,distance
...

Event though there is len(fields) "n", I can't simply use make([]Field, n), since each field may has components that expand into fields, and the field created from component may as well require expansion. Calculating the total of the fields is more expensive than just copying a slice, this is where I need to use array pool. The code in a nutshell:


mesg.Fields := d.fieldsArray[:0] // big enough to hold all fields
d.decodeFields(&mesg)
for _, field := range mesg.Fields {
    // d.expandComponent(mesg, field):
    value := field.Value
    for _, component := range field.Components {
        // Expand until value is depleted.
        if value == 0 {
            break
        }

        compField := factory.CreateField(mesg.Num, component.Num)
        mask := (1 << component.Bits) - 1
        compField.Value = value & mask
        value = value >> component.Bits

        mesg.Fields = append(mesg.Fields, compField)

        if len(compField.Components) > 0 {
            d.expandComponent(mesg, compField) // Recursive
        }
    }
}
mesg.Fields = append(mesg.Fields[:0:0], mesg.Fields...) // slices.Clone

Now, this is the related part of this issue, if by any chance len(fields) of a message is zero, the resulting messages may contains a message that still pointing to d.fieldsArray, this may cause the Decoder{} (or maybe array in sync.Pool if we ever use it) can't be freed until that message is no longer used. At scale, this may affect performance even though the chance of encountering that kind of file is uncommon, but it's not zero.

And also, in my opinion, having slices.Clone is just simply append(s[:0:0], s...) without optimization is only saving us a couple of character to type, but with respect for that now I know that I can do s[:0:0] variant in case I need it, I didn't think about it before. Thank you.