diff --git a/go/analysis/checker/checker.go b/go/analysis/checker/checker.go index a563f7cbeda..502ec922179 100644 --- a/go/analysis/checker/checker.go +++ b/go/analysis/checker/checker.go @@ -337,8 +337,14 @@ func (act *Action) execOnce() { TypeErrors: act.Package.TypeErrors, Module: module, - ResultOf: inputs, - Report: func(d analysis.Diagnostic) { act.Diagnostics = append(act.Diagnostics, d) }, + ResultOf: inputs, + Report: func(d analysis.Diagnostic) { + // Assert that SuggestedFixes are well formed. + if err := analysisinternal.ValidateFixes(act.Package.Fset, act.Analyzer, d.SuggestedFixes); err != nil { + panic(err) + } + act.Diagnostics = append(act.Diagnostics, d) + }, ImportObjectFact: act.ObjectFact, ExportObjectFact: act.exportObjectFact, ImportPackageFact: act.PackageFact, diff --git a/go/analysis/internal/checker/checker_test.go b/go/analysis/internal/checker/checker_test.go index 7f38ad1a094..76d45adceef 100644 --- a/go/analysis/internal/checker/checker_test.go +++ b/go/analysis/internal/checker/checker_test.go @@ -84,6 +84,7 @@ var otherAnalyzer = &analysis.Analyzer{ // like analyzer but with a different Na } func run(pass *analysis.Pass) (interface{}, error) { + // TODO(adonovan): replace this entangled test with something completely data-driven. const ( from = "bar" to = "baz" @@ -109,11 +110,39 @@ func run(pass *analysis.Pass) (interface{}, error) { } switch pass.Pkg.Name() { case conflict: - edits = append(edits, []analysis.TextEdit{ - {Pos: ident.Pos() - 1, End: ident.End(), NewText: []byte(to)}, - {Pos: ident.Pos(), End: ident.End() - 1, NewText: []byte(to)}, - {Pos: ident.Pos(), End: ident.End(), NewText: []byte("lorem ipsum")}, - }...) + // 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. diff --git a/go/analysis/internal/checker/fix_test.go b/go/analysis/internal/checker/fix_test.go index 81bc569e861..4063aed35cd 100644 --- a/go/analysis/internal/checker/fix_test.go +++ b/go/analysis/internal/checker/fix_test.go @@ -19,7 +19,9 @@ import ( "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/packages" "golang.org/x/tools/internal/testenv" ) @@ -126,15 +128,6 @@ func Foo() { _ = bar } -// the end -`, - "duplicate/dup.go": `package duplicate - -func Foo() { - bar := 14 - _ = bar -} - // the end `, } @@ -164,15 +157,6 @@ func Foo() { _ = baz } -// the end -`, - "duplicate/dup.go": `package duplicate - -func Foo() { - baz := 14 - _ = baz -} - // the end `, } @@ -182,7 +166,7 @@ func Foo() { } defer cleanup() - fix(t, dir, "rename,other", exitCodeDiagnostics, "rename", "duplicate") + fix(t, dir, "rename,other", exitCodeDiagnostics, "rename") for name, want := range fixed { path := path.Join(dir, "src", name) @@ -196,6 +180,117 @@ func Foo() { } } +// TestReportInvalidDiagnostic tests that a call to pass.Report with +// certain kind of invalid diagnostic (e.g. conflicting fixes) +// promptly results in a panic. +func TestReportInvalidDiagnostic(t *testing.T) { + testenv.NeedsGoPackages(t) + + // Load the errors package. + cfg := &packages.Config{Mode: packages.LoadAllSyntax} + initial, err := packages.Load(cfg, "errors") + if err != nil { + t.Fatal(err) + } + + for _, test := range []struct { + name string + want string + diag func(pos token.Pos) analysis.Diagnostic + }{ + // Diagnostic has two alternative fixes with the same Message. + { + "duplicate message", + `analyzer "a" suggests two fixes with same Message \(fix\)`, + func(pos token.Pos) analysis.Diagnostic { + return analysis.Diagnostic{ + Pos: pos, + Message: "oops", + SuggestedFixes: []analysis.SuggestedFix{ + {Message: "fix"}, + {Message: "fix"}, + }, + } + }, + }, + // TextEdit has invalid Pos. + { + "bad Pos", + `analyzer "a" suggests invalid fix .*: missing file info for pos`, + func(pos token.Pos) analysis.Diagnostic { + return analysis.Diagnostic{ + Pos: pos, + Message: "oops", + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "fix", + TextEdits: []analysis.TextEdit{{}}, + }, + }, + } + }, + }, + // TextEdit has invalid End. + { + "End < Pos", + `analyzer "a" suggests invalid fix .*: pos .* > end`, + func(pos token.Pos) analysis.Diagnostic { + return analysis.Diagnostic{ + Pos: pos, + Message: "oops", + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "fix", + TextEdits: []analysis.TextEdit{{ + Pos: pos + 2, + End: pos, + }}, + }, + }, + } + }, + }, + // Two TextEdits overlap. + { + "overlapping edits", + `analyzer "a" suggests invalid fix .*: overlapping edits to .*errors.go \(1:1-1:3 and 1:2-1:4\)`, + func(pos token.Pos) analysis.Diagnostic { + return analysis.Diagnostic{ + Pos: pos, + Message: "oops", + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "fix", + TextEdits: []analysis.TextEdit{ + {Pos: pos, End: pos + 2}, + {Pos: pos + 1, End: pos + 3}, + }, + }, + }, + } + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + reached := false + a := &analysis.Analyzer{Name: "a", Doc: "doc", Run: func(pass *analysis.Pass) (any, error) { + reached = true + panics(t, test.want, func() { + pos := pass.Files[0].FileStart + pass.Report(test.diag(pos)) + }) + return nil, nil + }} + if _, err := checker.Analyze([]*analysis.Analyzer{a}, initial, &checker.Options{}); err != nil { + t.Fatalf("Analyze failed: %v", err) + } + if !reached { + t.Error("analyzer was never invoked") + } + }) + } +} + // TestConflict ensures that checker.Run detects conflicts correctly. // This test fork/execs the main function above. func TestConflict(t *testing.T) { @@ -333,3 +428,17 @@ func init() { }, } } + +// panics asserts that f() panics with with a value whose printed form matches the regexp want. +func panics(t *testing.T, want string, f func()) { + defer func() { + if x := recover(); x == nil { + t.Errorf("function returned normally, wanted panic") + } else if m, err := regexp.MatchString(want, fmt.Sprint(x)); err != nil { + t.Errorf("panics: invalid regexp %q", want) + } else if !m { + t.Errorf("function panicked with value %q, want match for %q", x, want) + } + }() + f() +} diff --git a/go/analysis/unitchecker/unitchecker.go b/go/analysis/unitchecker/unitchecker.go index f723349010e..82c3db6a39d 100644 --- a/go/analysis/unitchecker/unitchecker.go +++ b/go/analysis/unitchecker/unitchecker.go @@ -367,17 +367,26 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re } pass := &analysis.Pass{ - Analyzer: a, - Fset: fset, - Files: files, - OtherFiles: cfg.NonGoFiles, - IgnoredFiles: cfg.IgnoredFiles, - Pkg: pkg, - TypesInfo: info, - TypesSizes: tc.Sizes, - TypeErrors: nil, // unitchecker doesn't RunDespiteErrors - ResultOf: inputs, - Report: func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) }, + Analyzer: a, + Fset: fset, + Files: files, + OtherFiles: cfg.NonGoFiles, + IgnoredFiles: cfg.IgnoredFiles, + Pkg: pkg, + TypesInfo: info, + TypesSizes: tc.Sizes, + TypeErrors: nil, // unitchecker doesn't RunDespiteErrors + ResultOf: inputs, + Report: func(d analysis.Diagnostic) { + // Unitchecker doesn't apply fixes, but it does report them in the JSON output. + if err := analysisinternal.ValidateFixes(fset, a, d.SuggestedFixes); err != nil { + // Since we have diagnostics, the exit code will be nonzero, + // so logging these errors is sufficient. + log.Println(err) + d.SuggestedFixes = nil + } + act.diagnostics = append(act.diagnostics, d) + }, ImportObjectFact: facts.ImportObjectFact, ExportObjectFact: facts.ExportObjectFact, AllObjectFacts: func() []analysis.ObjectFact { return facts.AllObjectFacts(factFilter) }, diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 4c5abbc23ce..d570c0a46ae 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -1131,6 +1131,11 @@ func (act *action) exec(ctx context.Context) (any, *actionSummary, error) { TypeErrors: apkg.typeErrors, ResultOf: inputs, Report: func(d analysis.Diagnostic) { + // Assert that SuggestedFixes are well formed. + if err := analysisinternal.ValidateFixes(apkg.pkg.FileSet(), analyzer, d.SuggestedFixes); err != nil { + bug.Reportf("invalid SuggestedFixes: %v", err) + d.SuggestedFixes = nil + } diagnostic, err := toGobDiagnostic(posToLocation, analyzer, d) if err != nil { // Don't bug.Report here: these errors all originate in diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go index 8bfba325b49..8f38fa604d8 100644 --- a/internal/analysisinternal/analysis.go +++ b/internal/analysisinternal/analysis.go @@ -8,6 +8,7 @@ package analysisinternal import ( "bytes" + "cmp" "fmt" "go/ast" "go/printer" @@ -356,3 +357,90 @@ func IsMethodNamed(obj types.Object, pkgPath string, typeName string, names ...s func isPackageLevel(obj types.Object) bool { return obj.Pkg() != nil && obj.Parent() == obj.Pkg().Scope() } + +// ValidateFixes validates the set of fixes for a single diagnostic. +// Any error indicates a bug in the originating analyzer. +// +// It updates fixes so that fixes[*].End.IsValid(). +// +// It may be used as part of an analysis driver implementation. +func ValidateFixes(fset *token.FileSet, a *analysis.Analyzer, fixes []analysis.SuggestedFix) error { + fixMessages := make(map[string]bool) + for i := range fixes { + fix := &fixes[i] + if fixMessages[fix.Message] { + return fmt.Errorf("analyzer %q suggests two fixes with same Message (%s)", a.Name, fix.Message) + } + fixMessages[fix.Message] = true + if err := validateFix(fset, fix); err != nil { + return fmt.Errorf("analyzer %q suggests invalid fix (%s): %v", a.Name, fix.Message, err) + } + } + return nil +} + +// validateFix validates a single fix. +// Any error indicates a bug in the originating analyzer. +// +// It updates fix so that fix.End.IsValid(). +func validateFix(fset *token.FileSet, fix *analysis.SuggestedFix) error { + + // Stably sort edits by Pos. This ordering puts insertions + // (end = start) before deletions (end > start) at the same + // point, but uses a stable sort to preserve the order of + // multiple insertions at the same point. + slices.SortStableFunc(fix.TextEdits, func(x, y analysis.TextEdit) int { + if sign := cmp.Compare(x.Pos, y.Pos); sign != 0 { + return sign + } + return cmp.Compare(x.End, y.End) + }) + + var prev *analysis.TextEdit + for i := range fix.TextEdits { + edit := &fix.TextEdits[i] + + // Validate edit individually. + start := edit.Pos + file := fset.File(start) + if file == nil { + return fmt.Errorf("missing file info for pos (%v)", edit.Pos) + } + if end := edit.End; end.IsValid() { + if end < start { + return fmt.Errorf("pos (%v) > end (%v)", edit.Pos, edit.End) + } + endFile := fset.File(end) + if endFile == nil { + return fmt.Errorf("malformed end position %v", end) + } + if endFile != file { + return fmt.Errorf("edit spans files %v and %v", file.Name(), endFile.Name()) + } + } else { + edit.End = start // update the SuggestedFix + } + if eof := token.Pos(file.Base() + file.Size()); edit.End > eof { + return fmt.Errorf("end is (%v) beyond end of file (%v)", edit.End, eof) + } + + // Validate the sequence of edits: + // properly ordered, no overlapping deletions + if prev != nil && edit.Pos < prev.End { + xpos := fset.Position(prev.Pos) + xend := fset.Position(prev.End) + ypos := fset.Position(edit.Pos) + yend := fset.Position(edit.End) + return fmt.Errorf("overlapping edits to %s (%d:%d-%d:%d and %d:%d-%d:%d)", + xpos.Filename, + xpos.Line, xpos.Column, + xend.Line, xend.Column, + ypos.Line, ypos.Column, + yend.Line, yend.Column, + ) + } + prev = edit + } + + return nil +}