Go version
go version go1.22.0 linux/amd64
Output of go env in your module/workspace:
.
What did you do?
package main
import (
"fmt"
"strings"
)
func foo() string {
for b, i := (strings.Builder{}), byte('a'); ; i++ {
b.WriteByte(i)
if i == 'z' {
return b.String()
}
}
}
func bar(callback func(*strings.Builder)) string {
for b, i := (strings.Builder{}), byte('a'); ; i++ {
b.WriteByte(i)
callback(&b) // <-- difference here
if i == 'z' {
return b.String()
}
}
}
func main() {
debugProcess := func(pb *strings.Builder) {
// do nothing
}
fmt.Println("foo:", foo())
fmt.Println("bar:", bar(debugProcess))
}
What did you see happen?
Consistent behavior between foo and bar with 1.22 compiler.
What did you expect to see?
Inconsistent behavior between foo and bar with 1.22 compiler.
Comment From: zigo101
~~The problem should exist for every value which contains self-reference pointers when it is used as loop variables.~~ (edit: looks not true)
Comment From: ianlancetaylor
CC @dr2chase
Comment From: zigo101
In the following program, bar function panics, but foo doesn't.
The fact reflects that, the compiler will check whether or not a declared
function will put the references of the argument somewhere after the
function exits, but not check this for local anonymous functions.
package main
import "strings"
//go:noinline
func f(b *strings.Builder) {
println(b.String())
}
func foo() {
for b := (strings.Builder{}); b.Len() < 2; {
b.WriteByte('!')
f(&b)
}
}
var p *strings.Builder
//go:noinline
func g(b *strings.Builder) {
println(b.String())
p = b
}
func bar() {
for b := (strings.Builder{}); b.Len() < 2; {
b.WriteByte('!')
g(&b)
}
}
func main() {
println("------------ foo:")
foo() // not panic
println("------------ bar:")
bar() // panic
}
Comment From: dr2chase
The panic I get is
panic: strings: illegal use of non-zero Builder copied by value
goroutine 1 [running]:
strings.(*Builder).copyCheck(...)
/Users/drchase/work/go/src/strings/builder.go:47
strings.(*Builder).WriteByte(...)
/Users/drchase/work/go/src/strings/builder.go:102
main.bar()
/Users/drchase/work/go/src/go101/main.go:28 +0xad
main.main()
/Users/drchase/work/go/src/go101/main.go:37 +0x4f
exit status 2
Is that what you get? You could have provided this information in your bug report and it would have been helpful.
Comment From: zigo101
Yes, we should get it for both functions (foo and bar), but only get it for the second one (bar).
Comment From: zigo101
An example to extend one of my previous comment:
package main
import (
"fmt"
"strings"
)
//go:noinline
func globalDebugProcess(pb *strings.Builder) {
// do nothing
}
func foo() string {
for b, i := (strings.Builder{}), byte('a'); ; i++ {
b.WriteByte(i)
globalDebugProcess(&b)
if i == 'z' {
return b.String()
}
}
}
func bar(callback func(*strings.Builder)) string {
for b, i := (strings.Builder{}), byte('a'); ; i++ {
b.WriteByte(i)
callback(&b) // <-- difference here
if i == 'z' {
return b.String()
}
}
}
func main() {
localDebugProcess := func(pb *strings.Builder) {
// do nothing
}
fmt.Println("foo:", foo())
fmt.Println(" foo done.")
fmt.Println("bar:", bar(localDebugProcess))
fmt.Println(" bar done.")
}
Comment From: zigo101
BTW, such cases can never happen in JS world. So this is a fundamental difference from JS.
Comment From: dr2chase
Is this causing problems in a real program? Also, in your examples, it is helpful if you provide information about what happened when you ran it. Just because I observe a problem when I run it, does not mean I observe the same problem.
Comment From: zigo101
? The problem is just there. I just expected the behavior doesn't change when adding/removing a no-op line.
The example is real.
Comment From: bcmills
I think this is a side effect of #47276.
Without the noescape hack in strings.Builder, foo panics in the same way that bar does: https://go.dev/play/p/wvJ7e8Jsv3g
Comment From: bcmills
That is: because strings.Builder violates the compiler's own unsafe.Pointer rules, the compiler performs an unsafe optimization in the foo function that causes the loop variable not to escape to the heap when it should.
The compiler does not perform that optimization in the bar function because the callback call (correctly) marks the variable as escaping.
Comment From: bcmills
(To be clear: fixing the bug in strings.Builder would cause it to successfully detect the incorrect copy and panic — it would not make the loop “correct”. 😅)
Comment From: rsc
The original proposal explicitly acknowledged that there would be corner-case programs that might be affected in one way or another. That's why the change is keyed by the language version in go.mod and the //go:build lines, so that you can update your code gradually to be safe for the new semantics, instead of having to convert an entire program all at once.
The compiler is doing a safe transformation. The use of unsafe in strings.Builder is the unsafe part, and your example is a casualty. But in this case at least, there is no reason to write the code this way. Basically everyone who writes Go would declare b above the loop.
I don't think there's anything to do here.
Comment From: zigo101
The original proposal explicitly acknowledged that there would be corner-case programs that might be affected in one way or another.
It would be great that Go official maintain a list of such so-called corner cases. Doing this will be much helpful to all Go programmers. BTW, I maintain an incomplete one.
That's why the change is keyed by the language version in go.mod and the //go:build lines, ...
This needs much more publicity. Hope the just mentioned article would help some. But no matter how it's publicized, there will always be gophers who don't get the message. So this is really a dangerous point for some Go projects.
The use of unsafe in strings.Builder is the unsafe part, and your example is a casualty.
I've had bad luck. :D I really didn't know that strings.Builder uses unsafe.
I will check the implementation when using a std function later
and avoid using it when the implantation uses unsafe.
But in this case at least, there is no reason to write the code this way.
My reason is: the syntax allows it and I like the flexibility the old semantics provide.
Basically everyone who writes Go would declare b above the loop.
Prior to Go 1.22, I can't vouch for that myself. Since Go 1.22, I surely will, because I be fully aware of the differences between the new and old semantics. But there will always some gophers who are not. Again, this needs much more publicity.
This issue was created mainly to make Go compiler more rigorous. If it is not so important, just put it aside.
Comment From: dr2chase
This corner case, as a source of possible Go errors, is not a large one. I know we have a tendency to view language definition changes as a sort of "broken promise" that somehow makes those potential bug causes loom larger than all the working-as-intended foot-guns ("the programmer should have known") but at scale, what matters is the rate/cost of bugs, no matter their cause. Assume code needs maintenance, I have never seen a piece of code of any significance that lacked a bug.
I extended the vet nocopy check to cover strings.Builder and for builder := BuilderTypedExpressions and scanned a large amount of Go code, and got very few hits of any kind, and on an eyeball scan, none using the for loop. I'm going to refine it to call out specifically this case and run it again, and it will probably get turned into a regular vet check because the "false" positive rate is very low (and by "false", I mean "why are you returning a strings.Builder instead of converting it to a string and returning that? The only safe thing you can do with that Builder is turn it into a string.")
For comparison, on this sample, I saw 152 occurrences of any kind of copying a strings.Builder, while the lock-oriented nocopy check fired at least 4848 times (I do not know the upper limit, I capped results at 5000, so that minus 152).
I'm benchmarking an alternate fix for this problem, but even if Builders become copy-able, this same problem would affect the other nocopy types, so vet needs a modification for that (and that small change probably does not require a proposal).
Comment From: gopherbot
Change https://go.dev/cl/570137 mentions this issue: go/analysis/passes: update copylock check for strings.Builder and 1.22 loopvar