Skip to content

Commit

Permalink
gopls/internal/golang: ignore effects for change signature refactoring
Browse files Browse the repository at this point in the history
While using the change signature refactoring, I never want it to
literalize changed signatures based on its (very coarse) effects
analysis. Disable this inlining analysis for the purpose of change
signature refactoring.

For golang/go#70599

Change-Id: I979c4f5b6be520b67a3441fa6e0c55afe1fe9196
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633255
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
findleyr committed Dec 4, 2024
1 parent 3901733 commit e334696
Show file tree
Hide file tree
Showing 7 changed files with 17 additions and 39 deletions.
6 changes: 5 additions & 1 deletion gopls/internal/golang/change_signature.go
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,11 @@ func rewriteCalls(ctx context.Context, rw signatureRewrite) (map[protocol.Docume
}

post := func(got []byte) []byte { return bytes.ReplaceAll(got, []byte(tag), nil) }
return inlineAllCalls(ctx, logf, rw.snapshot, rw.pkg, rw.pgf, rw.origDecl, calleeInfo, post)
opts := &inline.Options{
Logf: logf,
IgnoreEffects: true,
}
return inlineAllCalls(ctx, rw.snapshot, rw.pkg, rw.pgf, rw.origDecl, calleeInfo, post, opts)
}

// reTypeCheck re-type checks orig with new file contents defined by fileMask.
Expand Down
8 changes: 6 additions & 2 deletions gopls/internal/golang/inline_all.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import (
//
// The code below notes where are assumptions are made that only hold true in
// the case of parameter removal (annotated with 'Assumption:')
func inlineAllCalls(ctx context.Context, logf func(string, ...any), snapshot *cache.Snapshot, pkg *cache.Package, pgf *parsego.File, origDecl *ast.FuncDecl, callee *inline.Callee, post func([]byte) []byte) (map[protocol.DocumentURI][]byte, error) {
func inlineAllCalls(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, pgf *parsego.File, origDecl *ast.FuncDecl, callee *inline.Callee, post func([]byte) []byte, opts *inline.Options) (map[protocol.DocumentURI][]byte, error) {
// Collect references.
var refs []protocol.Location
{
Expand Down Expand Up @@ -215,7 +215,7 @@ func inlineAllCalls(ctx context.Context, logf func(string, ...any), snapshot *ca
Call: calls[currentCall],
Content: content,
}
res, err := inline.Inline(caller, callee, &inline.Options{Logf: logf})
res, err := inline.Inline(caller, callee, opts)
if err != nil {
return nil, fmt.Errorf("inlining failed: %v", err)
}
Expand Down Expand Up @@ -252,6 +252,10 @@ func inlineAllCalls(ctx context.Context, logf func(string, ...any), snapshot *ca
// anything in the surrounding scope.
//
// TODO(rfindley): improve this.
logf := func(string, ...any) {}
if opts != nil {
logf = opts.Logf
}
tpkg, tinfo, err = reTypeCheck(logf, callInfo.pkg, map[protocol.DocumentURI]*ast.File{uri: file}, true)
if err != nil {
return nil, bug.Errorf("type checking after inlining failed: %v", err)
Expand Down
5 changes: 1 addition & 4 deletions gopls/internal/test/marker/testdata/codeaction/moveparam.txt
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ func b() int { return 2 }
var _ = basic.Foo(2, 1)

// Check that we can refactor a call with effects in a toplevel var decl.
var _ = func() int {
var a int = a()
return basic.Foo(b(), a)
}()
var _ = basic.Foo(b(), a())

func _() {
// check various refactorings in a function body, and comment handling.
Expand Down
10 changes: 2 additions & 8 deletions gopls/internal/test/marker/testdata/codeaction/removeparam.txt
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,10 @@ func _() {
Ellipsis()
Ellipsis()
Ellipsis()
var _ []any = []any{1, f(), g()}
Ellipsis()
func(_ ...any) {
Ellipsis()
}(h())
var _ []any = i()
Ellipsis()
}

Expand Down Expand Up @@ -225,12 +223,8 @@ func f() int
func g() int

func _() {
var x, _ int = f(), g()
effects(x)
{
var x, _ int = f(), g()
effects(x)
}
effects(f())
effects(f())
}
-- recursive/recursive.go --
package recursive
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,42 +67,34 @@ package a

import (
"mod.test/b"
"mod.test/c"
)

func _() {
var _ c.C = <-b.Chan
b.B(<-b.Chan)
}

func _() {
var _ c.C = <-b.Chan
b.B(<-b.Chan)
}
-- @b/a/a2.go --
package a

import (
"mod.test/b"
"mod.test/c"
)

func _() {
var _ c.C = <-b.Chan
b.B(<-b.Chan)
var _ c.C = <-b.Chan
b.B(<-b.Chan)
}
-- @b/a/a1.go --
package a

import (
"mod.test/b"
"mod.test/c"
)

func _() {
var _ c.C = <-b.Chan
b.B(<-b.Chan)
}
-- @b/a/a4.go --
Expand All @@ -113,11 +105,9 @@ package a
import (
"mod.test/b"
. "mod.test/b"
"mod.test/c"
)

func _() {
var _ c.C = <-Chan
b.B(<-Chan)
}
-- @b/b/b.go --
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,7 @@ import "example.com/rm"

func _() {
x := new(rm.Basic)
var (
t rm.Basic = *x
_ int = sideEffects()
)
t.Foo()
x.Foo()
rm.Basic(1).Foo()
}

Expand Down Expand Up @@ -137,7 +133,6 @@ import "example.com/rm"

func _() {
x := rm.Missing{}
var _ int = sideEffects()
x.M(1, 3, 4)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,10 @@ func _() {
Ellipsis()
Ellipsis()
Ellipsis()
var _ []any = []any{1, f(), g()}
Ellipsis()
func(_ ...any) {
Ellipsis()
}(h())
var _ []any = i()
Ellipsis()
}

Expand Down Expand Up @@ -236,12 +234,8 @@ func f() int
func g() int

func _() {
var x, _ int = f(), g()
effects(x)
{
var x, _ int = f(), g()
effects(x)
}
effects(f())
effects(f())
}
-- recursive/recursive.go --
package recursive
Expand Down

0 comments on commit e334696

Please sign in to comment.