Skip to content

cmd/compile: loopvar doesn't trigger nocopy detection #66070

Open
@zigo101

Description

@zigo101

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.

Activity

changed the title [-]cmd/compile: incosistent behavior since Go 1.22[/-] [+]cmd/compile: loopvar can trigger nocopy detection[/+] on Mar 2, 2024
changed the title [-]cmd/compile: loopvar can trigger nocopy detection[/-] [+]cmd/compile: loopvar doesn't trigger nocopy detection[/+] on Mar 2, 2024
zigo101

zigo101 commented on Mar 2, 2024

@zigo101
Author

The problem should exist for every value which contains self-reference pointers when it is used as loop variables. (edit: looks not true)

ianlancetaylor

ianlancetaylor commented on Mar 2, 2024

@ianlancetaylor
Contributor
zigo101

zigo101 commented on Mar 3, 2024

@zigo101
Author

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
}
added
NeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.
on Mar 4, 2024
added this to the Backlog milestone on Mar 4, 2024
dr2chase

dr2chase commented on Mar 5, 2024

@dr2chase
Contributor

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.

added
WaitingForInfoIssue is not actionable because of missing required information, which needs to be provided.
on Mar 5, 2024
zigo101

zigo101 commented on Mar 6, 2024

@zigo101
Author

Yes, we should get it for both functions (foo and bar), but only get it for the second one (bar).

zigo101

zigo101 commented on Mar 6, 2024

@zigo101
Author

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.")
}
zigo101

zigo101 commented on Mar 6, 2024

@zigo101
Author

BTW, such cases can never happen in JS world. So this is a fundamental difference from JS.

dr2chase

dr2chase commented on Mar 6, 2024

@dr2chase
Contributor

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.

zigo101

zigo101 commented on Mar 6, 2024

@zigo101
Author

? The problem is just there. I just expected the behavior doesn't change when adding/removing a no-op line.

The example is real.

3 remaining items

rsc

rsc commented on Mar 6, 2024

@rsc
Contributor

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.

zigo101

zigo101 commented on Mar 7, 2024

@zigo101
Author

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.

dr2chase

dr2chase commented on Mar 7, 2024

@dr2chase
Contributor

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).

gopherbot

gopherbot commented on Mar 8, 2024

@gopherbot
Contributor

Change https://go.dev/cl/570137 mentions this issue: go/analysis/passes: update copylock check for strings.Builder and 1.22 loopvar

removed
WaitingForInfoIssue is not actionable because of missing required information, which needs to be provided.
on Mar 31, 2024
modified the milestones: Backlog, Go1.23 on Apr 10, 2024
modified the milestones: Go1.23, Go1.24 on Aug 13, 2024
modified the milestones: Go1.24, Go1.25 on Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

NeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.compiler/runtimeIssues related to the Go compiler and/or runtime.

Type

No type

Projects

Status

In Progress

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @rsc@mknyszek@dr2chase@ianlancetaylor@bcmills

      Issue actions

        cmd/compile: loopvar doesn't trigger nocopy detection · Issue #66070 · golang/go