gopls/internal/analysis/stubmethods: fix OOB panic in fromValueSpec

adonovan · gopherbot · commit f40889dc8bf0 · 2023-12-11T23:10:32.000Z
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>
diff --git 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,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
 	}
@@ -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
@@ -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)
+			}
+		}
+	})
+}
