go/analysis/passes/inline: add lazy-fix mode for gopls

Computation of inliner fixes can be expensive due to its
(unnecessary) strategy of formatting and parsing the file
repeatedly. Rather than fix that right now, this CL makes
the computation of fixes lazy when the analyzer is running
within gopls. An undocumented analyzer flag, set by gopls'
init, causes the analyzer to return an empty list of edits,
which triggers gopls to compute the edits lazily based on
the Diagnostic.Category string. As luck would have it, we
already have a lazy fix in place for the refactor.inline.call
Code Action, so we simply use that name again for the Category.

The change broke an MCP test that assumed that the inline
analyzer's fixes carry edits. There is a TODO in the
MCP code not to rely on this assumption, but rather than
do it, I rewrote the test to use the printf analyzer
instead.

For golang/go#75773

Change-Id: I5388e472e841cb23351fb9e90d8d7721dae3df07
Reviewed-on: https://go-review.googlesource.com/c/tools/+/721180
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>

Author: adonovan

go/analysis/passes/inline/inline.go

@@ -51,11 +51,16 @@ var Analyzer = &analysis.Analyzer{
 	},
 }
 
-var allowBindingDecl bool
+var (
+	allowBindingDecl bool
+	lazyEdits        bool
+)
 
 func init() {
 	Analyzer.Flags.BoolVar(&allowBindingDecl, "allow_binding_decl", false,
 		"permit inlinings that require a 'var params = args' declaration")
+	Analyzer.Flags.BoolVar(&lazyEdits, "lazy_edits", false,
+		"compute edits lazily (only meaningful to gopls driver)")
 }
 
 // analyzer holds the state for this analysis.
@@ -177,66 +182,88 @@ func (a *analyzer) inlineCall(call *ast.CallExpr, cur inspector.Cursor) {
 			return // don't inline a function from within its own test
 		}
 
-		// Inline the call.
-		content, err := a.readFile(call)
-		if err != nil {
-			a.pass.Reportf(call.Lparen, "invalid inlining candidate: cannot read source file: %v", err)
-			return
-		}
-		curFile := astutil.EnclosingFile(cur)
-		caller := &inline.Caller{
-			Fset:    a.pass.Fset,
-			Types:   a.pass.Pkg,
-			Info:    a.pass.TypesInfo,
-			File:    curFile,
-			Call:    call,
-			Content: content,
-			CountUses: func(pkgname *types.PkgName) int {
-				return moreiters.Len(a.index.Uses(pkgname))
-			},
-		}
-		res, err := inline.Inline(caller, callee, &inline.Options{Logf: discard})
-		if err != nil {
-			a.pass.Reportf(call.Lparen, "%v", err)
-			return
-		}
+		// Compute the edits.
+		//
+		// Ordinarily the analyzer reports a fix containing
+		// edits. However, the algorithm is somewhat expensive
+		// (unnecessarily so: see go.dev/issue/75773) so
+		// to reduce costs in gopls, we omit the edits,
+		// meaning that gopls must compute them on demand
+		// (based on the Diagnostic.Category) when they are
+		// requested via a code action.
+		//
+		// This does mean that the following categories of
+		// caller-dependent obstacles to inlining will be
+		// reported when the gopls user requests the fix,
+		// rather than by quietly suppressing the diagnostic:
+		// - shadowing problems
+		// - callee imports inaccessible "internal" packages
+		// - callee refers to nonexported symbols
+		// - callee uses too-new Go features
+		// - inlining call from a cgo file
+		var edits []analysis.TextEdit
+		if !lazyEdits {
+			// Inline the call.
+			content, err := a.readFile(call)
+			if err != nil {
+				a.pass.Reportf(call.Lparen, "invalid inlining candidate: cannot read source file: %v", err)
+				return
+			}
+			curFile := astutil.EnclosingFile(cur)
+			caller := &inline.Caller{
+				Fset:    a.pass.Fset,
+				Types:   a.pass.Pkg,
+				Info:    a.pass.TypesInfo,
+				File:    curFile,
+				Call:    call,
+				Content: content,
+				CountUses: func(pkgname *types.PkgName) int {
+					return moreiters.Len(a.index.Uses(pkgname))
+				},
+			}
+			res, err := inline.Inline(caller, callee, &inline.Options{Logf: discard})
+			if err != nil {
+				a.pass.Reportf(call.Lparen, "%v", err)
+				return
+			}
 
-		if res.Literalized {
-			// Users are not fond of inlinings that literalize
-			// f(x) to func() { ... }(), so avoid them.
-			//
-			// (Unfortunately the inliner is very timid,
-			// and often literalizes when it cannot prove that
-			// reducing the call is safe; the user of this tool
-			// has no indication of what the problem is.)
-			return
-		}
-		if res.BindingDecl && !allowBindingDecl {
-			// When applying fix en masse, users are similarly
-			// unenthusiastic about inlinings that cannot
-			// entirely eliminate the parameters and
-			// insert a 'var params = args' declaration.
-			// The flag allows them to decline such fixes.
-			return
-		}
-		got := res.Content
-
-		// Suggest the "fix".
-		var textEdits []analysis.TextEdit
-		for _, edit := range diff.Bytes(content, got) {
-			textEdits = append(textEdits, analysis.TextEdit{
-				Pos:     curFile.FileStart + token.Pos(edit.Start),
-				End:     curFile.FileStart + token.Pos(edit.End),
-				NewText: []byte(edit.New),
-			})
+			if res.Literalized {
+				// Users are not fond of inlinings that literalize
+				// f(x) to func() { ... }(), so avoid them.
+				//
+				// (Unfortunately the inliner is very timid,
+				// and often literalizes when it cannot prove that
+				// reducing the call is safe; the user of this tool
+				// has no indication of what the problem is.)
+				return
+			}
+			if res.BindingDecl && !allowBindingDecl {
+				// When applying fix en masse, users are similarly
+				// unenthusiastic about inlinings that cannot
+				// entirely eliminate the parameters and
+				// insert a 'var params = args' declaration.
+				// The flag allows them to decline such fixes.
+				return
+			}
+			got := res.Content
+
+			for _, edit := range diff.Bytes(content, got) {
+				edits = append(edits, analysis.TextEdit{
+					Pos:     curFile.FileStart + token.Pos(edit.Start),
+					End:     curFile.FileStart + token.Pos(edit.End),
+					NewText: []byte(edit.New),
+				})
+			}
 		}
+
 		a.pass.Report(analysis.Diagnostic{
-			Pos:     call.Pos(),
-			End:     call.End(),
-			Message: fmt.Sprintf("Call of %v should be inlined", callee),
+			Pos:      call.Pos(),
+			End:      call.End(),
+			Message:  fmt.Sprintf("Call of %v should be inlined", callee),
+			Category: "inline_call", // keep consistent with gopls/internal/golang.fixInlineCall
 			SuggestedFixes: []analysis.SuggestedFix{{
 				Message:   fmt.Sprintf("Inline call of %v", callee),
-				TextEdits: textEdits,
+				TextEdits: edits, // within gopls, this is nil => compute fix's edits lazily
 			}},
 		})
 	}

gopls/internal/golang/fix.go

@@ -54,7 +54,7 @@ const (
 	fixExtractVariableAll      = "extract_variable_all"
 	fixExtractFunction         = "extract_function"
 	fixExtractMethod           = "extract_method"
-	fixInlineCall              = "inline_call"
+	fixInlineCall              = "inline_call" // keep consistent with go/analysis/passes/inline Diagnostic.Category
 	fixInlineVariable          = "inline_variable"
 	fixInvertIfCondition       = "invert_if_condition"
 	fixSplitLines              = "split_lines"

gopls/internal/settings/analysis.go

@@ -5,6 +5,7 @@
 package settings
 
 import (
+	"log"
 	"slices"
 
 	"golang.org/x/tools/go/analysis"
@@ -241,7 +242,7 @@ var DefaultAnalyzers = []*Analyzer{
 		severity:    protocol.SeverityInformation,
 	},
 	// other simplifiers
-	{analyzer: inline.Analyzer, severity: protocol.SeverityHint},
+	{analyzer: inline.Analyzer, severity: protocol.SeverityHint}, // (in -lazy_edit mode)
 	{analyzer: infertypeargs.Analyzer, severity: protocol.SeverityInformation},
 	{analyzer: maprange.Analyzer, severity: protocol.SeverityHint},
 	{analyzer: unusedparams.Analyzer, severity: protocol.SeverityInformation},
@@ -281,3 +282,9 @@ var DefaultAnalyzers = []*Analyzer{
 	{analyzer: noresultvalues.Analyzer},
 	{analyzer: unusedvariable.Analyzer},
 }
+
+func init() {
+	if err := inline.Analyzer.Flags.Set("lazy_edits", "true"); err != nil {
+		log.Fatalf("setting inline -lazy_edits flag: %v", err)
+	}
+}

gopls/internal/test/marker/testdata/mcptools/file_diagnostics.txt

@@ -1,7 +1,13 @@
 This test exercises the "go_file_diagnostics" MCP tool.
 
+Some diagnostics from the "printf" analyzer carry fixes, and those fixes have edits.
+(Not all fixes have edits; some analyzers rely on gopls to compute them lazily.)
+
+The printf analyzer only reports a fix in files using >= go1.24.
+
 -- flags --
 -mcp
+-min_go_command=go1.24
 
 -- settings.json --
 {
@@ -12,6 +18,7 @@ This test exercises the "go_file_diagnostics" MCP tool.
 
 -- go.mod --
 module example.com
+go 1.24
 
 -- a/main.go --
 package main
@@ -38,52 +45,33 @@ Fix:
 -- b/main.go --
 package main
 
-func _() {
-	_ = deprecated([]string{"a"}, "a") //@loc(inline, "deprecated")
-
-	_ = deprecated([]string{"a"}, "a") //@loc(inline2, "deprecated")
-}
+import "fmt"
 
-//go:fix inline
-func deprecated(slice []string, s string) bool {
-	return proposed(slice, s, true)
-}
-
-func proposed(_ []string, _ string, _ bool) bool {
-	return false // fake
-}
+// (diagnostic with fix)
+var (
+	msg string
+	_ = fmt.Sprintf(msg) //@ diag("msg", re"non-constant format string")
+)
 
-//@mcptool("go_file_diagnostics", `{"file":"$WORKDIR/b/main.go"}`, output=diagnoseInline)
-//@diag(inline, re"inline")
-//@diag(inline2, re"inline")
--- @diagnoseInline --
-3:5-3:35: [Hint] Call of main.deprecated should be inlined
-Fix:
---- $WORKDIR/b/main.go
-+++ $WORKDIR/b/main.go
-@@ -1,7 +1,7 @@
- package main
- 
- func _() {
--	_ = deprecated([]string{"a"}, "a") //@loc(inline, "deprecated")
-+	_ = proposed([]string{"a"}, "a", true) //@loc(inline, "deprecated")
- 
- 	_ = deprecated([]string{"a"}, "a") //@loc(inline2, "deprecated")
- }
+// (diagnostic without fix)
+var _ = fmt.Sprintf("%d", "hello") //@ diag("%d", re"%d .* wrong type string")
 
+//@ mcptool("go_file_diagnostics", `{"file":"$WORKDIR/b/main.go"}`, output=diagnosePrintf)
 
-5:5-5:35: [Hint] Call of main.deprecated should be inlined
+-- @diagnosePrintf --
+7:17-7:20: [Warning] non-constant format string in call to fmt.Sprintf
 Fix:
 --- $WORKDIR/b/main.go
 +++ $WORKDIR/b/main.go
-@@ -3,7 +3,7 @@
- func _() {
- 	_ = deprecated([]string{"a"}, "a") //@loc(inline, "deprecated")
- 
--	_ = deprecated([]string{"a"}, "a") //@loc(inline2, "deprecated")
-+	_ = proposed([]string{"a"}, "a", true) //@loc(inline2, "deprecated")
- }
+@@ -5,7 +5,7 @@
+ // (diagnostic with fix)
+ var (
+ 	msg string
+-	_ = fmt.Sprintf(msg) //@ diag("msg", re"non-constant format string")
++	_ = fmt.Sprintf("%s", msg) //@ diag("msg", re"non-constant format string")
+ )
 
- //go:fix inline
+ // (diagnostic without fix)
 
 
+11:21-11:23: [Warning] fmt.Sprintf format %d has arg "hello" of wrong type string

internal/refactor/inline/inline.go

@@ -40,7 +40,7 @@ type Caller struct {
 	Info    *types.Info
 	File    *ast.File
 	Call    *ast.CallExpr
-	Content []byte // source of file containing
+	Content []byte // source of file containing (TODO(adonovan): see comment at Result.Content)
 
 	// CountUses is an optional optimized computation of
 	// the number of times pkgname appears in Info.Uses.
@@ -61,6 +61,18 @@ type Options struct {
 
 // Result holds the result of code transformation.
 type Result struct {
+	// TODO(adonovan): the only textual results that should be
+	// needed are (1) an edit in the vicinity of the call (either
+	// to the CallExpr or one of its ancestors), and optionally
+	// (2) an edit to the import declaration.
+	// Change the inliner API to return a list of edits,
+	// and not to accept a Caller.Content, as it is only
+	// temptation to use such algorithmically expensive
+	// operations as reformatting the entire file, which is
+	// a significant source of non-linear dynamic behavior;
+	// see https://go.dev/issue/75773.
+	// This will require a sequence of changes to the tests
+	// and the inliner algorithm itself.
 	Content     []byte // formatted, transformed content of caller file
 	Literalized bool   // chosen strategy replaced callee() with func(){...}()
 	BindingDecl bool   // transformation added "var params = args" declaration