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 https://github.com/golang/go/issues/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".