Related Issues

• runtime: add NoCopy documentation struct type? #8005 (closed)
• proposal: Go 2: non-copyable structs by default #30450 (closed)
• proposal: Go 2: NoCopy: official support in compiler (not just vet linter) #23764 (closed)
• proposal: embed "noCopy" for bytes.Buffer and strings.Builder #25907 (closed)

Related Code Changes

• std: pass bytes.Buffer and strings.Builder by pointer
• bytes: document that Buffer values must not be copied

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

This subtle behavior change might be worth noting (assuming this API lands and strings.Builder is replaced to use structs.NoCopyCheck):

var s strings.Builder
s2 := s // nocopy vet warning
s2.WriteString("test") // copy runtime panic

Copying the zero value is safe, s2 := s, after this change s2 := s would cause a vet error.

Not sure whether it matters, leaving this here.

EDIT: actually it is documented as "NoCopy helps statically detect copying of non-zero values of", so i assume that this would not happen (at least it these simple cases).

Exactly: the words "non-zero" are crucial.

Exactly: the words "non-zero" are crucial.

Yep, the current behaviour of the vet check does not seem to care whether it is zero value, hence the confusion.

Non-zero is a dynamic property. How is the static checker going to determine non-zero-ness?

Non-zero is a dynamic property. How is the static checker going to determine non-zero-ness?

The wording says "static analysis tools may report an error when they encounter statements that copy a NoCopy value"; the static check needn't insist that the value be non-zero.

But it could use a conservative heuristic. For example: a reference to a variable is assumed to be zero if and only if its declaration is local to the same function, and no control path from the def to the use refers to the variable as an l-value. But perhaps this is too conservative.

If this is about tools like vet catching things, you could just propose a magic name to put in a zero-sized field of the struct.

type T struct {
   something typed
   _do_not_copy_ struct{}
}

Such a convention will work fine for most of the need here. No need to add anything to the library. Your proposal doesn't carry its weight; it's too easy to say "it prevents bugs" to justify any complication.

I am saddened by all extra knobs, whistles, doodads, and festoonery that the library is accumulating these days. As Bjarne Stroustrup once said, looking the other direction, "If you want C++ you know where to get it."

While adding something to the standard library that has no effect other than being a hook for separate analysis tools is pretty weird, I think an advantage it has over just a naming convention is that a library symbol has an obvious place to go to find its documentation, and that documentation can then explain what it's for.

There are various little tricks and hacks that have become informal Go idiom over time, such as using an unnamed global variable to assert that type implements a particular interface so that the compiler will check it, and my experience has been that newcomers tend to come and ask me to explain what they mean because there is no clear path to their documentation. I would like there to be fewer of these things over time.

I think it could be justified to say that this isn't valuable enough to do at all and that instead the linter should just learn about specific existing standard library types like the ones mentioned in the proposal text, but if we agree that this is a problem worth solving then I'd personally rather solve it with a real language or library feature than to add another ad-hoc pattern that folks need to learn about by asking their peers.

@robpike how is a documented _do_not_copy_ field name any different than structs.NoCopy in terms of added API? If you object to NoCopyCheck that's a different matter, but I personally find a documented hint type to be perfectly reasonable.

Because it's not API at all and involves no types. You may have different criteria.

In general, Go programmers should assume, in the absence of explicit documentation or insider knowledge, that copying any arbitrary nonzero value of type T, where methods are defined on *T, is not safe.

If this is so, what information does structs.NoCopy convey? Not just "assume this unsafe to copy" but "this is definitely unsafe to copy and we're going to vet check it." If the default is that things are unsafe, shouldn't there be some way to signal "this is okay to copy"?

In #41733 it was proposed to add url.URL.Clone, but this was rejected because copying a url.URL is safe. @rsc wrote,

I still don't understand what exactly is worth calling out in URL's documentation.

It is a general property of data that if you want to make a copy before mutating you can do u2 := *u; mutate u2.

That seems like it was in the other direction: the default was it is safe to copy, so no clone method was needed.

Is there some way we can we teach go vet what makes something safe or unsafe to copy and then look for that? How many other unsafe to copy structs are there outside of sync (unsafe because it is doing CPU sync stuff) and strings.Builder (which is unsafe because it turns bytes into a string)? Plenty of other people do unsafe string conversions, but that seems like a known to be dangerous API choice people are opting into. Do we need to go out of our way to support it?

If this is so, what information does structs.NoCopy convey? Not just "assume this unsafe to copy" but "this is definitely unsafe to copy and we're going to vet check it." If the default is that things are unsafe, shouldn't there be some way to signal "this is okay to copy"?

You raise a good question. Based on the examples above, it would appear to signal "this type is particularly prone to bad things happening if you copy it, perhaps due to extralinguistic mechanisms (e.g. unsafe, scheduler)". But you're right that there's something odd about affirming what should be the default assumption, instead of flagging the non-default one.

Russ may be right that URLs are copyable, but that's not stated in its documentation, and URL has potential aliasing issues since at least one of its fields is indirect and it has public mutator methods like Parse. (Shallow copying followed by updating the copy's User field would cause unexpected effects on the original.) Existing practice has mostly been: read the source, and make a good guess based upon the name and API and doc comments. But URL's documentation should really state explicitly that it is copyable, if that is the intent, and ideally that intent should be recorded in a form amenable to tooling. But I can't imagine the inverse proposal (structs.Copyable) having much traction, as too much code assumes that structs are copyable.

Is there some way we can we teach go vet what makes something safe or unsafe to copy and then look for that?

Any kind of indirection among the struct fields is a potential problem; it's hard to be more specific.

I am against this proposal. It is not an feature that is "orthogonal" to other features, and adds immense mental complexity compared to its benefits.

This only benefit from this proposal is a fix to #47276 , but the real fix to that issue is for strings.Builder to either:

• not use unsafe
• write its logic in assembly that doesn't crash by playing well with the core runtime, as is done in the very few core parts of Go

I resonate with Rob's quote that recently:

I am saddened by all extra knobs, whistles, doodads, and festoonery that the library is accumulating these days. As Bjarne Stroustrup once said, looking the other direction, "If you want C++ you know where to get it."

there are more and more proposals suggesting keywords that don't play nice with the rest of the language, and most worryingly touching the most dangerous areas:

• memory and copying
• fancy type casting

I still remember the first time I saw Go in 2013 or 2014, and was totally in awe and amazed by how crystal clear reasoning memory and copying in Go is, compared to the dreaded C++ STL.
I really hope reasoning memory and types in Go stay simple, with only a very small set of rules that are orthogonal to each other needed.

Here's another real world use case. We have a struct whose constructor creates a background goroutine that lazily fills in one of its private fields. A developer accidentally wrote code to make a shallow copy of the struct. This of course did not create a copy of the background goroutine, so the copied struct never had that field filled in, resulting in a bug.

In general, it is not safe to shallow copy a struct with mutable fields that is intended to be used across multiple goroutines. Having a defense mechanism against developer mistakes around this point is very useful.

Here's another real world use case. We have a struct whose constructor creates a background goroutine that lazily fills in one of its private fields. A developer accidentally wrote code to make a shallow copy of the struct. This of course did not create a copy of the background goroutine, so the copied struct never had that field filled in, resulting in a bug.

If it was using a background goroutine, there must have been a lock, right? So then I think the existing vet check could have seen the lock being copied if it were embedded in the struct…

@earthboundkid In this case, due to another developer oversight there was no mutex. So yes, there was also a race condition, but the shallow copy made a bad situation way worse.

Regardless, it should also be noted that the current copylock check only works if you are using sync.Mutex. If you use *sync.Mutex instead (which is fairly common in our code), the check does not fire. It also won't fire if you are using the older atomic.LoadX and atomic.StoreX functions, or if you are using a channel to broadcast when the field has been assigned.

I would like to add that the NoCopy used as a directive is already widely used, spelled as "DoNotCopy", in Go the protobuf code: https://github.com/protocolbuffers/protobuf-go/blob/master/internal/pragma/pragma.go
The Go protobuf generator includes these in all types generated so they should make up a significant part of the Go corpus.

Furthermore currently the lack of these marker types lessens the readability of Go code, because many Go language libraries use their own tricks similar to _do_not_copy_. There are likely many Go projects that can use this marker type, leading to them becoming better documented. So I thinkthis type will pass the bar of being common enough to be included in the standard library.