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>
diff --git a/gopls/internal/golang/inline.go b/gopls/internal/golang/inline.go
index f3e213c..50e4935 100644
--- a/gopls/internal/golang/inline.go
+++ b/gopls/internal/golang/inline.go
@@ -113,14 +113,14 @@
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
}
diff --git a/gopls/internal/golang/inline_all.go b/gopls/internal/golang/inline_all.go
index b6439d8..addfe2b 100644
--- a/gopls/internal/golang/inline_all.go
+++ b/gopls/internal/golang/inline_all.go
@@ -193,11 +193,11 @@
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)
}
diff --git a/internal/refactor/inline/analyzer/analyzer.go b/internal/refactor/inline/analyzer/analyzer.go
index 704ef6f..8f9d19e 100644
--- a/internal/refactor/inline/analyzer/analyzer.go
+++ b/internal/refactor/inline/analyzer/analyzer.go
@@ -126,11 +126,12 @@
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
diff --git a/internal/refactor/inline/everything_test.go b/internal/refactor/inline/everything_test.go
index f611149..37904eb 100644
--- a/internal/refactor/inline/everything_test.go
+++ b/internal/refactor/inline/everything_test.go
@@ -173,16 +173,19 @@
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)
diff --git a/internal/refactor/inline/inline.go b/internal/refactor/inline/inline.go
index 78d3413..6f20d2c 100644
--- a/internal/refactor/inline/inline.go
+++ b/internal/refactor/inline/inline.go
@@ -40,20 +40,54 @@
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 = ©
+ // 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 @@
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,7 +338,17 @@
}
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 {
@@ -312,7 +356,7 @@
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 @@
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 @@
// 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 @@
objRenames[i] = newName
}
- res := &result{
+ res := &inlineCallResult{
newImports: newImports,
}
@@ -582,7 +626,7 @@
// 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 @@
(!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 @@
//
// 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 @@
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 @@
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 @@
// 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 @@
}
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 @@
//
// 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 @@
}
return false
}
+
+type unit struct{} // for representing sets as maps
diff --git a/internal/refactor/inline/inline_test.go b/internal/refactor/inline/inline_test.go
index 86b0d71..96d322b 100644
--- a/internal/refactor/inline/inline_test.go
+++ b/internal/refactor/inline/inline_test.go
@@ -244,7 +244,7 @@
// 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 @@
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 @@
}
// 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 @@
// 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 _() { 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 @@
}
// 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 @@
}
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 @@
t.Fatal(err)
}
+ gotContent := res.Content
+
// Compute a single-hunk line-based diff.
srcLines := strings.Split(callerContent, "\n")
gotLines := strings.Split(string(gotContent), "\n")
