From daa4aa59ed4efe3c28946c15b249fb8609074354 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 16 Nov 2023 16:51:56 -0500 Subject: [PATCH] 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 Auto-Submit: Alan Donovan Reviewed-by: Robert Findley --- .../lsp/analysis/stubmethods/stubmethods.go | 19 +++--- gopls/internal/lsp/regtest/marker.go | 22 ++++++- .../regtest/workspace/quickfix_test.go | 61 +++++++++++++++++++ 3 files changed, 93 insertions(+), 9 deletions(-) diff --git a/gopls/internal/lsp/analysis/stubmethods/stubmethods.go b/gopls/internal/lsp/analysis/stubmethods/stubmethods.go index f5b2ac55fd3..40913447199 100644 --- a/gopls/internal/lsp/analysis/stubmethods/stubmethods.go +++ b/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 } diff --git a/gopls/internal/lsp/regtest/marker.go b/gopls/internal/lsp/regtest/marker.go index e2793960e75..913894d9d8b 100644 --- a/gopls/internal/lsp/regtest/marker.go +++ b/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 { diff --git a/gopls/internal/regtest/workspace/quickfix_test.go b/gopls/internal/regtest/workspace/quickfix_test.go index 2fcdabda835..f76039bda48 100644 --- a/gopls/internal/regtest/workspace/quickfix_test.go +++ b/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) + } + } + }) +}