Skip to content

Commit

Permalink
gopls/internal/analysis/stubmethods: fix OOB panic in fromValueSpec
Browse files Browse the repository at this point in the history 
The loop over ValueSpec.Rhs assumed it was nonempty, but it's
possible to spuriously trigger a "cannot convert" error when
the problem is on the LHS. (I don't know if the attached test
case is what caused the panic in the field, but it seems
plausible.)

Added a test, similar to the one to fix golang/go#64087.

Also, audit for similar mistakes, and tidy the code up,
to use conventional variable names and simpler logic.

Fixes golang/go#64545

Change-Id: I49851435e7a363641a2844620633099b11f1e414
Reviewed-on: https://go-review.googlesource.com/c/tools/+/548737
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
@adonovan @gopherbot
adonovan authored and gopherbot committed Dec 11, 2023
1 parent 113a081 commit f40889d
Show file tree
Hide file tree
Showing 2 changed files with 138 additions and 74 deletions.
156 changes: 82 additions & 74 deletions gopls/internal/analysis/stubmethods/stubmethods.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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.
Expand All @@ -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,
}
Expand All @@ -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")
}
Expand All @@ -226,86 +232,91 @@ 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
}

// 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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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().
Expand All @@ -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
}
Expand Down
56 changes: 56 additions & 0 deletions gopls/internal/test/integration/workspace/quickfix_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}
})
}

0 comments on commit f40889d

Please sign in to comment.