gopls/internal/analysis/modernize: don't import slices within slices

This CL strengthens the check that the various modernizer passes
use to skip packages in which the fixes cannot be applied. Before,
we would not add an import of "slices" from withing the "slices"
package itself, but we cannot add this import from any package
that "slices" itself transitively depends upon, as this would
create an import cycle.

So, we consult the std dependency graph baked into the executable.

This feature was tested interactively by running modernize on std.

Updates golang/go#71847

Change-Id: Iaec6ef07b58ca07df498db63369dae8087331ab9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/653595
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

gopls/internal/analysis/modernize/maps.go

@@ -41,7 +41,9 @@ import (
 //	m = make(M)
 //	m = M{}
 func mapsloop(pass *analysis.Pass) {
-	if pass.Pkg.Path() == "maps " {
+	// Skip the analyzer in packages where its
+	// fixes would create an import cycle.
+	if within(pass, "maps", "bytes", "runtime") {
 		return
 	}
 

gopls/internal/analysis/modernize/modernize.go

@@ -18,8 +18,10 @@ import (
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/gopls/internal/util/astutil"
+	"golang.org/x/tools/gopls/internal/util/moreiters"
 	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/astutil/cursor"
+	"golang.org/x/tools/internal/stdlib"
 	"golang.org/x/tools/internal/versions"
 )
 
@@ -125,6 +127,26 @@ func filesUsing(inspect *inspector.Inspector, info *types.Info, version string)
 	}
 }
 
+// within reports whether the current pass is analyzing one of the
+// specified standard packages or their dependencies.
+func within(pass *analysis.Pass, pkgs ...string) bool {
+	path := pass.Pkg.Path()
+	return standard(path) &&
+		moreiters.Contains(stdlib.Dependencies(pkgs...), path)
+}
+
+// standard reports whether the specified package path belongs to a
+// package in the standard library (including internal dependencies).
+func standard(path string) bool {
+	// A standard package has no dot in its first segment.
+	// (It may yet have a dot, e.g. "vendor/golang.org/x/foo".)
+	slash := strings.IndexByte(path, '/')
+	if slash < 0 {
+		slash = len(path)
+	}
+	return !strings.Contains(path[:slash], ".") && path != "testdata"
+}
+
 var (
 	builtinAny     = types.Universe.Lookup("any")
 	builtinAppend  = types.Universe.Lookup("append")

gopls/internal/analysis/modernize/slices.go

@@ -46,8 +46,9 @@ import (
 // The fix does not always preserve nilness the of base slice when the
 // addends (a, b, c) are all empty.
 func appendclipped(pass *analysis.Pass) {
-	switch pass.Pkg.Path() {
-	case "slices", "bytes":
+	// Skip the analyzer in packages where its
+	// fixes would create an import cycle.
+	if within(pass, "slices", "bytes", "runtime") {
 		return
 	}
 

gopls/internal/analysis/modernize/slicescontains.go

@@ -47,8 +47,9 @@ import (
 // (Mostly this appears to be a desirable optimization, avoiding
 // redundantly repeated evaluation.)
 func slicescontains(pass *analysis.Pass) {
-	// Don't modify the slices package itself.
-	if pass.Pkg.Path() == "slices" {
+	// Skip the analyzer in packages where its
+	// fixes would create an import cycle.
+	if within(pass, "slices", "runtime") {
 		return
 	}
 

gopls/internal/analysis/modernize/slicesdelete.go

@@ -21,6 +21,12 @@ import (
 // Other variations that will also have suggested replacements include:
 // append(s[:i-1], s[i:]...) and append(s[:i+k1], s[i+k2:]) where k2 > k1.
 func slicesdelete(pass *analysis.Pass) {
+	// Skip the analyzer in packages where its
+	// fixes would create an import cycle.
+	if within(pass, "slices", "runtime") {
+		return
+	}
+
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	info := pass.TypesInfo
 	report := func(file *ast.File, call *ast.CallExpr, slice1, slice2 *ast.SliceExpr) {

gopls/internal/analysis/modernize/sortslice.go

@@ -36,7 +36,9 @@ import (
 //   - sort.Sort(x) where x has a named slice type whose Less method is the natural order.
 //     -> sort.Slice(x)
 func sortslice(pass *analysis.Pass) {
-	if !analysisinternal.Imports(pass.Pkg, "sort") {
+	// Skip the analyzer in packages where its
+	// fixes would create an import cycle.
+	if within(pass, "slices", "sort", "runtime") {
 		return
 	}
 

gopls/internal/util/moreiters/iters.go

@@ -14,3 +14,13 @@ func First[T any](seq iter.Seq[T]) (z T, ok bool) {
 	}
 	return z, false
 }
+
+// Contains reports whether x is an element of the sequence seq.
+func Contains[T comparable](seq iter.Seq[T], x T) bool {
+	for cand := range seq {
+		if cand == x {
+			return true
+		}
+	}
+	return false
+}