proposal: reflect: add DeepCopy

[changkun]

Previous proposal updates

As a duality complement of `DeepEqual`, I propose to add a new API for deep copy:

package reflect

// DeepCopy copies src to dst recursively.
//
// Two values of identical type are deeply copied if one of the following cases applies.
//
// Array and slices values are deeply copied, including its elements.
//
// Struct values are deeply copied for all fields, including exported and unexported.
//
// Interface values are deeply copied if the underlying type can be deeply copied.
//
// Map values are deeply copied for all of its key and corresponding values.
//
// Pointer values are deeply copied for its pointed value.
//
// One exception is Func value. It is not copiable, and still points to the same function.
//
// Other values - numbers, bools, strings, and channels - are deeply copied and
// have different underlying memory address.
func DeepCopy[T any](dst, src T)
// or
//
// DeepCopy panics if src and dst have different types.
func DeepCopy(dst, src any)
// or
//
// DeepCopy returns an error if src and dst have different types.
func DeepCopy(dst, src any) error

DeepCopy may likely be very commonly used. Implement it in reflect package may receive better runtime performance.

The frist version seems more preferrable because type parameters permits compile time type checking, but the others might also be optimal.

The proposed document of the API is preliminary.

---

Update:

Based on the discussions until #51520 (comment), the documented behavior could be:

// DeepCopy copies src to dst recursively.
//
// Two values of identical type are deeply copied if one of the following
// cases apply.
//
// Array and slices values are deeply copied, including its elements.
//
// Struct values are deeply copied for all fields, including exported
// and unexported.
//
// Map values are deeply copied for all of its key and corresponding
// values.
//
// Pointer values are deeply copied for their pointed value, and the
// pointer points to the deeply copied value.
//
// Numbers, bools, strings are deeply copied and have different underlying
// memory address.
//
// Interface values are deeply copied if the underlying type can be
// deeply copied.
//
// There are a few exceptions that may result in a deeply copied value not
// deeply equal (asserted by DeepEqual(dst, src)) to the source value:
//
// 1) Func values are still refer to the same function
// 2) Chan values are replaced by newly created channels
// 3) One-way Chan values (receive or read-only) values are still refer
//    to the same channel
//
// Note that for stateful copied values, such as the lock status in
// sync.Mutex, or underlying file descriptors in os.File and net.Conn,
// are retained but may result in unexpected consequences in follow-up
// usage, the caller should clear these values depending on usage context.
//
// The function panics/returns with an error if
//
// 1) source and destination values have different types
// 2) destination value is reachable from source value

Depending on the design choice, the content included and after exceptions could decide on different behaviors, see
#51520 (comment), and #51520 (comment) as examples.

The API signature could select one of the following possible options. The main differences between them include two aspects:

1. panic (break the code flow, and enter defer recover) or return an error (user handled directly)
2. destination value in either parameter (more GC friendly) or return value (always allocates)

func DeepCopy[T any](dst, src T) error
func DeepCopy[T any](dst, src T)
func DeepCopy(dst, src any) error
func DeepCopy(dst, src any)

func DeepCopy[T any](src T) (T, error)
func DeepCopy[T any](src T) T
func DeepCopy(src any) (any, error)
func DeepCopy(src any) error

Either version is optimal, and purely depending on the final choice.

Update (until #51520 (comment)):

I propose to add a new API for deep copy:

// DeepCopy copies src to dst recursively.
//
// Two values of identical type are deeply copied if one of the following
// cases apply.
//
// Numbers, bools, strings are deeply copied and have different underlying
// memory address.
//
// Slice and Array values are deeply copied, including its elements.
//
// Map values are deeply copied for all of its key and corresponding
// values.
//
// Pointer values are deeply copied for their pointed value, and the
// pointer points to the deeply copied value.
//
// Struct values are deeply copied for all fields, including exported
// and unexported.
//
// Interface values are deeply copied if the underlying type can be
// deeply copied.
//
// There are a few exceptions that may result in a deeply copied value not
// deeply equal (asserted by DeepEqual(dst, src)) to the source value:
//
// 1) Func values are still refer to the same function
// 2) Chan values are replaced by newly created channels
// 3) One-way Chan values (receive or read-only) values are still refer
//    to the same channel
//
// Note that while correct uses of DeepCopy do exist, they are not rare.
// The use of DeepCopy often indicates the copying object does not contain
// a singleton or is never meant to be copied, such as sync.Mutex, os.File,
// net.Conn, js.Value, etc. In these cases, the copied value retains the
// memory representations of the source value but may result in unexpected
// consequences in follow-up usage, the caller should clear these values
// depending on their usage context.
func DeepCopy[T any](src T) (dst T)

Here is an implementation for trying out: https://pkg.go.dev/golang.design/x/reflect

[mvdan]

Have you seen previous proposals like #18371?

[changkun]

Unlike #18371, this proposal suggests deep copy as a package API (as a complement of DeepEqual) other than a built-in function.

[mvdan]

The reflect API was already mentioned there, but in any case, it was rejected due to the functionality being proposed and not what package API it's under.

[changkun]

The previous decision was made based on the interpretation of it should not be built-in. The discussion of as a reflect API seems not to converge to a consensus yet (as how I understand the conversation).

Sure, the deep copy behavior can be implemented as an external package as there are many already. But as an API in reflect package, it can also receive runtime performance improvements (e.g. direct memory copy).

Moreover, it is also a standard implementation to avoid potentially incorrect implementation.

[ianlancetaylor]

How should DeepCopy handle circular data structures?

[changkun]

How should DeepCopy handle circular data structures?

I can imagine two options for as the behavior (if I understood the question correctly):

1. Panics if the data structure is circular
2. Handle them. For example:

package main

import (
	"unsafe"
	"reflect"
)

type A struct{ b *B }
type B struct{ a *A }

func main() {
	x := &A{}
	y := &B{}
	x.b = y
	y.a = x
	println(memAddr(x), memAddr(y), memAddr(x.b), memAddr(x.b.a), memAddr(x.b.a.b)) // 1 2 2 1 2
	
	var z *A
	reflect.DeepCopy(z, x)
	
	println(memAddr(z), memAddr(z.b), memAddr(z.b.a), memAddr(z.b.a.b)) // 3 4 3 4
}

func memAddr[T any](x *T) uintptr { return uintptr(unsafe.Pointer(x)) }

I would more prefer the second case, hence propose the second option.

[DeedleFake]

I feel like there is little benefit to the output value being an argument to the function. Any, for example, fields in a struct that are interfaces, pointers, maps, etc. are going to require the function to instantiate new values from scratch anyways if it's really going to create a full copy, so how about just func DeepCopy[T any](v T) T? That'll lead to less confusion and fewer potential errors with mismatched/incorrect types. Otherwise you also get questions like

What happens if I copy into a map that has existing data in it? Is the map cleared first? If I copy into a slice, does the slice's old capacity get preserved?

[DeedleFake]

@changkun

How does it handle overlapping cyclic structures? For example:

type S struct {
  Next *S
}

func main() {
  one := &S{}
  two := &S{Next: &S{Next: &S{Next: one}}}
  one.Next = two

  reflect.DeepCopy(one, two)
}

Obviously this would be a really weird case, but something similar could happen accidentally. If the address stored in one is used to store the output, then that means that two's data is getting changed by the copy, potentially resulting in some really strange, implementation-dependent results if not explicitly defined, likely an infinite loop as it continuously generates new structures to try to continue copying further and further downwards.

[changkun]

...so how about just func DeepCopy[T any](v T) T? That'll lead to less confusion and fewer potential errors with mismatched/incorrect types.

I think this is one of the possible optimal versions of deep copy. The pros are as you explained and don't require further document about the status of destination. The cons might be that it always have to allocate on heap.

The proposed versions have pros such as the destination might be stack allocated, hence GC friendly. The potential con could be addressed by documenting more:

The destination value must be a nil or points to a zero
value of source type, and itself not reachable from the
source value. Otherwise, the function panics/return
an error.

Either way is acceptable, and have their unique advantages. Depends on the final decision.

[changkun]

type S struct {
  Next *S
}

func main() {
  one := &S{}
  two := &S{Next: &S{Next: &S{Next: one}}}
  one.Next = two
  reflect.DeepCopy(one, two)
}

This is an interesting case and I would more prefer to panic as a misuse and not handling it.

It is solved by adding this constraint as my last comment elaborated:

The destination value must be a nil or points to a zero
value of source type, and itself not reachable from the
source value. Otherwise, the function panics/return
an error.

[rittneje]

One thing that is problematic is that currently the reflect package disallows setting private (unexported) struct fields. But your proposed version of DeepCopy would have to violate this rule.

Also, there are several interesting cases beyond just func. Like what does it mean to deep copy a mutex? Or a buffered channel? Or a one-way channel? Or a net.Conn? What if a type needs to override whatever the default deep copy logic is?

To me it seems like the de facto standard Clone method is a cleaner solution, because then there is a stable API contract around what it means to copy something.

[changkun]

One thing that is problematic is that currently the reflect package disallows setting private (unexported) struct fields. But your proposed version of DeepCopy would have to violate this rule.

I would argue that deep copy is not about violating setting private struct fields because one cannot use it to set a particular private field. Instead, the API duplicates one to the other completely.

Also, there are several interesting cases beyond just func. Like what does it mean to deep copy a mutex? Or a buffered channel? Or a one-way channel? Or a net.Conn? What if a type needs to override whatever the default deep copy logic is?

These are valid cases, but also make the argument of "why a standard implementation is necessary" stronger. To my senses, I think:

1. deep copy of a mutex results in a new mutex;
2. a buffered channel results in a buffered channel that has the same buffer size, but elements inside the buffer are not copied (intuitively, the elements should be received by the actual receiver, not a copied value) hence empty buffer;
3. one-way channels: a) a copy of a receive-only channel results in the same receive-only channel (intuitively as a new reader), or could result in nil (intuitively slightly matches the behavior in 2)). b) send-only channel results in the same send-only channel (intuitively as a new sender), or could result in nil.
4. a copy of net.Conn results in an invalid connection, can be either zero-valued or nil.

To me it seems like the de facto standard Clone method is a cleaner solution, because then there is a stable API contract around what it means to copy something.

I am not sure which Clone are you referring to and why it is considered as a cleaner de facto standard. Assuming net/http, there are different Clones but it seems that they are all customized object copy behavior, for instance:

func (h Header) Clone() Header
func (r *Request) Clone(ctx context.Context) *Request
func (t *Transport) Clone() *Transport

defining a common interface (as a Copiable constraint) is not possible, there is also no way to define NonCopiable that excludes certain types with the current type set design.

Let's still assume a type set type Copier[T] interface { Clone() T }, a non-pointer value (say type V) may not be subject to this constraint if the Clone signature is (*V).Clone() *V. In this case, only a pointer of V can be used as a type parameter of Copier (Copier[*V]).

[davecheney]

deep copy of a mutex results in a new mutex;

If the original mutex was locked, what is the state of the new mutex?

If the struct being copied contained a *os.File, what is the state of the cloned *os.File? Does it point a new a file descriptor, or the original file descriptor? What happens when code was using the mutex to serialise access to that file descriptor?

[changkun]

If the original mutex was locked, what is the state of the new mutex?

Either option is acceptable. Setting the new mutex to an unlocked state could be considered as the intuition of the newly created object is not locked; retaining the lock state fits more to have the exact memory representation. The choice between the two is a matter of decision.

If the struct being copied contained a *os.File, what is the state of the cloned *os.File? Does it point a new a file descriptor, or the original file descriptor? What happens when code was using the mutex to serialise access to that file descriptor?

This is a similar case. Depending on the choice, we could either retain the state of os.File, the same file descriptor. Or just set it as an invalid file descriptor.

Briefly summarize these exceptional cases, it all relates to the problem: what should we do when handling a stateful object, such as channels, net.Conn, os.File, sync.* etc.?
To answer this category of questions, we are dealing with the choice of 1) retaining the underlying memory representation; 2) or changing the semantic state of a copying object. As previously expressed, the two possibilities are acceptable objectively because they have their particular purpose in different directions. The actual design choice is a decision matter.

Subjectively, I would argue DeepCopy is more about retaining the underlying memory representation that maintains the pointer's relationship, hence very similar to copying an execution stack (assuming objects on the stack not referring to external things). Therefore, the API only needs to document the exceptional cases for the language-defined types and special objects in the standard library. Then other user-defined stateful objects fall back to those cases. Documenting copying these values may result in unexpected behavior should be acceptable.

One may also argue that changing the state of a copied object, such as mutex, violates the duplication semantics of "copy", but we would also need to be aware that a deep copied value does not necessarily DeepEqual because, according to the document of DeepEqual, "Func values are deeply equal if both are nil; otherwise they are not deeply equal."

[rittneje]

I would argue that deep copy is not about violating setting private struct fields because one cannot use it to set a particular private field. Instead, the API duplicates one to the other completely.

What I mean is that if this were to be implemented, the reflect package would have to do a lot of special casing. It cannot be implemented in terms of the existing operations.

Wit regards to sync.Mutex and os.File and such, unless you are going to special case them all in DeepCopy (which doesn't seem feasible), that strongly implies that the only two choices are (1) literally copying their current state byte-for-byte, or (2), allowing types to define some standard Clone() method to customize this behavior. The latter means that you will end up with something like a Cloneable interface anyway.

I am not sure which Clone are you referring to and why it is considered as a cleaner de facto standard.

The name of the method is ultimately immaterial. The important part is that it be a method, because the behavior of that method is now part of the type's contract. In particular, the returned object is actually valid/usable, whatever that means for the type in question.

[EbenezerJesuraj]

How will this Deep Copy feature of the reflect library of Golang be useful for making runtime dynamic conversion of any structured data-field into (say csv file) for audit - level entrypoint functionality.

[changkun]

What I mean is that if this were to be implemented, the reflect package would have to do a lot of special casing. It cannot be implemented in terms of the existing operations.

I still fail to see why it have to do "a lot" special casing, and how it "cannot" be implemented.
Can you name all special cases for discussion? If the decision maker decides to deal with them, the API need to deal with them. But the latest updated proposed behavior is not the case, see the tip of this thread.

I think we might be better ask: When will a developer consider to use an API for deep copy? If they have a struct with tons of net.Conn, mutexes, and os.File, how likely such a value needs a deep copy? What are the primary use cases of this API? How much they can benifit from DeepCopy? #51520 (comment) is a reasonable usecase.

Wit regards to sync.Mutex and os.File and such, unless you are going to special case them all in DeepCopy (which doesn't seem feasible), that strongly implies that the only two choices are (1) literally copying their current state byte-for-byte, or (2), allowing types to define some standard Clone() method to customize this behavior. The latter means that you will end up with something like a Cloneable interface anyway.

Sorry, I don't think this argument is strong enough, as it was already argued that a Cloneable interface is not easily possible and already incompatible with existing usage. See #51520 (comment)

The name of the method is ultimately immaterial. The important part is that it be a method, because the behavior of that method is now part of the type's contract. In particular, the returned object is actually valid/usable, whatever that means for the type in question.

I think this is the suggested behavior based on the discussion until now (copied from the tip of the thread):

// DeepCopy copies src to dst recursively.
//
// Two values of identical type are deeply copied if one of the following
// cases apply.
//
// Array and slices values are deeply copied, including its elements.
//
// Struct values are deeply copied for all fields, including exported
// and unexported.
//
// Map values are deeply copied for all of its key and corresponding
// values.
//
// Pointer values are deeply copied for their pointed value, and the
// pointer points to the deeply copied value.
//
// Numbers, bools, strings are deeply copied and have different underlying
// memory address.
//
// Interface values are deeply copied if the underlying type can be
// deeply copied.
//
// There are a few exceptions that may result in a deeply copied value not
// deeply equal (asserted by DeepEqual(dst, src)) to the source value:
//
// 1) Func values are still refer to the same function
// 2) Chan values are replaced by newly created channels
// 3) One-way Chan values (receive or read-only) values are still refer
//    to the same channel
//
// Note that for stateful copied values, such as the lock status in
// sync.Mutex, or underlying file descriptors in os.File and net.Conn,
// are retained but may result in unexpected consequences in follow-up
// usage, the caller should clear these values depending on usage context.
//
// The function panics/returns with an error if
//
// 1) source and destination values have different types
// 2) destination value is reachable from source value

[rittneje]

@changkun First, I never said it cannot be implemented. I said it cannot be implemented in terms of the existing operations, because the reflect package does not allow setting private fields. So it would have to make some internal exception when it is doing so as part of your proposed method implementation. This also means that the reflect package is the only place that such a method could be implemented.

I think we might be better ask: When will a developer consider to use an API for deep copy? If they have a struct with tons of net.Conn, mutexes, and os.File, how likely such a value needs a deep copy? What are the primary use cases of this API? How much they can benifit from DeepCopy? #51520 (comment) is a reasonable usecase.

To be honest, I don't understand what the use case from @EbenezerJesuraj is. I don't see any relationship between DeepCopy and CSVs. I would need something more concrete here.

Sorry, I don't think this argument is strong enough, as it was already argued that a Cloneable interface is not easily possible and already incompatible with existing usage. See #51520 (comment)

It is merely a statement of fact. Either the reflect package applies a one-size-fits-all solution (meaning it always copies struct fields blindly, regardless of whether it's tls.Config or sync.Mutex or net.TCPConn), or it allows types to customize this behavior via a method. And if it is going to allow such customization, then you have something that looks like an interface, regardless of the specific method name. Those are the only two options I see.

Frankly, I challenge the two claims you made in the original post.

may likely be very commonly used

reflect.DeepCopy is not something I feel would provide much if any value to our code base, especially if the behavior is not customizable in any way. It works well enough for primitive types and simple DTOs, but falters for anything more complex. Personally I feel it should have a contract more like json.Marshal - it will only consider public fields while cloning, and it does not work for "unusual" types like functions and channels.

Implement it in reflect package may receive better runtime performance.

Better runtime performance compared to what? If you mean compared to writing the same code outside the reflect package, then maybe. But compared to a Clone method that doesn't rely on reflection at all, probably not.

[changkun]

reflect.DeepCopy is not something I feel would provide much if any value to our code base, especially if the behavior is not customizable in any way. It works well enough for primitive types and simple DTOs, but falters for anything more complex. Personally I feel it should have a contract more like json.Marshal - it will only consider public fields while cloning, and it does not work for "unusual" types like functions and channels.

I am not sure what your codebase looks like, but the wide Go user code bases certainly have the need, with evidence in https://pkg.go.dev/search?q=deep+copy&m= . Customization seems to fall out of the discussion and not the purpose of this proposal, it might be worth having another proposal.

Implement it in reflect package may receive better runtime performance.

Better runtime performance compared to what? If you mean compared to writing the same code outside the reflect package, then maybe. But compared to a Clone method that doesn't rely on reflection at all, probably not.

Note that the sentence was not a claim but only communicating possibilities. This is based on the fact that the reflect has closer access to runtime itself, such as direct memory copy, collaboration with GC, etc. Discussing the actual performance in the implementation and benchmarking seems too early as the proposal is not yet accepted.

[EbenezerJesuraj]

@changkun First, I never said it cannot be implemented. I said it cannot be implemented in terms of the existing operations, because the reflect package does not allow setting private fields. So it would have to make some internal exception when it is doing so as part of your proposed method implementation. This also means that the reflect package is the only place that such a method could be implemented.

I think we might be better ask: When will a developer consider to use an API for deep copy? If they have a struct with tons of net.Conn, mutexes, and os.File, how likely such a value needs a deep copy? What are the primary use cases of this API? How much they can benifit from DeepCopy? #51520 (comment) is a reasonable usecase.

To be honest, I don't understand what the use case from @EbenezerJesuraj is. I don't see any relationship between DeepCopy and CSVs. I would need something more concrete here.

Sorry, I don't think this argument is strong enough, as it was already argued that a Cloneable interface is not easily possible and already incompatible with existing usage. See #51520 (comment)

It is merely a statement of fact. Either the reflect package applies a one-size-fits-all solution (meaning it always copies struct fields blindly, regardless of whether it's tls.Config or sync.Mutex or net.TCPConn), or it allows types to customize this behavior via a method. And if it is going to allow such customization, then you have something that looks like an interface, regardless of the specific method name. Those are the only two options I see.

Frankly, I challenge the two claims you made in the original post.

may likely be very commonly used

reflect.DeepCopy is not something I feel would provide much if any value to our code base, especially if the behavior is not customizable in any way. It works well enough for primitive types and simple DTOs, but falters for anything more complex. Personally I feel it should have a contract more like json.Marshal - it will only consider public fields while cloning, and it does not work for "unusual" types like functions and channels.

Implement it in reflect package may receive better runtime performance.

Better runtime performance compared to what? If you mean compared to writing the same code outside the reflect package, then maybe. But compared to a Clone method that doesn't rely on reflection at all, probably not.

EJ Reply:

Package reflect implements run-time reflection, allowing a program to manipulate objects with arbitrary types. The typical use is to take a value with static type interface{} and extract its dynamic type information by calling TypeOf, which returns a Type.

CSV's was an example scenario implementing the Deep Copy feature fo the reflect library.

(Say I receive a set of string related data in a structure format, reflect libraries DeepCopy feature should be able to understnad the nature of the data-type(s) if there the structure not only holds strings but a combination of a lot of primitive data-types and our need is to store and view it in an excel sheet format (csv or tsv and so on). Consider the above scenario which is required/performed on a Golang based scaffold using an hexagonal architectural pattern) will the Deep Copy feature of reflect library naturally support this use-cae.

[EbenezerJesuraj]

Auxillary Info:

We can also say that the program knows the meta-data of the different types of data present in the struct without even reading the actual structure/db

[EbenezerJesuraj]

Auxillary Info 2: csv was just a particular use-case scenario. The Storage can be of any means known by the industry. Shallow Copy will only make a pointer point to the same location which considering an endpoint-level changes happening in a business-application is highly infeasible(a project which i am working on currently). So will DeepCopy.reflect support such use-cases.

[changkun]

Speaking of use cases, I can provide one example from my practice. In graphics applications, geometric data are represented by mesh consisting of vertices, edges, faces, etc. Vertices have different attributes, edges can be directional, faces can be polygon-based. Clone/Copy/Duplicate such an object is not easy and requires many developers' effort. But with DeepCopy, one can easily create a copy of a given mesh. This is very helpful for geometry processing cases where caching cascaded meshes are convenient for different kinds of solvers.

This case can generalize to any graph data.

[rittneje]

(Say I receive a set of string related data in a structure format, reflect libraries DeepCopy feature should be able to understnad the nature of the data-type(s) if there the structure not only holds strings but a combination of a lot of primitive data-types and our need is to store and view it in an excel sheet format (csv or tsv and so on). Consider the above scenario which is required/performed on a Golang based scaffold using an hexagonal architectural pattern) will the Deep Copy feature of reflect library naturally support this use-cae.

@EbenezerJesuraj Can you clarify how something like DeepCopy helps in this situation? Why can you not just convert the original structure to CSV directly?

In graphics applications, geometric data are represented by mesh consisting of vertices, edges, faces, etc. Vertices have different attributes, edges can be directional, faces can be polygon-based. Clone/Copy/Duplicate such an object is not easy and requires many developers' effort. But with DeepCopy, one can easily create a copy of a given mesh. This is very helpful for geometry processing cases where caching cascaded meshes are convenient for different kinds of solvers.

@changkun I'm not familiar with such applications. For the kinds of structs you would be copying, do they typically have private fields you would also need to copy?

[EbenezerJesuraj]

@rittneje Hi, In a Business-grade application we may have to deal with a large set of applications dynamically at run-time. (Quick intro on Concurrency: Go's Concurrency of executing non-related independent tasks provides very good optimizations.)

We may have to develop different types of applications for different scenarios and one can avoid writing code manually / avoid re-inventing the wheel by developing a generic module like Deep Copy to store the data received from Clients to the Server. If this generic module supports conversion of any data input field (say in the form of struct to any tabular or any other feasible means of storage) then DeepCopy.reflect might look like an ideal choice in my opinion.

Say if we did a Shallow Copy we will have to use pointers and if they are not managed properly can bring a lot of mess into the code and might possibly expose business data to scammers and phishers, which is not preferrable.

[rittneje]

@changkun I don't understand your question. If an *os.File gets deep copied naively (meaning that all struct fields are deep copied recursively), you will end up with two logical files that are using the exact same file descriptor. This is extremely dangerous, because now the file descriptor can be closed through one logical file, while it is still being used through the copy.

Imagine I have a library that exposes a struct like this:

type Foo struct {
    privateField string
}

Your code might then leverage your proposed reflect.DeepCopy to make deep copies of Foos. And at first it will work. But now in a newer version of my library, I want to add an *os.File to it for one reason or another.

type Foo struct {
    privateField string
    someFile *os.File
}

Now your code is dangerously broken for the aforementioned reason.

If however I had a Copy (or Clone or whatever) method, then there is an actual contract, and the burden is on me as the library author to maintain it.

func (f *Foo) Copy() *Foo {
    return &Foo{
        privateField: f.privateField,
        // this method doesn't exist today, but you get the idea
        someFile: f.someFile.Copy(),
    }
}

Please note that *os.File is only meant to illustrate a specific example of where your proposed DeepCopy is a problem. The general issue is that you are trying to violate the principle of abstraction.

[changkun]

Sure in this case it is not safe to use after deep copy. As warned in the doc:

// Note that for stateful copied values, such as the lock status in
// sync.Mutex, or underlying file descriptors in os.File and net.Conn,
// are retained but may result in unexpected consequences in follow-up
// usage, the caller should clear these values depending on usage context.

But let's take one step back: I don't fully understand the motivation of your described use case too. How frequent for a struct will hold an unexported file? Or Is this really a good practice? When the file will be closed? Automatically using a finalizer? Ask the user to remember to call an exported Close method for closing the file? Either way seems to have a danger of not closing the file. If a package offers a customized copy method, should not using DeepCopy be considered as misuse in this package?

[rittneje]

Your doc comment shifts the burden to the wrong place. The whole point under discussion here is that some types cannot be naively copied, and a DeepCopy implementation should not copy them like that in the first place. Asking the client to do it isn't really a solution, especially because it may be some private field they don't have access to.

How frequent for a struct will hold an unexported file?

I obviously cannot give a concrete answer here, but it is entirely within a library author's jurisdiction to introduce such changes. And keep in mind that just because the struct has a reference to the file, does not mean that it is what created it. As a contrived example:

func NewFooWithFile(f *os.File) *Foo {
    return &Foo{someFile: f}
}

And as I mentioned, *os.File was just one example where this doesn't work. Mutexes are also a problem, and they are going to come up for any struct that requires thread-safety. But then you will have to hold the lock while doing the deep copy, which means that the copied struct will have its mutex locked. Then you have to somehow unlock the copy, which may not be feasible depending on the API of the library.

If a package offers a customized copy method, should not using DeepCopy be considered as misuse in this package?

Again, I don't see any purpose for DeepCopy to exist. It really just seems like a way to avoid writing a proper Copy method, but only for specific cases where the struct in question is a DTO. However, there is no way for a library author to actually agree to this contract. And without that, you are definitely in the realm of relying on implementation details of your library.

[changkun]

The whole point under discussion here is that some types cannot be naively copied

Of course, the discussion was about singleton that should not be naively copied. The counter argumentation was if this is the case, why should not DeepCopy be avoided in this case, and considered as a misuse of this API? I think your argument is about "as long as an API can't design in a way that its user can't blindly throw arguments in and get an intuitive response out, we shouldn't have it." There are enough examples against this, and we have discussed them before.

Again, I don't see any purpose for DeepCopy to exist. It really just seems like a way to avoid writing a proper Copy method, but only for specific cases where the struct in question is a DTO. However, there is no way for a library author to actually agree to this contract. And without that, you are definitely in the realm of relying on implementation details of your library.

Furthermore, even we have the concerning danger. One can still use a DeepCopy to save a lot of copying work without introducing any danger when offering customized Copy:

func (f *Foo) Copy() *Foo {
    var nf *Foo
    reflect.DeepCopy(nf, f)         // save a lot of work on dealing with field assignments
    nf.someFile = f.someFile.Copy() // clear this danger for the API user
    return nf
}

This is particularly helpful when Foo is a linked list, tree, graph, etc.

[rittneje]

What happens if Foo is itself part of another struct, say Bar?

type Bar struct {
  f *Foo
  ...
}

What does reflect.DeepCopy(*Bar) do here? Something needs to call f.Copy(). But earlier you felt that DeepCopy should not support custom behavior. So Foo has "tainted" Bar, meaning we now need something like:

func (b *Bar) Copy() *Bar {
    b2 := reflect.DeepCopy(b)
    b2.f = b2.f.Copy()
}

It very much seems like if reflect.DeepCopy is going to exist, it needs to respect the Copy method (or whatever we call it) somehow. Otherwise, you are going to have breaking changes. For instance, clients previously would have done reflect.DeepCopy(f). But now they'd have to switch to f.Copy, or their code is broken. However, if DeepCopy called the Copy method then it would just work.

I also still believe that if reflect.DeepEqual is going to exist it should ignore private fields by default, like json.Marshal does. If they should be copied along, you should implement a proper Copy method. But, as a convenience for simple cases, perhaps we can use struct tags to opt them in?

type Foo struct {
    someField string `deepcopy:"true"`
    someFile *os.File
}

func (f *Foo) Copy() *Foo {
   type foo Foo
   f2 := reflect.DeepCopy((*foo)(f))
   f2.someFile = f.someFile.Copy()
   return (*Foo)(f2)
}

[changkun]

[...] meaning we now need something like:

func (b *Bar) Copy() *Bar {
    b2 := reflect.DeepCopy(b)
    b2.f = b2.f.Copy()
}

This is not true because a DeepCopy already copied f. Respect a customized Copy or equivalent, is similar to what was rejected in #20444

I also still believe that if reflect.DeepEqual is going to exist it should ignore private fields by default, like json.Marshal does.

Then we don't need deep copy anyway, one can simply marshal and unmarshal.

If they should be copied along, you should implement a proper Copy method. But, as a convenience for simple cases, perhaps we can use struct tags to opt them in?

I personally object to this idea. No special reason, sorry. (Perhaps you could fill a separate proposal?)

[rittneje]

This is not true because a DeepCopy already copied f.

No, DeepCopy naively copied f, meaning that it naively copied f.someFile, which, as mentioned several times, is unacceptable. One way or another, something has to call f.Copy(). If DeepCopy doesn't, then it is useless, because in general a Copy method could be added at any time in the future, and so we are back to square one.

Respect a customized Copy or equivalent, is similar to what was rejected in #20444

That proposal isn't relevant. DeepCopy is more like json.Marshal/json.Unmarshal (which does support customization) than reflect.DeepEqual, especially given that you have already mentioned a few times that a deep copy won't be deep equal to the original value. In fact, it was argued against specifically because of the implication with unexported fields, but your original proposal violates this anyway.

I will also mention that personally, I've never had any use for reflect.DeepEqual, not just because of its lack of customization, but also because it lacks any mechanism to tell you what the discrepancy was. Honestly, I think adding it to the standard library was a mistake. I don't consider it a good yardstick to measure any new API against.

Then we don't need deep copy anyway, one can simply marshal and unmarshal.

You can, except that it will be significantly less efficient and cannot handle circular data structures.

Anyway, I feel like we're just cycling through the same points again and again, so I will leave it with this:

1. I am opposed to this API being in the standard library in its present form. I feel it will only encourage fragile code that is reliant upon implementation details that are subject to change. I am also opposed any reliance on the unsafe package (to copy unexported fields) that doesn't require the client to explicitly import unsafe.
2. To me, methods are always better than reflect magic, because there is an explicit contract that the library author has agreed to uphold.
3. There are several situations in which naively deep copying is wrong or ill-defined: *os.File, *sync.Mutex, *sync.Once, <-chan X, etc. Without the ability to customize DeepCopy, introducing any such fields can become a breaking change, even if the library author never intended the struct in question to be deep copied.

[changkun]

Anyway, I feel like we're just cycling through the same points again and again, so I will leave it with this:

I agree. Until now, all of us are simply repeating the same topic repeatedly. All arguments have been repeated too many times, and yet no new insights. I'll stop arguing if there are no more new interesting argumentations:

1. I am opposed to this API being in the standard library in its present form. I feel it will only encourage fragile code that is reliant upon implementation details that are subject to change. I am also opposed any reliance on the unsafe package (to copy unexported fields) that doesn't require the client to explicitly import unsafe.

You are requesting feature extensions that had been constantly rejected from the DeepEqual perspective. Referring to the third point of #51520 (comment)

2. To me, methods are always better than reflect magic, because there is an explicit contract that the library author has agreed to uphold.

This is a purely subjective opinion. If that's the case, the reflect package should not exist.

3. There are several situations in which naively deep copying is wrong or ill-defined: *os.File, *sync.Mutex, *sync.Once, <-chan X, etc. Without the ability to customize DeepCopy, introducing any such fields can become a breaking change, even if the library author never intended the struct in question to be deep copied.

This is once again the first point. Referring to the updated proposal a few weeks ago:

// There are a few exceptions that may result in a deeply copied value not
// deeply equal (asserted by DeepEqual(dst, src)) to the source value:
//
// 1) Func values are still refer to the same function
// 2) Chan values are replaced by newly created channels
// 3) One-way Chan values (receive or read-only) values are still refer
//    to the same channel
//
// Note that for stateful copied values, such as the lock status in
// sync.Mutex, or underlying file descriptors in os.File and net.Conn,
// are retained but may result in unexpected consequences in follow-up
// usage, the caller should clear these values depending on usage context.
//
// The function panics/returns with an error if
//
// 1) source and destination values have different types
// 2) destination value is reachable from source value

If not enough, this could be:

// Note that while correct uses of DeepCopy do exist, they are not rare,
// but the use of DeepCopy is often indicating the copying object does not
// contain a singleton that should not be copied.

which holds similar functionality to what was documented in Mutex.TryLock.

[changkun]

For anyone who would like to try out this proposal, or argue against no actual implementation yet, I quickly prototyped an implementation here:
https://pkg.go.dev/golang.design/x/reflect

After the implementation, I think this is the latest proposal:

// DeepCopy copies src to dst recursively.
//
// Two values of identical type are deeply copied if one of the following
// cases apply.
//
// Numbers, bools, strings are deeply copied and have different underlying
// memory address.
//
// Slice and Array values are deeply copied, including its elements.
//
// Map values are deeply copied for all of its key and corresponding
// values.
//
// Pointer values are deeply copied for their pointed value, and the
// pointer points to the deeply copied value.
//
// Struct values are deeply copied for all fields, including exported
// and unexported.
//
// Interface values are deeply copied if the underlying type can be
// deeply copied.
//
// There are a few exceptions that may result in a deeply copied value not
// deeply equal (asserted by DeepEqual(dst, src)) to the source value:
//
// 1) Func values are still refer to the same function
// 2) Chan values are replaced by newly created channels
// 3) One-way Chan values (receive or read-only) values are still refer
//    to the same channel
//
// Note that while correct uses of DeepCopy do exist, they are not rare.
// The use of DeepCopy often indicates the copying object does not contain
// a singleton or is never meant to be copied, such as sync.Mutex, os.File,
// net.Conn, js.Value, etc. In these cases, the copied value retains the
// memory representations of the source value but may result in unexpected
// consequences in follow-up usage, the caller should clear these values
// depending on their usage context.
func DeepCopy[T any](src T) (dst T)

Thanks to everyone for previous clarifying discussions.

---

PS. There was a large discussion on customization. A branched prototype implements a possible proposal (although this would be a different proposal):

// DeepCopyOption represents an option to customize deep copied results.
type DeepCopyOption func(opt *copyConfig)

// DisallowCopyUnexported returns a DeepCopyOption that disables the behavior
// of copying unexported fields.
func DisallowCopyUnexported() DeepCopyOption

// DisallowCopyCircular returns a DeepCopyOption that disables the behavior
// of copying circular structures.
func DisallowCopyCircular() DeepCopyOption

// DisallowCopyBidirectionalChan returns a DeepCopyOption that disables
// the behavior of producing new channel when a bidirectional channel is copied.
func DisallowCopyBidirectionalChan() DeepCopyOption

// DisallowTypes returns a DeepCopyOption that disallows copying any types
// that are in given values.
func DisallowTypes(val ...any) DeepCopyOption

// DeepCopy copies src to dst recursively.
//
// (...)
//
// To change these predefined behaviors, use provided DeepCopyOption.
func DeepCopy[T any](src T, opts ...DeepCopyOption) (dst T)

[rsc]

It doesn't sound like there is any consensus about adding this, which means we should probably decline this proposal.

[rsc]

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

[changkun]

It doesn't sound like there is any consensus about adding this

Frankly, it is not convincing and constructive enough to reject a proposal because there is no consensus in a discussion. The argument would be much stronger for technical reasons, such as language spec violation, implementation difficulty, etc. If there is a decision difficulty, it might be a reasonably well-balanced decision to put this on hold for a few years rather than close it right away. In this way, the issue can collect more opinions regarding the proposal.

[mvdan]

@changkun respectfully, I disagree that keeping this proposal open will help. If you want to continue the flow of ideas, I would once again suggest that you start a third-party package and use the issue tracker or discussion forum there. It's worked very well for go-cmp for example, which culminated into #45200 after years of effort and validation from real users.

I would also like to give my two cents from the point of view of someone who has been quietly reading this thread: I'm getting the impression that you are defending this proposal rather than trying to see what the best outcome for Go would be, even if reaching that best outcome takes more experimentation outside of this repository. I understand that spending time on a proposal and having it rejected can be underwhelming, but this is part of what sets Go apart from other languages: it has kept the language and standard library small by rejecting many ideas.

[changkun]

I would once again suggest that you start a third-party package and use the issue tracker or discussion forum there. It's worked very well for go-cmp for example, which culminated into #45200 after years of effort and validation from real users.

I am not really objecting to this idea and have already placed the prototyped implementation in a third location. However, on behalf of the purpose of this proposal, I remained a defending position in this thread.

The "go-cmp" is another special case where the home organization account is google, which is more attractive to the public hence (I think) nicely yielding a successful example. The situation becomes different when such a location is fully a third party, which creates less impactful attraction and is considered much more untrustable. Based on my (naive) observation, I think the levels of trust and reliability decrease, from the standard package to golang.org/x, top-grade organizations, and then individual sources. Therefore it might be more helpful to gain more exposure when the association maintains.

As (the majority, I think) developers love to gain their individual impact, and sometimes might even be more frustrating for them when their idea is rejected from somewhere, achieved success in a third place, then being "stolen" (emphasize: this is not the optimal wording, but sometimes being abused under this context) to the place where originally said no. This may be considered harmful for both parties at extreme cases.

I would also like to give my two cents from the point of view of someone who has been quietly reading this thread: I'm getting the impression that you are defending this proposal rather than trying to see what the best outcome for Go would be, even if reaching that best outcome takes more experimentation outside of this repository. I understand that spending time on a proposal and having it rejected can be underwhelming, but this is part of what sets Go apart from other languages: it has kept the language and standard library small by rejecting many ideas.

Now, this is even more off-topic. Your encouraging words are quite appreciated in a community discussion. I think I am simply taking this chance to explore and exploit the current decision tendency when it comes to a borderline proposal. Specifically, when we (potentially) reach the Pareto optimality of a proposal, it is interesting to see the factors that determine a decision choice between all found Pareto frontiers. Compared to a controversial arena proposal, the latest comment was to add it under an experiment flag other than a similar decision of "likely decline." While I agree they might not be a completely fair comparison, this still reveals more observed potentially contradicting behaviors. In terms of this proposal, I'd say there are no actual hard feelings but simply observing a choice after an explorative discussion.

Subsequently, this made me an idea of having an official field experiment location might be interesting, where every controversial or suboptimal proposal could encourage people to contribute to the implementation. That place has no guarantee of any kind. Nevertheless, I'll stop here as it is completely not the topic of this proposal discussion.

[mvdan]

The "go-cmp" is another special case where the home organization account is google

Okay, so perhaps more examples would help. Take https://github.com/frankban/quicktest, which led to accepted proposals like #41260 and #32111.

While I agree they might not be a completely fair comparison

It really is comparing apples and oranges. It is literally impossible to implement a runtime change like arenas outside of the Go source tree.

It is of course within your right to stick to your arguments - my argument was that you would have a better chance of success if you emulate success stories like go-cmp or quicktest. It takes a lot more patience and effort to do it that way, but that's what it takes to get APIs right and write good proposals with detail and user experience.

[changkun]

Okay, so perhaps more examples would help. Take https://github.com/frankban/quicktest, which led to accepted proposals like #41260 and #32111.

Agree. On behalf of the proposal, I think we could continue arguing this, such as those examples exist, but are still rare compared to the declined ones, etc. :)

It really is comparing apples and oranges. It is literally impossible to implement a runtime change like arenas outside of the Go source tree.

This part I am not sure about. It might be more suitable to say comparing ants and elephants. From the discussion there, it seems it remains possible to implement the arena in a third package.

It is of course within your right to stick to your arguments - my argument was that you would have a better chance of success if you emulate success stories like go-cmp or quicktest. It takes a lot more patience and effort to do it that way, but that's what it takes to get APIs right and write good proposals with detail and user experience.

I do not object to this argument at all. That's why the proposal summarizes many existing practices from pkg.go.dev.

[atdiar]

I don't think I will be able to convince you, because you seem resolute that it is a good idea to have eventually such a function in the standard library but I still see problems.
Any type whose constructor function is side-effectful cannot be safely deep-copied because it would break the semantics/invariants.

But you don't know at which depth such types may appear in a datastructure. Especially if these are unexported types that appear in unexported fields.

So even if deep-copying is possible it might not be a good idea in the general case to do it indiscriminately. Now in the few cases where it is desirable, it shouldn't be difficult to write the copying function by hand a priori.

Note that comparing values does not suffer from this because a comparison is not effectful in general. Which is why having deepEqual is less a problem.

I think that warrants caution and while I appreciate that you mentioned a few edge cases in the documentation, I think the issue can be more pervasive and seem underestimated.

[EbenezerJesuraj]

Hi, Having a function such as Deep - Copy which is again shared by many popular languages such as Python,JAVA in itself is a very good valid argument to have the feature.

In my opinion i don't see any drawbacks in language having such a feature in a standard library.

The issue as of now in my opinion seems to be more of a developer shortage one to implement such a feature in the library. But again the Benefits far outweigh the cons is what i am able to infer from here.

[EbenezerJesuraj]

Okay, so perhaps more examples would help. Take https://github.com/frankban/quicktest, which led to accepted proposals like #41260 and #32111.

Agree. On behalf of the proposal, I think we could continue arguing this, such as those examples exist, but are still rare compared to the declined ones, etc. :)

It really is comparing apples and oranges. It is literally impossible to implement a runtime change like arenas outside of the Go source tree.

This part I am not sure about. It might be more suitable to say comparing ants and elephants. From the discussion there, it seems it remains possible to implement the arena in a third package.

It is of course within your right to stick to your arguments - my argument was that you would have a better chance of success if you emulate success stories like go-cmp or quicktest. It takes a lot more patience and effort to do it that way, but that's what it takes to get APIs right and write good proposals with detail and user experience.

I do not object to this argument at all. That's why the proposal summarizes many existing practices from pkg.go.dev.

I do like the Apples and Oranges references here..

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group