Skip to content

proposal: add a builtin function for making an accurate copy of any value #18371

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

Closed
feyeleanor opened this issue Dec 19, 2016 · 15 comments
Closed

Comments

@feyeleanor
Copy link

Problem

Currently there seems to be no general method for making a shallow but complete copy of an existing value of arbitrary type in a Go program. This is particularly true when a value is a struct with unexported fields.

Even using reflection to perform a direct binary copy is a minefield of segfaults, unaddressable values and faked []byte using reflection which makes such code difficult to reason about.

Proposal

There are cases where making a full and accurate copy of an existing value - even a struct value with unexported fields - is a perfectly reasonable thing to do so I propose two possible additions to Go which would support this without compromising the key premise of unexported fields: that they cannot be modified except by functions and methods in the package where they're defined.

(Making an exact copy is not conceptually the same as changing the field contents, the problem is an artefact of Go's reflection system being implemented as a package rather than with some other API).

A new builtin function with signature duplicate(v Type) Type would return a complete and faithful shallow copy of any value. At the byte level it would be binary identical to the value it's created from and all the usual rules would apply to it, so unexported fields would still only be changeable by functions defined in its source package.

Usage

A snippet of how code would look using this function:

a := struct{ x, y int } { 3, 4 }
b := duplicate(x)
fmt.Printf("a.x: %v, a.y: %v\n", a.x, a.y)
fmt.Printf("b.x: %v, b.y: %v\n", b.x, b.y)

in both cases fmt.Printf() will print out the same values for the x and y fields.

Problems

In the case of reference types such as maps, slices, interfaces, pointers, channels, etc. a decision would have to be made as to whether to make a copy of the reference (slice header, etc.) or a copy of the underlying values.

  • The former is inexpensive but possibly too shallow;
  • the latter may not make sense for all reference types;
  • perhaps reference types shouldn't duplicate at all;
  • perhaps duplicate() is dangerous enough to be part of unsafe.
@minux
Copy link
Member

minux commented Dec 19, 2016 via email

@minux minux added the Proposal label Dec 19, 2016
@bradfitz
Copy link
Contributor

Even using reflection to perform a direct binary copy is a minefield of segfaults, unaddressable values and faked []byte using reflection which makes such code difficult to reason about.

So, put it in a library, rather than asking each user to do it by hand? Actually, it exists already: https://godoc.org/?q=deepcopy

Why does it need to be part of the language?

And if it were part of the language, I'm sure it would surprise people to learn that a function named duplicate only makes a shallow and not a deep clone of its argument.

@minux
Copy link
Member

minux commented Dec 19, 2016 via email

@rogpeppe
Copy link
Contributor

As others have already pointed out, making an shallow copy of a type in Go is already straightforward.

Implementing a general deep copy is problematic. What should
happen when there's a diamond dependency? For the general case, if two
objects both point to the same object, they should continue to do so after copying.
But this is hard to do efficiently. Here's an example:

type A struct {
     SliceV []A
     SliceP []*A
 }
 x := A{SliceV: make([]A, 3)}
 y := A{SliceV: x.SliceV[1:]}
 z = A{SliceV: make([]A, 2)}
 x.SliceP = []*A{&x.SliceV[0], &x.SliceV[1], &z.SliceV[0]}
 y.SliceP = []*A{&x.SliceV[1]}
 w := deepcopy(&y)

If we were to copy this correctly, then all the slice and pointer references would
be maintained. This requires quite a bit of information to be retained
when copying. I had a go at doing this (https://godoc.org/github.com/rogpeppe/rog-go/exp/deepcopy) ages ago, but the code no longer compiles and I've never
come across a case where this general functionality is actually needed.

The only time I've actually needed a general deep copy, json.Marshal
followed by json.Unmarshal was sufficient.

func deepCopy(x interface{}) (interface{}, error) {
	data, err := json.Marshal(x)
	if err != nil {
		return nil, err
	}
	destv := reflect.New(reflect.TypeOf(x))
	err = json.Unmarshal(data, destv.Interface())
	if err != nil {
		return nil, err
	}
	return destv.Elem().Interface(), nil
}

It would be useful to have some specific cases where you see a more
general version of DeepCopy being useful.

But still, I think needing to do deep copy on a value suggests
an API design problem.

I tend to agree with this.

@feyeleanor
Copy link
Author

This is not a proposal for a DeepCopy mechanism.

This is a method for making an exact duplicate of a value, including its unexported fields.

@minux
Copy link
Member

minux commented Dec 19, 2016 via email

@bradfitz
Copy link
Contributor

And even if you need to use reflect, it's just:

func duplicate(v interface{}) interface{} {
	rv := reflect.New(reflect.TypeOf(v)).Elem()
	rv.Set(reflect.ValueOf(v))
	return rv.Interface()
}

@feyeleanor
Copy link
Author

Not if it has unexported fields.

@cespare
Copy link
Contributor

cespare commented Dec 20, 2016

@feyeleanor minux's first reply shows that assignment does indeed copy the unexported struct fields.

@minux
Copy link
Member

minux commented Dec 20, 2016 via email

@bradfitz
Copy link
Contributor

I'm going to close this, since it's unclear what benefit there is having this more built-in than it already is. Feel free to reopen if you have a more formal proposal.

@feyeleanor
Copy link
Author

feyeleanor commented Dec 20, 2016

In which case I've misunderstood the documentation for reflect.Value.Set() and reflect.Value.CanSet().

If @bradfitz's code is robust in all circumstances (in addition to @minux's example I've tried it with an unexported struct type containing unexported fields from one of my own packages & that works) then it would still make sense to either have a convenience method on reflect.Value:

func Value.Duplicate() Value {
	rv := New(TypeOf(v)).Elem()
	rv.Set(ValueOf(v))
	return rv
}

or to include an example in the documentation for reflect.

@bradfitz
Copy link
Contributor

https://blog.golang.org/laws-of-reflection is worth reading.

@feyeleanor
Copy link
Author

Unfortunately Laws of Reflection doesn't cover this and the Third Law coverage which is relevant is obscured by using an example where addressability is the basis for settability.

An example in the reflect documentation would leave no room for misunderstanding.

@rogpeppe
Copy link
Contributor

@feyeleanor Assuming you actually mean this:

func (v Value) Duplicate() Value {
        rv := New(v.Type())
        rv.Set(v)
        return rv.Elem()
}

but even if this trivial function was provided, what does this
give you that v.Interface() does not?

The documentation of Value.Set and Value.CanSet seems right
to me. "A Value can be changed only if it is addressable and was not obtained by the use of unexported struct fields." This doesn't say anything about the fields inside the
value. "x's value must be assignable to v's type." This implies the usual Go
assignment rules which do not prohibit assignment of unexported fields
as part of a larger value.

BTW the description of this issue says "add a builtin function for making an accurate copy of any value". None of the reflect package functions are built in. The builtin "function" for making an accurate copy of any value is the assignment operator.

@golang golang locked and limited conversation to collaborators Dec 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants