internal/refactor/inline/analyzer: use "//go:fix inline" annotation

This CL changes the proof-of-concept "inliner" analyzer to
use "//go:fix inline" as the marker of a function that should
be inlined. This seems to be the syntax emerging from the
proposal golang/go#32816.

Also, skip inlinings that cause literalization of the callee.
Users of batch tools only want to see reductions.

Updates golang/go#32816
Updates golang/go#70717

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

Author: adonovan

internal/refactor/inline/analyzer/analyzer.go

@@ -9,8 +9,7 @@ import (
 	"go/ast"
 	"go/token"
 	"go/types"
-	"os"
-	"strings"
+	"slices"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
@@ -20,28 +19,26 @@ import (
 	"golang.org/x/tools/internal/refactor/inline"
 )
 
-const Doc = `inline calls to functions with "inlineme" doc comment`
+const Doc = `inline calls to functions with "//go:fix inline" doc comment`
 
 var Analyzer = &analysis.Analyzer{
 	Name:      "inline",
 	Doc:       Doc,
 	URL:       "https://pkg.go.dev/golang.org/x/tools/internal/refactor/inline/analyzer",
 	Run:       run,
-	FactTypes: []analysis.Fact{new(inlineMeFact)},
+	FactTypes: []analysis.Fact{new(goFixInlineFact)},
 	Requires:  []*analysis.Analyzer{inspect.Analyzer},
 }
 
-func run(pass *analysis.Pass) (interface{}, error) {
+func run(pass *analysis.Pass) (any, error) {
 	// Memoize repeated calls for same file.
-	// TODO(adonovan): the analysis.Pass should abstract this (#62292)
-	// as the driver may not be reading directly from the file system.
 	fileContent := make(map[string][]byte)
 	readFile := func(node ast.Node) ([]byte, error) {
 		filename := pass.Fset.File(node.Pos()).Name()
 		content, ok := fileContent[filename]
 		if !ok {
 			var err error
-			content, err = os.ReadFile(filename)
+			content, err = pass.ReadFile(filename)
 			if err != nil {
 				return nil, err
 			}
@@ -50,40 +47,37 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		return content, nil
 	}
 
-	// Pass 1: find functions annotated with an "inlineme"
-	// comment, and export a fact for each one.
+	// Pass 1: find functions annotated with a "//go:fix inline"
+	// comment (the syntax proposed by #32816),
+	// and export a fact for each one.
 	inlinable := make(map[*types.Func]*inline.Callee) // memoization of fact import (nil => no fact)
 	for _, file := range pass.Files {
 		for _, decl := range file.Decls {
-			if decl, ok := decl.(*ast.FuncDecl); ok {
-				// TODO(adonovan): this is just a placeholder.
-				// Use the precise go:fix syntax in the proposal.
-				// Beware that //go: comments are treated specially
-				// by (*ast.CommentGroup).Text().
-				// TODO(adonovan): alternatively, consider using
-				// the universal annotation mechanism sketched in
-				// https://go.dev/cl/489835 (which doesn't yet have
-				// a proper proposal).
-				if strings.Contains(decl.Doc.Text(), "inlineme") {
-					content, err := readFile(file)
-					if err != nil {
-						pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: cannot read source file: %v", err)
-						continue
-					}
-					callee, err := inline.AnalyzeCallee(discard, pass.Fset, pass.Pkg, pass.TypesInfo, decl, content)
-					if err != nil {
-						pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: %v", err)
-						continue
-					}
-					fn := pass.TypesInfo.Defs[decl.Name].(*types.Func)
-					pass.ExportObjectFact(fn, &inlineMeFact{callee})
-					inlinable[fn] = callee
+			if decl, ok := decl.(*ast.FuncDecl); ok &&
+				slices.ContainsFunc(directives(decl.Doc), func(d *directive) bool {
+					return d.Tool == "go" && d.Name == "fix" && d.Args == "inline"
+				}) {
+
+				content, err := readFile(decl)
+				if err != nil {
+					pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: cannot read source file: %v", err)
+					continue
+				}
+				callee, err := inline.AnalyzeCallee(discard, pass.Fset, pass.Pkg, pass.TypesInfo, decl, content)
+				if err != nil {
+					pass.Reportf(decl.Doc.Pos(), "invalid inlining candidate: %v", err)
+					continue
 				}
+				fn := pass.TypesInfo.Defs[decl.Name].(*types.Func)
+				pass.ExportObjectFact(fn, &goFixInlineFact{callee})
+				inlinable[fn] = callee
 			}
 		}
 	}
 
 	// Pass 2. Inline each static call to an inlinable function.
+	//
+	// TODO(adonovan):  handle multiple diffs that each add the same import.
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	nodeFilter := []ast.Node{
 		(*ast.File)(nil),
@@ -100,7 +94,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			// Inlinable?
 			callee, ok := inlinable[fn]
 			if !ok {
-				var fact inlineMeFact
+				var fact goFixInlineFact
 				if pass.ImportObjectFact(fn, &fact) {
 					callee = fact.Callee
 					inlinable[fn] = callee
@@ -129,6 +123,16 @@ func run(pass *analysis.Pass) (interface{}, error) {
 				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
+			}
 			got := res.Content
 
 			// Suggest the "fix".
@@ -156,9 +160,11 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return nil, nil
 }
 
-type inlineMeFact struct{ Callee *inline.Callee }
+// A goFixInlineFact is exported for each function marked "//go:fix inline".
+// It holds information about the callee to support inlining.
+type goFixInlineFact struct{ Callee *inline.Callee }
 
-func (f *inlineMeFact) String() string { return "inlineme " + f.Callee.String() }
-func (*inlineMeFact) AFact()           {}
+func (f *goFixInlineFact) String() string { return "goFixInline " + f.Callee.String() }
+func (*goFixInlineFact) AFact()           {}
 
 func discard(string, ...any) {}

internal/refactor/inline/analyzer/directive.go

@@ -0,0 +1,90 @@
+// 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 analyzer
+
+import (
+	"go/ast"
+	"go/token"
+	"strings"
+)
+
+// -- plundered from the future (CL 605517, issue #68021) --
+
+// TODO(adonovan): replace with ast.Directive after go1.24 (#68021).
+
+// A directive is a comment line with special meaning to the Go
+// toolchain or another tool. It has the form:
+//
+//	//tool:name args
+//
+// The "tool:" portion is missing for the three directives named
+// line, extern, and export.
+//
+// See https://go.dev/doc/comment#Syntax for details of Go comment
+// syntax and https://pkg.go.dev/cmd/compile#hdr-Compiler_Directives
+// for details of directives used by the Go compiler.
+type directive struct {
+	Pos  token.Pos // of preceding "//"
+	Tool string
+	Name string
+	Args string // may contain internal spaces
+}
+
+// directives returns the directives within the comment.
+func directives(g *ast.CommentGroup) (res []*directive) {
+	if g != nil {
+		// Avoid (*ast.CommentGroup).Text() as it swallows directives.
+		for _, c := range g.List {
+			if len(c.Text) > 2 &&
+				c.Text[1] == '/' &&
+				c.Text[2] != ' ' &&
+				isDirective(c.Text[2:]) {
+
+				tool, nameargs, ok := strings.Cut(c.Text[2:], ":")
+				if !ok {
+					// Must be one of {line,extern,export}.
+					tool, nameargs = "", tool
+				}
+				name, args, _ := strings.Cut(nameargs, " ") // tab??
+				res = append(res, &directive{
+					Pos:  c.Slash,
+					Tool: tool,
+					Name: name,
+					Args: strings.TrimSpace(args),
+				})
+			}
+		}
+	}
+	return
+}
+
+// isDirective reports whether c is a comment directive.
+// This code is also in go/printer.
+func isDirective(c string) bool {
+	// "//line " is a line directive.
+	// "//extern " is for gccgo.
+	// "//export " is for cgo.
+	// (The // has been removed.)
+	if strings.HasPrefix(c, "line ") || strings.HasPrefix(c, "extern ") || strings.HasPrefix(c, "export ") {
+		return true
+	}
+
+	// "//[a-z0-9]+:[a-z0-9]"
+	// (The // has been removed.)
+	colon := strings.Index(c, ":")
+	if colon <= 0 || colon+1 >= len(c) {
+		return false
+	}
+	for i := 0; i <= colon+1; i++ {
+		if i == colon {
+			continue
+		}
+		b := c[i]
+		if !('a' <= b && b <= 'z' || '0' <= b && b <= '9') {
+			return false
+		}
+	}
+	return true
+}

internal/refactor/inline/analyzer/testdata/src/a/a.go

@@ -8,10 +8,10 @@ func f() {
 
 type T struct{}
 
-// inlineme
-func One() int { return one } // want One:`inlineme a.One`
+//go:fix inline
+func One() int { return one } // want One:`goFixInline a.One`
 
 const one = 1
 
-// inlineme
-func (T) Two() int { return 2 } // want Two:`inlineme \(a.T\).Two`
+//go:fix inline
+func (T) Two() int { return 2 } // want Two:`goFixInline \(a.T\).Two`

internal/refactor/inline/analyzer/testdata/src/a/a.go.golden

@@ -8,10 +8,10 @@ func f() {
 
 type T struct{}
 
-// inlineme
-func One() int { return one } // want One:`inlineme a.One`
+//go:fix inline
+func One() int { return one } // want One:`goFixInline a.One`
 
 const one = 1
 
-// inlineme
-func (T) Two() int { return 2 } // want Two:`inlineme \(a.T\).Two`
+//go:fix inline
+func (T) Two() int { return 2 } // want Two:`goFixInline \(a.T\).Two`