diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 4300490a445..8c62c56fa84 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -36,6 +36,12 @@ import ( // and populates it with a GOPATH-style project using filemap (which // maps file names to contents). On success it returns the name of the // directory and a cleanup function to delete it. +// +// TODO(adonovan): provide a newer version that accepts a testing.T, +// calls T.TempDir, and calls T.Fatal on any error, avoiding the need +// to return cleanup or err: +// +// func WriteFilesToTmp(t *testing.T filemap map[string]string) string func WriteFiles(filemap map[string]string) (dir string, cleanup func(), err error) { gopath, err := os.MkdirTemp("", "analysistest") if err != nil { diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index a4cddeb2c6e..fb3c47b1625 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -17,9 +17,8 @@ import ( "flag" "fmt" "go/format" - "go/token" "io" - "io/ioutil" + "maps" "log" "os" @@ -32,10 +31,11 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/checker" + "golang.org/x/tools/go/analysis/internal" "golang.org/x/tools/go/analysis/internal/analysisflags" "golang.org/x/tools/go/packages" + "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/diff" - "golang.org/x/tools/internal/robustio" ) var ( @@ -55,8 +55,12 @@ var ( // IncludeTests indicates whether test files should be analyzed too. IncludeTests = true - // Fix determines whether to apply all suggested fixes. + // Fix determines whether to apply (!Diff) or display (Diff) all suggested fixes. Fix bool + + // Diff causes the file updates to be displayed, but not applied. + // This flag has no effect unless Fix is true. + Diff bool ) // RegisterFlags registers command-line flags used by the analysis driver. @@ -72,6 +76,7 @@ func RegisterFlags() { flag.BoolVar(&IncludeTests, "test", IncludeTests, "indicates whether test files should be analyzed, too") flag.BoolVar(&Fix, "fix", false, "apply all suggested fixes") + flag.BoolVar(&Diff, "diff", false, "with -fix, don't update the files, but print a unified diff") } // Run loads the packages specified by args using go/packages, @@ -170,13 +175,18 @@ func Run(args []string, analyzers []*analysis.Analyzer) int { return 1 } - // Apply all fixes from the root actions. + // Don't print the diagnostics, + // but apply all fixes from the root actions. if Fix { - if err := applyFixes(graph.Roots); err != nil { + if err := applyFixes(graph.Roots, Diff); err != nil { // Fail when applying fixes failed. log.Print(err) return 1 } + // TODO(adonovan): don't proceed to print the text or JSON output + // if we applied fixes; stop here. + // + // return pkgsExitCode } // Print the results. If !RunDespiteErrors and there @@ -265,7 +275,13 @@ func load(patterns []string, allSyntax bool) ([]*packages.Package, error) { } mode |= packages.NeedModule conf := packages.Config{ - Mode: mode, + Mode: mode, + // Ensure that child process inherits correct alias of PWD. + // (See discussion at Dir field of [exec.Command].) + // However, this currently breaks some tests. + // TODO(adonovan): Investigate. + // + // Dir: os.Getenv("PWD"), Tests: IncludeTests, } initial, err := packages.Load(&conf, patterns...) @@ -275,181 +291,257 @@ func load(patterns []string, allSyntax bool) ([]*packages.Package, error) { return initial, err } -// applyFixes applies suggested fixes associated with diagnostics -// reported by the specified actions. It verifies that edits do not -// conflict, even through file-system level aliases such as symbolic -// links, and then edits the files. -func applyFixes(actions []*checker.Action) error { - // Visit all of the actions and accumulate the suggested edits. - paths := make(map[robustio.FileID]string) - editsByAction := make(map[robustio.FileID]map[*checker.Action][]diff.Edit) +// applyFixes attempts to apply the first suggested fix associated +// with each diagnostic reported by the specified actions. +// All fixes must have been validated by [analysisinternal.ValidateFixes]. +// +// Each fix is treated as an independent change; fixes are merged in +// an arbitrary deterministic order as if by a three-way diff tool +// such as the UNIX diff3 command or 'git merge'. Any fix that cannot be +// cleanly merged is discarded, in which case the final summary tells +// the user to re-run the tool. +// TODO(adonovan): make the checker tool re-run the analysis itself. +// +// When the same file is analyzed as a member of both a primary +// package "p" and a test-augmented package "p [p.test]", there may be +// duplicate diagnostics and fixes. One set of fixes will be applied +// and the other will be discarded; but re-running the tool may then +// show zero fixes, which may cause the confused user to wonder what +// happened to the other ones. +// TODO(adonovan): consider pre-filtering completely identical fixes. +// +// A common reason for overlapping fixes is duplicate additions of the +// same import. The merge algorithm may often cleanly resolve such +// fixes, coalescing identical edits, but the merge may sometimes be +// confused by nearby changes. +// +// Even when merging succeeds, there is no guarantee that the +// composition of the two fixes is semantically correct. Coalescing +// identical edits is appropriate for imports, but not for, say, +// increments to a counter variable; the correct resolution in that +// case might be to increment it twice. Or consider two fixes that +// each delete the penultimate reference to an import or local +// variable: each fix is sound individually, and they may be textually +// distant from each other, but when both are applied, the program is +// no longer valid because it has an unreferenced import or local +// variable. +// TODO(adonovan): investigate replacing the final "gofmt" step with a +// formatter that applies the unused-import deletion logic of +// "goimports". +// +// Merging depends on both the order of fixes and they order of edits +// within them. For example, if three fixes add import "a" twice and +// import "b" once, the two imports of "a" may be combined if they +// appear in order [a, a, b], or not if they appear as [a, b, a]. +// TODO(adonovan): investigate an algebraic approach to imports; +// that is, for fixes to Go source files, convert changes within the +// import(...) portion of the file into semantic edits, compose those +// edits algebraically, then convert the result back to edits. +// +// applyFixes returns success if all fixes are valid, could be cleanly +// merged, and the corresponding files were successfully updated. +// +// If showDiff, instead of updating the files it display the final +// patch composed of all the cleanly merged fixes. +// +// TODO(adonovan): handle file-system level aliases such as symbolic +// links using robustio.FileID. +func applyFixes(actions []*checker.Action, showDiff bool) error { + + // Select fixes to apply. + // + // If there are several for a given Diagnostic, choose the first. + // Preserve the order of iteration, for determinism. + type fixact struct { + fix *analysis.SuggestedFix + act *checker.Action + } + var fixes []*fixact for _, act := range actions { - editsForTokenFile := make(map[*token.File][]diff.Edit) for _, diag := range act.Diagnostics { - for _, sf := range diag.SuggestedFixes { - for _, edit := range sf.TextEdits { - // Validate the edit. - // Any error here indicates a bug in the analyzer. - start, end := edit.Pos, edit.End - file := act.Package.Fset.File(start) - if file == nil { - return fmt.Errorf("analysis %q suggests invalid fix: missing file info for pos (%v)", - act.Analyzer.Name, edit.Pos) - } - if !end.IsValid() { - end = start - } - if start > end { - return fmt.Errorf("analysis %q suggests invalid fix: pos (%v) > end (%v)", - act.Analyzer.Name, edit.Pos, edit.End) - } - if eof := token.Pos(file.Base() + file.Size()); end > eof { - return fmt.Errorf("analysis %q suggests invalid fix: end (%v) past end of file (%v)", - act.Analyzer.Name, edit.End, eof) - } - edit := diff.Edit{ - Start: file.Offset(start), - End: file.Offset(end), - New: string(edit.NewText), - } - editsForTokenFile[file] = append(editsForTokenFile[file], edit) + for i := range diag.SuggestedFixes { + fix := &diag.SuggestedFixes[i] + if i == 0 { + fixes = append(fixes, &fixact{fix, act}) + } else { + // TODO(adonovan): abstract the logger. + log.Printf("%s: ignoring alternative fix %q", act, fix.Message) } } } + } - for f, edits := range editsForTokenFile { - id, _, err := robustio.GetFileID(f.Name()) + // Read file content on demand, from the virtual + // file system that fed the analyzer (see #62292). + // + // This cache assumes that all successful reads for the same + // file name return the same content. + // (It is tempting to group fixes by package and do the + // merge/apply/format steps one package at a time, but + // packages are not disjoint, due to test variants, so this + // would not really address the issue.) + baselineContent := make(map[string][]byte) + getBaseline := func(readFile analysisinternal.ReadFileFunc, filename string) ([]byte, error) { + content, ok := baselineContent[filename] + if !ok { + var err error + content, err = readFile(filename) if err != nil { - return err - } - if _, hasId := paths[id]; !hasId { - paths[id] = f.Name() - editsByAction[id] = make(map[*checker.Action][]diff.Edit) + return nil, err } - editsByAction[id][act] = edits + baselineContent[filename] = content } + return content, nil } - // Validate and group the edits to each actual file. - editsByPath := make(map[string][]diff.Edit) - for id, actToEdits := range editsByAction { - path := paths[id] - actions := make([]*checker.Action, 0, len(actToEdits)) - for act := range actToEdits { - actions = append(actions, act) - } + // Apply each fix, updating the current state + // only if the entire fix can be cleanly merged. + accumulatedEdits := make(map[string][]diff.Edit) + goodFixes := 0 +fixloop: + for _, fixact := range fixes { + readFile := internal.Pass(fixact.act).ReadFile + + // Convert analysis.TextEdits to diff.Edits, grouped by file. + // Precondition: a prior call to validateFix succeeded. + fileEdits := make(map[string][]diff.Edit) + fset := fixact.act.Package.Fset + for _, edit := range fixact.fix.TextEdits { + file := fset.File(edit.Pos) + + baseline, err := getBaseline(readFile, file.Name()) + if err != nil { + log.Printf("skipping fix to file %s: %v", file.Name(), err) + continue fixloop + } - // Does any action create conflicting edits? - for _, act := range actions { - edits := actToEdits[act] - if _, invalid := validateEdits(edits); invalid > 0 { - name, x, y := act.Analyzer.Name, edits[invalid-1], edits[invalid] - return diff3Conflict(path, name, name, []diff.Edit{x}, []diff.Edit{y}) + // We choose to treat size mismatch as a serious error, + // as it indicates a concurrent write to at least one file, + // and possibly others (consider a git checkout, for example). + if file.Size() != len(baseline) { + return fmt.Errorf("concurrent file modification detected in file %s (size changed from %d -> %d bytes); aborting fix", + file.Name(), file.Size(), len(baseline)) } + + fileEdits[file.Name()] = append(fileEdits[file.Name()], diff.Edit{ + Start: file.Offset(edit.Pos), + End: file.Offset(edit.End), + New: string(edit.NewText), + }) } - // Does any pair of different actions create edits that conflict? - for j := range actions { - for k := range actions[:j] { - x, y := actions[j], actions[k] - if x.Analyzer.Name > y.Analyzer.Name { - x, y = y, x - } - xedits, yedits := actToEdits[x], actToEdits[y] - combined := append(xedits, yedits...) - if _, invalid := validateEdits(combined); invalid > 0 { - // TODO: consider applying each action's consistent list of edits entirely, - // and then using a three-way merge (such as GNU diff3) on the resulting - // files to report more precisely the parts that actually conflict. - return diff3Conflict(path, x.Analyzer.Name, y.Analyzer.Name, xedits, yedits) + // Apply each set of edits by merging atop + // the previous accumulated state. + after := make(map[string][]diff.Edit) + for file, edits := range fileEdits { + if prev := accumulatedEdits[file]; len(prev) > 0 { + merged, ok := diff.Merge(prev, edits) + if !ok { + // debugging + if false { + log.Printf("%s: fix %s conflicts", fixact.act, fixact.fix.Message) + } + continue fixloop // conflict } + edits = merged } + after[file] = edits } - var edits []diff.Edit - for act := range actToEdits { - edits = append(edits, actToEdits[act]...) + // The entire fix applied cleanly; commit it. + goodFixes++ + maps.Copy(accumulatedEdits, after) + // debugging + if false { + log.Printf("%s: fix %s applied", fixact.act, fixact.fix.Message) } - editsByPath[path], _ = validateEdits(edits) // remove duplicates. already validated. } + badFixes := len(fixes) - goodFixes - // Now we've got a set of valid edits for each file. Apply them. - // TODO(adonovan): don't abort the operation partway just because one file fails. - for path, edits := range editsByPath { - // TODO(adonovan): this should really work on the same - // gulp from the file system that fed the analyzer (see #62292). - contents, err := os.ReadFile(path) - if err != nil { - return err + // Show diff or update files to final state. + var files []string + for file := range accumulatedEdits { + files = append(files, file) + } + sort.Strings(files) // for deterministic -diff + var filesUpdated, totalFiles int + for _, file := range files { + edits := accumulatedEdits[file] + if len(edits) == 0 { + continue // the diffs annihilated (a miracle?) } - out, err := diff.ApplyBytes(contents, edits) + // Apply accumulated fixes. + baseline := baselineContent[file] // (cache hit) + final, err := diff.ApplyBytes(baseline, edits) if err != nil { - return err - } - - // Try to format the file. - if formatted, err := format.Source(out); err == nil { - out = formatted + log.Fatalf("internal error in diff.ApplyBytes: %v", err) } - if err := os.WriteFile(path, out, 0644); err != nil { - return err + // Attempt to format each file. + if formatted, err := format.Source(final); err == nil { + final = formatted } - } - return nil -} -// validateEdits returns a list of edits that is sorted and -// contains no duplicate edits. Returns the index of some -// overlapping adjacent edits if there is one and <0 if the -// edits are valid. -func validateEdits(edits []diff.Edit) ([]diff.Edit, int) { - if len(edits) == 0 { - return nil, -1 - } - equivalent := func(x, y diff.Edit) bool { - return x.Start == y.Start && x.End == y.End && x.New == y.New - } - diff.SortEdits(edits) - unique := []diff.Edit{edits[0]} - invalid := -1 - for i := 1; i < len(edits); i++ { - prev, cur := edits[i-1], edits[i] - // 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]". - if !equivalent(prev, cur) { - unique = append(unique, cur) - if prev.End > cur.Start { - invalid = i + if showDiff { + // Since we formatted the file, we need to recompute the diff. + unified := diff.Unified(file+" (old)", file+" (new)", string(baseline), string(final)) + // TODO(adonovan): abstract the I/O. + os.Stdout.WriteString(unified) + + } else { + // write + totalFiles++ + // TODO(adonovan): abstract the I/O. + if err := os.WriteFile(file, final, 0644); err != nil { + log.Println(err) + continue } + filesUpdated++ } } - return unique, invalid -} - -// diff3Conflict returns an error describing two conflicting sets of -// edits on a file at path. -func diff3Conflict(path string, xlabel, ylabel string, xedits, yedits []diff.Edit) error { - contents, err := ioutil.ReadFile(path) - if err != nil { - return err - } - oldlabel, old := "base", string(contents) - xdiff, err := diff.ToUnified(oldlabel, xlabel, old, xedits, diff.DefaultContextLines) - if err != nil { - return err - } - ydiff, err := diff.ToUnified(oldlabel, ylabel, old, yedits, diff.DefaultContextLines) - if err != nil { - return err + // TODO(adonovan): consider returning a structured result that + // maps each SuggestedFix to its status: + // - invalid + // - secondary, not selected + // - applied + // - had conflicts. + // and a mapping from each affected file to: + // - its final/original content pair, and + // - whether formatting was successful. + // Then file writes and the UI can be applied by the caller + // in whatever form they like. + + // If victory was incomplete, report an error that indicates partial progress. + // + // badFixes > 0 indicates that we decided not to attempt some + // fixes due to conflicts or failure to read the source; still + // it's a relatively benign situation since the user can + // re-run the tool, and we may still make progress. + // + // filesUpdated < totalFiles indicates that some file updates + // failed. This should be rare, but is a serious error as it + // may apply half a fix, or leave the files in a bad state. + // + // These numbers are potentially misleading: + // The denominator includes duplicate conflicting fixes due to + // common files in packages "p" and "p [p.test]", which may + // have been fixed fixed and won't appear in the re-run. + // TODO(adonovan): eliminate identical fixes as an initial + // filtering step. + // + // TODO(adonovan): should we log that n files were updated in case of total victory? + if badFixes > 0 || filesUpdated < totalFiles { + if showDiff { + return fmt.Errorf("%d of %d fixes skipped (e.g. due to conflicts)", badFixes, len(fixes)) + } else { + return fmt.Errorf("applied %d of %d fixes; %d files updated. (Re-run the command to apply more.)", + goodFixes, len(fixes), filesUpdated) + } } - return fmt.Errorf("conflicting edits from %s and %s on %s\nfirst edits:\n%s\nsecond edits:\n%s", - xlabel, ylabel, path, xdiff, ydiff) + return nil } // needFacts reports whether any analysis required by the specified set diff --git a/go/analysis/internal/checker/checker_test.go b/go/analysis/internal/checker/checker_test.go index 76d45adceef..fcf5f66e03e 100644 --- a/go/analysis/internal/checker/checker_test.go +++ b/go/analysis/internal/checker/checker_test.go @@ -5,8 +5,6 @@ package checker_test import ( - "fmt" - "go/ast" "os" "path/filepath" "reflect" @@ -17,7 +15,6 @@ import ( "golang.org/x/tools/go/analysis/analysistest" "golang.org/x/tools/go/analysis/internal/checker" "golang.org/x/tools/go/analysis/passes/inspect" - "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/internal/testenv" "golang.org/x/tools/internal/testfiles" "golang.org/x/tools/txtar" @@ -68,101 +65,6 @@ func Foo() { defer cleanup() } -var renameAnalyzer = &analysis.Analyzer{ - Name: "rename", - Requires: []*analysis.Analyzer{inspect.Analyzer}, - Run: run, - Doc: "renames symbols named bar to baz", - RunDespiteErrors: true, -} - -var otherAnalyzer = &analysis.Analyzer{ // like analyzer but with a different Name. - Name: "other", - Requires: []*analysis.Analyzer{inspect.Analyzer}, - Run: run, - Doc: "renames symbols named bar to baz only in package 'other'", -} - -func run(pass *analysis.Pass) (interface{}, error) { - // TODO(adonovan): replace this entangled test with something completely data-driven. - const ( - from = "bar" - to = "baz" - conflict = "conflict" // add conflicting edits to package conflict. - duplicate = "duplicate" // add duplicate edits to package conflict. - other = "other" // add conflicting edits to package other from different analyzers. - ) - - if pass.Analyzer.Name == other { - if pass.Pkg.Name() != other { - return nil, nil // only apply Analyzer other to packages named other - } - } - - inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - nodeFilter := []ast.Node{(*ast.Ident)(nil)} - inspect.Preorder(nodeFilter, func(n ast.Node) { - ident := n.(*ast.Ident) - if ident.Name == from { - msg := fmt.Sprintf("renaming %q to %q", from, to) - edits := []analysis.TextEdit{ - {Pos: ident.Pos(), End: ident.End(), NewText: []byte(to)}, - } - switch pass.Pkg.Name() { - case conflict: - // Conflicting edits are legal, so long as they appear in different fixes. - pass.Report(analysis.Diagnostic{ - Pos: ident.Pos(), - End: ident.End(), - Message: msg, - SuggestedFixes: []analysis.SuggestedFix{{ - Message: msg, TextEdits: []analysis.TextEdit{ - {Pos: ident.Pos() - 1, End: ident.End(), NewText: []byte(to)}, - }, - }}, - }) - pass.Report(analysis.Diagnostic{ - Pos: ident.Pos(), - End: ident.End(), - Message: msg, - SuggestedFixes: []analysis.SuggestedFix{{ - Message: msg, TextEdits: []analysis.TextEdit{ - {Pos: ident.Pos(), End: ident.End() - 1, NewText: []byte(to)}, - }, - }}, - }) - pass.Report(analysis.Diagnostic{ - Pos: ident.Pos(), - End: ident.End(), - Message: msg, - SuggestedFixes: []analysis.SuggestedFix{{ - Message: msg, TextEdits: []analysis.TextEdit{ - {Pos: ident.Pos(), End: ident.End(), NewText: []byte("lorem ipsum")}, - }, - }}, - }) - return - - case duplicate: - // Duplicate (non-insertion) edits are disallowed, - // so this is a buggy analyzer, and validatedFixes should reject it. - edits = append(edits, edits...) - case other: - if pass.Analyzer.Name == other { - edits[0].Pos++ // shift by one to mismatch analyzer and other - } - } - pass.Report(analysis.Diagnostic{ - Pos: ident.Pos(), - End: ident.End(), - Message: msg, - SuggestedFixes: []analysis.SuggestedFix{{Message: msg, TextEdits: edits}}}) - } - }) - - return nil, nil -} - func TestRunDespiteErrors(t *testing.T) { testenv.NeedsGoPackages(t) testenv.RedirectStderr(t) // associate checker.Run output with this test diff --git a/go/analysis/internal/checker/fix_test.go b/go/analysis/internal/checker/fix_test.go index 4063aed35cd..8fb7506ac70 100644 --- a/go/analysis/internal/checker/fix_test.go +++ b/go/analysis/internal/checker/fix_test.go @@ -5,47 +5,44 @@ package checker_test import ( + "bytes" "flag" "fmt" + "go/ast" "go/token" "log" "os" "os/exec" - "path" + "path/filepath" "regexp" "runtime" + "slices" "strings" "testing" "golang.org/x/tools/go/analysis" - "golang.org/x/tools/go/analysis/analysistest" "golang.org/x/tools/go/analysis/checker" "golang.org/x/tools/go/analysis/multichecker" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/expect" "golang.org/x/tools/go/packages" + "golang.org/x/tools/internal/diff" "golang.org/x/tools/internal/testenv" + "golang.org/x/tools/internal/testfiles" + "golang.org/x/tools/txtar" ) -// These are the analyzers available to the multichecker. -// (Tests may add more in init functions as needed.) -var candidates = map[string]*analysis.Analyzer{ - renameAnalyzer.Name: renameAnalyzer, - otherAnalyzer.Name: otherAnalyzer, -} - func TestMain(m *testing.M) { - // If the ANALYZERS=a,..,z environment is set, then this - // process should behave like a multichecker with the - // named analyzers. - if s, ok := os.LookupEnv("ANALYZERS"); ok { - var analyzers []*analysis.Analyzer - for _, name := range strings.Split(s, ",") { - a := candidates[name] - if a == nil { - log.Fatalf("no such analyzer: %q", name) - } - analyzers = append(analyzers, a) - } - multichecker.Main(analyzers...) + // If the CHECKER_TEST_CHILD environment variable is set, + // this process should behave like a multichecker. + // Analyzers are selected by flags. + if _, ok := os.LookupEnv("CHECKER_TEST_CHILD"); ok { + multichecker.Main( + markerAnalyzer, + noendAnalyzer, + renameAnalyzer, + ) panic("unreachable") } @@ -60,126 +57,6 @@ const ( exitCodeDiagnostics = 3 // diagnostics were reported ) -// fix runs a multichecker subprocess with -fix in the specified -// directory, applying the comma-separated list of named analyzers to -// the packages matching the patterns. It returns the CombinedOutput. -func fix(t *testing.T, dir, analyzers string, wantExit int, patterns ...string) string { - testenv.NeedsExec(t) - testenv.NeedsTool(t, "go") - - cmd := exec.Command(os.Args[0], "-fix") - cmd.Args = append(cmd.Args, patterns...) - cmd.Env = append(os.Environ(), - "ANALYZERS="+analyzers, - "GOPATH="+dir, - "GO111MODULE=off", - "GOPROXY=off") - - clean := func(s string) string { - return strings.ReplaceAll(s, os.TempDir(), "os.TempDir/") - } - outBytes, err := cmd.CombinedOutput() - switch err := err.(type) { - case nil: - // success - case *exec.ExitError: - if code := err.ExitCode(); code != wantExit { - // plan9 ExitCode() currently only returns 0 for success or 1 for failure - if !(runtime.GOOS == "plan9" && wantExit != exitCodeSuccess && code != exitCodeSuccess) { - t.Errorf("exit code was %d, want %d", code, wantExit) - } - } - default: - t.Fatalf("failed to execute multichecker: %v", err) - } - - out := clean(string(outBytes)) - t.Logf("$ %s\n%s", clean(fmt.Sprint(cmd)), out) - - return out -} - -// TestFixes ensures that checker.Run applies fixes correctly. -// This test fork/execs the main function above. -func TestFixes(t *testing.T) { - 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() - - fix(t, dir, "rename,other", exitCodeDiagnostics, "rename") - - for name, want := range fixed { - path := path.Join(dir, "src", name) - contents, err := os.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) - } - } -} - // TestReportInvalidDiagnostic tests that a call to pass.Report with // certain kind of invalid diagnostic (e.g. conflicting fixes) // promptly results in a panic. @@ -291,142 +168,420 @@ func TestReportInvalidDiagnostic(t *testing.T) { } } -// TestConflict ensures that checker.Run detects conflicts correctly. -// This test fork/execs the main function above. -func TestConflict(t *testing.T) { - files := map[string]string{ - "conflict/foo.go": `package conflict - -func Foo() { - bar := 12 - _ = bar -} +// TestScript runs script-driven tests in testdata/*.txt. +// Each file is a txtar archive, expanded to a temporary directory. +// +// The comment section of the archive is a script, with the following +// commands: +// +// # comment +// ignored +// blank line +// ignored +// skip k=v... +// Skip the test if any k=v string is a substring of the string +// "GOOS=darwin GOARCH=arm64" appropriate to the current build. +// checker args... +// Run the checker command with the specified space-separated +// arguments; this fork+execs the [TestMain] function above. +// If the archive has a "stdout" section, its contents must +// match the stdout output of the checker command. +// Do NOT use this for testing -diff: tests should not +// rely on the particulars of the diff algorithm. +// exit int +// Assert that previous checker command had this exit code. +// stderr regexp +// Assert that stderr output from previous checker run matches this pattern. +// +// The script must include at least one 'checker' command. +func TestScript(t *testing.T) { + testenv.NeedsExec(t) + testenv.NeedsGoPackages(t) -// the end -`, - } - dir, cleanup, err := analysistest.WriteFiles(files) + txtfiles, err := filepath.Glob("testdata/*.txt") if err != nil { - t.Fatalf("Creating test files failed with %s", err) + t.Fatal(err) } - defer cleanup() - - out := fix(t, dir, "rename,other", exitCodeFailed, "conflict") + for _, txtfile := range txtfiles { + t.Run(txtfile, func(t *testing.T) { + t.Parallel() + + // Expand archive into tmp tree. + ar, err := txtar.ParseFile(txtfile) + if err != nil { + t.Fatal(err) + } + fs, err := txtar.FS(ar) + if err != nil { + t.Fatal(err) + } + dir := testfiles.CopyToTmp(t, fs) + + // Parse txtar comment as a script. + const noExitCode = -999 + var ( + // state variables operated on by script + lastExitCode = noExitCode + lastStderr string + ) + for i, line := range strings.Split(string(ar.Comment), "\n") { + line = strings.TrimSpace(line) + if line == "" || line[0] == '#' { + continue // skip blanks and comments + } - pattern := `conflicting edits from rename and rename on .*foo.go` - matched, err := regexp.MatchString(pattern, out) - if err != nil { - t.Errorf("error matching pattern %s: %v", pattern, err) - } else if !matched { - t.Errorf("output did not match pattern: %s", pattern) + command, rest, _ := strings.Cut(line, " ") + prefix := fmt.Sprintf("%s:%d: %s", txtfile, i+1, command) // for error messages + switch command { + case "checker": + cmd := exec.Command(os.Args[0], strings.Fields(rest)...) + cmd.Dir = dir + cmd.Stdout = new(strings.Builder) + cmd.Stderr = new(strings.Builder) + cmd.Env = append(os.Environ(), "CHECKER_TEST_CHILD=1", "GOPROXY=off") + if err := cmd.Run(); err != nil { + if err, ok := err.(*exec.ExitError); ok { + lastExitCode = err.ExitCode() + // fall through + } else { + t.Fatalf("%s: failed to execute checker: %v (%s)", prefix, err, cmd) + } + } else { + lastExitCode = 0 // success + } + + // Eliminate nondeterministic strings from the output. + clean := func(x any) string { + s := fmt.Sprint(x) + pwd, _ := os.Getwd() + if realDir, err := filepath.EvalSymlinks(dir); err == nil { + // Work around checker's packages.Load failing to + // set Config.Dir to dir, causing the filenames + // of loaded packages not to be a subdir of dir. + s = strings.ReplaceAll(s, realDir, dir) + } + s = strings.ReplaceAll(s, dir, string(os.PathSeparator)+"TMP") + s = strings.ReplaceAll(s, pwd, string(os.PathSeparator)+"PWD") + s = strings.ReplaceAll(s, cmd.Path, filepath.Base(cmd.Path)) + return s + } + + lastStderr = clean(cmd.Stderr) + stdout := clean(cmd.Stdout) + + // Detect bad markers out of band: + // though they cause a non-zero exit, + // that may be expected. + if strings.Contains(lastStderr, badMarker) { + t.Errorf("marker analyzer encountered errors; stderr=%s", lastStderr) + } + + // debugging + if false { + t.Logf("%s: $ %s\nstdout:\n%s\nstderr:\n%s", prefix, clean(cmd), stdout, lastStderr) + } + + unified := func(xlabel, ylabel string, x, y []byte) string { + x = append(slices.Clip(bytes.TrimSpace(x)), '\n') + y = append(slices.Clip(bytes.TrimSpace(y)), '\n') + return diff.Unified(xlabel, ylabel, string(x), string(y)) + } + + // Check stdout, if there's a section of that name. + // + // Do not use this for testing -diff! It exposes tests to the + // internals of our (often suboptimal) diff algorithm. + // Instead, use the want/ mechanism. + if f := section(ar, "stdout"); f != nil { + got, want := []byte(stdout), f.Data + if diff := unified("got", "want", got, want); diff != "" { + t.Errorf("%s: unexpected stdout: -- got --\n%s-- want --\n%s-- diff --\n%s", + prefix, + got, want, diff) + } + } + + for _, f := range ar.Files { + // For each file named want/X, assert that the + // current content of X now equals want/X. + if filename, ok := strings.CutPrefix(f.Name, "want/"); ok { + fixed, err := os.ReadFile(filepath.Join(dir, filename)) + if err != nil { + t.Errorf("reading %s: %v", filename, err) + continue + } + var original []byte + if f := section(ar, filename); f != nil { + original = f.Data + } + want := f.Data + if diff := unified(filename+" (fixed)", filename+" (want)", fixed, want); diff != "" { + t.Errorf("%s: unexpected %s content:\n"+ + "-- original --\n%s\n"+ + "-- fixed --\n%s\n"+ + "-- want --\n%s\n"+ + "-- diff original fixed --\n%s\n"+ + "-- diff fixed want --\n%s", + prefix, filename, + original, + fixed, + want, + unified(filename+" (original)", filename+" (fixed)", original, fixed), + diff) + } + } + } + + case "skip": + config := fmt.Sprintf("GOOS=%s GOARCH=%s", runtime.GOOS, runtime.GOARCH) + for _, word := range strings.Fields(rest) { + if strings.Contains(config, word) { + t.Skip(word) + } + } + + case "exit": + if lastExitCode == noExitCode { + t.Fatalf("%s: no prior 'checker' command", prefix) + } + var want int + if _, err := fmt.Sscanf(rest, "%d", &want); err != nil { + t.Fatalf("%s: requires one numeric operand", prefix) + } + if want != lastExitCode { + // plan9 ExitCode() currently only returns 0 for success or 1 for failure + if !(runtime.GOOS == "plan9" && want != exitCodeSuccess && lastExitCode != exitCodeSuccess) { + t.Errorf("%s: exit code was %d, want %d", prefix, lastExitCode, want) + } + } + + case "stderr": + if lastExitCode == noExitCode { + t.Fatalf("%s: no prior 'checker' command", prefix) + } + if matched, err := regexp.MatchString(rest, lastStderr); err != nil { + t.Fatalf("%s: invalid regexp: %v", prefix, err) + } else if !matched { + t.Errorf("%s: output didn't match pattern %q:\n%s", prefix, rest, lastStderr) + } + + default: + t.Errorf("%s: unknown command", prefix) + } + } + if lastExitCode == noExitCode { + t.Errorf("test script contains no 'checker' command") + } + }) } +} - // No files updated - for name, want := range files { - path := path.Join(dir, "src", name) - contents, err := os.ReadFile(path) - if err != nil { - t.Errorf("error reading %s: %v", path, err) +const badMarker = "[bad marker]" + +// The marker analyzer generates fixes from @marker annotations in the +// source. Each marker is of the form: +// +// @message("pattern", "replacement) +// +// The "message" is used for both the Diagnostic.Message and +// SuggestedFix.Message field. Multiple markers with the same +// message form a single diagnostic and fix with a list of textedits. +// +// The "pattern" is a regular expression that must match on the +// current line (though it may extend beyond if the pattern starts +// with "(?s)"), and whose extent forms the TextEdit.{Pos,End} +// deletion. If the pattern contains one subgroup, its range will be +// used; this allows contextual matching. +// +// The "replacement" is a literal string that forms the +// TextEdit.NewText. +// +// Fixes are applied in the order they are first mentioned in the +// source. +var markerAnalyzer = &analysis.Analyzer{ + Name: "marker", + Doc: "doc", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: func(pass *analysis.Pass) (_ any, err error) { + // Errors returned by this analyzer cause the + // checker command to exit non-zero, but that + // may be the expected outcome for other reasons + // (e.g. there were diagnostics). + // + // So, we report these errors out of band by logging + // them with a special badMarker string that the + // TestScript harness looks for, to ensure that the + // test fails in that case. + defer func() { + if err != nil { + log.Printf("%s: %v", badMarker, err) + } + }() + + // Parse all notes in the files. + var keys []string + edits := make(map[string][]analysis.TextEdit) + for _, file := range pass.Files { + tokFile := pass.Fset.File(file.FileStart) + content, err := pass.ReadFile(tokFile.Name()) + if err != nil { + return nil, err + } + notes, err := expect.ExtractGo(pass.Fset, file) + if err != nil { + return nil, err + } + for _, note := range notes { + edit, err := markerEdit(tokFile, content, note) + if err != nil { + return nil, fmt.Errorf("%s: %v", tokFile.Position(note.Pos), err) + } + // Preserve note order as it determines fix order. + if edits[note.Name] == nil { + keys = append(keys, note.Name) + } + edits[note.Name] = append(edits[note.Name], edit) + } } - if got := string(contents); got != want { - t.Errorf("contents of %s file updated. got=%s, want=%s", path, got, want) + + // Report each fix in its own Diagnostic. + for _, key := range keys { + edits := edits[key] + // debugging + if false { + log.Printf("%s: marker: @%s: %+v", pass.Fset.Position(edits[0].Pos), key, edits) + } + pass.Report(analysis.Diagnostic{ + Pos: edits[0].Pos, + End: edits[0].Pos, + Message: key, + SuggestedFixes: []analysis.SuggestedFix{{ + Message: key, + TextEdits: edits, + }}, + }) } - } + return nil, nil + }, } -// TestOther ensures that checker.Run reports conflicts from -// distinct actions correctly. -// This test fork/execs the main function above. -func TestOther(t *testing.T) { - files := map[string]string{ - "other/foo.go": `package other - -func Foo() { - bar := 12 - _ = bar -} +// markerEdit returns the TextEdit denoted by note. +func markerEdit(tokFile *token.File, content []byte, note *expect.Note) (analysis.TextEdit, error) { + if len(note.Args) != 2 { + return analysis.TextEdit{}, fmt.Errorf("got %d args, want @%s(pattern, replacement)", len(note.Args), note.Name) + } -// the end -`, + pattern, ok := note.Args[0].(string) + if !ok { + return analysis.TextEdit{}, fmt.Errorf("got %T for pattern, want string", note.Args[0]) } - dir, cleanup, err := analysistest.WriteFiles(files) + rx, err := regexp.Compile(pattern) if err != nil { - t.Fatalf("Creating test files failed with %s", err) + return analysis.TextEdit{}, fmt.Errorf("invalid pattern regexp: %v", err) } - defer cleanup() - - // The 'rename' and 'other' analyzers suggest conflicting fixes. - out := fix(t, dir, "rename,other", exitCodeFailed, "other") - pattern := `.*conflicting edits from other and rename on .*foo.go` - matched, err := regexp.MatchString(pattern, out) - if err != nil { - t.Errorf("error matching pattern %s: %v", pattern, err) - } else if !matched { - t.Errorf("output did not match pattern: %s", pattern) + // Match the pattern against the current line. + lineStart := tokFile.LineStart(tokFile.Position(note.Pos).Line) + lineStartOff := tokFile.Offset(lineStart) + lineEndOff := tokFile.Offset(note.Pos) + matches := rx.FindSubmatchIndex(content[lineStartOff:]) + if len(matches) == 0 { + return analysis.TextEdit{}, fmt.Errorf("no match for regexp %q", rx) } - - // No files updated - for name, want := range files { - path := path.Join(dir, "src", name) - contents, err := os.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 updated. got=%s, want=%s", path, got, want) - } + var start, end int // line-relative offset + switch len(matches) { + case 2: + // no subgroups: return the range of the regexp expression + start, end = matches[0], matches[1] + case 4: + // one subgroup: return its range + start, end = matches[2], matches[3] + default: + return analysis.TextEdit{}, fmt.Errorf("invalid location regexp %q: expect either 0 or 1 subgroups, got %d", rx, len(matches)/2-1) + } + if start > lineEndOff-lineStartOff { + // The start of the match must be between the start of the line and the + // marker position (inclusive). + return analysis.TextEdit{}, fmt.Errorf("no matching range found starting on the current line") } -} -// TestNoEnd tests that a missing SuggestedFix.End position is -// correctly interpreted as if equal to SuggestedFix.Pos (see issue #64199). -func TestNoEnd(t *testing.T) { - files := map[string]string{ - "a/a.go": "package a\n\nfunc F() {}", + replacement, ok := note.Args[1].(string) + if !ok { + return analysis.TextEdit{}, fmt.Errorf("second argument must be pattern, got %T", note.Args[1]) } - dir, cleanup, err := analysistest.WriteFiles(files) - if err != nil { - t.Fatalf("Creating test files failed with %s", err) + + // debugging: show matched portion + if false { + log.Printf("%s: %s: r%q (%q) -> %q", + tokFile.Position(note.Pos), + note.Name, + pattern, + content[lineStartOff+start:lineStartOff+end], + replacement) } - defer cleanup() - fix(t, dir, "noend", exitCodeDiagnostics, "a") + return analysis.TextEdit{ + Pos: lineStart + token.Pos(start), + End: lineStart + token.Pos(end), + NewText: []byte(replacement), + }, nil +} - got, err := os.ReadFile(path.Join(dir, "src/a/a.go")) - if err != nil { - t.Fatal(err) - } - const want = "package a\n\n/*hello*/\nfunc F() {}\n" - if string(got) != want { - t.Errorf("new file contents were <<%s>>, want <<%s>>", got, want) - } +var renameAnalyzer = &analysis.Analyzer{ + Name: "rename", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Doc: "renames symbols named bar to baz", + RunDespiteErrors: true, + Run: func(pass *analysis.Pass) (any, 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) { + ident := n.(*ast.Ident) + if ident.Name == from { + msg := fmt.Sprintf("renaming %q to %q", from, to) + pass.Report(analysis.Diagnostic{ + Pos: ident.Pos(), + End: ident.End(), + Message: msg, + SuggestedFixes: []analysis.SuggestedFix{{ + Message: msg, + TextEdits: []analysis.TextEdit{{ + Pos: ident.Pos(), + End: ident.End(), + NewText: []byte(to), + }}, + }}, + }) + } + }) + return nil, nil + }, } -func init() { - candidates["noend"] = &analysis.Analyzer{ - Name: "noend", - Doc: "inserts /*hello*/ before first decl", - Run: func(pass *analysis.Pass) (any, error) { - decl := pass.Files[0].Decls[0] - pass.Report(analysis.Diagnostic{ - Pos: decl.Pos(), - End: token.NoPos, +var noendAnalyzer = &analysis.Analyzer{ + Name: "noend", + Doc: "inserts /*hello*/ before first decl", + Run: func(pass *analysis.Pass) (any, error) { + decl := pass.Files[0].Decls[0] + pass.Report(analysis.Diagnostic{ + Pos: decl.Pos(), + End: token.NoPos, + Message: "say hello", + SuggestedFixes: []analysis.SuggestedFix{{ Message: "say hello", - SuggestedFixes: []analysis.SuggestedFix{{ - Message: "say hello", - TextEdits: []analysis.TextEdit{ - { - Pos: decl.Pos(), - End: token.NoPos, - NewText: []byte("/*hello*/"), - }, - }, + TextEdits: []analysis.TextEdit{{ + Pos: decl.Pos(), + End: token.NoPos, + NewText: []byte("/*hello*/"), }}, - }) - return nil, nil - }, - } + }}, + }) + return nil, nil + }, } // panics asserts that f() panics with with a value whose printed form matches the regexp want. @@ -442,3 +597,13 @@ func panics(t *testing.T, want string, f func()) { }() f() } + +// section returns the named archive section, or nil. +func section(ar *txtar.Archive, name string) *txtar.File { + for i, f := range ar.Files { + if f.Name == name { + return &ar.Files[i] + } + } + return nil +} diff --git a/go/analysis/internal/checker/testdata/conflict.txt b/go/analysis/internal/checker/testdata/conflict.txt new file mode 100644 index 00000000000..c4a4b13b9ab --- /dev/null +++ b/go/analysis/internal/checker/testdata/conflict.txt @@ -0,0 +1,30 @@ +# Conflicting edits are legal, so long as they appear in different fixes. +# The driver will apply them in some order, and discard those that conflict. +# +# fix1 appears first, so is applied first; it succeeds. +# fix2 and fix3 conflict with it and are rejected. + +checker -marker -fix example.com/a +exit 1 +stderr applied 1 of 3 fixes; 1 files updated...Re-run + +-- go.mod -- +module example.com + +go 1.22 + +-- a/a.go -- +package a + +func f() { + bar := 12 //@ fix1("\tbar", "baz"), fix2("ar ", "baz"), fix3("bar", "lorem ipsum") + _ = bar //@ fix1(" bar", "baz") +} + +-- want/a/a.go -- +package a + +func f() { + baz := 12 //@ fix1("\tbar", "baz"), fix2("ar ", "baz"), fix3("bar", "lorem ipsum") + _ = baz //@ fix1(" bar", "baz") +} diff --git a/go/analysis/internal/checker/testdata/diff.txt b/go/analysis/internal/checker/testdata/diff.txt new file mode 100644 index 00000000000..5a0c9c2a3b2 --- /dev/null +++ b/go/analysis/internal/checker/testdata/diff.txt @@ -0,0 +1,36 @@ +# Basic test of -diff: ensure that stdout contains a diff, +# and the file system is unchanged. +# +# (Most tests of fixes should use want/* not -diff + stdout +# to avoid dependency on the diff algorithm.) +# +# File slashes assume non-Windows. + +skip GOOS=windows +checker -rename -fix -diff example.com/p +exit 3 +stderr renaming "bar" to "baz" + +-- go.mod -- +module example.com +go 1.22 + +-- p/p.go -- +package p + +var bar int + +-- want/p/p.go -- +package p + +var bar int + +-- stdout -- +--- /TMP/p/p.go (old) ++++ /TMP/p/p.go (new) +@@ -1,4 +1,3 @@ + package p + +-var bar int +- ++var baz int diff --git a/go/analysis/internal/checker/testdata/fixes.txt b/go/analysis/internal/checker/testdata/fixes.txt new file mode 100644 index 00000000000..89f245f9ace --- /dev/null +++ b/go/analysis/internal/checker/testdata/fixes.txt @@ -0,0 +1,59 @@ +# Ensure that fixes are applied correctly, in +# particular when processing duplicate fixes for overlapping packages +# in the same directory ("p", "p [p.test]", "p_test [p.test]"). + +checker -rename -fix example.com/p +exit 3 +stderr renaming "bar" to "baz" + +-- go.mod -- +module example.com +go 1.22 + +-- p/p.go -- +package p + +func Foo() { + bar := 12 + _ = bar +} + +-- p/p_test.go -- +package p + +func InTestFile() { + bar := 13 + _ = bar +} + +-- p/p_x_test.go -- +package p_test + +func Foo() { + bar := 14 + _ = bar +} + +-- want/p/p.go -- +package p + +func Foo() { + baz := 12 + _ = baz +} + +-- want/p/p_test.go -- +package p + +func InTestFile() { + baz := 13 + _ = baz +} + +-- want/p/p_x_test.go -- +package p_test + +func Foo() { + baz := 14 + _ = baz +} diff --git a/go/analysis/internal/checker/testdata/importdup.txt b/go/analysis/internal/checker/testdata/importdup.txt new file mode 100644 index 00000000000..e1783777858 --- /dev/null +++ b/go/analysis/internal/checker/testdata/importdup.txt @@ -0,0 +1,29 @@ +# Test that duplicate imports--and, more generally, duplicate +# identical insertions--are coalesced. + +checker -marker -fix example.com/a +exit 3 + +-- go.mod -- +module example.com +go 1.22 + +-- a/a.go -- +package a + +import ( + _ "errors" + //@ fix1("()//", `"foo"`), fix2("()//", `"foo"`) +) + +func f() {} //@ fix1("()}", "n++"), fix2("()}", "n++") + +-- want/a/a.go -- +package a + +import ( + _ "errors" + "foo" //@ fix1("()//", `"foo"`), fix2("()//", `"foo"`) +) + +func f() { n++ } //@ fix1("()}", "n++"), fix2("()}", "n++") diff --git a/go/analysis/internal/checker/testdata/importdup2.txt b/go/analysis/internal/checker/testdata/importdup2.txt new file mode 100644 index 00000000000..118fdc0184b --- /dev/null +++ b/go/analysis/internal/checker/testdata/importdup2.txt @@ -0,0 +1,60 @@ +# Test of import de-duplication behavior. +# +# In packages a and b, there are three fixes, +# each adding one of two imports, but in different order. +# +# In package a, the fixes are [foo, foo, bar], +# and they are resolved as follows: +# - foo is applied -> [foo] +# - foo is coalesced -> [foo] +# - bar is applied -> [foo bar] +# The result is then formatted to [bar foo]. +# +# In package b, the fixes are [foo, bar, foo]: +# - foo is applied -> [foo] +# - bar is applied -> [foo bar] +# - foo is coalesced -> [foo bar] +# The same result is again formatted to [bar foo]. +# +# In more complex examples, the result +# may be more subtly order-dependent. + +checker -marker -fix example.com/a example.com/b +exit 3 + +-- go.mod -- +module example.com +go 1.22 + +-- a/a.go -- +package a + +import ( + //@ fix1("()//", "\"foo\"\n"), fix2("()//", "\"foo\"\n"), fix3("()//", "\"bar\"\n") +) + +-- want/a/a.go -- +package a + +import ( + "bar" + "foo" + // @ fix1("()//", "\"foo\"\n"), fix2("()//", "\"foo\"\n"), fix3("()//", "\"bar\"\n") +) + +-- b/b.go -- +package b + +import ( + //@ fix1("()//", "\"foo\"\n"), fix2("()//", "\"bar\"\n"), fix3("()//", "\"foo\"\n") +) + +-- want/b/b.go -- +package b + +import ( + "bar" + "foo" + // @ fix1("()//", "\"foo\"\n"), fix2("()//", "\"bar\"\n"), fix3("()//", "\"foo\"\n") +) + diff --git a/go/analysis/internal/checker/testdata/json.txt b/go/analysis/internal/checker/testdata/json.txt new file mode 100644 index 00000000000..8e6091aebbc --- /dev/null +++ b/go/analysis/internal/checker/testdata/json.txt @@ -0,0 +1,42 @@ +# Test basic JSON output. +# +# File slashes assume non-Windows. + +skip GOOS=windows +checker -rename -json example.com/p +exit 0 + +-- go.mod -- +module example.com +go 1.22 + +-- p/p.go -- +package p + +func f(bar int) {} + +-- stdout -- +{ + "example.com/p": { + "rename": [ + { + "posn": "/TMP/p/p.go:3:8", + "message": "renaming \"bar\" to \"baz\"", + "suggested_fixes": [ + { + "message": "renaming \"bar\" to \"baz\"", + "edits": [ + { + "filename": "/TMP/p/p.go", + "start": 18, + "end": 21, + "new": "baz" + } + ] + } + ] + } + ] + } +} + diff --git a/go/analysis/internal/checker/testdata/noend.txt b/go/analysis/internal/checker/testdata/noend.txt new file mode 100644 index 00000000000..2d6be074565 --- /dev/null +++ b/go/analysis/internal/checker/testdata/noend.txt @@ -0,0 +1,21 @@ +# Test that a missing SuggestedFix.End position is correctly +# interpreted as if equal to SuggestedFix.Pos (see issue #64199). + +checker -noend -fix example.com/a +exit 3 +stderr say hello + +-- go.mod -- +module example.com +go 1.22 + +-- a/a.go -- +package a + +func f() {} + +-- want/a/a.go -- +package a + +/*hello*/ +func f() {} diff --git a/go/analysis/internal/checker/testdata/overlap.txt b/go/analysis/internal/checker/testdata/overlap.txt new file mode 100644 index 00000000000..f556ef308b9 --- /dev/null +++ b/go/analysis/internal/checker/testdata/overlap.txt @@ -0,0 +1,34 @@ +# This test exercises an edge case of merging. +# +# Two analyzers generate overlapping fixes for this package: +# - 'rename' changes "bar" to "baz" +# - 'marker' changes "ar" to "baz" +# Historically this used to cause a conflict, but as it happens, +# the new merge algorithm splits the rename fix, since it overlaps +# the marker fix, into two subedits: +# - a deletion of "b" and +# - an edit from "ar" to "baz". +# The deletion is of course nonoverlapping, and the edit, +# by happy chance, is identical to the marker fix, so the two +# are coalesced. +# +# (This is a pretty unlikely situation, but it corresponds +# to a historical test, TestOther, that used to check for +# a conflict, and it seemed wrong to delete it without explanation.) + +checker -rename -marker -fix example.com/a +exit 3 + +-- go.mod -- +module example.com +go 1.22 + +-- a/a.go -- +package a + +func f(bar int) {} //@ fix("ar", "baz") + +-- want/a/a.go -- +package a + +func f(baz int) {} //@ fix("ar", "baz")