gopls/internal/lsp/source: stubmethods: fix out-of-bounds index

The attatchedd test case spuriously triggered stubmethods
(the "cannot convert" error is about something else) in a function
whose return an functype arities were mismatched.

The new behavior is "nothing happens", no code actions are
offered, nothing is logged, and no suggested fix is applied,
or fails to apply. (Along the way I implemented
suggestedfixerr before I realized it would not be useful;
but it may be useful later.)

Fixes golang/go#64087

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

gopls/internal/lsp/analysis/stubmethods/stubmethods.go

@@ -203,25 +203,30 @@ func fromCallExpr(fset *token.FileSet, ti *types.Info, pos token.Pos, ce *ast.Ca
 //
 // For example, func() io.Writer { return myType{} }
 // would return StubInfo with the interface being io.Writer and the concrete type being myType{}.
-func fromReturnStmt(fset *token.FileSet, ti *types.Info, pos token.Pos, path []ast.Node, rs *ast.ReturnStmt) (*StubInfo, error) {
+func fromReturnStmt(fset *token.FileSet, ti *types.Info, pos token.Pos, path []ast.Node, ret *ast.ReturnStmt) (*StubInfo, error) {
 	returnIdx := -1
-	for i, r := range rs.Results {
+	for i, r := range ret.Results {
 		if pos >= r.Pos() && pos <= r.End() {
 			returnIdx = i
 		}
 	}
 	if returnIdx == -1 {
-		return nil, fmt.Errorf("pos %d not within return statement bounds: [%d-%d]", pos, rs.Pos(), rs.End())
+		return nil, fmt.Errorf("pos %d not within return statement bounds: [%d-%d]", pos, ret.Pos(), ret.End())
 	}
-	concObj, pointer := concreteType(rs.Results[returnIdx], ti)
+	concObj, pointer := concreteType(ret.Results[returnIdx], ti)
 	if concObj == nil || concObj.Obj().Pkg() == nil {
 		return nil, nil
 	}
-	ef := enclosingFunction(path, ti)
-	if ef == nil {
+	funcType := enclosingFunction(path, ti)
+	if funcType == nil {
 		return nil, fmt.Errorf("could not find the enclosing function of the return statement")
 	}
-	iface := ifaceType(ef.Results.List[returnIdx].Type, ti)
+	if len(funcType.Results.List) != len(ret.Results) {
+		return nil, fmt.Errorf("%d-operand return statement in %d-result function",
+			len(ret.Results),
+			len(funcType.Results.List))
+	}
+	iface := ifaceType(funcType.Results.List[returnIdx].Type, ti)
 	if iface == nil {
 		return nil, nil
 	}

gopls/internal/lsp/regtest/marker.go

@@ -263,6 +263,11 @@ var update = flag.Bool("update", false, "if set, update test data during marker
 //     This action is executed for its editing effects on the source files.
 //     Like rename, the golden directory contains the expected transformed files.
 //
+//   - suggestedfixerr(location, regexp, kind, wantError): specifies that the
+//     suggestedfix operation should fail with an error that matches the expectation.
+//     (Failures in the computation to offer a fix do not generally result
+//     in LSP errors, so this marker is not appropriate for testing them.)
+//
 //   - rank(location, ...completionItem): executes a textDocument/completion
 //     request at the given location, and verifies that each expected
 //     completion item occurs in the results, in the expected order. Other
@@ -770,6 +775,7 @@ var actionMarkerFuncs = map[string]func(marker){
 	"signature":        actionMarkerFunc(signatureMarker),
 	"snippet":          actionMarkerFunc(snippetMarker),
 	"suggestedfix":     actionMarkerFunc(suggestedfixMarker),
+	"suggestedfixerr":  actionMarkerFunc(suggestedfixErrMarker),
 	"symbol":           actionMarkerFunc(symbolMarker),
 	"token":            actionMarkerFunc(tokenMarker),
 	"typedef":          actionMarkerFunc(typedefMarker),
@@ -2139,6 +2145,20 @@ func suggestedfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, g
 	checkDiffs(mark, changed, golden)
 }
 
+func suggestedfixErrMarker(mark marker, loc protocol.Location, re *regexp.Regexp, wantErr wantError) {
+	loc.Range.End = loc.Range.Start // diagnostics ignore end position.
+	// Find and remove the matching diagnostic.
+	diag, ok := removeDiagnostic(mark, loc, re)
+	if !ok {
+		mark.errorf("no diagnostic at %v matches %q", loc, re)
+		return
+	}
+
+	// Apply the fix it suggests.
+	_, err := codeAction(mark.run.env, loc.URI, diag.Range, "quickfix", &diag, nil)
+	wantErr.check(mark, err)
+}
+
 // codeAction executes a textDocument/codeAction request for the specified
 // location and kind. If diag is non-nil, it is used as the code action
 // context.
@@ -2254,8 +2274,6 @@ func codeActionChanges(env *Env, uri protocol.DocumentURI, rng protocol.Range, a
 	return nil, nil
 }
 
-// TODO(adonovan): suggestedfixerr
-
 // refsMarker implements the @refs marker.
 func refsMarker(mark marker, src protocol.Location, want ...protocol.Location) {
 	refs := func(includeDeclaration bool, want []protocol.Location) error {

gopls/internal/regtest/workspace/quickfix_test.go

@@ -331,3 +331,64 @@ func main() {}
 		})
 	}
 }
+
+func TestStubMethods64087(t *testing.T) {
+	// We can't use the @fix or @suggestedfixerr or @codeactionerr
+	// because the error now reported by the corrected logic
+	// is internal and silently causes no fix to be offered.
+
+	const files = `
+This is a regression test for a panic (issue #64087) in stub methods.
+
+The illegal expression int("") caused a "cannot convert" error that
+spuriously triggered the "stub methods" in a function whose return
+statement had too many operands, leading to an out-of-bounds index.
+
+-- go.mod --
+module mod.com
+go 1.18
+
+-- a.go --
+package a
+
+func f() error {
+	return nil, myerror{int("")}
+}
+
+type myerror struct{any}
+`
+	Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("a.go")
+
+		// Expect a "wrong result count" diagnostic.
+		var d protocol.PublishDiagnosticsParams
+		env.AfterChange(ReadDiagnostics("a.go", &d))
+
+		// In no particular order, we expect:
+		//  "...too many return values..." (compiler)
+		//  "...cannot convert..." (compiler)
+		// and possibly:
+		//  "...too many return values..." (fillreturns)
+		// We check only for the first of these.
+		found := false
+		for i, diag := range d.Diagnostics {
+			t.Logf("Diagnostics[%d] = %q (%s)", i, diag.Message, diag.Source)
+			if strings.Contains(diag.Message, "too many return") {
+				found = true
+			}
+		}
+		if !found {
+			t.Fatalf("Expected WrongResultCount diagnostic not found.")
+		}
+
+		// GetQuickFixes should not panic (the original bug).
+		fixes := env.GetQuickFixes("a.go", d.Diagnostics)
+
+		// We should not be offered a "stub methods" fix.
+		for _, fix := range fixes {
+			if strings.Contains(fix.Title, "Implement error") {
+				t.Errorf("unexpected 'stub methods' fix: %#v", fix)
+			}
+		}
+	})
+}