go/analysis: allow for identical SuggestedFixes

Stop treating identical SuggestedFixes as overlapping text edits.
Packages can be loaded in multiple ways due to testing, e.g.
"p" and "p [p.test]". Currently SuggestedFixes from both are
considered overlapping and so are not applied.

Updates applyFixes() to report errors in more situations.

Fixes golang/go#54740

Change-Id: I73acb3b73d88535144cfae5161faabb0615a1774
Reviewed-on: https://go-review.googlesource.com/c/tools/+/426734
Reviewed-by: Abirdcfly Abirdcfly <[email protected]>
Run-TryBot: Tim King <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>

Author: timothy-king

go/analysis/internal/checker/checker.go

@@ -147,7 +147,11 @@ func Run(args []string, analyzers []*analysis.Analyzer) (exitcode int) {
 	roots := analyze(initial, analyzers)
 
 	if Fix {
-		applyFixes(roots)
+		if err := applyFixes(roots); err != nil {
+			// Fail when applying fixes failed.
+			log.Print(err)
+			return 1
+		}
 	}
 	return printDiagnostics(roots)
 }
@@ -305,15 +309,17 @@ func analyze(pkgs []*packages.Package, analyzers []*analysis.Analyzer) []*action
 	return roots
 }
 
-func applyFixes(roots []*action) {
+func applyFixes(roots []*action) error {
 	visited := make(map[*action]bool)
 	var apply func(*action) error
 	var visitAll func(actions []*action) error
 	visitAll = func(actions []*action) error {
 		for _, act := range actions {
 			if !visited[act] {
 				visited[act] = true
-				visitAll(act.deps)
+				if err := visitAll(act.deps); err != nil {
+					return err
+				}
 				if err := apply(act); err != nil {
 					return err
 				}
@@ -332,6 +338,10 @@ func applyFixes(roots []*action) {
 		edit        offsetedit
 		left, right *node
 	}
+	// Edits x and y are equivalent.
+	equiv := func(x, y offsetedit) bool {
+		return x.start == y.start && x.end == y.end && bytes.Equal(x.newText, y.newText)
+	}
 
 	var insert func(tree **node, edit offsetedit) error
 	insert = func(treeptr **node, edit offsetedit) error {
@@ -345,6 +355,13 @@ func applyFixes(roots []*action) {
 		} else if edit.start >= tree.edit.end {
 			return insert(&tree.right, edit)
 		}
+		if equiv(edit, tree.edit) { // equivalent edits?
+			// We skip over equivalent edits without considering them
+			// an error. This handles identical edits coming from the
+			// multiple ways of loading a package into a
+			// *go/packages.Packages for testing, e.g. packages "p" and "p [p.test]".
+			return nil
+		}
 
 		// Overlapping text edit.
 		return fmt.Errorf("analyses applying overlapping text edits affecting pos range (%v, %v) and (%v, %v)",
@@ -384,14 +401,16 @@ func applyFixes(roots []*action) {
 		return nil
 	}
 
-	visitAll(roots)
+	if err := visitAll(roots); err != nil {
+		return 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.
 	for f, tree := range editsForFile {
 		contents, err := ioutil.ReadFile(f.Name())
 		if err != nil {
-			log.Fatal(err)
+			return err
 		}
 
 		cur := 0 // current position in the file
@@ -432,8 +451,11 @@ func applyFixes(roots []*action) {
 			}
 		}
 
-		ioutil.WriteFile(f.Name(), out.Bytes(), 0644)
+		if err := ioutil.WriteFile(f.Name(), out.Bytes(), 0644); err != nil {
+			return err
+		}
 	}
+	return nil
 }
 
 // printDiagnostics prints the diagnostics for the root packages in either

go/analysis/internal/checker/checker_test.go

@@ -19,14 +19,9 @@ import (
 	"golang.org/x/tools/internal/testenv"
 )
 
-var from, to string
-
 func TestApplyFixes(t *testing.T) {
 	testenv.NeedsGoPackages(t)
 
-	from = "bar"
-	to = "baz"
-
 	files := map[string]string{
 		"rename/test.go": `package rename
 
@@ -75,6 +70,10 @@ var analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
+	const (
+		from = "bar"
+		to   = "baz"
+	)
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	nodeFilter := []ast.Node{(*ast.Ident)(nil)}
 	inspect.Preorder(nodeFilter, func(n ast.Node) {

go/analysis/internal/checker/fix_test.go

@@ -0,0 +1,143 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package checker_test
+
+import (
+	"flag"
+	"io/ioutil"
+	"os"
+	"os/exec"
+	"path"
+	"runtime"
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/internal/checker"
+	"golang.org/x/tools/internal/testenv"
+)
+
+func main() {
+	checker.Fix = true
+	patterns := flag.Args()
+
+	code := checker.Run(patterns, []*analysis.Analyzer{analyzer})
+	os.Exit(code)
+}
+
+// TestFixes ensures that checker.Run applies fixes correctly.
+// This test fork/execs the main function above.
+func TestFixes(t *testing.T) {
+	oses := map[string]bool{"darwin": true, "linux": true}
+	if !oses[runtime.GOOS] {
+		t.Skipf("skipping fork/exec test on this platform")
+	}
+
+	if os.Getenv("TESTFIXES_CHILD") == "1" {
+		// child process
+
+		// replace [progname -test.run=TestFixes -- ...]
+		//      by [progname ...]
+		os.Args = os.Args[2:]
+		os.Args[0] = "vet"
+		main()
+		panic("unreachable")
+	}
+
+	testenv.NeedsTool(t, "go")
+
+	files := map[string]string{
+		"rename/foo.go": `package rename
+
+func Foo() {
+	bar := 12
+	_ = bar
+}
+
+// the end
+`,
+		"rename/intestfile_test.go": `package rename
+
+func InTestFile() {
+	bar := 13
+	_ = bar
+}
+
+// the end
+`,
+		"rename/foo_test.go": `package rename_test
+
+func Foo() {
+	bar := 14
+	_ = bar
+}
+
+// the end
+`,
+	}
+	fixed := map[string]string{
+		"rename/foo.go": `package rename
+
+func Foo() {
+	baz := 12
+	_ = baz
+}
+
+// the end
+`,
+		"rename/intestfile_test.go": `package rename
+
+func InTestFile() {
+	baz := 13
+	_ = baz
+}
+
+// the end
+`,
+		"rename/foo_test.go": `package rename_test
+
+func Foo() {
+	baz := 14
+	_ = baz
+}
+
+// the end
+`,
+	}
+	dir, cleanup, err := analysistest.WriteFiles(files)
+	if err != nil {
+		t.Fatalf("Creating test files failed with %s", err)
+	}
+	defer cleanup()
+
+	args := []string{"-test.run=TestFixes", "--", "rename"}
+	cmd := exec.Command(os.Args[0], args...)
+	cmd.Env = append(os.Environ(), "TESTFIXES_CHILD=1", "GOPATH="+dir, "GO111MODULE=off", "GOPROXY=off")
+
+	out, err := cmd.CombinedOutput()
+	if len(out) > 0 {
+		t.Logf("%s: out=<<%s>>", args, out)
+	}
+	var exitcode int
+	if err, ok := err.(*exec.ExitError); ok {
+		exitcode = err.ExitCode() // requires go1.12
+	}
+
+	const diagnosticsExitCode = 3
+	if exitcode != diagnosticsExitCode {
+		t.Errorf("%s: exited %d, want %d", args, exitcode, diagnosticsExitCode)
+	}
+
+	for name, want := range fixed {
+		path := path.Join(dir, "src", name)
+		contents, err := ioutil.ReadFile(path)
+		if err != nil {
+			t.Errorf("error reading %s: %v", path, err)
+		}
+		if got := string(contents); got != want {
+			t.Errorf("contents of %s file did not match expectations. got=%s, want=%s", path, got, want)
+		}
+	}
+}