Skip to content

proposal: structs: NoCopy #70811

Open
Open
@adonovan

Description

Background: In general, copying of mutable Go data structures is a risky business. Go has no analogue of C++'s DISALLOW_COPY_CONSTRUCTORS macro or Rust's Copy trait, so it is always possible to copy values of any type. However, shallow copying may hide very subtle aliasing bugs:

  • The original and the copy can no longer both be used. For example, after copying a bytes.Buffer, subsequent mutations to either the original or the copy may cause unpredictable effects on the other one.
  • "Linear copying", in which the old value is no longer used after the copy, is typically less hazardous, but may still be unsafe or unpredictable. In the past, bytes.Buffer contained a small inline array that was used as the initial space for slice allocation. Copying a Buffer would cause the new copy to refer to the array field of the original. (This wasn't incorrect, but it was surprising and unintended.)
  • More complex data structures are not safe to use even after linear copying. For example, sync.WaitGroup and sync.Mutex contain a semaphore, which has special treatment in the runtime; a copy won't do. And strings.Builder uses unsafe tricks to avoid allocation; mutating a copy leads to crashes.

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.

However, for certain types (such as those mentioned above) the potential for latent serious bugs is high, and it is worth making an effort to detect copying mistakes statically (using cmd/vet's copylock check) and perhaps even dynamically, using the self-pointer technique demonstrated by strings.Builder.

Proposal: We propose to publish two new types, shown below, in the structs package. The first is merely an annotation for static analysis tools; the second additionally performs a dynamic check (at the cost of one extra word in your data structure).

We also propose to rename vet's copylock check to nocopy, and change it to look for structs.NoCopy fields instead of the presence of Lock/Unlock methods (which are an indirect proxy for non-copyability that dates from when the check applied only to Mutex and WaitGroup).

package structs

// NoCopy helps statically detect copying of non-zero values of
// mutable data types that must not be copied. (Examples include
// [strings.Builder] and [sync.WaitGroup].)
//
// Embed this type within your data structure to indicate that copying
// non-zero values of that type is not safe. This type has no dynamic
// effect, but static analysis tools may report an error when they
// encounter statements that copy a NoCopy value.
//
// See [NoCopyCheck] for a variant that additionally incorporates a
// run-time check.
//
// As a rule, unless explicitly documented to the contrary, you should
// assume that it is not safe to make copies of any non-zero value of
// a type T that defines methods on *T. Calling such a method may
// cause the data structure to incorporate its original address in
// some way--for example, by causing one field to point to
// another--such that copying the value violates invariants of the
// representation.
type NoCopy struct{}

// NoCopyCheck helps detect copying of mutable data types that must
// not be copied (for example, [strings.Builder]), using both runtime
// assertions and static checks.
//
// Embed a NoCopyCheck within your data structure, and call its check
// method at the start of each method:
//
//	type Buffer struct {
//		data   []byte
//		nocopy NoCopyCheck
//	}
//
//	func (b *Buffer) Write(data []byte) {
//		b.nocopy.Check()
//		b.data = append(b.data, data)
//	}
//
// If the structure has been copied, it will panic:
//
//	var old Buffer
//	old.Write([]byte("hello"))
//	new := old // Buffer values must not be copied!
//	old.Write([]byte("world"))
type NoCopyCheck struct {
	_ NoCopy
	ptr *NoCopyCheck // points to self after first call of Check
}

// Check panics if the NoCopyCheck has been copied since the previous
// call to Check, if any.
func (nocopy *NoCopyCheck) Check() {
	if nocopy.ptr == nil {
		nocopy.ptr = nocopy
	} else if nocopy.ptr != nocopy {
		panic("a struct with a NoCopyCheck field has been copied")
	}
}

Related:

(Thanks to @ianthehat for pointing out that NoCopyCheck can contain a NoCopy so that vet need only consider the latter.)

Metadata

Assignees

No one assigned

    Labels

    AnalysisIssues related to static analysis (vet, x/tools/go/analysis)Proposal

    Type

    No type

    Projects

    • Status

      Incoming

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions