x/tools/internal/refactor/inline: analyzer generates suggested fixes that move comments incorrectly

[lfolger]

Go version

n/a

Output of go env in your module/workspace:

n/a

What did you do?

This seems like a lot to parse. The important part is (I think) that the analyzer tries to inline a function that requires two new imports one of which is renamed.

func TestImport(t *testing.T) {
	files := map[string]string{
		"define/my/typ/foo.go": `
		package typ
		type T int
		`,

		"some/other/pkg/foo.go": `
		package pkg
		import "context"
		import "define/my/typ"
		func Foo(typ.T) context.Context{ return nil }
		`,

		"one/more/pkg/foo.go": `
		package pkg
		func Bar() {}
		`,

		"to/be/inlined/foo.go": `
			package inlined

			import "context"
			import "some/other/pkg"
			import "define/my/typ"

			// inlineme
			func Baz(ctx context.Context) context.Context {
				return pkg.Foo(typ.T(5))
			}
		`,

		"b/c/foo.go": `
package c
import (
	"context"
	"to/be/inlined"
	"one/more/pkg"
)

const (
	// This is a variable
	someConst = 5
)

func foo() {
	inlined.Baz(context.TODO()) // want "inline call of inlined.Baz"
	pkg.Bar()
}
		`,
		"b/c/foo.go.golden": `package c
import (
	"context"
	"one/more/pkg"
	pkg0 "some/other/pkg"
	
	// This is za variable
	"define/my/typ"
)

const (
	someConst = 5
)

func foo() {
	var _ context.Context = context.TODO()
	pkg0.Foo(typ.T(5)) // want "inline call of inlined.Baz"
	pkg.Bar()
}`,
	}
	dir, cleanup, err := analysistest.WriteFiles(files)
	if err != nil {
		t.Fatal(err)
	}
	analysistest.RunWithSuggestedFixes(t, dir, analyzer.Analyzer, "b/c")
	cleanup()
}

What did you see happen?

The test failed with:

=== RUN   TestImport
    ...analysistest/analysistest.go:263: suggested fixes failed for /tmp/analysistest883636049/src/b/c/foo.go:
        --- /tmp/analysistest883636049/src/b/c/foo.go.golden
        +++ actual
        @@ -4,12 +4,12 @@
                "context"
                "one/more/pkg"
                pkg0 "some/other/pkg"
        -
        +
        +       // This is a variable
                "define/my/typ"
         )

        -const (
        -       // This is a variable
        +const (
                someConst = 5
         )

--- FAIL: TestImport (1.93s)

Sometimes the test fails with a different error due to https://go.dev/issue/67335.

What did you expect to see?

I expected the test to pass.

[lfolger]

cc: @adonovan

[adonovan]

Thanks for the helpful testcase. I suspect this is just another symptom of not storing comments in the tree; that is, #20744.

[findleyr]

Deferring this until we have a better solution for comments.

[gopherbot]

Change https://go.dev/cl/629979 mentions this issue: internal/refactor/inline: fix comment movement due to added imports