Better argument validation for core methods.

runtests

@@ -195,8 +195,8 @@ def run_test(path):
     for line in expect_error:
       fails.append('Expected error on line ' + str(line) + ' and got none.')
     if expect_runtime_error:
-      fails.append('Expected runtime error ' + expect_runtime_error +
-          ' and got none.')
+      fails.append('Expected runtime error "' + expect_runtime_error +
+          '" and got none.')
 
   # Validate the exit code.
   if proc.returncode != expect_return:

src/wren_core.c

@@ -87,29 +87,6 @@ const char* coreLibSource =
 "  ... other { return new Range(this, other - 1) }\n"
 "}\n";
 
-// TODO: Remove this and migrate everything to use validateIndex().
-// Validates that [index] is an integer within `[0, count)`. Also allows
-// negative indices which map backwards from the end. Returns the valid positive
-// index value, or -1 if the index wasn't valid (not a number, not an int, out
-// of bounds).
-static int validateIndexOld(Value index, int count)
-{
-  if (!IS_NUM(index)) return -1;
-
-  double indexNum = AS_NUM(index);
-  int intIndex = (int)indexNum;
-  // Make sure the index is an integer.
-  if (indexNum != intIndex) return -1;
-
-  // Negative indices count from the end.
-  if (indexNum < 0) indexNum = count + indexNum;
-
-  // Check bounds.
-  if (indexNum < 0 || indexNum >= count) return -1;
-
-  return indexNum;
-}
-
 // Validates that the given argument in [args] is a Num. Returns true if it is.
 // If not, reports an error and returns false.
 static bool validateNum(WrenVM* vm, Value* args, int index, const char* argName)
@@ -161,6 +138,19 @@ static int validateIndex(WrenVM* vm, Value* args, int count, int argIndex,
   return -1;
 }
 
+// Validates that the given argument in [args] is a String. Returns true if it
+// is. If not, reports an error and returns false.
+static bool validateString(WrenVM* vm, Value* args, int index,
+                           const char* argName)
+{
+  if (IS_STRING(args[index])) return true;
+
+  char message[100];
+  snprintf(message, 100, "%s must be a string.", argName);
+  args[0] = wrenNewString(vm, message, strlen(message));
+  return false;
+}
+
 DEF_NATIVE(bool_not)
 {
   RETURN_BOOL(!AS_BOOL(args[0]));
@@ -244,10 +234,8 @@ DEF_NATIVE(list_iteratorValue)
 DEF_NATIVE(list_removeAt)
 {
   ObjList* list = AS_LIST(args[0]);
-  int index = validateIndexOld(args[1], list->count);
-  // TODO: Instead of returning null here, should signal an error explicitly
-  // somehow.
-  if (index == -1) RETURN_NULL;
+  int index = validateIndex(vm, args, list->count, 1, "Index");
+  if (index == -1) return PRIM_ERROR;
 
   RETURN_VAL(wrenListRemoveAt(vm, list, index));
 }
@@ -255,11 +243,8 @@ DEF_NATIVE(list_removeAt)
 DEF_NATIVE(list_subscript)
 {
   ObjList* list = AS_LIST(args[0]);
-
-  int index = validateIndexOld(args[1], list->count);
-  // TODO: Instead of returning null here, should signal an error explicitly
-  // somehow.
-  if (index == -1) RETURN_NULL;
+  int index = validateIndex(vm, args, list->count, 1, "Subscript");
+  if (index == -1) return PRIM_ERROR;
 
   RETURN_VAL(list->elements[index]);
 }
@@ -267,11 +252,8 @@ DEF_NATIVE(list_subscript)
 DEF_NATIVE(list_subscriptSetter)
 {
   ObjList* list = AS_LIST(args[0]);
-
-  int index = validateIndexOld(args[1], list->count);
-  // TODO: Instead of returning null here, should signal an error explicitly
-  // somehow.
-  if (index == -1) RETURN_NULL;
+  int index = validateIndex(vm, args, list->count, 1, "Subscript");
+  if (index == -1) return PRIM_ERROR;
 
   list->elements[index] = args[2];
   RETURN_VAL(args[2]);
@@ -409,8 +391,9 @@ DEF_NATIVE(object_type)
 
 DEF_NATIVE(string_contains)
 {
+  if (!validateString(vm, args, 1, "Argument")) return PRIM_ERROR;
+
   const char* string = AS_CSTRING(args[0]);
-  // TODO: Check type of arg first!
   const char* search = AS_CSTRING(args[1]);
 
   // Corner case, the empty string contains the empty string.
@@ -468,24 +451,12 @@ DEF_NATIVE(string_bangeq)
 
 DEF_NATIVE(string_subscript)
 {
-  // TODO: Instead of returning null here, all of these failure cases should
-  // signal an error explicitly somehow.
-  if (!IS_NUM(args[1])) RETURN_NULL;
-
-  double indexNum = AS_NUM(args[1]);
-  int index = (int)indexNum;
-  // Make sure the index is an integer.
-  if (indexNum != index) RETURN_NULL;
-
   ObjString* string = AS_STRING(args[0]);
-
-  // Negative indices count from the end.
   // TODO: Strings should cache their length.
   int length = (int)strlen(string->value);
-  if (index < 0) index = length + index;
 
-  // Check bounds.
-  if (index < 0 || index >= length) RETURN_NULL;
+  int index = validateIndex(vm, args, length, 1, "Subscript");
+  if (index == -1) return PRIM_ERROR;
 
   // The result is a one-character string.
   // TODO: Handle UTF-8.

test/list/remove_at.wren

@@ -23,10 +23,5 @@ var f = [1, 2, 3]
 f.removeAt(-1)
 IO.write(f) // expect: [1, 2]
 
-// Out of bounds.
-// TODO: Signal error in better way.
-IO.write([1, 2, 3].removeAt(3)) // expect: null
-IO.write([1, 2, 3].removeAt(-4)) // expect: null
-
 // Return the removed value.
 IO.write([3, 4, 5].removeAt(1)) // expect: 4

test/list/remove_at_index_not_int.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.removeAt(1.5) // expect runtime error: Index must be an integer.

test/list/remove_at_index_not_num.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.removeAt("2") // expect runtime error: Index must be a number.

test/list/remove_at_index_too_large.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.removeAt(4) // expect runtime error: Index out of bounds.

test/list/remove_at_index_too_small.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.removeAt(-5) // expect runtime error: Index out of bounds.

test/list/subscript.wren

@@ -10,16 +10,3 @@ IO.write(list[-4]) // expect: a
 IO.write(list[-3]) // expect: b
 IO.write(list[-2]) // expect: c
 IO.write(list[-1]) // expect: d
-
-// Handle out of bounds.
-// TODO: Should halt the fiber or raise an error somehow.
-IO.write(list[4])  // expect: null
-IO.write(list[-5]) // expect: null
-
-// Handle wrong argument type.
-// TODO: Should halt the fiber or raise an error somehow.
-IO.write(list[true]) // expect: null
-
-// Handle non-integer index.
-// TODO: Should halt the fiber or raise an error somehow.
-IO.write(list[1.5]) // expect: null

test/list/subscript_not_int.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a[1.5] // expect runtime error: Subscript must be an integer.

test/list/subscript_not_num.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a["2"] // expect runtime error: Subscript must be a number.

test/list/subscript_setter_not_int.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a[1.5] = 1 // expect runtime error: Subscript must be an integer.

test/list/subscript_setter_not_num.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a["2"] = 1 // expect runtime error: Subscript must be a number.

test/list/subscript_setter_too_large.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a[4] = 1 // expect runtime error: Subscript out of bounds.

test/list/subscript_setter_too_small.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a[-5] = 1 // expect runtime error: Subscript out of bounds.

test/list/subscript_too_large.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a[4] // expect runtime error: Subscript out of bounds.

test/list/subscript_too_small.wren

@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a[-5] // expect runtime error: Subscript out of bounds.

test/string/contains.wren

@@ -4,5 +4,3 @@ IO.write("something".contains("meth")) // expect: true
 IO.write("something".contains("some")) // expect: true
 IO.write("something".contains("ing"))  // expect: true
 IO.write("something".contains("math")) // expect: false
-
-// TODO: Passing non-string as argument.

test/string/contains_argument_not_string.wren

@@ -0,0 +1 @@
+"foo".contains(1) // expect runtime error: Argument must be a string.

test/string/subscript.wren

@@ -9,16 +9,3 @@ IO.write("abcd"[-4]) // expect: a
 IO.write("abcd"[-3]) // expect: b
 IO.write("abcd"[-2]) // expect: c
 IO.write("abcd"[-1]) // expect: d
-
-// Handle out of bounds.
-// TODO: Should halt the fiber or raise an error somehow.
-IO.write("abcd"[4])  // expect: null
-IO.write("abcd"[-5]) // expect: null
-
-// Handle wrong argument type.
-// TODO: Should halt the fiber or raise an error somehow.
-IO.write("abcd"[true]) // expect: null
-
-// Handle non-integer index.
-// TODO: Should halt the fiber or raise an error somehow.
-IO.write("abcd"[1.5]) // expect: null

test/string/subscript_not_int.wren

@@ -0,0 +1,2 @@
+var a = "123"
+a[1.5] // expect runtime error: Subscript must be an integer.

test/string/subscript_not_num.wren

@@ -0,0 +1,2 @@
+var a = "123"
+a["2"] // expect runtime error: Subscript must be a number.

test/string/subscript_too_large.wren

@@ -0,0 +1,2 @@
+var a = "123"
+a[4] // expect runtime error: Subscript out of bounds.

test/string/subscript_too_small.wren

@@ -0,0 +1,2 @@
+var a = "123"
+a[-5] // expect runtime error: Subscript out of bounds.