strings: Builder.String should either reset the Builder or avoid using unsafe semantics

[oakad]

Affects all Go versions since ever.

In some situation it may be desirable to build several strings which happen to prefix each other. Examples of such uses are filesystem hierarchies, x509 OIDs and other similar objects.

Nothing in the strings.Builder documentation implies, that an enterprising user will not be able to keep adding fragments to the builder after calling the String method once or twice.

Let's consider the following example:

var b strings.Builder
b.Grow(20)
b.WriteString("aaa")
x := b.String()
fmt.Printf("%s %p\n", x, unsafe.StringData(x))
b.WriteString("bbb")
y := b.String()
fmt.Printf("%s %p\n", y, unsafe.StringData(y))

Outputs:

aaa 0xc0000b2000
aaabbb 0xc0000b2000

Basically, we are able to break a cornerstone Go invariant with some really basic usage of "beginner" level facilities without any warning.

As a side note, it is not obvious why Builder is explicitly made non-copyable. Nothing bad will happen if a similar structure is copied here and there, it keeps working just fine, unlike this here issue.

[mateusz834]

But it is safe to do so in go, strings are immutable. Right?

[seankhliao]

I don't see anything to do here. strings consist of more than just the data part, appending to the builder won't change the original string.

[mknyszek]

As others have said, I'm not sure I see what the problem here is. Strings are safely allowed to alias, and IIRC the only requirement is that their underlying data is immutable. You also can't re-slice a string to be larger like you can with a slice, because it doesn't have a capacity like slices do. Finally, strings.Builder provides no facility for mutating that data, only appending.

Basically, we are able to break a cornerstone Go invariant with some really basic usage of "beginner" level facilities without any warning.

Which invariant do you mean?

As a side note, it is not obvious why Builder is explicitly made non-copyable. Nothing bad will happen if a similar structure is copied here and there, it keeps working just fine, unlike this here issue.

This requirement is necessary to achieve the performance characteristics Builder strives for while maintaining string invariants. Namely, if you copy Builder and write to both copies, the buffer the internal implementation carries may have the same memory written to more than once. Making that safe for copying would require having String return a copy of the internal buffer, which the type is explicitly trying to avoid.

This restriction does make for some weird semantics, but there's precedent for no-copy data structures elsewhere (e.g. sync.Mutex). It doesn't seem unreasonable to me to require the same of another type, even if it is just for performance.