internal/refactor/inline: extensible API

This CL introduces Options and Result struct types to the
public Inline function so that we can extend the input
and outputs as needed.

Options.IgnoreEffects allows a client to choose to
ignore consideration of potential side effects of
call arguments, an unsound "style optimization".

Result.Literalized reports whether the chosen strategy
was literalization. (Some clients may reject the
transformation in that case.)

A follow-up change will refine the API to separate
the diff at the callsite from the logical diff to
the import declaration.

Updates golang/go#67049

Change-Id: Ifcec19d8cfd18fa797e16c7d6994ac916d77dab5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/581802
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>

Author: adonovan

gopls/internal/golang/inline.go

@@ -113,14 +113,14 @@ func inlineCall(ctx context.Context, snapshot *cache.Snapshot, callerPkg *cache.
 		Content: callerPGF.Src,
 	}
 
-	got, err := inline.Inline(logf, caller, callee)
+	res, err := inline.Inline(caller, callee, &inline.Options{Logf: logf})
 	if err != nil {
 		return nil, nil, err
 	}
 
 	return callerPkg.FileSet(), &analysis.SuggestedFix{
 		Message:   fmt.Sprintf("inline call of %v", callee),
-		TextEdits: diffToTextEdits(callerPGF.Tok, diff.Bytes(callerPGF.Src, got)),
+		TextEdits: diffToTextEdits(callerPGF.Tok, diff.Bytes(callerPGF.Src, res.Content)),
 	}, nil
 }
 

gopls/internal/golang/inline_all.go

@@ -193,11 +193,11 @@ func inlineAllCalls(ctx context.Context, logf func(string, ...any), snapshot *ca
 				Call:    calls[currentCall],
 				Content: content,
 			}
-			var err error
-			content, err = inline.Inline(logf, caller, callee)
+			res, err := inline.Inline(caller, callee, &inline.Options{Logf: logf})
 			if err != nil {
 				return nil, fmt.Errorf("inlining failed: %v", err)
 			}
+			content = res.Content
 			if post != nil {
 				content = post(content)
 			}

internal/refactor/inline/analyzer/analyzer.go

@@ -126,11 +126,12 @@ func run(pass *analysis.Pass) (interface{}, error) {
 				Call:    call,
 				Content: content,
 			}
-			got, err := inline.Inline(discard, caller, callee)
+			res, err := inline.Inline(caller, callee, &inline.Options{Logf: discard})
 			if err != nil {
 				pass.Reportf(call.Lparen, "%v", err)
 				return
 			}
+			got := res.Content
 
 			// Suggest the "fix".
 			var textEdits []analysis.TextEdit

internal/refactor/inline/everything_test.go

@@ -173,16 +173,19 @@ func TestEverything(t *testing.T) {
 						t.Fatal(err)
 					}
 
-					got, err := inline.Inline(t.Logf, caller, callee)
+					res, err := inline.Inline(caller, callee, &inline.Options{
+						Logf: t.Logf,
+					})
 					if err != nil {
 						// Write error to a log, but this ok.
 						t.Log(err)
 						return
 					}
+					got := res.Content
 
 					// Print the diff.
 					t.Logf("Got diff:\n%s",
-						diff.Unified("old", "new", string(callerContent), string(got)))
+						diff.Unified("old", "new", string(callerContent), string(res.Content)))
 
 					// Parse and type-check the transformed source.
 					f, err := parser.ParseFile(caller.Fset, callPosn.Filename, got, parser.SkipObjectResolution)

internal/refactor/inline/inline.go

@@ -40,20 +40,54 @@ type Caller struct {
 	enclosingFunc *ast.FuncDecl // top-level function/method enclosing the call, if any
 }
 
-type unit struct{} // for representing sets as maps
+// Options specifies parameters affecting the inliner algorithm.
+// All fields are optional.
+type Options struct {
+	Logf          func(string, ...any) // log output function, records decision-making process
+	IgnoreEffects bool                 // ignore potential side effects of arguments (unsound)
+}
+
+// Result holds the result of code transformation.
+type Result struct {
+	Content     []byte // formatted, transformed content of caller file
+	Literalized bool   // chosen strategy replaced callee() with func(){...}()
+
+	// TODO(adonovan): provide an API for clients that want structured
+	// output: a list of import additions and deletions plus one or more
+	// localized diffs (or even AST transformations, though ownership and
+	// mutation are tricky) near the call site.
+}
 
 // Inline inlines the called function (callee) into the function call (caller)
 // and returns the updated, formatted content of the caller source file.
 //
 // Inline does not mutate any public fields of Caller or Callee.
-//
-// The log records the decision-making process.
-//
-// TODO(adonovan): provide an API for clients that want structured
-// output: a list of import additions and deletions plus one or more
-// localized diffs (or even AST transformations, though ownership and
-// mutation are tricky) near the call site.
-func Inline(logf func(string, ...any), caller *Caller, callee *Callee) ([]byte, error) {
+func Inline(caller *Caller, callee *Callee, opts *Options) (*Result, error) {
+	copy := *opts // shallow copy
+	opts = &copy
+	// Set default options.
+	if opts.Logf == nil {
+		opts.Logf = func(string, ...any) {}
+	}
+
+	st := &state{
+		caller: caller,
+		callee: callee,
+		opts:   opts,
+	}
+	return st.inline()
+}
+
+// state holds the working state of the inliner.
+type state struct {
+	caller *Caller
+	callee *Callee
+	opts   *Options
+}
+
+func (st *state) inline() (*Result, error) {
+	logf, caller, callee := st.opts.Logf, st.caller, st.callee
+
 	logf("inline %s @ %v",
 		debugFormatNode(caller.Fset, caller.Call),
 		caller.Fset.PositionFor(caller.Call.Lparen, false))
@@ -68,7 +102,7 @@ func Inline(logf func(string, ...any), caller *Caller, callee *Callee) ([]byte,
 		return nil, fmt.Errorf("cannot inline calls from files that import \"C\"")
 	}
 
-	res, err := inline(logf, caller, &callee.impl)
+	res, err := st.inlineCall()
 	if err != nil {
 		return nil, err
 	}
@@ -304,15 +338,25 @@ func Inline(logf func(string, ...any), caller *Caller, callee *Callee) ([]byte,
 		}
 		newSrc = formatted
 	}
-	return newSrc, nil
+
+	literalized := false
+	if call, ok := res.new.(*ast.CallExpr); ok && is[*ast.FuncLit](call.Fun) {
+		literalized = true
+	}
+
+	return &Result{
+		Content:     newSrc,
+		Literalized: literalized,
+	}, nil
+
 }
 
 type newImport struct {
 	pkgName string
 	spec    *ast.ImportSpec
 }
 
-type result struct {
+type inlineCallResult struct {
 	newImports []newImport
 	// If elideBraces is set, old is an ast.Stmt and new is an ast.BlockStmt to
 	// be spliced in. This allows the inlining analysis to assert that inlining
@@ -329,9 +373,7 @@ type result struct {
 	old, new    ast.Node // e.g. replace call expr by callee function body expression
 }
 
-type logger = func(string, ...any)
-
-// inline returns a pair of an old node (the call, or something
+// inlineCall returns a pair of an old node (the call, or something
 // enclosing it) and a new node (its replacement, which may be a
 // combination of caller, callee, and new nodes), along with the set
 // of new imports needed.
@@ -350,7 +392,9 @@ type logger = func(string, ...any)
 // candidate for evaluating an alternative fully self-contained tree
 // representation, such as any proposed solution to #20744, or even
 // dst or some private fork of go/ast.)
-func inline(logf logger, caller *Caller, callee *gobCallee) (*result, error) {
+func (st *state) inlineCall() (*inlineCallResult, error) {
+	logf, caller, callee := st.opts.Logf, st.caller, &st.callee.impl
+
 	checkInfoFields(caller.Info)
 
 	// Inlining of dynamic calls is not currently supported,
@@ -554,7 +598,7 @@ func inline(logf logger, caller *Caller, callee *gobCallee) (*result, error) {
 		objRenames[i] = newName
 	}
 
-	res := &result{
+	res := &inlineCallResult{
 		newImports: newImports,
 	}
 
@@ -582,7 +626,7 @@ func inline(logf logger, caller *Caller, callee *gobCallee) (*result, error) {
 
 	// Gather the effective call arguments, including the receiver.
 	// Later, elements will be eliminated (=> nil) by parameter substitution.
-	args, err := arguments(caller, calleeDecl, assign1)
+	args, err := st.arguments(caller, calleeDecl, assign1)
 	if err != nil {
 		return nil, err // e.g. implicit field selection cannot be made explicit
 	}
@@ -880,7 +924,7 @@ func inline(logf logger, caller *Caller, callee *gobCallee) (*result, error) {
 			(!needBindingDecl || (bindingDecl != nil && len(bindingDecl.names) == 0)) {
 
 			// Reduces to: { var (bindings); lhs... := rhs... }
-			if newStmts, ok := assignStmts(logf, caller, stmt, callee, results); ok {
+			if newStmts, ok := st.assignStmts(stmt, results); ok {
 				logf("strategy: reduce assign-context call to { return exprs }")
 				clearPositions(calleeDecl.Body)
 
@@ -1151,7 +1195,7 @@ type argument struct {
 //
 // We compute type-based predicates like pure, duplicable,
 // freevars, etc, now, before we start modifying syntax.
-func arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 func(*types.Var) bool) ([]*argument, error) {
+func (st *state) arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 func(*types.Var) bool) ([]*argument, error) {
 	var args []*argument
 
 	callArgs := caller.Call.Args
@@ -1175,7 +1219,7 @@ func arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 func(*types.Var
 				typ:        caller.Info.TypeOf(recvArg),
 				constant:   caller.Info.Types[recvArg].Value,
 				pure:       pure(caller.Info, assign1, recvArg),
-				effects:    effects(caller.Info, recvArg),
+				effects:    st.effects(caller.Info, recvArg),
 				duplicable: duplicable(caller.Info, recvArg),
 				freevars:   freeVars(caller.Info, recvArg),
 			}
@@ -1229,7 +1273,7 @@ func arguments(caller *Caller, calleeDecl *ast.FuncDecl, assign1 func(*types.Var
 			constant:   tv.Value,
 			spread:     is[*types.Tuple](tv.Type), // => last
 			pure:       pure(caller.Info, assign1, expr),
-			effects:    effects(caller.Info, expr),
+			effects:    st.effects(caller.Info, expr),
 			duplicable: duplicable(caller.Info, expr),
 			freevars:   freeVars(caller.Info, expr),
 		})
@@ -1911,7 +1955,7 @@ func freeishNames(free map[string]bool, t ast.Expr) {
 // effects reports whether an expression might change the state of the
 // program (through function calls and channel receives) and affect
 // the evaluation of subsequent expressions.
-func effects(info *types.Info, expr ast.Expr) bool {
+func (st *state) effects(info *types.Info, expr ast.Expr) bool {
 	effects := false
 	ast.Inspect(expr, func(n ast.Node) bool {
 		switch n := n.(type) {
@@ -1939,6 +1983,15 @@ func effects(info *types.Info, expr ast.Expr) bool {
 		}
 		return true
 	})
+
+	// Even if consideration of effects is not desired,
+	// we continue to compute, log, and discard them.
+	if st.opts.IgnoreEffects && effects {
+		effects = false
+		st.opts.Logf("ignoring potential effects of argument %s",
+			debugFormatNode(st.caller.Fset, expr))
+	}
+
 	return effects
 }
 
@@ -2766,7 +2819,9 @@ func declares(stmts []ast.Stmt) map[string]bool {
 //
 // Note: assignStmts may return (nil, true) if it determines that the rewritten
 // assignment consists only of _ = nil assignments.
-func assignStmts(logf logger, caller *Caller, callerStmt *ast.AssignStmt, callee *gobCallee, returnOperands []ast.Expr) ([]ast.Stmt, bool) {
+func (st *state) assignStmts(callerStmt *ast.AssignStmt, returnOperands []ast.Expr) ([]ast.Stmt, bool) {
+	logf, caller, callee := st.opts.Logf, st.caller, &st.callee.impl
+
 	assert(len(callee.Returns) == 1, "unexpected multiple returns")
 	resultInfo := callee.Returns[0]
 
@@ -3084,3 +3139,5 @@ func hasNonTrivialReturn(returnInfo [][]returnOperandFlags) bool {
 	}
 	return false
 }
+
+type unit struct{} // for representing sets as maps

internal/refactor/inline/inline_test.go

@@ -244,7 +244,7 @@ func doInlineNote(logf func(string, ...any), pkg *packages.Package, file *ast.Fi
 
 	// Do the inlining. For the purposes of the test,
 	// AnalyzeCallee and Inline are a single operation.
-	got, err := func() ([]byte, error) {
+	res, err := func() (*inline.Result, error) {
 		filename := calleePkg.Fset.File(calleeDecl.Pos()).Name()
 		content, err := os.ReadFile(filename)
 		if err != nil {
@@ -267,7 +267,7 @@ func doInlineNote(logf func(string, ...any), pkg *packages.Package, file *ast.Fi
 
 		check := checkNoMutation(caller.File)
 		defer check()
-		return inline.Inline(logf, caller, callee)
+		return inline.Inline(caller, callee, &inline.Options{Logf: logf})
 	}()
 	if err != nil {
 		if wantRE, ok := want.(*regexp.Regexp); ok {
@@ -280,6 +280,7 @@ func doInlineNote(logf func(string, ...any), pkg *packages.Package, file *ast.Fi
 	}
 
 	// Inline succeeded.
+	got := res.Content
 	if want, ok := want.([]byte); ok {
 		got = append(bytes.TrimSpace(got), '\n')
 		want = append(bytes.TrimSpace(want), '\n')
@@ -331,7 +332,7 @@ const funcName = "f"
 // strategy with the checklist of concerns enumerated in the package
 // doc comment.
 type testcase struct {
-	descr          string
+	descr          string // description; substrings enable options (e.g. "IgnoreEffects")
 	callee, caller string // Go source files (sans package decl) of caller, callee
 	want           string // expected new portion of caller file, or "error: regexp"
 }
@@ -1223,6 +1224,12 @@ func TestSubstitutionPreservesArgumentEffectOrder(t *testing.T) {
 			`func _() { f(g(1), g(2), g(3)) }`,
 			`func _() { func() { defer println(any(g(1)), any(g(2)), g(3)) }() }`,
 		},
+		{
+			"Effects are ignored when IgnoreEffects",
+			`func f(x, y int) { println(y, x) }; func g(int) int`,
+			`func _() { f(g(1), g(2)) }`,
+			`func _() { println(g(2), g(1)) }`,
+		},
 	})
 }
 
@@ -1417,7 +1424,7 @@ func runTests(t *testing.T, tests []testcase) {
 			}
 
 			// Analyze callee and inline call.
-			doIt := func() ([]byte, error) {
+			doIt := func() (*inline.Result, error) {
 				callee, err := inline.AnalyzeCallee(t.Logf, fset, pkg, info, decl, []byte(calleeContent))
 				if err != nil {
 					return nil, err
@@ -1436,9 +1443,12 @@ func runTests(t *testing.T, tests []testcase) {
 				}
 				check := checkNoMutation(caller.File)
 				defer check()
-				return inline.Inline(t.Logf, caller, callee)
+				return inline.Inline(caller, callee, &inline.Options{
+					Logf:          t.Logf,
+					IgnoreEffects: strings.Contains(test.descr, "IgnoreEffects"),
+				})
 			}
-			gotContent, err := doIt()
+			res, err := doIt()
 
 			// Want error?
 			if rest := strings.TrimPrefix(test.want, "error: "); rest != test.want {
@@ -1459,6 +1469,8 @@ func runTests(t *testing.T, tests []testcase) {
 				t.Fatal(err)
 			}
 
+			gotContent := res.Content
+
 			// Compute a single-hunk line-based diff.
 			srcLines := strings.Split(callerContent, "\n")
 			gotLines := strings.Split(string(gotContent), "\n")