gopls/internal/analysis/modernize: document workflow

Also, add and document a -category flag on the modernize
analyzer. (This is a stopgap until the checker driver supports
this flag generally; see issue 72008 and CL 655555.)

Fixes golang/go#72008

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

Author: adonovan

gopls/doc/analyzers.md

@@ -476,39 +476,80 @@ Package documentation: [lostcancel](https://pkg.go.dev/golang.org/x/tools/go/ana
 
 
 This analyzer reports opportunities for simplifying and clarifying
-existing code by using more modern features of Go, such as:
-
-  - replacing an if/else conditional assignment by a call to the
-    built-in min or max functions added in go1.21;
-  - replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
-    by a call to slices.Sort(s), added in go1.21;
-  - replacing interface{} by the 'any' type added in go1.18;
-  - replacing append([]T(nil), s...) by slices.Clone(s) or
-    slices.Concat(s), added in go1.21;
-  - replacing a loop around an m[k]=v map update by a call
-    to one of the Collect, Copy, Clone, or Insert functions
-    from the maps package, added in go1.21;
-  - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),
-    added in go1.19;
-  - replacing uses of context.WithCancel in tests with t.Context, added in
-    go1.24;
-  - replacing omitempty by omitzero on structs, added in go1.24;
-  - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),
-    added in go1.21
-  - replacing a 3-clause for i := 0; i < n; i++ {} loop by
-    for i := range n {}, added in go1.22;
-  - replacing Split in "for range strings.Split(...)" by go1.24's
-    more efficient SplitSeq, or Fields with FieldSeq;
+existing code by using more modern features of Go and its standard
+library.
+
+Each diagnostic provides a fix. Our intent is that these fixes may
+be safely applied en masse without changing the behavior of your
+program. In some cases the suggested fixes are imperfect and may
+lead to (for example) unused imports or unused local variables,
+causing build breakage. However, these problems are generally
+trivial to fix. We regard any modernizer whose fix changes program
+behavior to have a serious bug and will endeavor to fix it.
 
 To apply all modernization fixes en masse, you can use the
 following command:
 
-	$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test ./...
+	$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...
 
 If the tool warns of conflicting fixes, you may need to run it more
 than once until it has applied all fixes cleanly. This command is
 not an officially supported interface and may change in the future.
 
+Changes produced by this tool should be reviewed as usual before
+being merged. In some cases, a loop may be replaced by a simple
+function call, causing comments within the loop to be discarded.
+Human judgment may be required to avoid losing comments of value.
+
+Each diagnostic reported by modernize has a specific category. (The
+categories are listed below.) Diagnostics in some categories, such
+as "efaceany" (which replaces "interface{}" with "any" where it is
+safe to do so) are particularly numerous. It may ease the burden of
+code review to apply fixes in two passes, the first change
+consisting only of fixes of category "efaceany", the second
+consisting of all others. This can be achieved using the -category flag:
+
+	$ modernize -category=efaceany  -fix -test ./...
+	$ modernize -category=-efaceany -fix -test ./...
+
+Categories of modernize diagnostic:
+
+  - minmax: replace an if/else conditional assignment by a call to
+    the built-in min or max functions added in go1.21.
+
+  - sortslice: replace sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
+    by a call to slices.Sort(s), added in go1.21.
+
+  - efaceany: replace interface{} by the 'any' type added in go1.18.
+
+  - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or
+    slices.Concat(s), added in go1.21.
+
+  - mapsloop: replace a loop around an m[k]=v map update by a call
+    to one of the Collect, Copy, Clone, or Insert functions from
+    the maps package, added in go1.21.
+
+  - fmtappendf: replace []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),
+    added in go1.19.
+
+  - testingcontext: replace uses of context.WithCancel in tests
+    with t.Context, added in go1.24.
+
+  - omitzero: replace omitempty by omitzero on structs, added in go1.24.
+
+  - bloop: replace "for i := range b.N" or "for range b.N" in a
+    benchmark with "for b.Loop()", and remove any preceding calls
+    to b.StopTimer, b.StartTimer, and b.ResetTimer.
+
+  - slicesdelete: replace append(s[:i], s[i+1]...) by
+    slices.Delete(s, i, i+1), added in go1.21.
+
+  - rangeint: replace a 3-clause "for i := 0; i < n; i++" loop by
+    "for i := range n", added in go1.22.
+
+  - stringseq: replace Split in "for range strings.Split(...)" by go1.24's
+    more efficient SplitSeq, or Fields with FieldSeq.
+
 Default: on.
 
 Package documentation: [modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)

gopls/internal/analysis/modernize/doc.go

@@ -9,36 +9,77 @@
 // modernize: simplify code by using modern constructs
 //
 // This analyzer reports opportunities for simplifying and clarifying
-// existing code by using more modern features of Go, such as:
-//
-//   - replacing an if/else conditional assignment by a call to the
-//     built-in min or max functions added in go1.21;
-//   - replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
-//     by a call to slices.Sort(s), added in go1.21;
-//   - replacing interface{} by the 'any' type added in go1.18;
-//   - replacing append([]T(nil), s...) by slices.Clone(s) or
-//     slices.Concat(s), added in go1.21;
-//   - replacing a loop around an m[k]=v map update by a call
-//     to one of the Collect, Copy, Clone, or Insert functions
-//     from the maps package, added in go1.21;
-//   - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),
-//     added in go1.19;
-//   - replacing uses of context.WithCancel in tests with t.Context, added in
-//     go1.24;
-//   - replacing omitempty by omitzero on structs, added in go1.24;
-//   - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),
-//     added in go1.21
-//   - replacing a 3-clause for i := 0; i < n; i++ {} loop by
-//     for i := range n {}, added in go1.22;
-//   - replacing Split in "for range strings.Split(...)" by go1.24's
-//     more efficient SplitSeq, or Fields with FieldSeq;
+// existing code by using more modern features of Go and its standard
+// library.
+//
+// Each diagnostic provides a fix. Our intent is that these fixes may
+// be safely applied en masse without changing the behavior of your
+// program. In some cases the suggested fixes are imperfect and may
+// lead to (for example) unused imports or unused local variables,
+// causing build breakage. However, these problems are generally
+// trivial to fix. We regard any modernizer whose fix changes program
+// behavior to have a serious bug and will endeavor to fix it.
 //
 // To apply all modernization fixes en masse, you can use the
 // following command:
 //
-//	$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test ./...
+//	$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...
 //
 // If the tool warns of conflicting fixes, you may need to run it more
 // than once until it has applied all fixes cleanly. This command is
 // not an officially supported interface and may change in the future.
+//
+// Changes produced by this tool should be reviewed as usual before
+// being merged. In some cases, a loop may be replaced by a simple
+// function call, causing comments within the loop to be discarded.
+// Human judgment may be required to avoid losing comments of value.
+//
+// Each diagnostic reported by modernize has a specific category. (The
+// categories are listed below.) Diagnostics in some categories, such
+// as "efaceany" (which replaces "interface{}" with "any" where it is
+// safe to do so) are particularly numerous. It may ease the burden of
+// code review to apply fixes in two passes, the first change
+// consisting only of fixes of category "efaceany", the second
+// consisting of all others. This can be achieved using the -category flag:
+//
+//	$ modernize -category=efaceany  -fix -test ./...
+//	$ modernize -category=-efaceany -fix -test ./...
+//
+// Categories of modernize diagnostic:
+//
+//   - minmax: replace an if/else conditional assignment by a call to
+//     the built-in min or max functions added in go1.21.
+//
+//   - sortslice: replace sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
+//     by a call to slices.Sort(s), added in go1.21.
+//
+//   - efaceany: replace interface{} by the 'any' type added in go1.18.
+//
+//   - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or
+//     slices.Concat(s), added in go1.21.
+//
+//   - mapsloop: replace a loop around an m[k]=v map update by a call
+//     to one of the Collect, Copy, Clone, or Insert functions from
+//     the maps package, added in go1.21.
+//
+//   - fmtappendf: replace []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),
+//     added in go1.19.
+//
+//   - testingcontext: replace uses of context.WithCancel in tests
+//     with t.Context, added in go1.24.
+//
+//   - omitzero: replace omitempty by omitzero on structs, added in go1.24.
+//
+//   - bloop: replace "for i := range b.N" or "for range b.N" in a
+//     benchmark with "for b.Loop()", and remove any preceding calls
+//     to b.StopTimer, b.StartTimer, and b.ResetTimer.
+//
+//   - slicesdelete: replace append(s[:i], s[i+1]...) by
+//     slices.Delete(s, i, i+1), added in go1.21.
+//
+//   - rangeint: replace a 3-clause "for i := 0; i < n; i++" loop by
+//     "for i := range n", added in go1.22.
+//
+//   - stringseq: replace Split in "for range strings.Split(...)" by go1.24's
+//     more efficient SplitSeq, or Fields with FieldSeq.
 package modernize

gopls/internal/analysis/modernize/modernize.go

@@ -12,6 +12,7 @@ import (
 	"go/types"
 	"iter"
 	"regexp"
+	"slices"
 	"strings"
 
 	"golang.org/x/tools/go/analysis"
@@ -36,6 +37,15 @@ var Analyzer = &analysis.Analyzer{
 	URL:      "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
 }
 
+// Stopgap until general solution in CL 655555 lands. A change to the
+// cmd/vet CLI requires a proposal whereas a change to an analyzer's
+// flag set does not.
+var category string
+
+func init() {
+	Analyzer.Flags.StringVar(&category, "category", "", "comma-separated list of categories to apply; with a leading '-', a list of categories to ignore")
+}
+
 func run(pass *analysis.Pass) (any, error) {
 	// Decorate pass.Report to suppress diagnostics in generated files.
 	//
@@ -55,6 +65,10 @@ func run(pass *analysis.Pass) (any, error) {
 			if diag.Category == "" {
 				panic("Diagnostic.Category is unset")
 			}
+			// TODO(adonovan): stopgap until CL 655555 lands.
+			if !enabledCategory(category, diag.Category) {
+				return
+			}
 			if _, ok := generated[pass.Fset.File(diag.Pos)]; ok {
 				return // skip checking if it's generated code
 			}
@@ -76,14 +90,7 @@ func run(pass *analysis.Pass) (any, error) {
 	sortslice(pass)
 	testingContext(pass)
 
-	// TODO(adonovan):
-	// - more modernizers here; see #70815.
-	// - opt: interleave these micro-passes within a single inspection.
-	// - solve the "duplicate import" problem (#68765) when a number of
-	//   fixes in the same file are applied in parallel and all add
-	//   the same import. The tests exhibit the problem.
-	// - should all diagnostics be of the form "x can be modernized by y"
-	//   or is that a foolish consistency?
+	// TODO(adonovan): opt: interleave these micro-passes within a single inspection.
 
 	return nil, nil
 }
@@ -159,3 +166,22 @@ var (
 	byteSliceType  = types.NewSlice(types.Typ[types.Byte])
 	omitemptyRegex = regexp.MustCompile(`(?:^json| json):"[^"]*(,omitempty)(?:"|,[^"]*")\s?`)
 )
+
+// enabledCategory reports whether a given category is enabled by the specified
+// filter. filter is a comma-separated list of categories, optionally prefixed
+// with `-` to disable all provided categories. All categories are enabled with
+// an empty filter.
+//
+// (Will be superseded by https://go.dev/cl/655555.)
+func enabledCategory(filter, category string) bool {
+	if filter == "" {
+		return true
+	}
+	// negation must be specified at the start
+	filter, exclude := strings.CutPrefix(filter, "-")
+	filters := strings.Split(filter, ",")
+	if slices.Contains(filters, category) {
+		return !exclude
+	}
+	return exclude
+}