starlark: fix a crash in UnpackArgs (#89)

When computing the error message, the parameter's Type method is
called. Mostly this works, but it may panic for a user-defined
type whose Type method doesn't work on the zero value.
Recover from the panic and show the Go type instead.

+ Test

Author: adonovan

starlark/eval_test.go

@@ -552,3 +552,34 @@ g(z=7)
 		t.Errorf("got <<%s>>, want <<%s>>", got, want)
 	}
 }
+
+type badType string
+
+func (b *badType) String() string        { return "badType" }
+func (b *badType) Type() string          { return "badType:" + string(*b) } // panics if b==nil
+func (b *badType) Truth() starlark.Bool  { return true }
+func (b *badType) Hash() (uint32, error) { return 0, nil }
+func (b *badType) Freeze()               {}
+
+var _ starlark.Value = new(badType)
+
+// TestUnpackErrorBadType verifies that the Unpack functions fail
+// gracefully when a parameter's default value's Type method panics.
+func TestUnpackErrorBadType(t *testing.T) {
+	for _, test := range []struct {
+		x    *badType
+		want string
+	}{
+		{new(badType), "got NoneType, want badType"},       // Starlark type name
+		{nil, "got NoneType, want *starlark_test.badType"}, // Go type name
+	} {
+		err := starlark.UnpackArgs("f", starlark.Tuple{starlark.None}, nil, "x", &test.x)
+		if err == nil {
+			t.Errorf("UnpackArgs succeeded unexpectedly")
+			continue
+		}
+		if !strings.Contains(err.Error(), test.want) {
+			t.Errorf("UnpackArgs error %q does not contain %q", err, test.want)
+		}
+	}
+}

starlark/library.go

@@ -307,21 +307,44 @@ func unpackOneArg(v Value, ptr interface{}) error {
 			return fmt.Errorf("got %s, want iterable", v.Type())
 		}
 	default:
+		// v must have type *V, where V is some subtype of starlark.Value.
 		ptrv := reflect.ValueOf(ptr)
 		if ptrv.Kind() != reflect.Ptr {
-			log.Fatalf("internal error: not a pointer: %T", ptr)
+			log.Panicf("internal error: not a pointer: %T", ptr)
 		}
-		param := ptrv.Elem()
-		if !reflect.TypeOf(v).AssignableTo(param.Type()) {
-			// Detect mistakes by caller.
-			if !param.Type().AssignableTo(reflect.TypeOf(new(Value)).Elem()) {
-				log.Fatalf("internal error: invalid pointer type: %T", ptr)
+		paramVar := ptrv.Elem()
+		if !reflect.TypeOf(v).AssignableTo(paramVar.Type()) {
+			// The value is not assignable to the variable.
+
+			// Detect a possible bug in the Go program that called Unpack:
+			// If the variable *ptr is not a subtype of Value,
+			// no value of v can possibly work.
+			if !paramVar.Type().AssignableTo(reflect.TypeOf(new(Value)).Elem()) {
+				log.Panicf("pointer element type does not implement Value: %T", ptr)
 			}
-			// Assume it's safe to call Type() on a zero instance.
-			paramType := param.Interface().(Value).Type()
+
+			// Report Starlark dynamic type error.
+			//
+			// We prefer the Starlark Value.Type name over
+			// its Go reflect.Type name, but calling the
+			// Value.Type method on the variable is not safe
+			// in general. If the variable is an interface,
+			// the call will fail. Even if the variable has
+			// a concrete type, it might not be safe to call
+			// Type() on a zero instance. Thus we must use
+			// recover.
+
+			// Default to Go reflect.Type name
+			paramType := paramVar.Type().String()
+
+			// Attempt to call Value.Type method.
+			func() {
+				defer func() { recover() }()
+				paramType = paramVar.MethodByName("Type").Call(nil)[0].String()
+			}()
 			return fmt.Errorf("got %s, want %s", v.Type(), paramType)
 		}
-		param.Set(reflect.ValueOf(v))
+		paramVar.Set(reflect.ValueOf(v))
 	}
 	return nil
 }