code review 5372095: allow copy of struct containing unexported fields

allow copy of struct containing unexported fields

An experiment: allow structs to be copied even if they
contain unexported fields.  This gives packages the
ability to return opaque values in their APIs, like reflect
does for reflect.Value but without the kludgy hacks reflect
resorts to.

In general, we trust programmers not to do silly things
like *x = *y on a package's struct pointers, just as we trust
programmers not to do unicode.Letter = unicode.Digit,
but packages that want a harder guarantee can introduce
an extra level of indirection, like in the changes to os.File
in this CL or by using an interface type.

All in one CL so that it can be rolled back more easily if
we decide this is a bad idea.

Originally discussed in March 2011.
https://groups.google.com/group/golang-dev/t/3f5d30938c7c45ef

[rsc, 2011-11-14 06:11:45]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[adg, 2011-11-14 06:22:14]

LGTM

I'm pretty happy with how much this removes from the spec, compiler, and tests.
Looking forward to seeing what effect, if any, this has on client code.

http://codereview.appspot.com/5372095/diff/2003/src/pkg/os/file_plan9.go
File src/pkg/os/file_plan9.go (right):

http://codereview.appspot.com/5372095/diff/2003/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:13: type File {
Missing 'struct'

[dvyukov, 2011-11-14 06:26:15]

Additional level of indirection enforces sharing rather than prevents copying. I
am worry about structs like Mutexes. Copying is almost always the wrong thing
for them. Moreover what if it gets copied in a locked state? Sharing and dynamic
allocation is both expensive and does not solve the problem. Is it just becomes
a users problem?
In C++ I can prevent copying of mutexes, moreover I still can return them by
value thanks to move constructors.

[rsc, 2011-11-14 06:33:52]

On Mon, Nov 14, 2011 at 01:26,  <dvyukov@google.com> wrote:
> Additional level of indirection enforces sharing rather than prevents
> copying. I am worry about structs like Mutexes. Copying is almost always
> the wrong thing for them. Moreover what if it gets copied in a locked
> state? Sharing and dynamic allocation is both expensive and does not
> solve the problem. Is it just becomes a users problem?

Generally, if an API is returning pointers (like *os.File)
you should continue to use those pointers instead of making
copies.

It is true that we are giving up a capability here (prevent copying)
but we are gaining a capability too (allow APIs with opaque values).
We have a growing list of cases where the latter would be useful,
and not too many examples where the former is critical.

It is probably worth noting in the sync docs that mutexes
should not be copied.  I will add that to this CL.

> In C++ I can prevent copying of mutexes, moreover I still can return
> them by value thanks to move constructors.

It is true: C++ can prevent many things.

Russ

[dvyukov, 2011-11-14 06:39:04]

On 2011/11/14 06:33:52, rsc wrote:
> On Mon, Nov 14, 2011 at 01:26,  <mailto:dvyukov@google.com> wrote:
> > Additional level of indirection enforces sharing rather than prevents
> > copying. I am worry about structs like Mutexes. Copying is almost always
> > the wrong thing for them. Moreover what if it gets copied in a locked
> > state? Sharing and dynamic allocation is both expensive and does not
> > solve the problem. Is it just becomes a users problem?
> 
> Generally, if an API is returning pointers (like *os.File)
> you should continue to use those pointers instead of making
> copies.
> 
> It is true that we are giving up a capability here (prevent copying)
> but we are gaining a capability too (allow APIs with opaque values).
> We have a growing list of cases where the latter would be useful,
> and not too many examples where the former is critical.
> 
> It is probably worth noting in the sync docs that mutexes
> should not be copied.  I will add that to this CL.

OK, I see.
Yes, I think we should document what sync objects are copyable and what are not
(WaitGroup, Once, etc). If an object with a non-trivial multi-threaded semantics
becomes copyable we should document how copying interacts with muti-threading as
well.

[r, 2011-11-14 16:20:09]

LGTM

i think

http://codereview.appspot.com/5372095/diff/2018/doc/go_spec.html
File doc/go_spec.html (left):

http://codereview.appspot.com/5372095/diff/2018/doc/go_spec.html#oldcode1377
doc/go_spec.html:1377: </p>
this is good, but is there another place where it needs to be explicitly
allowed? that is, is there another sentence that implies this, with less
precision?

[rsc, 2011-11-14 16:23:17]

On Mon, Nov 14, 2011 at 11:20,  <r@golang.org> wrote:
> this is good, but is there another place where it needs to be explicitly
> allowed? that is, is there another sentence that implies this, with less
> precision?

I don't believe there is.  Atom symbol pointed out back
in March that it's not implied by any of the rest of the spec,
which is what got me thinking about it.

Russ

[bradfitz, 2011-11-14 16:30:18]

On Sun, Nov 13, 2011 at 10:33 PM, Russ Cox <rsc@golang.org> wrote:

> On Mon, Nov 14, 2011 at 01:26,  <dvyukov@google.com> wrote:
> > Additional level of indirection enforces sharing rather than prevents
> > copying. I am worry about structs like Mutexes. Copying is almost always
> > the wrong thing for them. Moreover what if it gets copied in a locked
> > state? Sharing and dynamic allocation is both expensive and does not
> > solve the problem. Is it just becomes a users problem?
>
> Generally, if an API is returning pointers (like *os.File)
> you should continue to use those pointers instead of making
> copies.
>
> It is true that we are giving up a capability here (prevent copying)
> but we are gaining a capability too (allow APIs with opaque values).
> We have a growing list of cases where the latter would be useful,
> and not too many examples where the former is critical.
>
> It is probably worth noting in the sync docs that mutexes
> should not be copied.  I will add that to this CL.
>

could promote struct tag's convention to be a language thing...

type Mutex struct {
        state int32 `go:"nocopy"`
        sema  uint32
}

(mostly joking. nocopy doesn't really apply to a field, but rather the
whole struct, anyway.)

[jnml, 2011-11-14 17:35:07]

On Monday, November 14, 2011 7:11:45 AM UTC+1, rsc wrote:
>
> In general, we trust programmers not to do silly things
> like *x = *y on a package's struct pointers, just as we trust
> programmers not to do unicode.Letter = unicode.Digit,
>
Considering how many silly questions/attempts to do incredibly wrong things 
are routinely discussed at golang-nuts - I'm a bit skeptical about such 
optimism. It will give more power to responsible programmers while 
increasing the potential to bite the others badly. I'm not able to estimate 
the ratio of those two populations.

>

[rsc, 2011-11-14 17:45:26]

On Mon, Nov 14, 2011 at 12:33, Jan Mercl <jan.mercl@nic.cz> wrote:
> Considering how many silly questions/attempts to do incredibly wrong things
> are routinely discussed at golang-nuts - I'm a bit skeptical about such
> optimism. It will give more power to responsible programmers while
> increasing the potential to bite the others badly. I'm not able to estimate
> the ratio of those two populations.

This CL is not about giving power to responsible programmers.
It is about letting packages create a new, useful kind of API:
something returning an opaque struct value.

A side effect is that programmers can write a couple
more bad programs.

Russ

[gri, 2011-11-14 17:53:46]

LGTM

[rsc, 2011-11-15 17:21:03]

*** Submitted as http://code.google.com/p/go/source/detail?r=6f8b9654e929 ***

allow copy of struct containing unexported fields

An experiment: allow structs to be copied even if they
contain unexported fields.  This gives packages the
ability to return opaque values in their APIs, like reflect
does for reflect.Value but without the kludgy hacks reflect
resorts to.

In general, we trust programmers not to do silly things
like *x = *y on a package's struct pointers, just as we trust
programmers not to do unicode.Letter = unicode.Digit,
but packages that want a harder guarantee can introduce
an extra level of indirection, like in the changes to os.File
in this CL or by using an interface type.

All in one CL so that it can be rolled back more easily if
we decide this is a bad idea.

Originally discussed in March 2011.
https://groups.google.com/group/golang-dev/t/3f5d30938c7c45ef

R=golang-dev, adg, dvyukov, r, bradfitz, jan.mercl, gri
CC=golang-dev
http://codereview.appspot.com/5372095