runtime: add NoCopy documentation struct type?

[bradfitz]

From a conversation with Alan Donovan about how we might detect accidental copies of
types that shouldn't be copyable, I proposed:

package runtime  // or wherever

// NoCopy is an annotation that the containing type shouldn't be copied
// by value. It has zero size so may be embedded freely.
type NoCopy struct{}

Then we could do govet checks.

Or even compiler checks (more controversial probably).

Then sync.Cond and sync.Mutex could use it. The latter has no checks, because
sync.Cond's runtime copyChecker takes up space.

Only tagging this Go1.4 for eventual discussion.

[adonovan]

Comment 1:

For the record, I like this idea.  I recently came across the need for it
while working on
https://code.google.com/p/go/source/browse/container/intsets/sparse.go?repo=tools.
 One downside of govet-time checking is that you only run it when
you've
found most of the bugs, so there's still a strong motivation to add a
dynamic check (as sync.Cond and intsets.Sparse do).

[ianlancetaylor]

Comment 2:

For vet purposes, we could also consider using (yet another) magic comment.
I'm curious as to how often this arises.  At first thought it seems that it can only
arise on a small struct that we expect people to embed directly rather than via a
pointer.  I'm fine with adding something to vet, but I wonder whether this is really
common enough to merit a language change.

Labels changed: added repo-main.

[rsc]

Comment 3:

Too late.

Labels changed: added release-go1.5, removed release-go1.4.

[rsc]

Too late (again) for this cycle. If this is going to happen, it needs an owner who will push on getting it discussed and done during the main cycle.

I do think this might be worth doing, with the vet check. (Separately, I want to make vet a bit more central to the dev process for 1.6 somehow. Rob filed a bug for that, but I don't have the number handy. If that happened, the main objection to vet goes away.)

Not sure about the name. For what it's worth, DISALLOW_EVIL_CONSTRUCTORS seems to be available again. :-)

[minux]

The vet issue is probably #11041.

[OneOfOne]

Since the cycle just started.

I like the type NoCopy struct{} idea or a yucky-yet-helpful //go:nocopy magic comment.

Go is a safe language, so having that check at compile time would rather be nice and will protect people from losing hair during late night coding sessions.

[bradfitz]

Indeed.

[Merovius]

I think the comment should be preferred - adding even a zero-size field to a struct might have unintended consequences so would require people who use this to know/think about it or suffer potential semantic changes.

[bradfitz]

Latest from @rsc:

I don't really want people importing runtime to get at this, nor do I want to establish a convention that package runtime is where you put magic types like this. This isn't really about runtime at all. It's about a pseudo-compiler check.

We established a convention for special comments: "//who:something", where who says who the comment is for. We use //go: for the Go compiler. Following that convention seems like it would suggest "//vet:nocopy" as a special comment. Or if for some reason vet is unable to use comments (possible but unfortunate if so) then maybe use any type named 'vetNoCopy' in any package. Either way, please leave it out of runtime (and the whole standard library).

[robpike]

I don't believe a comment does the job. You put the comment into foo and import it, but the importer won't see it.

While acknowledging the problem, I encourage not rushing into a resolution.

[dsymonds]

I think a comment could work, if it is in the right place. The parser already picks up comments, and the typechecker can help with propagating a bit.

[robpike]

It needs to be in the export data, and I don't see that happening.

[dsymonds]

I thought the typechecker required access to the source rather than the .a files, but I could be wrong. If so, yes, you're right.

[kostya-sh]

How about using field tags:

dummyField struct{} `vet:"nocopy"`

?

Another idea is to support tags for types in Go. Though it is much bigger change.

[dsymonds]

Struct tags are a nice idea. They survive in the export data. They can be put on any unexported field and so won't even affect the godoc output.

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

[ThisGuyCodes]

The "put on any field to affect the parent" feels icky, but it certainly gets the job done. Putting the tag on the actual object you don't want copied seems cleaner to me, though it may affect godoc output (which perhaps could be changed to simply display "this should not be copied"?)

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

[mdempsky]

@conslo Go (at least currently) only supports tags on struct fields. Your suggestion would require a language extension.

[ThisGuyCodes]

@mdempsky right, sorry. Don't know where my head was at there

[minux]

What about using tags on a zero-sized blank field for vet hints?

type Mutex struct {
_ struct{} vet:"nocopy"
state int32
sema uint32
}

It will be included into the export data, so vet will have no
trouble find such hints. And it will not affect the size of the
type, and we can make godoc recognize such patterns,
and generate appropriate documentation.

[bradfitz]

@minux, sounds perfect. And thanks to @kostya-sh for mentioning struct tags.

[rogpeppe]

govet is fine, but I wonder if a stronger guarantee, always checked by the compiler, might be better.

How about this?

The compiler provides a predefined zero-size type, say "nocopy" or "static".
No value with this type may be copied.

To mark a structure so that it can't be copied, just embed
it in the struct:

type Mutex struct {
    nocopy
    state int32
    sema  uint32
}

Note that there's no way to initialize a nocopy
member because we can't copy a value into it, so
this means that such structs must be initialised with a composite literal
with keyed fields so that the nocopy member can be omitted,
but keyed literals are recommended practice anyway
so this shouldn't be a great obstacle.

[davecheney]

I like this suggestion.

On Thu, 25 Feb 2016, 20:55 Roger Peppe notifications@github.com wrote:

govet is fine, but I wonder if a stronger guarantee, always checked by the
compiler, might be better.

How about this?

The compiler provides a predefined zero-size type, say "nocopy" or
"static".
No value with this type may be copied.

To mark a structure so that it can't be copied, just embed
it in the struct:

type Mutex struct {
nocopy
state int32
sema uint32
}

This means that such structs must be initialised with a composite literal
with keyed fields, but that's recommended practice anyway.

—
Reply to this email directly or view it on GitHub
#8005 (comment).

[adg]

What about using tags on a zero-sized blank field for vet hints?

The tag needn't be on a blank field. It could just be on any field in the struct.

[stemar94]

Will vet be able to do these checks then also on composite types, i.e. having

type Mutex struct {
    state int32 `vet:"nocopy"`
    sema  uint32
}
type RWMutex struct {
    w           Mutex
    writerSem   uint32
    readerSem   uint32
    readerCount int32
    readerWait  int32
}

Will a copy of a RWMutex be detected easily as well, without a tag of its own?

[adg]

Yes.

On 25 February 2016 at 21:58, stemar94 notifications@github.com wrote:

Will vet be able to do these checks then also on composite types, i.e.
having

type Mutex struct {
state int32 vet:"nocopy"
sema uint32
}type RWMutex struct {
w Mutex
writerSem uint32
readerSem uint32
readerCount int32
readerWait int32
}

Will a copy of an RWMutex be detected as well without an tag of its own?

—
Reply to this email directly or view it on GitHub
#8005 (comment).

[josharian]

Vet can do this right now. Add a Lock method to your type and the copylocks check should fire. So folks wanting to experiment can do so immediately. Which is to say that we already have a mechanism for this--it's a method rather than a comment, a struct field, etc.

Of the new mechanisms proposed, a comment seems the least invasive. (struct{} fields are not no-ops, despite appearances to the contrary, nor are struct tags.) If the problem with comments is that they don't make it into the export data, perhaps we could add directive-y comments into the export data.

I still don't quite understand the case for this check being in the compiler instead of vet or for a language change. There are lots of unsafe things one can do with the language, and vet, the race detector, and other tools help on that front. I don't see why this is different.

[josharian]

Also, a minor advantage to methods and comments for this is that they can be applied to any type, not just structs, for example, an int32 used with atomics.

[rogpeppe]

I still don't quite understand the case for this check being in the compiler instead of vet or for a language change. There are lots of unsafe things one can do with the language, and vet, the race detector, and other tools help on that front. I don't see why this is different.

I agree that there are unsafe things that one can do with the language, but they're almost all concurrency-related and hard for the compiler to check. The things that go vet checks are mostly things that are unwise rather than actually unsafe.

I think there's a definite advantage to having a single, definitive and always-checked way to say that a given struct must not be copied. It's a check that can be made easily, and if the runtime is aware of it, the check can reach places that it's hard for go vet to check. For example, go vet doesn't print an error on this program, even though it copies a mutex. With compiler and runtime support, the reflect package could easily make that check at runtime.

I've used minux's anonymous empty type trick before and it's useful, but for such a core language thing I'm afraid it looks quite ugly to me.

[josharian]

That program is interesting; there are two things going on.

(1) t gets stuffed into an interface{}, which generates a copy of t. Vet could (and probably should) be taught about that. For stuff like that, if the compiler can do it, vet can, and vice versa.

(2) t gets hidden inside a reflect.Value. Neither vet nor the compiler can help 100% with that, although vet could issue a warning and the compiler can't (because the compiler doesn't do warnings and doesn't do false positives). In order to catch that in the general case, I think we'd need runtime support as well as compiler support; that's a bigger change.

[rogpeppe]

(1) t gets stuffed into an interface{}, which generates a copy of t. Vet could (and probably should) be taught about that. For stuff like that, if the compiler can do it, vet can, and vice versa.

I think you might have overlooked the fact that that program puts a pointer to T into the interface, not T itself. That's perfectly OK - just as it's perfectly fine to reflect on any pointer to struct containing a mutex. So there's nothing wrong for vet to see there.

2. t gets hidden inside a reflect.Value. Neither vet nor the compiler can help 100% with that, although vet could issue a warning and the compiler can't (because the compiler doesn't do warnings and doesn't do false positives). In order to catch that in the general case, I think we'd need runtime support as well as compiler support; that's a bigger change.

Again, it's fine for *T to be put inside a reflect.Value. It's even fine to call Elem on the resulting reflect.Value, because that doesn't copy the element. It's not fine to call Interface on that though, and that's where that the reflect package could panic rather than allow.

If the compiler kept track of nocopy, reflect.rtype and reflect.Value could keep track of the nocopy status of a value (in the flag field) and panic when the value is copied when it should not be.

Putting the nocopy element in a struct also makes for a convenient way of automatically documenting that a type must not be copied - godoc already prints "// contains filtered or unexported fields" when there's an unexported field. It could also print "// This value may not be copied", which makes sense if nocopy is something defined in the language, but not so much if it just looks like an annotation for govet.

[minux]

Josh, you mentioned "struct{} fields are not no-ops, despite appearances to the contrary, nor are struct tags". Could you please elaborate? They are not no-ops, but they shouldn't affect the runtime performance or memory usage. (It does add reflect data and if the empty struct is the last field, it might make the structure larger.)

[josharian]

I think you might have overlooked the fact that that program puts a pointer to T into the interface, not T itself.

I did, thanks. Nevertheless: #14529

the reflect package could panic

Right--a runtime check, not just a compiler check. To my mind, that makes this a lot more like the race detector than a souped-up, compiler-enforced vet.

Putting the nocopy element in a struct ... could also print "// This value may not be copied" ...

To reiterate a few things (apologies): This limits it to just structs, and godoc can be readily adapted to whatever purpose serves the users best.

It does add reflect data and if the empty struct is the last field, it might make the structure larger.

This is what I meant; thanks, @minux, for summing it up. :) The last-field thing in particular seems like an abstraction leak. It is ugly to have to document "the nocopy field should go first, if present, because, ummm, reasons having to do with GC" and ugly docs are a sign that something is wrong. Also, if the user wrote "_ nocopy" instead of just "nocopy", that'll impact the generated algs. All small stuff, but why have to fuss with it at all?

Having said all that, here's a concrete, minimalist counter-proposal that I think satisfies the original request from @bradfitz and @alandonovan:

• Stick with vet-only for the moment. Document that the presence of a NoCopy method on a type indicates that it should not be copied. Add vet support; this is a trivial adaptation of the existing copylocks check. Fixing cmd/vet: teach vet that putting a large value in an interface makes a copy #14529 would be nice. (If there is concern about adding new API surface area, we could use noCopy instead, but that makes it less visible to the user in many ways.) The method will be trivial to compile, will add negligible space to the object files, and will be stripped by the linker.
• Optional: Teach go doc to look for a NoCopy method and surface it more visibly to the user--although exported methods are already pretty visible.
• Wait and watch what people do with it. The existence of this annotation in the source makes it possible to run experiments at scale--take existing code, teach the compiler to look for the NoCopy method, do all the runtime instrumentation, and find out what the additional benefit in practice is of the deeper language integration. If deeper language integration is worthwhile, it would then be very easily to do an automated rewrite that removes the NoCopy method and inserts whatever other mechanism is desired, be at a comment directive, a predeclared identifier, a struct tag, a type tag (maybe someday), etc.

[rogpeppe]

Right--a runtime check, not just a compiler check. To my mind, that makes this a lot more like the race detector than a souped-up, compiler-enforced vet.

To me it's more like the panic you get when trying to compare two incomparable types that are in interfaces. The compiler can catch almost all cases statically, and we could make it possible to guard against the panic by providing a CanCopy method along the lines of CanInterface. Not that reflect-based code should be copying these kind of structs anyway - if it's happening I really want to know about it!

I don't mind much about the "must go first" thing. It only matters if you're really needing to save space and it's just the same with any zero-length type. Since it doesn't affect correctness, to my mind it's just like any other optimisation tip - nice to know but not essential.

I'm not keen on the method suggestion. It's perfectly OK to embed a pointer to a type in a value type but that doesn't mean that the outer type can't be copied, even though it would inherit the NoCopy method.

For example, it's fine to copy values of the following type:

type X struct {
    *sync.Mutex
}

You could try to define different rules for inheritance of NoCopy methods but... no. :-)

[josharian]

Good point about method promotion via embedding. It doesn't confuse vet, and the method would never be called, so in some sense it doesn't matter, but it still doesn't feel right.

[robpike]

There are very few types for which this is a fundamental question, and vet already knows about them. For others, well, if you create a type that can't be copied, you can usually construct the API so that it won't be copied by accident. The remaining cases are rare enough that putting a check for them into a tool everyone uses daily isn't worth the cost. Vet must remain fast enough for everyone and we need to curate its test suite.

At a more personal level, I fear that once we start annotating our programs with hints for heuristic tools, our programs become uglier. Then the floodgates will be open and the ugliness will soak the land. I lived through the lint era. We do not want to go back there; it was a soggy mess.

So as a proposal for vet, I say strongly, "no".

I would like to close this issue at least as far as vet is concerned. (The original issue is about the runtime, and although I feel strongly there too, I will let someone else decide how to respond to the original question.) I will compensate by providing a proper README for vet that explains the criteria used to gate additions to its suite.

[rogpeppe]

There are very few types for which this is a fundamental question and vet already knows about them. For others, well, if you create a type that can't be copied, you can usually construct the API so that it won't be copied by accident.

I agree that by adding an extra level of indirection, it's usually possible to construct a type such that it can be copied without problems, but it requires extra effort that most people won't go to, and you pay for it with extra allocations and lack of cache locality.

For example, any struct value that has a field that points directly to another field in the type (for example a slice into a static buffer defined as part of the type) will probably be broken if it's copied. I doubt this is that uncommon.

That's why I think it might be worth adding a language feature to make it straightforward to make types that are not possible to copy - not as a heuristic but as a guarantee.

[davecheney]

On Tue, 1 Mar 2016, 21:59 Roger Peppe, notifications@github.com wrote:

There are very few types for which this is a fundamental question and vet
already knows about them. For others, well, if you create a type that can't
be copied, you can usually construct the API so that it won't be copied by
accident.

I agree that by adding an extra level of indirection, it's usually
possible to construct a type such that it can be copied without problems,
but it requires extra effort that most people won't go to, and you pay for
it with extra allocations and lack of cache locality.

After watching programmers for more than a decade, I'm convinced that
making the wrong thing difficult is no obstacle to those who are truly
determined.

For example, any struct value that has a field that points directly to

another field in the type (for example a slice into a static buffer defined
as part of the type) will probably be broken if it's copied. I doubt this
is that uncommon.

That's why I think it might be worth adding a language feature to make it
straightforward to make types that are not possible to copy - not as a
heuristic but as a guarantee.

—
Reply to this email directly or view it on GitHub
#8005 (comment).

[alandonovan]

Rob said:

you can usually construct the API so that it won't be copied by accident.

Without hiding representation details and exposing only interfaces containing pointers, you cannot prevent these accidents. Buffer and Mutex are the textbook examples of the problem, but any type that has complex internal aliasing and is routinely used as a field of another struct type is at risk. Users define such types all the time, and indeed we encourage them to do so. The difference between values, variables, and pointers of a given type is one of the most subtle aspects of Go. (It confused even Linus Torvalds, as I recall.)

The speed of vet is a red herring. The cost of loading and type-checking a program is much greater than the cost of simple checking passes over the AST.

The struct field tag seems like a good compromise since it needs no standard library changes and no compiler changes (at least until we have experience), has no runtime cost, is relatively self-explanatory, and persists in export data. It is obviously restricted to struct types, but in practice that's where the problem is since unexported struct fields are Go's main means of data encapsulation; for all other types, you know what you've got.

[rsc]

There have been a number of OK-but-not-obviously-right proposals for saying "you can't copy this". My view of the situation is:

1. Go doesn't support this today, and in fact it used to but we took that out.
2. The original motivation here was sync.Mutex, which is handled by the current cmd/vet copylock check (which looks for a Lock method), and sync.Cond, which isn't handled but could easily be added to vet in some way.
3. The most common way to create types that cannot be copied is to embed a sync.Mutex, and those types are already handled by the cmd/vet check.

Instead of building more generality into vet as a kind of back-door language change, let's leave things alone for now - without any attempt to expand the generality of the copylock check - and wait to see if a better idea or more compelling evidence comes along. We should probably also make sure vet understands that sync.Cond cannot be copied, if it does not already. I opened a separate issue for that: #14582.

Note that code that absolutely must opt in to the vet check can already do so. A package can define:

type noCopy struct{}
func (*noCopy) Lock() {}

and then put a noCopy noCopy into any struct that must be flagged by vet.

[valyala]

@rsc, copylock check doesn't detect certain certain copy cases:

        var a sync.Mutex
        b := a   // doesn't detect
        var c = a  // doesn't detect
        b = a  // detects

[josharian]

@valyala thanks. please file a new issue for that and post a link to it here. And if anyone on this thread wants to try their hand at vet, I think it'd be a good beginner bug to fix.

[valyala]

@josharian , created an issue #14664 .