[tools] go/analysis/passes/modernize: avoid non-bool assignment rewrite in slicescontains

0 views
Skip to first unread message

Madeline Kalil (Gerrit)

unread,
2:54 PM (7 hours ago) 2:54 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Madeline Kalil has uploaded the change for review

Commit message

go/analysis/passes/modernize: avoid non-bool assignment rewrite in slicescontains

Adapted from Chencheng Jiang, cl/755141

The slicescontains modernizer folds certain search loops into a
slices.Contains assignment. The special case:

found = false
for i, elem := range s {
	if elem == needle {
		found = true
		break
	}
}

is transformed to:

found = slices.Contains(...)

For this special case, it previously checked whether the
pre-loop and loop-body assignments were opposite boolean literals,
but did not verify that they were actually boolean literals.

For non-boolean expressions isTrueOrFalse returns 0, which caused the
condition to match accidentally (0 == -0). This could incorrectly
rewrite non-boolean assignments.

Require the loop-body assignment to be a boolean literal before
applying the special rewrite, otherwise apply the default rewrite.

Fixes golang/go#78149
Change-Id: Icf736e119fd91b8b5fc87b500af9236e57410bcd

Change diff

diff --git a/go/analysis/passes/modernize/slicescontains.go b/go/analysis/passes/modernize/slicescontains.go
index 19e5978..c1d4218 100644
--- a/go/analysis/passes/modernize/slicescontains.go
+++ b/go/analysis/passes/modernize/slicescontains.go
@@ -327,12 +327,13 @@
 					len(assign.Rhs) == 1 {
 
 					// Have: body={ lhs = rhs; break }
+					assignBool := isTrueOrFalse(info, assign.Rhs[0])
 					if prevAssign, ok := prevStmt.(*ast.AssignStmt); ok &&
 						len(prevAssign.Lhs) == 1 &&
 						len(prevAssign.Rhs) == 1 &&
+						assignBool != 0 && // non-bool assignments don't apply in this case
 						astutil.EqualSyntax(prevAssign.Lhs[0], assign.Lhs[0]) &&
-						isTrueOrFalse(info, assign.Rhs[0]) ==
-							-isTrueOrFalse(info, prevAssign.Rhs[0]) {
+						assignBool == -isTrueOrFalse(info, prevAssign.Rhs[0]) {
 
 						// Have:
 						//    lhs = false
@@ -343,7 +344,7 @@
 						// TODO(adonovan):
 						// - support "var lhs bool = false" and variants.
 						// - allow the break to be omitted.
-						neg := cond(isTrueOrFalse(info, assign.Rhs[0]) < 0, "!", "")
+						neg := cond(assignBool < 0, "!", "")
 						report([]analysis.TextEdit{
 							// Replace "rhs" of previous assignment by [!]slices.Contains(...)
 							{
diff --git a/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go b/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go
index 7c324bb..b9734e4 100644
--- a/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go
+++ b/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go
@@ -1,6 +1,9 @@
 package slicescontains
 
-import "slices"
+import (
+	"fmt"
+	"slices"
+)
 
 var _ = slices.Contains[[]int] // force import of "slices" to avoid duplicate import edits
 
@@ -232,3 +235,19 @@
 		}
 	}
 }
+
+type Object struct{}
+
+func (o Object) Do() Object { return Object{} }
+func (o Object) Print()     { fmt.Println("Hello, World!") }
+
+func issue78149nonboolassign(obj Object, opts ...string) {
+	o := obj
+	for _, opt := range opts { // want "Loop can be simplified using slices.Contains"
+		if opt == "something" {
+			o = o.Do()
+			break
+		}
+	}
+	o.Print()
+}
diff --git a/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go.golden b/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go.golden
index 07e99b2..6b9275d 100644
--- a/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go.golden
+++ b/go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go.golden
@@ -1,6 +1,9 @@
 package slicescontains
 
-import "slices"
+import (
+	"fmt"
+	"slices"
+)
 
 var _ = slices.Contains[[]int] // force import of "slices" to avoid duplicate import edits
 
@@ -174,3 +177,16 @@
 		}
 	}
 }
+
+type Object struct{}
+
+func (o Object) Do() Object { return Object{} }
+func (o Object) Print()     { fmt.Println("Hello, World!") }
+
+func issue78149nonboolassign(obj Object, opts ...string) {
+	o := obj
+	if slices.Contains(opts, "something") {
+		o = o.Do()
+	}
+	o.Print()
+}

Change information

Files:
  • M go/analysis/passes/modernize/slicescontains.go
  • M go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go
  • M go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go.golden
Change size: S
Delta: 3 files changed, 41 insertions(+), 5 deletions(-)
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newchange
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Icf736e119fd91b8b5fc87b500af9236e57410bcd
Gerrit-Change-Number: 757020
Gerrit-PatchSet: 1
Gerrit-Owner: Madeline Kalil <mka...@google.com>
unsatisfied_requirement
satisfied_requirement
open
diffy

Madeline Kalil (Gerrit)

unread,
2:57 PM (7 hours ago) 2:57 PM
to goph...@pubsubhelper.golang.org, Alan Donovan, Chencheng Jiang, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Chencheng Jiang

Madeline Kalil voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Alan Donovan
  • Chencheng Jiang
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Icf736e119fd91b8b5fc87b500af9236e57410bcd
Gerrit-Change-Number: 757020
Gerrit-PatchSet: 1
Gerrit-Owner: Madeline Kalil <mka...@google.com>
Gerrit-Reviewer: Alan Donovan <adon...@google.com>
Gerrit-Reviewer: Chencheng Jiang <dorb...@gmail.com>
Gerrit-Reviewer: Madeline Kalil <mka...@google.com>
Gerrit-Attention: Alan Donovan <adon...@google.com>
Gerrit-Attention: Chencheng Jiang <dorb...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Mar 2026 18:56:58 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
unsatisfied_requirement
satisfied_requirement
open
diffy

Madeline Kalil (Gerrit)

unread,
2:57 PM (7 hours ago) 2:57 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Chencheng Jiang

Madeline Kalil uploaded new patchset

Madeline Kalil uploaded patch set #2 to this change.
Open in Gerrit

Related details

Attention is currently required from:
  • Alan Donovan
  • Chencheng Jiang
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newpatchset
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Icf736e119fd91b8b5fc87b500af9236e57410bcd
Gerrit-Change-Number: 757020
Gerrit-PatchSet: 2
Gerrit-Owner: Madeline Kalil <mka...@google.com>
Gerrit-Reviewer: Alan Donovan <adon...@google.com>
Gerrit-Reviewer: Chencheng Jiang <dorb...@gmail.com>
Gerrit-Reviewer: Madeline Kalil <mka...@google.com>
unsatisfied_requirement
satisfied_requirement
open
diffy

Alan Donovan (Gerrit)

unread,
3:23 PM (7 hours ago) 3:23 PM
to Madeline Kalil, goph...@pubsubhelper.golang.org, Go LUCI, Chencheng Jiang, golang-co...@googlegroups.com
Attention needed from Chencheng Jiang and Madeline Kalil

Alan Donovan voted Code-Review+2

Code-Review+2
Open in Gerrit

Related details

Attention is currently required from:
  • Chencheng Jiang
  • Madeline Kalil
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement satisfiedReview-Enforcement
  • requirement satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Icf736e119fd91b8b5fc87b500af9236e57410bcd
Gerrit-Change-Number: 757020
Gerrit-PatchSet: 2
Gerrit-Owner: Madeline Kalil <mka...@google.com>
Gerrit-Reviewer: Alan Donovan <adon...@google.com>
Gerrit-Reviewer: Chencheng Jiang <dorb...@gmail.com>
Gerrit-Reviewer: Madeline Kalil <mka...@google.com>
Gerrit-Attention: Madeline Kalil <mka...@google.com>
Gerrit-Attention: Chencheng Jiang <dorb...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Mar 2026 19:23:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Madeline Kalil (Gerrit)

unread,
3:36 PM (7 hours ago) 3:36 PM
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Alan Donovan, Go LUCI, Chencheng Jiang, golang-co...@googlegroups.com

Madeline Kalil submitted the change

Change information

Commit message:
go/analysis/passes/modernize: avoid non-bool assignment rewrite in slicescontains

Adapted from Chencheng Jiang, CL 755141


The slicescontains modernizer folds certain search loops into a
slices.Contains assignment. The special case:

found = false
for i, elem := range s {
	if elem == needle {
		found = true
		break
	}
}

is transformed to:

found = slices.Contains(...)

For this special case, it previously checked whether the
pre-loop and loop-body assignments were opposite boolean literals,
but did not verify that they were actually boolean literals.

For non-boolean expressions isTrueOrFalse returns 0, which caused the
condition to match accidentally (0 == -0). This could incorrectly
rewrite non-boolean assignments.

Require the loop-body assignment to be a boolean literal before
applying the special rewrite, otherwise apply the default rewrite.

Fixes golang/go#78149
Change-Id: Icf736e119fd91b8b5fc87b500af9236e57410bcd
Reviewed-by: Alan Donovan <adon...@google.com>
Files:
  • M go/analysis/passes/modernize/slicescontains.go
  • M go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go
  • M go/analysis/passes/modernize/testdata/src/slicescontains/slicescontains.go.golden
Change size: S
Delta: 3 files changed, 41 insertions(+), 5 deletions(-)
Branch: refs/heads/master
Submit Requirements:
  • requirement satisfiedCode-Review: +2 by Alan Donovan
  • requirement satisfiedTryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
 Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit



 Gerrit-MessageType: merged 


 Gerrit-Project: tools 


 Gerrit-Branch: master 


 Gerrit-Change-Id: Icf736e119fd91b8b5fc87b500af9236e57410bcd 


 Gerrit-Change-Number: 757020 


 Gerrit-PatchSet: 3 
open
diffy
satisfied_requirement
Reply all
Reply to author
Forward
0 new messages