pkg/proc: return better error attempting to call nonexistent function (#4062)

Fixes #4051

_fixtures/issue4051.go

@@ -0,0 +1,45 @@
+package main
+
+import (
+	"fmt"
+	"runtime"
+)
+
+var os = func() func() {
+	a := 1
+	return func() {
+		fmt.Println(a)
+		a++
+	}
+}()
+
+func Hello(name string) string {
+	msg := "Hello, " + name
+	fmt.Println(msg)
+	return msg
+}
+
+func f(i int) func() {
+	return func() {
+		fmt.Println("Function f called with:", i)
+	}
+}
+
+func main() {
+	fmt.Println("Program started")
+	fmt.Println("Ready for Delve call")
+	runtime.Breakpoint()
+
+	type m struct {
+		Hello string
+	}
+	main := m{
+		Hello: "World",
+	}
+	fmt.Println(main.Hello)
+	fn := f(42)
+	runtime.Breakpoint()
+	fn()
+
+	os()
+}

pkg/proc/proc_test.go

@@ -5479,7 +5479,7 @@ func TestReadClosure(t *testing.T) {
 	}
 	withTestProcess("closurecontents", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
 		avalues := []int64{0, 3, 9, 27}
-		for i := 0; i < 4; i++ {
+		for i := range 4 {
 			assertNoError(grp.Continue(), t, "Continue()")
 			accV := evalVariable(p, t, "acc")
 			t.Log(api.ConvertVar(accV).MultilineString("", ""))

pkg/proc/variables.go

@@ -1136,12 +1136,21 @@ func (v *Variable) structMember(memberName string) (*Variable, error) {
 			v = &v.Children[0]
 		}
 	case reflect.Func:
+		fn := v.bi.PCToFunc(v.Base)
 		v.loadFunctionPtr(0, LoadConfig{MaxVariableRecurse: -1})
 		if v.Unreadable != nil {
+			cst := fn.extra(v.bi).closureStructType
+			if cst == nil || cst.ByteSize == 0 {
+				// Not a closure, normal function
+				if _, ok := v.bi.PackageMap[vname]; ok {
+					return nil, fmt.Errorf("package %s has no function %s", vname, memberName)
+				}
+				return nil, fmt.Errorf("%s has no member %s", vname, memberName)
+			}
 			return nil, v.Unreadable
 		}
 		if v.closureAddr != 0 {
-			fn := v.bi.PCToFunc(v.Base)
+			fn = v.bi.PCToFunc(v.Base)
 			if fn != nil {
 				cst := fn.extra(v.bi).closureStructType
 				v = v.newVariable(v.Name, v.closureAddr, cst, v.mem)

pkg/proc/variables_test.go

@@ -1575,6 +1575,58 @@ func testCallFunctionIntl(t *testing.T, grp *proc.TargetGroup, p *proc.Target, t
 	}
 }
 
+func TestIssue4051(t *testing.T) {
+	protest.MustSupportFunctionCalls(t, testBackend)
+	protest.AllowRecording(t)
+	withTestProcess("issue4051", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
+		err := grp.Continue()
+		assertNoError(err, t, "initial continue to breakpoint failed")
+
+		err = proc.EvalExpressionWithCalls(grp, p.SelectedGoroutine(), `main.Hello("world")`, pnormalLoadConfig, true)
+		if err == nil {
+			t.Fatal("expected error, got nil")
+		}
+		expectedError := "package main has no function Hello"
+		if err.Error() != expectedError {
+			t.Fatalf("expected error %q, got %q", expectedError, err)
+		}
+
+		err = grp.Continue()
+		assertNoError(err, t, "initial continue to breakpoint failed")
+
+		err = proc.EvalExpressionWithCalls(grp, p.SelectedGoroutine(), `main.Hello("world")`, pnormalLoadConfig, true)
+		if err == nil {
+			t.Fatal("expected error, got nil")
+		}
+		expectedError = `expression "main.Hello" is not a function`
+		if err.Error() != expectedError {
+			t.Fatalf("expected error %q, got %q", expectedError, err)
+		}
+
+		v, err := evalVariableWithCfg(p, "main.Hello", pshortLoadConfig)
+		assertNoError(err, t, "eval of main.Hello returned an error")
+		assertVariable(t, v, varTest{"main.Hello", true, `"World"`, ``, `string`, nil})
+
+		v, err = evalVariableWithCfg(p, "os.a", pshortLoadConfig)
+		assertNoError(err, t, "eval of os.a returned an error")
+		assertVariable(t, v, varTest{"os.a", true, "1", ``, `int`, nil})
+
+		// TODO(deparker): we *should* get an error here, but the one we expect in this test
+		// is not the ideal error. We should really improve type checking in the evaluator.
+		v, err = evalVariableWithCfg(p, "main.f.func1.i", pshortLoadConfig)
+		expectedError = `main.f has no member func1`
+		if err.Error() != expectedError {
+			t.Fatalf("expected error %q, got %q", expectedError, err)
+		}
+
+		_, err = evalVariableWithCfg(p, "main.f.func1.somethingthatdoesntexist", pshortLoadConfig)
+		expectedError = `main.f has no member func1`
+		if err.Error() != expectedError {
+			t.Fatalf("expected error %q, got %q", expectedError, err)
+		}
+	})
+}
+
 func TestIssue1531(t *testing.T) {
 	// Go 1.12 introduced a change to the map representation where empty cells can be marked with 1 instead of just 0.
 	withTestProcess("issue1531", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {