Skip to content

Commit

Permalink
internal/refactor/inline: avoid binding decl for name used by other args
Browse files Browse the repository at this point in the history
When inlining, a removed argument does not contain the last use of a
name if that name is also used by another arg that is referenced by the
callee. Implementing this logic improved a test case for
golang/go#65217.

For golang/go#65217

Change-Id: I486306f4ed57d759d5ab65bb390db5e81332d3ef
Reviewed-on: https://go-review.googlesource.com/c/tools/+/629295
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
findleyr authored and gopherbot committed Nov 18, 2024
1 parent 63e03c3 commit 0c01408
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ func _() {

func _() {
var s S
var _ S = s
i := f(s.Int())
_ = i
}
Expand Down
29 changes: 25 additions & 4 deletions internal/refactor/inline/inline.go
Original file line number Diff line number Diff line change
Expand Up @@ -763,11 +763,15 @@ func (st *state) inlineCall() (*inlineCallResult, error) {
n := len(params) - 1
ordinary, extra := args[:n], args[n:]
var elts []ast.Expr
freevars := make(map[string]bool)
pure, effects := true, false
for _, arg := range extra {
elts = append(elts, arg.expr)
pure = pure && arg.pure
effects = effects || arg.effects
for k, v := range arg.freevars {
freevars[k] = v
}
}
args = append(ordinary, &argument{
expr: &ast.CompositeLit{
Expand All @@ -779,7 +783,7 @@ func (st *state) inlineCall() (*inlineCallResult, error) {
pure: pure,
effects: effects,
duplicable: false,
freevars: nil, // not needed
freevars: freevars,
})
}
}
Expand Down Expand Up @@ -1453,9 +1457,26 @@ next:
// references among other arguments which have non-zero references
// within the callee.
if v, ok := caller.lookup(free).(*types.Var); ok && within(v.Pos(), caller.enclosingFunc.Body) && !isUsedOutsideCall(caller, v) {
logf("keeping param %q: arg contains perhaps the last reference to caller local %v @ %v",
param.info.Name, v, caller.Fset.PositionFor(v.Pos(), false))
continue next

// Check to see if the substituted var is used within other args
// whose corresponding params ARE used in the callee
usedElsewhere := func() bool {
for i, param := range params {
if i < len(args) && len(param.info.Refs) > 0 { // excludes original param
for name := range args[i].freevars {
if caller.lookup(name) == v {
return true
}
}
}
}
return false
}
if !usedElsewhere() {
logf("keeping param %q: arg contains perhaps the last reference to caller local %v @ %v",
param.info.Name, v, caller.Fset.PositionFor(v.Pos(), false))
continue next
}
}
}
}
Expand Down
16 changes: 14 additions & 2 deletions internal/refactor/inline/inline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -731,13 +731,25 @@ func TestSubstitution(t *testing.T) {
`func _() { var local int; _ = local }`,
},
{
"Arguments that are used are detected",
"Arguments that are used by other arguments are detected",
`func f(x, y int) { print(x) }`,
`func _() { var z int; f(z, z) }`,
`func _() { var z int; print(z) }`,
},
{
"Arguments that are used by other variadic arguments are detected",
`func f(x int, ys ...int) { print(ys) }`,
`func _() { var z int; f(z, 1, 2, 3, z) }`,
`func _() { var z int; print([]int{1, 2, 3, z}) }`,
},
{
"Arguments that are used by other variadic arguments are detected, 2",
`func f(x int, ys ...int) { print(ys) }`,
`func _() { var z int; f(z) }`,
`func _() {
var z int
var _ int = z
print(z)
print([]int{})
}`,
},
{
Expand Down

0 comments on commit 0c01408

Please sign in to comment.