Skip to content

Commit

Permalink
Merge remote-tracking branch 'origin/master' into gopls-release-branc…
Browse files Browse the repository at this point in the history
…h.0.16

For golang/go#67936

Change-Id: Ic3981ea862fd2f8203214b29349543725d526315
  • Loading branch information
findleyr committed Jun 18, 2024
2 parents 663dde6 + 1239d04 commit 9c7c6b4
Show file tree
Hide file tree
Showing 48 changed files with 1,571 additions and 799 deletions.
16 changes: 13 additions & 3 deletions go/analysis/analysistest/analysistest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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...)

Expand Down
17 changes: 14 additions & 3 deletions go/analysis/diagnostic.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down
78 changes: 25 additions & 53 deletions go/analysis/internal/checker/checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ package checker
import (
"bytes"
"encoding/gob"
"errors"
"flag"
"fmt"
"go/format"
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand All @@ -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].
Expand Down
48 changes: 33 additions & 15 deletions go/analysis/internal/checker/checker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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},
Expand All @@ -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+.
Expand All @@ -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{}
Expand Down
86 changes: 70 additions & 16 deletions go/analysis/passes/stringintconv/string.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
})
Expand Down
Loading

0 comments on commit 9c7c6b4

Please sign in to comment.