go/analysis/internal/checker: validate SuggestedFixes

This change causes the Pass.Report operation of all our
drivers:
- internal/checker, used by {single,multi}checker,
  analysistest, and the public checker API;
- unitchecker, used by cmd/vet; and
- gopls' analysis driver
to assert that SuggestedFixes are valid, and to establish
postconditions such as the fix.End is valid.

Also, add a test that pass.Report panics informatively.

Change-Id: I7ee4ac621852ab0a39d47edce1ab6e2304bfc53b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/643715
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

Author: adonovan

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,

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.

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()
+}

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) },

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