internal/lsp/analysis/fillreturns: update fillreturns for new type errs

The error messages produced by go/types related to an incorrect number
of return values have changed, and have been repositioned. Update the
fillreturns analyzer to handle the Go 1.18 form of these errors.

Ideally we'd use error codes here, but can't because they are not
forwarded by go/packages. Added a TODO to revisit when error codes are
exposed.

Change-Id: I4c594fd2fd1cb7423f37ff9c6a57e9490dcbc2a4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/367714
Trust: Robert Findley <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Suzy Mueller <[email protected]>

Author: findleyr

gopls/doc/analyzers.md

@@ -578,7 +578,7 @@ check for constraints that could be simplified to "any"
 
 ## **fillreturns**
 
-suggested fixes for "wrong number of return values (want %d, got %d)"
+suggest fixes for errors due to an incorrect number of return values
 
 This checker provides suggested fixes for type errors of the
 type "wrong number of return values (want %d, got %d)". For example:

gopls/internal/regtest/misc/fix_test.go

@@ -62,7 +62,6 @@ func Foo() {
 }
 
 func TestFillReturns(t *testing.T) {
-	t.Skip("temporarily skipped until CL 367196 is submitted")
 	const files = `
 -- go.mod --
 module mod.com
@@ -79,7 +78,8 @@ func Foo() error {
 		env.OpenFile("main.go")
 		var d protocol.PublishDiagnosticsParams
 		env.Await(OnceMet(
-			env.DiagnosticAtRegexpWithMessage("main.go", `return`, "wrong number of return values"),
+			// The error message here changed in 1.18; "return values" covers both forms.
+			env.DiagnosticAtRegexpWithMessage("main.go", `return`, "return values"),
 			ReadDiagnostics("main.go", &d),
 		))
 		codeActions := env.CodeAction("main.go", d.Diagnostics)

internal/lsp/analysis/fillreturns/fillreturns.go

@@ -14,7 +14,6 @@ import (
 	"go/format"
 	"go/types"
 	"regexp"
-	"strconv"
 	"strings"
 
 	"golang.org/x/tools/go/analysis"
@@ -23,7 +22,7 @@ import (
 	"golang.org/x/tools/internal/typeparams"
 )
 
-const Doc = `suggested fixes for "wrong number of return values (want %d, got %d)"
+const Doc = `suggest fixes for errors due to an incorrect number of return values
 
 This checker provides suggested fixes for type errors of the
 type "wrong number of return values (want %d, got %d)". For example:
@@ -46,8 +45,6 @@ var Analyzer = &analysis.Analyzer{
 	RunDespiteErrors: true,
 }
 
-var wrongReturnNumRegex = regexp.MustCompile(`wrong number of return values \(want (\d+), got (\d+)\)`)
-
 func run(pass *analysis.Pass) (interface{}, error) {
 	info := pass.TypesInfo
 	if info == nil {
@@ -58,7 +55,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 outer:
 	for _, typeErr := range errors {
 		// Filter out the errors that are not relevant to this analyzer.
-		if !FixesError(typeErr.Msg) {
+		if !FixesError(typeErr) {
 			continue
 		}
 		var file *ast.File
@@ -79,20 +76,32 @@ outer:
 		}
 		typeErrEndPos := analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), typeErr.Pos)
 
+		// TODO(rfindley): much of the error handling code below returns, when it
+		// should probably continue.
+
 		// Get the path for the relevant range.
 		path, _ := astutil.PathEnclosingInterval(file, typeErr.Pos, typeErrEndPos)
 		if len(path) == 0 {
 			return nil, nil
 		}
-		// Check to make sure the node of interest is a ReturnStmt.
-		ret, ok := path[0].(*ast.ReturnStmt)
-		if !ok {
+
+		// Find the enclosing return statement.
+		var ret *ast.ReturnStmt
+		var retIdx int
+		for i, n := range path {
+			if r, ok := n.(*ast.ReturnStmt); ok {
+				ret = r
+				retIdx = i
+				break
+			}
+		}
+		if ret == nil {
 			return nil, nil
 		}
 
 		// Get the function type that encloses the ReturnStmt.
 		var enclosingFunc *ast.FuncType
-		for _, n := range path {
+		for _, n := range path[retIdx+1:] {
 			switch node := n.(type) {
 			case *ast.FuncLit:
 				enclosingFunc = node.Type
@@ -109,7 +118,7 @@ outer:
 
 		// Skip any generic enclosing functions, since type parameters don't
 		// have 0 values.
-		// TODO(rstambler): We should be able to handle this if the return
+		// TODO(rfindley): We should be able to handle this if the return
 		// values are all concrete types.
 		if tparams := typeparams.ForFuncType(enclosingFunc); tparams != nil && tparams.NumFields() > 0 {
 			return nil, nil
@@ -127,7 +136,8 @@ outer:
 			return nil, nil
 		}
 
-		// Skip any return statements that contain function calls with multiple return values.
+		// Skip any return statements that contain function calls with multiple
+		// return values.
 		for _, expr := range ret.Results {
 			e, ok := expr.(*ast.CallExpr)
 			if !ok {
@@ -244,16 +254,23 @@ func matchingTypes(want, got types.Type) bool {
 	return types.AssignableTo(want, got) || types.ConvertibleTo(want, got)
 }
 
-func FixesError(msg string) bool {
-	matches := wrongReturnNumRegex.FindStringSubmatch(strings.TrimSpace(msg))
-	if len(matches) < 3 {
-		return false
-	}
-	if _, err := strconv.Atoi(matches[1]); err != nil {
-		return false
-	}
-	if _, err := strconv.Atoi(matches[2]); err != nil {
-		return false
+// Error messages have changed across Go versions. These regexps capture recent
+// incarnations.
+//
+// TODO(rfindley): once error codes are exported and exposed via go/packages,
+// use error codes rather than string matching here.
+var wrongReturnNumRegexes = []*regexp.Regexp{
+	regexp.MustCompile(`wrong number of return values \(want (\d+), got (\d+)\)`),
+	regexp.MustCompile(`too many return values`),
+	regexp.MustCompile(`not enough return values`),
+}
+
+func FixesError(err types.Error) bool {
+	msg := strings.TrimSpace(err.Msg)
+	for _, rx := range wrongReturnNumRegexes {
+		if rx.MatchString(msg) {
+			return true
+		}
 	}
-	return true
+	return false
 }

internal/lsp/analysis/fillreturns/fillreturns_test.go

@@ -13,7 +13,6 @@ import (
 )
 
 func Test(t *testing.T) {
-	t.Skip("temporarily skipped until CL 367196 is submitted and this test is adjusted accordingly")
 	testdata := analysistest.TestData()
 	tests := []string{"a"}
 	if typeparams.Enabled {

internal/lsp/analysis/fillreturns/testdata/src/a/a.go

@@ -25,80 +25,82 @@ func x() error {
 	return errors.New("foo")
 }
 
+// The error messages below changed in 1.18; "return values" covers both forms.
+
 func b() (string, int, error) {
-	return "", errors.New("foo") // want "wrong number of return values \\(want 3, got 2\\)"
+	return "", errors.New("foo") // want "return values"
 }
 
 func c() (string, int, error) {
-	return 7, errors.New("foo") // want "wrong number of return values \\(want 3, got 2\\)"
+	return 7, errors.New("foo") // want "return values"
 }
 
 func d() (string, int, error) {
-	return "", 7 // want "wrong number of return values \\(want 3, got 2\\)"
+	return "", 7 // want "return values"
 }
 
 func e() (T, error, *bool) {
-	return (z(http.ListenAndServe))("", nil) // want "wrong number of return values \\(want 3, got 1\\)"
+	return (z(http.ListenAndServe))("", nil) // want "return values"
 }
 
 func preserveLeft() (int, int, error) {
-	return 1, errors.New("foo") // want "wrong number of return values \\(want 3, got 2\\)"
+	return 1, errors.New("foo") // want "return values"
 }
 
 func matchValues() (int, error, string) {
-	return errors.New("foo"), 3 // want "wrong number of return values \\(want 3, got 2\\)"
+	return errors.New("foo"), 3 // want "return values"
 }
 
 func preventDataOverwrite() (int, string) {
-	return errors.New("foo") // want "wrong number of return values \\(want 2, got 1\\)"
+	return errors.New("foo") // want "return values"
 }
 
 func closure() (string, error) {
 	_ = func() (int, error) {
-		return // want "wrong number of return values \\(want 2, got 0\\)"
+		return // want "return values"
 	}
-	return // want "wrong number of return values \\(want 2, got 0\\)"
+	return // want "return values"
 }
 
 func basic() (uint8, uint16, uint32, uint64, int8, int16, int32, int64, float32, float64, complex64, complex128, byte, rune, uint, int, uintptr, string, bool, error) {
-	return // want "wrong number of return values \\(want 20, got 0\\)"
+	return // want "return values"
 }
 
 func complex() (*int, []int, [2]int, map[int]int) {
-	return // want "wrong number of return values \\(want 4, got 0\\)"
+	return // want "return values"
 }
 
 func structsAndInterfaces() (T, url.URL, T1, I, I1, io.Reader, Client, ast2.Stmt) {
-	return // want "wrong number of return values \\(want 8, got 0\\)"
+	return // want "return values"
 }
 
 func m() (int, error) {
 	if 1 == 2 {
-		return // want "wrong number of return values \\(want 2, got 0\\)"
+		return // want "return values"
 	} else if 1 == 3 {
-		return errors.New("foo") // want "wrong number of return values \\(want 2, got 1\\)"
+		return errors.New("foo") // want "return values"
 	} else {
-		return 1 // want "wrong number of return values \\(want 2, got 1\\)"
+		return 1 // want "return values"
 	}
-	return // want "wrong number of return values \\(want 2, got 0\\)"
+	return // want "return values"
 }
 
 func convertibleTypes() (ast2.Expr, int) {
-	return &ast2.ArrayType{} // want "wrong number of return values \\(want 2, got 1\\)"
+	return &ast2.ArrayType{} // want "return values"
 }
 
 func assignableTypes() (map[string]int, int) {
 	type X map[string]int
 	var x X
-	return x // want "wrong number of return values \\(want 2, got 1\\)"
+	return x // want "return values"
 }
 
 func interfaceAndError() (I, int) {
-	return errors.New("foo") // want "wrong number of return values \\(want 2, got 1\\)"
+	return errors.New("foo") // want "return values"
 }
 
 func funcOneReturn() (string, error) {
-	return strconv.Itoa(1) // want "wrong number of return values \\(want 2, got 1\\)"
+	return strconv.Itoa(1) // want "return values"
 }
 
 func funcMultipleReturn() (int, error, string) {
@@ -110,16 +112,16 @@ func localFuncMultipleReturn() (string, int, error, string) {
 }
 
 func multipleUnused() (int, string, string, string) {
-	return 3, 4, 5 // want "wrong number of return values \\(want 4, got 3\\)"
+	return 3, 4, 5 // want "return values"
 }
 
 func gotTooMany() int {
 	if true {
-		return 0, "" // want "wrong number of return values \\(want 1, got 2\\)"
+		return 0, "" // want "return values"
 	} else {
-		return 1, 0, nil // want "wrong number of return values \\(want 1, got 3\\)"
+		return 1, 0, nil // want "return values"
 	}
-	return 0, 5, false // want "wrong number of return values \\(want 1, got 3\\)"
+	return 0, 5, false // want "return values"
 }
 
 func fillVars() (int, string, ast.Node, bool, error) {
@@ -128,10 +130,10 @@ func fillVars() (int, string, ast.Node, bool, error) {
 	var t bool
 	if true {
 		err := errors.New("fail")
-		return // want "wrong number of return values \\(want 5, got 0\\)"
+		return // want "return values"
 	}
 	n := ast.NewIdent("ident")
 	int := 3
 	var b bool
-	return "" // want "wrong number of return values \\(want 5, got 1\\)"
+	return "" // want "return values"
 }