gopls/internal/analysis/modernize: fix bug in slicescontains
Previously, a fix was offered to use slices.Contains(haystack, needle)
even when the slice element type was not identical to the needle,
resulting in a type error. (We really should make RunWithSuggestedFixes
verify that the fixed code is still well-typed.)
Now, we don't report a diagnostic if the types differ.
(We could insert a widening cast in one of the two cases,
but that's left for a follow-up.)
Fixes golang/go#71313
Change-Id: I41764da93085eef8ea5d30337d01fe03ee345f6d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/645856
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/analysis/modernize/slicescontains.go b/gopls/internal/analysis/modernize/slicescontains.go
index 062083c..dc0aa61 100644
--- a/gopls/internal/analysis/modernize/slicescontains.go
+++ b/gopls/internal/analysis/modernize/slicescontains.go
@@ -85,13 +85,33 @@
switch cond := ifStmt.Cond.(type) {
case *ast.BinaryExpr:
if cond.Op == token.EQL {
+ var elem ast.Expr
if isSliceElem(cond.X) {
funcName = "Contains"
+ elem = cond.X
arg2 = cond.Y // "if elem == needle"
} else if isSliceElem(cond.Y) {
funcName = "Contains"
+ elem = cond.Y
arg2 = cond.X // "if needle == elem"
}
+
+ // Reject if elem and needle have different types.
+ if elem != nil {
+ tElem := info.TypeOf(elem)
+ tNeedle := info.TypeOf(arg2)
+ if !types.Identical(tElem, tNeedle) {
+ // Avoid ill-typed slices.Contains([]error, any).
+ if !types.AssignableTo(tNeedle, tElem) {
+ return
+ }
+ // TODO(adonovan): relax this check to allow
+ // slices.Contains([]error, error(any)),
+ // inserting an explicit widening conversion
+ // around the needle.
+ return
+ }
+ }
}
case *ast.CallExpr:
diff --git a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go
index ecb7371..6116ce1 100644
--- a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go
+++ b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go
@@ -127,3 +127,22 @@
}
func predicate(int) bool
+
+// Regression tests for bad fixes when needle
+// and haystack have different types (#71313):
+
+func nopeNeedleHaystackDifferentTypes(x any, args []error) {
+ for _, arg := range args {
+ if arg == x {
+ return
+ }
+ }
+}
+
+func nopeNeedleHaystackDifferentTypes2(x error, args []any) {
+ for _, arg := range args {
+ if arg == x {
+ return
+ }
+ }
+}
diff --git a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden
index 561e42f..2d67395 100644
--- a/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden
+++ b/gopls/internal/analysis/modernize/testdata/src/slicescontains/slicescontains.go.golden
@@ -83,3 +83,22 @@
}
func predicate(int) bool
+
+// Regression tests for bad fixes when needle
+// and haystack have different types (#71313):
+
+func nopeNeedleHaystackDifferentTypes(x any, args []error) {
+ for _, arg := range args {
+ if arg == x {
+ return
+ }
+ }
+}
+
+func nopeNeedleHaystackDifferentTypes2(x error, args []any) {
+ for _, arg := range args {
+ if arg == x {
+ return
+ }
+ }
+}
