From 0eebaabce71008160b146cfde709cd0ab971f928 Mon Sep 17 00:00:00 2001 From: Tim King Date: Tue, 30 Aug 2022 10:28:12 -0700 Subject: [PATCH] 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 Run-TryBot: Tim King Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot gopls-CI: kokoro --- go/analysis/internal/checker/checker.go | 34 ++++- go/analysis/internal/checker/checker_test.go | 9 +- go/analysis/internal/checker/fix_test.go | 143 +++++++++++++++++++ 3 files changed, 175 insertions(+), 11 deletions(-) create mode 100644 go/analysis/internal/checker/fix_test.go diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index ca77a764ce..c4eae37887 100644 --- a/go/analysis/internal/checker/checker.go +++ b/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,7 +309,7 @@ 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 @@ -313,7 +317,9 @@ func applyFixes(roots []*action) { 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 diff --git a/go/analysis/internal/checker/checker_test.go b/go/analysis/internal/checker/checker_test.go index eee211c21a..383a8e1dd5 100644 --- a/go/analysis/internal/checker/checker_test.go +++ b/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) { diff --git a/go/analysis/internal/checker/fix_test.go b/go/analysis/internal/checker/fix_test.go new file mode 100644 index 0000000000..a114c01b64 --- /dev/null +++ b/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) + } + } +}