gopls/internal/analysis/modernize: quick fixes to modernize Go code

This CL is the start of an analyzer that offers a suite of code
transformations to modernize Go code, by taking advantage of
newer features of the language when it would clarify the code
to do so. Because it reports diagnostics on correct code,
its severity is downgraded to INFO.

Only one is implemented so far: the replacement of if/else
conditional assignments by a call to min or max.

+ relnote and test

Updates golang/go#70815

Change-Id: I686db57c2b95dfa399b44e5c6fdc478272cee084
Reviewed-on: https://go-review.googlesource.com/c/tools/+/635777
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/doc/analyzers.md

@@ -436,6 +436,20 @@ Default: on.
 
 Package documentation: [lostcancel](https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/lostcancel)
 
+<a id='modernize'></a>
+## `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 if/else conditional assignments by a call to the
+    built-in min or max functions.
+
+Default: on.
+
+Package documentation: [modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)
+
 <a id='nilfunc'></a>
 ## `nilfunc`: check for useless comparisons between functions and nil
 

gopls/doc/release/v0.18.0.md

@@ -4,6 +4,18 @@
 
 # New features
 
+## New `modernize` analyzer
+
+Gopls will now report when code could be simplified or clarified by
+using more modern features of Go, and provides a quick fix to apply
+the change.
+
+Examples:
+
+- replacement of conditional assignment using an if/else statement by
+  a call to the `min` or `max` built-in functions added in Go 1.18;
+
+
 ## "Implementations" supports generics
 
 At long last, the "Go to Implementations" feature now fully supports

gopls/internal/analysis/modernize/doc.go

@@ -0,0 +1,16 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package modernize providers the modernizer analyzer.
+//
+// # Analyzer modernize
+//
+// 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 if/else conditional assignments by a call to the
+//     built-in min or max functions.
+package modernize

gopls/internal/analysis/modernize/main.go

@@ -0,0 +1,16 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build ignore
+
+// The modernize command suggests (or, with -fix, applies) fixes that
+// clarify Go code by using more modern features.
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/singlechecker"
+	"golang.org/x/tools/gopls/internal/analysis/modernize"
+)
+
+func main() { singlechecker.Main(modernize.Analyzer) }

gopls/internal/analysis/modernize/minmax.go

@@ -0,0 +1,190 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package modernize
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/internal/astutil/cursor"
+)
+
+// The minmax pass replaces if/else statements with calls to min or max.
+//
+// Patterns:
+//
+//  1. if a < b { x = a } else { x = b }        =>      x = min(a, b)
+//  2. x = a; if a < b { x = b }                =>      x = max(a, b)
+//
+// Variants:
+// - all four ordered comparisons
+// - "x := a" or "x = a" or "var x = a" in pattern 2
+// - "x < b" or "a < b" in pattern 2
+func minmax(pass *analysis.Pass) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	for curIfStmt := range cursor.Root(inspect).Preorder((*ast.IfStmt)(nil)) {
+		ifStmt := curIfStmt.Node().(*ast.IfStmt)
+
+		if compare, ok := ifStmt.Cond.(*ast.BinaryExpr); ok &&
+			isInequality(compare.Op) != 0 &&
+			isAssignBlock(ifStmt.Body) {
+
+			tassign := ifStmt.Body.List[0].(*ast.AssignStmt)
+
+			// Have: if a < b { lhs = rhs }
+			var (
+				a   = compare.X
+				b   = compare.Y
+				lhs = tassign.Lhs[0]
+				rhs = tassign.Rhs[0]
+			)
+
+			scope := pass.TypesInfo.Scopes[ifStmt.Body]
+			sign := isInequality(compare.Op)
+
+			if fblock, ok := ifStmt.Else.(*ast.BlockStmt); ok && isAssignBlock(fblock) {
+				fassign := fblock.List[0].(*ast.AssignStmt)
+
+				// Have: if a < b { lhs = rhs } else { lhs2 = rhs2 }
+				lhs2 := fassign.Lhs[0]
+				rhs2 := fassign.Rhs[0]
+
+				// For pattern 1, check that:
+				// - lhs = lhs2
+				// - {rhs,rhs2} = {a,b}
+				if equalSyntax(lhs, lhs2) {
+					if equalSyntax(rhs, a) && equalSyntax(rhs2, b) {
+						sign = +sign
+					} else if equalSyntax(rhs2, a) || equalSyntax(rhs, b) {
+						sign = -sign
+					} else {
+						continue
+					}
+
+					sym := cond(sign < 0, "min", "max")
+
+					if _, obj := scope.LookupParent(sym, ifStmt.Pos()); !is[*types.Builtin](obj) {
+						continue // min/max function is shadowed
+					}
+
+					// pattern 1
+					//
+					// TODO(adonovan): if lhs is declared "var lhs T" on preceding line,
+					// simplify the whole thing to "lhs := min(a, b)".
+					pass.Report(analysis.Diagnostic{
+						// Highlight the condition a < b.
+						Pos:      compare.Pos(),
+						End:      compare.End(),
+						Category: "minmax",
+						Message:  fmt.Sprintf("if/else statement can be modernized using %s", sym),
+						SuggestedFixes: []analysis.SuggestedFix{{
+							Message: fmt.Sprintf("Replace if statement with %s", sym),
+							TextEdits: []analysis.TextEdit{{
+								// Replace IfStmt with lhs = min(a, b).
+								Pos: ifStmt.Pos(),
+								End: ifStmt.End(),
+								NewText: []byte(fmt.Sprintf("%s = %s(%s, %s)",
+									formatNode(pass.Fset, lhs),
+									sym,
+									formatNode(pass.Fset, a),
+									formatNode(pass.Fset, b))),
+							}},
+						}},
+					})
+				}
+
+			} else if prev, ok := curIfStmt.PrevSibling(); ok && is[*ast.AssignStmt](prev.Node()) {
+				fassign := prev.Node().(*ast.AssignStmt)
+
+				// Have: lhs2 = rhs2; if a < b { lhs = rhs }
+				// For pattern 2, check that
+				// - lhs = lhs2
+				// - {rhs,rhs2} = {a,b}, but allow lhs2 to
+				//   stand for rhs2.
+				// TODO(adonovan): accept "var lhs2 = rhs2" form too.
+				lhs2 := fassign.Lhs[0]
+				rhs2 := fassign.Rhs[0]
+
+				if equalSyntax(lhs, lhs2) {
+					if equalSyntax(rhs, a) && (equalSyntax(rhs2, b) || equalSyntax(lhs2, b)) {
+						sign = +sign
+					} else if (equalSyntax(rhs2, a) || equalSyntax(lhs2, a)) && equalSyntax(rhs, b) {
+						sign = -sign
+					} else {
+						continue
+					}
+					sym := cond(sign < 0, "min", "max")
+
+					if _, obj := scope.LookupParent(sym, ifStmt.Pos()); !is[*types.Builtin](obj) {
+						continue // min/max function is shadowed
+					}
+
+					// pattern 2
+					pass.Report(analysis.Diagnostic{
+						// Highlight the condition a < b.
+						Pos:      compare.Pos(),
+						End:      compare.End(),
+						Category: "minmax",
+						Message:  fmt.Sprintf("if statement can be modernized using %s", sym),
+						SuggestedFixes: []analysis.SuggestedFix{{
+							Message: fmt.Sprintf("Replace if/else with %s", sym),
+							TextEdits: []analysis.TextEdit{{
+								// Replace rhs2 and IfStmt with min(a, b)
+								Pos: rhs2.Pos(),
+								End: ifStmt.End(),
+								NewText: []byte(fmt.Sprintf("%s(%s, %s)",
+									sym,
+									formatNode(pass.Fset, a),
+									formatNode(pass.Fset, b))),
+							}},
+						}},
+					})
+				}
+			}
+		}
+	}
+}
+
+// isInequality reports non-zero if tok is one of < <= => >:
+// +1 for > and -1 for <.
+func isInequality(tok token.Token) int {
+	switch tok {
+	case token.LEQ, token.LSS:
+		return -1
+	case token.GEQ, token.GTR:
+		return +1
+	}
+	return 0
+}
+
+// isAssignBlock reports whether b is a block of the form { lhs = rhs }.
+func isAssignBlock(b *ast.BlockStmt) bool {
+	if len(b.List) != 1 {
+		return false
+	}
+	assign, ok := b.List[0].(*ast.AssignStmt)
+	return ok && assign.Tok == token.ASSIGN && len(assign.Lhs) == 1 && len(assign.Rhs) == 1
+}
+
+// -- utils --
+
+func is[T any](x any) bool {
+	_, ok := x.(T)
+	return ok
+}
+
+func cond[T any](cond bool, t, f T) T {
+	if cond {
+		return t
+	} else {
+		return f
+	}
+}

gopls/internal/analysis/modernize/modernize.go

@@ -0,0 +1,50 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package modernize
+
+import (
+	_ "embed"
+	"go/ast"
+	"go/format"
+	"go/token"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/gopls/internal/util/astutil"
+	"golang.org/x/tools/internal/analysisinternal"
+)
+
+//go:embed doc.go
+var doc string
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "modernize",
+	Doc:      analysisinternal.MustExtractDoc(doc, "modernize"),
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+	URL:      "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	minmax(pass)
+	// TODO(adonovan): more modernizers here; see #70815.
+	return nil, nil
+}
+
+// -- helpers --
+
+// equalSyntax reports whether x and y are syntactically equal (ignoring comments).
+func equalSyntax(x, y ast.Expr) bool {
+	sameName := func(x, y *ast.Ident) bool { return x.Name == y.Name }
+	return astutil.Equal(x, y, sameName)
+}
+
+// formatNode formats n.
+func formatNode(fset *token.FileSet, n ast.Node) string {
+	var buf strings.Builder
+	format.Node(&buf, fset, n) // ignore errors
+	return buf.String()
+}

gopls/internal/analysis/modernize/modernize_test.go

@@ -0,0 +1,17 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package modernize_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/gopls/internal/analysis/modernize"
+)
+
+func Test(t *testing.T) {
+	analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), modernize.Analyzer,
+		"minmax")
+}

gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go

@@ -0,0 +1,65 @@
+package minmax
+
+func ifmin(a, b int) {
+	x := a
+	if a < b { // want "if statement can be modernized using max"
+		x = b
+	}
+	print(x)
+}
+
+func ifmax(a, b int) {
+	x := a
+	if a > b { // want "if statement can be modernized using min"
+		x = b
+	}
+	print(x)
+}
+
+func ifminvariant(a, b int) {
+	x := a
+	if x > b { // want "if statement can be modernized using min"
+		x = b
+	}
+	print(x)
+}
+
+func ifmaxvariant(a, b int) {
+	x := b
+	if a < x { // want "if statement can be modernized using min"
+		x = a
+	}
+	print(x)
+}
+
+func ifelsemin(a, b int) {
+	var x int
+	if a <= b { // want "if/else statement can be modernized using min"
+		x = a
+	} else {
+		x = b
+	}
+	print(x)
+}
+
+func ifelsemax(a, b int) {
+	var x int
+	if a >= b { // want "if/else statement can be modernized using max"
+		x = a
+	} else {
+		x = b
+	}
+	print(x)
+}
+
+func shadowed() int {
+	hour, min := 3600, 60
+
+	var time int
+	if hour < min { // silent: the built-in min function is shadowed here
+		time = hour
+	} else {
+		time = min
+	}
+	return time
+}