Merge remote-tracking branch 'origin/master' into gopls-release-branch.0.16

For golang/go#67936

Change-Id: Ic3981ea862fd2f8203214b29349543725d526315

Author: findleyr

go/analysis/analysistest/analysistest.go

@@ -114,9 +114,9 @@ type Testing interface {
 // into nonconflicting parts.
 //
 // Conflicts of the second kind can be avoided by giving the
-// alternative fixes different names (SuggestedFix.Message) and using
-// a multi-section .txtar file with a named section for each
-// alternative fix.
+// alternative fixes different names (SuggestedFix.Message) and
+// defining the .golden file as a multi-section txtar file with a
+// named section for each alternative fix, as shown above.
 //
 // Analyzers that compute fixes from a textual diff of the
 // before/after file contents (instead of directly from syntax tree
@@ -126,6 +126,16 @@ type Testing interface {
 // sufficient separation of the statements in the test input so that
 // the computed diffs do not overlap. If that fails, break the test
 // into smaller parts.
+//
+// TODO(adonovan): the behavior of RunWithSuggestedFixes as documented
+// above is impractical for tests that report multiple diagnostics and
+// offer multiple alternative fixes for the same diagnostic, and it is
+// inconsistent with the interpretation of multiple diagnostics
+// described at Diagnostic.SuggestedFixes.
+// We need to rethink the analyzer testing API to better support such
+// cases. In the meantime, users of RunWithSuggestedFixes testing
+// analyzers that offer alternative fixes are advised to put each fix
+// in a separate .go file in the testdata.
 func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result {
 	r := Run(t, dir, a, patterns...)
 

go/analysis/diagnostic.go

@@ -33,8 +33,17 @@ type Diagnostic struct {
 	URL string
 
 	// SuggestedFixes is an optional list of fixes to address the
-	// problem described by the diagnostic, each one representing
+	// problem described by the diagnostic. Each one represents
 	// an alternative strategy; at most one may be applied.
+	//
+	// Fixes for different diagnostics should be treated as
+	// independent changes to the same baseline file state,
+	// analogous to a set of git commits all with the same parent.
+	// Combining fixes requires resolving any conflicts that
+	// arise, analogous to a git merge.
+	// Any conflicts that remain may be dealt with, depending on
+	// the tool, by discarding fixes, consulting the user, or
+	// aborting the operation.
 	SuggestedFixes []SuggestedFix
 
 	// Related contains optional secondary positions and messages
@@ -58,8 +67,10 @@ type RelatedInformation struct {
 //
 // The TextEdits must not overlap, nor contain edits for other packages.
 type SuggestedFix struct {
-	// A description for this suggested fix to be shown to a user deciding
-	// whether to accept it.
+	// A verb phrase describing the fix, to be shown to
+	// a user trying to decide whether to accept it.
+	//
+	// Example: "Remove the surplus argument"
 	Message   string
 	TextEdits []TextEdit
 }

go/analysis/internal/checker/checker.go

@@ -11,7 +11,6 @@ package checker
 import (
 	"bytes"
 	"encoding/gob"
-	"errors"
 	"flag"
 	"fmt"
 	"go/format"
@@ -134,16 +133,21 @@ func Run(args []string, analyzers []*analysis.Analyzer) (exitcode int) {
 	allSyntax := needFacts(analyzers)
 	initial, err := load(args, allSyntax)
 	if err != nil {
-		if _, ok := err.(typeParseError); !ok {
-			// Fail when some of the errors are not
-			// related to parsing nor typing.
-			log.Print(err)
-			return 1
-		}
-		// TODO: filter analyzers based on RunDespiteError?
+		log.Print(err)
+		return 1
 	}
 
-	// Run the analysis.
+	pkgsExitCode := 0
+	// Print package errors regardless of RunDespiteErrors.
+	// Do not exit if there are errors, yet.
+	if n := packages.PrintErrors(initial); n > 0 {
+		pkgsExitCode = 1
+	}
+
+	// Run the analyzers. On each package with (transitive)
+	// errors, we run only the subset of analyzers that are
+	// marked (and whose transitive requirements are also
+	// marked) with RunDespiteErrors.
 	roots := analyze(initial, analyzers)
 
 	// Apply fixes.
@@ -155,18 +159,18 @@ func Run(args []string, analyzers []*analysis.Analyzer) (exitcode int) {
 		}
 	}
 
-	// Print the results.
-	return printDiagnostics(roots)
-}
-
-// typeParseError represents a package load error
-// that is related to typing and parsing.
-type typeParseError struct {
-	error
+	// Print the results. If !RunDespiteErrors and there
+	// are errors in the packages, this will have 0 exit
+	// code. Otherwise, we prefer to return exit code
+	// indicating diagnostics.
+	if diagExitCode := printDiagnostics(roots); diagExitCode != 0 {
+		return diagExitCode // there were diagnostics
+	}
+	return pkgsExitCode // package errors but no diagnostics
 }
 
-// load loads the initial packages. If all loading issues are related to
-// typing and parsing, the returned error is of type typeParseError.
+// load loads the initial packages. Returns only top-level loading
+// errors. Does not consider errors in packages.
 func load(patterns []string, allSyntax bool) ([]*packages.Package, error) {
 	mode := packages.LoadSyntax
 	if allSyntax {
@@ -178,44 +182,12 @@ func load(patterns []string, allSyntax bool) ([]*packages.Package, error) {
 		Tests: IncludeTests,
 	}
 	initial, err := packages.Load(&conf, patterns...)
-	if err == nil {
-		if len(initial) == 0 {
-			err = fmt.Errorf("%s matched no packages", strings.Join(patterns, " "))
-		} else {
-			err = loadingError(initial)
-		}
+	if err == nil && len(initial) == 0 {
+		err = fmt.Errorf("%s matched no packages", strings.Join(patterns, " "))
 	}
 	return initial, err
 }
 
-// loadingError checks for issues during the loading of initial
-// packages. Returns nil if there are no issues. Returns error
-// of type typeParseError if all errors, including those in
-// dependencies, are related to typing or parsing. Otherwise,
-// a plain error is returned with an appropriate message.
-func loadingError(initial []*packages.Package) error {
-	var err error
-	if n := packages.PrintErrors(initial); n > 1 {
-		err = fmt.Errorf("%d errors during loading", n)
-	} else if n == 1 {
-		err = errors.New("error during loading")
-	} else {
-		// no errors
-		return nil
-	}
-	all := true
-	packages.Visit(initial, nil, func(pkg *packages.Package) {
-		for _, err := range pkg.Errors {
-			typeOrParse := err.Kind == packages.TypeError || err.Kind == packages.ParseError
-			all = all && typeOrParse
-		}
-	})
-	if all {
-		return typeParseError{err}
-	}
-	return err
-}
-
 // TestAnalyzer applies an analyzer to a set of packages (and their
 // dependencies if necessary) and returns the results.
 // The analyzer must be valid according to [analysis.Validate].

go/analysis/internal/checker/checker_test.go

@@ -68,10 +68,11 @@ func Foo() {
 }
 
 var renameAnalyzer = &analysis.Analyzer{
-	Name:     "rename",
-	Requires: []*analysis.Analyzer{inspect.Analyzer},
-	Run:      run,
-	Doc:      "renames symbols named bar to baz",
+	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.
@@ -140,13 +141,28 @@ func TestRunDespiteErrors(t *testing.T) {
 func Foo(s string) int {
 	return s + 1
 }
-`}
+`,
+		"cperr/test.go": `package copyerr
+
+import "sync"
+
+func bar() { } // for renameAnalyzer
+
+type T struct{ mu sync.Mutex }
+type T1 struct{ t *T }
+
+func NewT1() *T1 { return &T1{T} }
+`,
+	}
 
 	testdata, cleanup, err := analysistest.WriteFiles(files)
 	if err != nil {
 		t.Fatal(err)
 	}
-	path := filepath.Join(testdata, "src/rderr/test.go")
+	defer cleanup()
+
+	rderrFile := "file=" + filepath.Join(testdata, "src/rderr/test.go")
+	cperrFile := "file=" + filepath.Join(testdata, "src/cperr/test.go")
 
 	// A no-op analyzer that should finish regardless of
 	// parse or type errors in the code.
@@ -159,8 +175,8 @@ func Foo(s string) int {
 		RunDespiteErrors: true,
 	}
 
-	// A no-op analyzer that should finish regardless of
-	// parse or type errors in the code.
+	// A no-op analyzer, with facts, that should finish
+	// regardless of parse or type errors in the code.
 	noopWithFact := &analysis.Analyzer{
 		Name:     "noopfact",
 		Requires: []*analysis.Analyzer{inspect.Analyzer},
@@ -178,30 +194,34 @@ func Foo(s string) int {
 		code      int
 	}{
 		// parse/type errors
-		{name: "skip-error", pattern: []string{"file=" + path}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 1},
+		{name: "skip-error", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 1},
 		// RunDespiteErrors allows a driver to run an Analyzer even after parse/type errors.
 		//
 		// The noop analyzer doesn't use facts, so the driver loads only the root
 		// package from source. For the rest, it asks 'go list' for export data,
 		// which fails because the compiler encounters the type error.  Since the
 		// errors come from 'go list', the driver doesn't run the analyzer.
-		{name: "despite-error", pattern: []string{"file=" + path}, analyzers: []*analysis.Analyzer{noop}, code: 1},
+		{name: "despite-error", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{noop}, code: 1},
 		// The noopfact analyzer does use facts, so the driver loads source for
 		// all dependencies, does type checking itself, recognizes the error as a
 		// type error, and runs the analyzer.
-		{name: "despite-error-fact", pattern: []string{"file=" + path}, analyzers: []*analysis.Analyzer{noopWithFact}, code: 0},
+		{name: "despite-error-fact", pattern: []string{rderrFile}, analyzers: []*analysis.Analyzer{noopWithFact}, code: 1},
 		// combination of parse/type errors and no errors
-		{name: "despite-error-and-no-error", pattern: []string{"file=" + path, "sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
+		{name: "despite-error-and-no-error", pattern: []string{rderrFile, "sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
 		// non-existing package error
 		{name: "no-package", pattern: []string{"xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 1},
 		{name: "no-package-despite-error", pattern: []string{"abc"}, analyzers: []*analysis.Analyzer{noop}, code: 1},
 		{name: "no-multi-package-despite-error", pattern: []string{"xyz", "abc"}, analyzers: []*analysis.Analyzer{noop}, code: 1},
 		// combination of type/parsing and different errors
-		{name: "different-errors", pattern: []string{"file=" + path, "xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
+		{name: "different-errors", pattern: []string{rderrFile, "xyz"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
 		// non existing dir error
 		{name: "no-match-dir", pattern: []string{"file=non/existing/dir"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 1},
 		// no errors
 		{name: "no-errors", pattern: []string{"sort"}, analyzers: []*analysis.Analyzer{renameAnalyzer, noop}, code: 0},
+		// duplicate list error with no findings
+		{name: "list-error", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{noop}, code: 1},
+		// duplicate list errors with findings (issue #67790)
+		{name: "list-error-findings", pattern: []string{cperrFile}, analyzers: []*analysis.Analyzer{renameAnalyzer}, code: 3},
 	} {
 		if test.name == "despite-error" && testenv.Go1Point() < 20 {
 			// The behavior in the comment on the despite-error test only occurs for Go 1.20+.
@@ -211,8 +231,6 @@ func Foo(s string) int {
 			t.Errorf("got incorrect exit code %d for test %s; want %d", got, test.name, test.code)
 		}
 	}
-
-	defer cleanup()
 }
 
 type EmptyFact struct{}

go/analysis/passes/stringintconv/string.go

@@ -16,6 +16,7 @@ import (
 	"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/aliases"
+	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/typeparams"
 )
 
@@ -73,9 +74,15 @@ func typeName(t types.Type) string {
 func run(pass *analysis.Pass) (interface{}, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	nodeFilter := []ast.Node{
+		(*ast.File)(nil),
 		(*ast.CallExpr)(nil),
 	}
+	var file *ast.File
 	inspect.Preorder(nodeFilter, func(n ast.Node) {
+		if n, ok := n.(*ast.File); ok {
+			file = n
+			return
+		}
 		call := n.(*ast.CallExpr)
 
 		if len(call.Args) != 1 {
@@ -167,27 +174,74 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 		diag := analysis.Diagnostic{
 			Pos:     n.Pos(),
-			Message: fmt.Sprintf("conversion from %s to %s yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)", source, target),
+			Message: fmt.Sprintf("conversion from %s to %s yields a string of one rune, not a string of digits", source, target),
+		}
+		addFix := func(message string, edits []analysis.TextEdit) {
+			diag.SuggestedFixes = append(diag.SuggestedFixes, analysis.SuggestedFix{
+				Message:   message,
+				TextEdits: edits,
+			})
 		}
 
+		// Fix 1: use fmt.Sprint(x)
+		//
+		// Prefer fmt.Sprint over strconv.Itoa, FormatInt,
+		// or FormatUint, as it works for any type.
+		// Add an import of "fmt" as needed.
+		//
+		// Unless the type is exactly string, we must retain the conversion.
+		//
+		// Do not offer this fix if type parameters are involved,
+		// as there are too many combinations and subtleties.
+		// Consider x = rune | int16 | []byte: in all cases,
+		// string(x) is legal, but the appropriate diagnostic
+		// and fix differs. Similarly, don't offer the fix if
+		// the type has methods, as some {String,GoString,Format}
+		// may change the behavior of fmt.Sprint.
+		if len(ttypes) == 1 && len(vtypes) == 1 && types.NewMethodSet(V0).Len() == 0 {
+			fmtName, importEdit := analysisinternal.AddImport(pass.TypesInfo, file, arg.Pos(), "fmt", "fmt")
+			if types.Identical(T0, types.Typ[types.String]) {
+				// string(x) -> fmt.Sprint(x)
+				addFix("Format the number as a decimal", []analysis.TextEdit{
+					importEdit,
+					{
+						Pos:     call.Fun.Pos(),
+						End:     call.Fun.End(),
+						NewText: []byte(fmtName + ".Sprint"),
+					},
+				})
+			} else {
+				// mystring(x) -> mystring(fmt.Sprint(x))
+				addFix("Format the number as a decimal", []analysis.TextEdit{
+					importEdit,
+					{
+						Pos:     call.Lparen + 1,
+						End:     call.Lparen + 1,
+						NewText: []byte(fmtName + ".Sprint("),
+					},
+					{
+						Pos:     call.Rparen,
+						End:     call.Rparen,
+						NewText: []byte(")"),
+					},
+				})
+			}
+		}
+
+		// Fix 2: use string(rune(x))
 		if convertibleToRune {
-			diag.SuggestedFixes = []analysis.SuggestedFix{
+			addFix("Convert a single rune to a string", []analysis.TextEdit{
 				{
-					Message: "Did you mean to convert a rune to a string?",
-					TextEdits: []analysis.TextEdit{
-						{
-							Pos:     arg.Pos(),
-							End:     arg.Pos(),
-							NewText: []byte("rune("),
-						},
-						{
-							Pos:     arg.End(),
-							End:     arg.End(),
-							NewText: []byte(")"),
-						},
-					},
+					Pos:     arg.Pos(),
+					End:     arg.Pos(),
+					NewText: []byte("rune("),
 				},
-			}
+				{
+					Pos:     arg.End(),
+					End:     arg.End(),
+					NewText: []byte(")"),
+				},
+			})
 		}
 		pass.Report(diag)
 	})