diff --git a/gopls/internal/analysis/stubmethods/stubmethods.go b/gopls/internal/analysis/stubmethods/stubmethods.go index 690cea91b4b..8f9f8c7900b 100644 --- a/gopls/internal/analysis/stubmethods/stubmethods.go +++ b/gopls/internal/analysis/stubmethods/stubmethods.go @@ -119,26 +119,26 @@ type StubInfo struct { // function call. This is essentially what the refactor/satisfy does, // more generally. Refactor to share logic, after auditing 'satisfy' // for safety on ill-typed code. -func GetStubInfo(fset *token.FileSet, ti *types.Info, path []ast.Node, pos token.Pos) *StubInfo { +func GetStubInfo(fset *token.FileSet, info *types.Info, path []ast.Node, pos token.Pos) *StubInfo { for _, n := range path { switch n := n.(type) { case *ast.ValueSpec: - return fromValueSpec(fset, ti, n, pos) + return fromValueSpec(fset, info, n, pos) case *ast.ReturnStmt: // An error here may not indicate a real error the user should know about, but it may. // Therefore, it would be best to log it out for debugging/reporting purposes instead of ignoring // it. However, event.Log takes a context which is not passed via the analysis package. // TODO(marwan-at-work): properly log this error. - si, _ := fromReturnStmt(fset, ti, pos, path, n) + si, _ := fromReturnStmt(fset, info, pos, path, n) return si case *ast.AssignStmt: - return fromAssignStmt(fset, ti, n, pos) + return fromAssignStmt(fset, info, n, pos) case *ast.CallExpr: // Note that some call expressions don't carry the interface type // because they don't point to a function or method declaration elsewhere. // For eaxmple, "var Interface = (*Concrete)(nil)". In that case, continue // this loop to encounter other possibilities such as *ast.ValueSpec or others. - si := fromCallExpr(fset, ti, pos, n) + si := fromCallExpr(fset, info, pos, n) if si != nil { return si } @@ -150,23 +150,26 @@ func GetStubInfo(fset *token.FileSet, ti *types.Info, path []ast.Node, pos token // fromCallExpr tries to find an *ast.CallExpr's function declaration and // analyzes a function call's signature against the passed in parameter to deduce // the concrete and interface types. -func fromCallExpr(fset *token.FileSet, ti *types.Info, pos token.Pos, ce *ast.CallExpr) *StubInfo { - paramIdx := -1 - for i, p := range ce.Args { - if pos >= p.Pos() && pos <= p.End() { - paramIdx = i +func fromCallExpr(fset *token.FileSet, info *types.Info, pos token.Pos, call *ast.CallExpr) *StubInfo { + // Find argument containing pos. + argIdx := -1 + var arg ast.Expr + for i, callArg := range call.Args { + if callArg.Pos() <= pos && pos <= callArg.End() { + argIdx = i + arg = callArg break } } - if paramIdx == -1 { + if arg == nil { return nil } - p := ce.Args[paramIdx] - concObj, pointer := concreteType(p, ti) - if concObj == nil || concObj.Obj().Pkg() == nil { + + concType, pointer := concreteType(arg, info) + if concType == nil || concType.Obj().Pkg() == nil { return nil } - tv, ok := ti.Types[ce.Fun] + tv, ok := info.Types[call.Fun] if !ok { return nil } @@ -175,13 +178,13 @@ func fromCallExpr(fset *token.FileSet, ti *types.Info, pos token.Pos, ce *ast.Ca return nil } var paramType types.Type - if sig.Variadic() && paramIdx >= sig.Params().Len()-1 { + if sig.Variadic() && argIdx >= sig.Params().Len()-1 { v := sig.Params().At(sig.Params().Len() - 1) if s, _ := v.Type().(*types.Slice); s != nil { paramType = s.Elem() } - } else if paramIdx < sig.Params().Len() { - paramType = sig.Params().At(paramIdx).Type() + } else if argIdx < sig.Params().Len() { + paramType = sig.Params().At(argIdx).Type() } if paramType == nil { return nil // A type error prevents us from determining the param type. @@ -192,7 +195,7 @@ func fromCallExpr(fset *token.FileSet, ti *types.Info, pos token.Pos, ce *ast.Ca } return &StubInfo{ Fset: fset, - Concrete: concObj, + Concrete: concType, Pointer: pointer, Interface: iface, } @@ -203,21 +206,24 @@ 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, ret *ast.ReturnStmt) (*StubInfo, error) { +func fromReturnStmt(fset *token.FileSet, info *types.Info, pos token.Pos, path []ast.Node, ret *ast.ReturnStmt) (*StubInfo, error) { + // Find return operand containing pos. returnIdx := -1 for i, r := range ret.Results { - if pos >= r.Pos() && pos <= r.End() { + if r.Pos() <= pos && pos <= r.End() { returnIdx = i + break } } if returnIdx == -1 { return nil, fmt.Errorf("pos %d not within return statement bounds: [%d-%d]", pos, ret.Pos(), ret.End()) } - concObj, pointer := concreteType(ret.Results[returnIdx], ti) - if concObj == nil || concObj.Obj().Pkg() == nil { + + concType, pointer := concreteType(ret.Results[returnIdx], info) + if concType == nil || concType.Obj().Pkg() == nil { return nil, nil } - funcType := enclosingFunction(path, ti) + funcType := enclosingFunction(path, info) if funcType == nil { return nil, fmt.Errorf("could not find the enclosing function of the return statement") } @@ -226,13 +232,13 @@ func fromReturnStmt(fset *token.FileSet, ti *types.Info, pos token.Pos, path []a len(ret.Results), len(funcType.Results.List)) } - iface := ifaceType(funcType.Results.List[returnIdx].Type, ti) + iface := ifaceType(funcType.Results.List[returnIdx].Type, info) if iface == nil { return nil, nil } return &StubInfo{ Fset: fset, - Concrete: concObj, + Concrete: concType, Pointer: pointer, Interface: iface, }, nil @@ -240,72 +246,77 @@ func fromReturnStmt(fset *token.FileSet, ti *types.Info, pos token.Pos, path []a // fromValueSpec returns *StubInfo from a variable declaration such as // var x io.Writer = &T{} -func fromValueSpec(fset *token.FileSet, ti *types.Info, vs *ast.ValueSpec, pos token.Pos) *StubInfo { - var idx int - for i, vs := range vs.Values { - if pos >= vs.Pos() && pos <= vs.End() { - idx = i +func fromValueSpec(fset *token.FileSet, info *types.Info, spec *ast.ValueSpec, pos token.Pos) *StubInfo { + // Find RHS element containing pos. + var rhs ast.Expr + for _, r := range spec.Values { + if r.Pos() <= pos && pos <= r.End() { + rhs = r break } } + if rhs == nil { + return nil // e.g. pos was on the LHS (#64545) + } - valueNode := vs.Values[idx] - ifaceNode := vs.Type - callExp, ok := valueNode.(*ast.CallExpr) - // if the ValueSpec is `var _ = myInterface(...)` - // as opposed to `var _ myInterface = ...` - if ifaceNode == nil && ok && len(callExp.Args) == 1 { - ifaceNode = callExp.Fun - valueNode = callExp.Args[0] - } - concObj, pointer := concreteType(valueNode, ti) - if concObj == nil || concObj.Obj().Pkg() == nil { + // Possible implicit/explicit conversion to interface type? + ifaceNode := spec.Type // var _ myInterface = ... + if call, ok := rhs.(*ast.CallExpr); ok && ifaceNode == nil && len(call.Args) == 1 { + // var _ = myInterface(v) + ifaceNode = call.Fun + rhs = call.Args[0] + } + concType, pointer := concreteType(rhs, info) + if concType == nil || concType.Obj().Pkg() == nil { return nil } - ifaceObj := ifaceType(ifaceNode, ti) + ifaceObj := ifaceType(ifaceNode, info) if ifaceObj == nil { return nil } return &StubInfo{ Fset: fset, - Concrete: concObj, + Concrete: concType, Interface: ifaceObj, Pointer: pointer, } } -// fromAssignStmt returns *StubInfo from a variable re-assignment such as +// fromAssignStmt returns *StubInfo from a variable assignment such as // var x io.Writer // x = &T{} -func fromAssignStmt(fset *token.FileSet, ti *types.Info, as *ast.AssignStmt, pos token.Pos) *StubInfo { - idx := -1 +func fromAssignStmt(fset *token.FileSet, info *types.Info, assign *ast.AssignStmt, pos token.Pos) *StubInfo { + // The interface conversion error in an assignment is against the RHS: + // + // var x io.Writer + // x = &T{} // error: missing method + // ^^^^ + // + // Find RHS element containing pos. var lhs, rhs ast.Expr - // Given a re-assignment interface conversion error, - // the compiler error shows up on the right hand side of the expression. - // For example, x = &T{} where x is io.Writer highlights the error - // under "&T{}" and not "x". - for i, hs := range as.Rhs { - if pos >= hs.Pos() && pos <= hs.End() { - idx = i + for i, r := range assign.Rhs { + if r.Pos() <= pos && pos <= r.End() { + if i >= len(assign.Lhs) { + // This should never happen as we would get a + // "cannot assign N values to M variables" + // before we get an interface conversion error. + // But be defensive. + return nil + } + lhs = assign.Lhs[i] + rhs = r break } } - if idx == -1 { + if lhs == nil || rhs == nil { return nil } - // Technically, this should never happen as - // we would get a "cannot assign N values to M variables" - // before we get an interface conversion error. Nonetheless, - // guard against out of range index errors. - if idx >= len(as.Lhs) { - return nil - } - lhs, rhs = as.Lhs[idx], as.Rhs[idx] - ifaceObj := ifaceType(lhs, ti) + + ifaceObj := ifaceType(lhs, info) if ifaceObj == nil { return nil } - concType, pointer := concreteType(rhs, ti) + concType, pointer := concreteType(rhs, info) if concType == nil || concType.Obj().Pkg() == nil { return nil } @@ -382,11 +393,9 @@ func RelativeToFiles(concPkg *types.Package, concFile *ast.File, ifaceImports [] } } -// ifaceType will try to extract the types.Object that defines -// the interface given the ast.Expr where the "missing method" -// or "conversion" errors happen. -func ifaceType(n ast.Expr, ti *types.Info) *types.TypeName { - tv, ok := ti.Types[n] +// ifaceType returns the named interface type to which e refers, if any. +func ifaceType(e ast.Expr, info *types.Info) *types.TypeName { + tv, ok := info.Types[e] if !ok { return nil } @@ -398,8 +407,7 @@ func ifaceObjFromType(t types.Type) *types.TypeName { if !ok { return nil } - _, ok = named.Underlying().(*types.Interface) - if !ok { + if !types.IsInterface(named) { return nil } // Interfaces defined in the "builtin" package return nil a Pkg(). @@ -418,8 +426,8 @@ func ifaceObjFromType(t types.Type) *types.TypeName { // method will return a nil *types.Named. The second return parameter // is a boolean that indicates whether the concreteType was defined as a // pointer or value. -func concreteType(n ast.Expr, ti *types.Info) (*types.Named, bool) { - tv, ok := ti.Types[n] +func concreteType(e ast.Expr, info *types.Info) (*types.Named, bool) { + tv, ok := info.Types[e] if !ok { return nil, false } diff --git a/gopls/internal/test/integration/workspace/quickfix_test.go b/gopls/internal/test/integration/workspace/quickfix_test.go index cf440756117..1524f56bcde 100644 --- a/gopls/internal/test/integration/workspace/quickfix_test.go +++ b/gopls/internal/test/integration/workspace/quickfix_test.go @@ -336,6 +336,8 @@ 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. + // + // See also the similar TestStubMethods64545 below. const files = ` This is a regression test for a panic (issue #64087) in stub methods. @@ -392,3 +394,57 @@ type myerror struct{any} } }) } + +func TestStubMethods64545(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. + // + // TODO(adonovan): we may need to generalize this test and + // TestStubMethods64087 if this happens a lot. + + const files = ` +This is a regression test for a panic (issue #64545) in stub methods. + +The illegal expression int("") caused a "cannot convert" error that +spuriously triggered the "stub methods" in a function whose var +spec had no RHS values, leading to an out-of-bounds index. + +-- go.mod -- +module mod.com +go 1.18 + +-- a.go -- +package a + +var _ [int("")]byte +` + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("a.go") + + // Expect a "cannot convert" diagnostic, and perhaps others. + var d protocol.PublishDiagnosticsParams + env.AfterChange(ReadDiagnostics("a.go", &d)) + + found := false + for i, diag := range d.Diagnostics { + t.Logf("Diagnostics[%d] = %q (%s)", i, diag.Message, diag.Source) + if strings.Contains(diag.Message, "cannot convert") { + found = true + } + } + if !found { + t.Fatalf("Expected 'cannot convert' 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) + } + } + }) +}