x/tools/go/analysis: The first time singlechecker -fix parameter is executed, it does not fix the problem in the _test.go file

Question

x/tools/go/analysis: The first time singlechecker -fix parameter is executed, it does not fix the problem in the _test.go file

Abirdcfly opened this issue 2 years ago · 7 comments

What version of Go are you using (`go version`)?

$ go version
go version go1.19 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN="/usr/local/opt/go/libexec/bin"
GOCACHE="/Users/fupeng/Library/Caches/go-build"
GOENV="/Users/fupeng/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/fupeng/go/pkg/mod"
GONOPROXY="*tenxcloud.com"
GONOSUMDB="*tenxcloud.com"
GOOS="darwin"
GOPATH="/Users/fupeng/go"
GOPRIVATE="*tenxcloud.com"
GOPROXY="https://goproxy.cn"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/fupeng/go/src/dupword/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3v/mkwyck4x0f5bxv2gdv4_l8v00000gn/T/go-build2181850160=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

1. Create the simplest linter by following the example in the singlechecker documentation

// GOPATH/showlintererr/main.go
package main

import (
        "golang.org/x/tools/go/analysis/passes/assign"    <------- this Analyzer detects useless assignments.
        "golang.org/x/tools/go/analysis/singlechecker"
)

func main() {
        singlechecker.Main(assign.Analyzer)
}

2. give some error example

// GOPATH/showlinterexample/a.go
package showlinterexample

func A() {
        x := 2
        x = x
        y := 3
        y = y

}

and _test.go:

// GOPATH/showlinterexample/a_test.go
package showlinterexample

import (
        "testing"
)

func TestA(t *testing.T) {
        z := 4
        z = z
        _ = z
}

3. run this linter

$ go run ../showlintererr/main.go -fix -test ./...
/Users/fupeng/go/src/showlinterexample/a.go:5:2: self-assignment of x to x
/Users/fupeng/go/src/showlinterexample/a.go:7:2: self-assignment of y to y
/Users/fupeng/go/src/showlinterexample/a_test.go:9:2: self-assignment of z to z   <---- error in _test.go display !
exit status 3
$ cat a.go
package showlinterexample

func A() {
        x := 2
                                      <------------ error in source file auto-fix
        y := 3

        _, _ = x, y
}
$ cat a_test.go
package showlinterexample

import (
        "testing"
)

func TestA(t *testing.T) {
        z := 4
        z = z                        <------------ error in _test.go file ** not ** auto fix
        _ = z
}
$ go run ../showlintererr/main.go -fix -test ./...
/Users/fupeng/go/src/showlinterexample/a_test.go:9:2: self-assignment of z to z
exit status 3
$ cat a_test.go
package showlinterexample

import (
        "testing"
)

func TestA(t *testing.T) {
        z := 4
                                   <------------ error in _test.go file auto fix run command again.
        _ = z
}

What did you expect to see?

auto-fix should run once, and all error should be fix.

What did you see instead?

errors in the test file are displayed, but not automatically fixed.

Answer 1 · 2022-08-29T16:11:29.000Z

I did some debug.

--- a/vendor/golang.org/x/tools/go/analysis/internal/checker/checker.go
+++ b/vendor/golang.org/x/tools/go/analysis/internal/checker/checker.go
@@ -384,7 +384,9 @@ func applyFixes(roots []*action) {
                return nil
        }

-       visitAll(roots)
+       if err := visitAll(roots); err != nil {
+               fmt.Println(err)
+       }

        fset := token.NewFileSet() // Shared by parse calls below
        // Now we've got a set of valid edits for each file. Get the new file contents.

run again:

$ go run ../showlintererr/main.go -fix -test ./...
analyses applying overlapping text edits affecting pos range (47, 52) and (47, 52)
/Users/fupeng/go/src/showlinterexample/a.go:5:2: self-assignment of x to x
/Users/fupeng/go/src/showlinterexample/a.go:7:2: self-assignment of y to y
/Users/fupeng/go/src/showlinterexample/a_test.go:9:2: self-assignment of z to z
exit status 3

So the problem is that there are multiple copies of the same SuggestedFixes at fix. But SuggestedFixes is part of Diagnostics, so why is the display of Diagnostics not duplicated? Because when it is displayed, the filtering is done manually:
https://github.com/golang/tools/blob/717a671622f7b89965123f259e4db4c2fccbeb4e/go/analysis/internal/checker/checker.go#L502-L508

					k := key{posn, end, act.a, diag.Message}
					if seen[k] {
						continue // duplicate
					}
					seen[k] = true

					analysisflags.PrintPlain(act.pkg.Fset, diag)

but if the json is output, it still shows 2 copies:

$ go run ../showlintererr/main.go -json ./...
{
        "showlinterexample": {
                "assign": [
                        {
                                "posn": "/Users/fupeng/go/src/showlinterexample/a.go:5:2",
                                "message": "self-assignment of x to x"
                        },
                        {
                                "posn": "/Users/fupeng/go/src/showlinterexample/a.go:7:2",
                                "message": "self-assignment of y to y"
                        }
                ]
        },
        "showlinterexample [showlinterexample.test]": {
                "assign": [
                        {
                                "posn": "/Users/fupeng/go/src/showlinterexample/a.go:5:2",
                                "message": "self-assignment of x to x"
                        },
                        {
                                "posn": "/Users/fupeng/go/src/showlinterexample/a.go:7:2",
                                "message": "self-assignment of y to y"
                        },
                        {
                                "posn": "/Users/fupeng/go/src/showlinterexample/a_test.go:9:2",
                                "message": "self-assignment of z to z"
                        }
                ]
        }
}

this json show why.
https://github.com/golang/tools/blob/717a671622f7b89965123f259e4db4c2fccbeb4e/go/packages/packages.go#L185-L197

	// If Tests is set, the loader includes not just the packages
	// matching a particular pattern but also any related test packages,
	// including test-only variants of the package and the test executable.
	//
	// For example, when using the go command, loading "fmt" with Tests=true
	// returns four packages, with IDs "fmt" (the standard package),
	// "fmt [fmt.test]" (the package as compiled for the test),
	// "fmt_test" (the test functions from source files in package fmt_test),
	// and "fmt.test" (the test binary).
	//
	// In build systems with explicit names for tests,
	// setting Tests may have no effect.
	Tests bool

I would like to know how I can avoid outputting the same 2 copies of Diagnostics in linter (with the -test parameter enabled), except for upstream code changes. Thanks.

Answer 2 · 2022-08-29T19:02:26.000Z

cc @matloob @timothy-king @guodongli-google @zpavlinovic

Answer 3 · 2022-08-29T21:46:45.000Z

A Diagnostic per go/packages.Package is the expected behavior. The -test copied above explains what the difference between "showlinterexample" and "showlinterexample [showlinterexample.test]" is. This is the cause of the json containing multiple outputs.

I think it is fine to not have an error if there are overlapping identical edits. This would effectively fix this issue with multiple "go/packages.Packages" referring to the same .go files due to tests. This is also an easy fix.

Thoughts from @zpavlinovic and @adonovan ?

(As a side note, we should be writing out applyFixes errors to the same location as printDiagnostics if we are not already.)

Answer 4 · 2022-08-29T23:35:08.000Z

A Diagnostic per go/packages.Package is the expected behavior. The -test copied above explains what the difference between "showlinterexample" and "showlinterexample [showlinterexample.test]" is. This is the cause of the json containing multiple outputs.

I agree with this.

I think it is fine to not have an error if there are overlapping identical edits. This would effectively fix this issue with multiple "go/packages.Packages" referring to the same .go files due to tests. This is also an easy fix.

Thoughts from @zpavlinovic and @adonovan ?

Could you explain the solution in more detail?

It seems to me that the fix is being applied to showlinterexample [showlinterexample.test], while it should also be applied to showlinterexample (skipped as it is deemed redundant). The fix to showlinterexample [showlinterexample.test] does not show up because it is an artificially created package. One solution would be to not apply fixes to artificially created packages. Or if applying a fix is an idempotent operations, we could also extend the definition of key to include package name.

Answer 5 · 2022-08-30T00:26:41.000Z

We have 5 diagnostics and let's name them after the variable + package: x [no_test], y [no_test], x [test], y [test], and z [test]. To apply fixes, analysis iterates per action (analyzer, package) pair here. We visit showlinterexample and apply x [no_test] and y [no_test]. We then visit showlinterexample [showlinterexample.test], encounter x [test], return err here, and stop.

My proposed fix is to do a deep equality check on offsetedits and not report an error on equality here. We could also instead of returning skip failed suggested fixes. Though requiring all of the suggested fixes of an action to be accepted sounds like it will be more likely to give coherent fixes.

Answer 6 · 2022-08-30T03:02:45.000Z

Change https://go.dev/cl/426594 mentions this issue: go/analysis: skip same SuggestedFixes

Answer 7 · 2022-08-30T17:34:17.000Z

Change https://go.dev/cl/426734 mentions this issue: go/analysis: allow for identical SuggestedFixes

What version of Go are you using (go version)?