bytes: Buffer should add copy check

[xpunch]

What version of Go are you using (go version)?

$ go version
go version go1.17 windows/amd64

What did you do?

package main

import (
	"bytes"
	"fmt"
)

func main() {
	var a bytes.Buffer
	a.WriteString("aaaaaaaaaaaaa-")
	b := a
	a.WriteString("a")
	fmt.Printf("Before - %s\n", a.String())
	b.WriteString("b")
	fmt.Printf("After  - %s\n", a.String())
}

Before - aaaaaaaaaaaaa-a
After  - aaaaaaaaaaaaa-b

What did you expect to see?

Provide copy check to prevent value copy of bytes.Buffer.

What did you see instead?

a influence by b

[seankhliao]

Duplicate of #25907

[xpunch]

This is not duplicate of #25907, strings.Builder do dynamic copy check, but bytes.Buffer is leak of dynamic copy check.

[ianlancetaylor]

I agree that this is not quite a dup of #25907. But I don't think we should add a dynamic copy check here. For strings.Builder the dynamic copy check is required for correctness, as otherwise a string value could change while the program is running. The same consideration does not arise for bytes.Buffer. It is OK to copy a bytes.Buffer, even if the results can be unexpected in some cases. It's not different than using assignment to make multiple copies of a slice.

[xpunch]

Not everyone who using golang knows the internal implementation of bytes.Buffer, so we should not think the "unexpected result" is common sense. If this is not a duplicate of #25907, I think we should reopen this issue, instead of closing it roughly.

[ianlancetaylor]

OK, I will reopen it, but I expect the result will be to close it gently. Copying a bytes.Buffer is not wrong. What is wrong is copying a bytes.Buffer and then writing to both copies. If we add a copy check to bytes.Buffer today we will most likely break existing working code.

[xpunch]

bytes.Buffer already existed for 12 years, add a copy check will break some working codes indeed.
At least we should add some comments or print some warnings in go vet or staticcheck.

// A Buffer is a variable-sized buffer of bytes with Read and Write methods.
// The zero value for Buffer is an empty buffer ready to use.
// Do not copy a non-zero Buffer.
type Buffer struct {
}

[adonovan]

Not everyone who using golang knows the internal implementation of bytes.Buffer, so we should not think the "unexpected result" is common sense.

I would argue that we should. You don't need to know Buffer's internals to know that in Go, it is generally a bad idea to copy a value of type T that has methods associated with *T, because method calls may lead to subtle aliasing relationships. For example, a call may cause one field of a struct to refer to another field. Go's direct copy semantics are not appropriate for such values, because they cause both the copy and the original to refer to the original. You need to call a Clone method, if the type provides one, or avoid copying altogether.

[xpunch]

We all know the consequence of copy a non-zero value of bytes.Buffer, but there is no warning or mechanism to prevent it. We don't want golang users need to know those tricks before writing their codes. Your example is correct, but bytes.Buffer is in standard library, it should be out-of-the-box. I think that's the reason why strings.Builder have dynamic copy check.