x/tools/internal/refactor/inline: analyzer is non-deterministic

[lfolger]

Go version

not relevant

Output of go env in your module/workspace:

not relevant

What did you do?

While preparing the reproducer for (https://go.dev/issue/67336), I encounter non-determinism in the analyzer or test-framework. I don't know if there is a smaller reproducer either.

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 a 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?

Sometimes the test fails (non-deterministically) with:

=== RUN   TestImport
    third_party/golang/go_tools/go/analysis/analysistest/analysistest.go:263: suggested fixes failed for /tmp/analysistest1470885990/src/b/c/foo.go:
        --- /tmp/analysistest1470885990/src/b/c/foo.go.golden
        +++ actual
        @@ -14,7 +14,8 @@
         )
         
         func foo() {
        -       var _ context.Context = context.TODO()
        +       var _ context.
        +               Context = context.TODO()
                pkg0.Foo(typ.T(5)) // want "inline call of inlined.Baz"
                pkg.Bar()
         }
--- FAIL: TestImport (1.43s)

I don't know if this is an issue with the test framework or with the analyzer.

What did you expect to see?

I would expect the test to always pass (minus the bug tracked in https://go.dev/issue/67336).

[lfolger]

cc: @adonovan

[adonovan]

Thanks for the very helpful test case, which reproduces the bug for me. My immediate guess as to the cause is that the order in which the files (e.g. caller and callee) are added to the token.FileSet is nondeterministic, and that the relative token.Pos values affect the formatter logic that decides when to insert a line break. (That would make this a symptom of #20744.)

[Update: doesn't seem to be exactly that. The Pos values of caller and callee are completely deterministic in the test. When I enable logging (Options.Logf) I notice that the logged binding decl has the same nondeterminism problem in isolation when printed with debugFormatNode(caller.Fset...). Passing new(token.FileSet) makes the logging deterministic though.]

[findleyr]

Seems like this is not going to make it to v0.17. @adonovan it sounds like you've spent some time thinking about this -- is there a quick fix?

[gopherbot]

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

[findleyr]

I could not reproduce this, I'm afraid. I suspect it has to do with the offsets caused by import reformatting, which I have fixed in #67336.

Therefore, I think this fixed but can't be sure, so I'll bump to the next release.

[adonovan]

Running the test exactly as it appears in the issue body, I can still reproduce the failure nondeterministically (20%) at master (1e0d4ee).

I also notice that the output deterministically (100%) includes a blank line after the import of "context".

[findleyr]

Just ran into this in a test I added, and could reproduce reliable within a couple seconds.

The problem is (again) that we're formatting callee nodes using the caller fset. If ever the callee positions are actually in range of a file in the caller fileset, we can end up with an erroneous line break in between nodes.

I have a fix, which is to clear positions correctly before we ever splice in callee nodes.

[gopherbot]

Change https://go.dev/cl/633256 mentions this issue: internal/refactor/inline: substitute groups of dependent arguments

[gopherbot]

Change https://go.dev/cl/633718 mentions this issue: internal/refactor/inline: substitute groups of dependent arguments