From a89386b4e8dfb885fb864ff9aeef74e7e7b016db Mon Sep 17 00:00:00 2001
From: Bob Nystrom <robert@stuffwithstuff.com>
Date: Thu, 2 Jan 2014 07:32:00 -0800
Subject: [PATCH] Better argument validation for some list methods.

---
 src/wren_common.h                             |   2 -
 src/wren_core.c                               | 147 +++++++++++-------
 test/list/insert.wren                         |   3 -
 test/list/insert_index_not_int.wren           |   2 +
 test/list/insert_index_not_num.wren           |   2 +
 test/list/insert_index_too_large.wren         |   2 +
 test/list/insert_index_too_small.wren         |   2 +
 test/list/iterate.wren                        |   7 +
 test/list/iterate_iterator_not_int.wren       |   2 +
 test/list/iterate_iterator_not_num.wren       |   2 +
 test/list/iterator_value.wren                 |   5 +
 .../list/iterator_value_iterator_not_int.wren |   2 +
 .../list/iterator_value_iterator_not_num.wren |   2 +
 .../iterator_value_iterator_too_large.wren    |   2 +
 .../iterator_value_iterator_too_small.wren    |   2 +
 test/number/divide_operand_not_num.wren       |   1 +
 test/number/divide_wrong_argument_type.wren   |   1 -
 test/number/minus_operand_not_num.wren        |   1 +
 test/number/minus_wrong_argument_type.wren    |   1 -
 test/number/mod_operand_not_num.wren          |   1 +
 test/number/mod_wrong_argument_type.wren      |   1 -
 test/number/multiply_operand_not_num.wren     |   1 +
 test/number/multiply_wrong_argument_type.wren |   1 -
 test/number/plus_operand_not_num.wren         |   3 +
 test/number/plus_wrong_argument_type.wren     |   3 -
 25 files changed, 133 insertions(+), 65 deletions(-)
 create mode 100644 test/list/insert_index_not_int.wren
 create mode 100644 test/list/insert_index_not_num.wren
 create mode 100644 test/list/insert_index_too_large.wren
 create mode 100644 test/list/insert_index_too_small.wren
 create mode 100644 test/list/iterate.wren
 create mode 100644 test/list/iterate_iterator_not_int.wren
 create mode 100644 test/list/iterate_iterator_not_num.wren
 create mode 100644 test/list/iterator_value.wren
 create mode 100644 test/list/iterator_value_iterator_not_int.wren
 create mode 100644 test/list/iterator_value_iterator_not_num.wren
 create mode 100644 test/list/iterator_value_iterator_too_large.wren
 create mode 100644 test/list/iterator_value_iterator_too_small.wren
 create mode 100644 test/number/divide_operand_not_num.wren
 delete mode 100644 test/number/divide_wrong_argument_type.wren
 create mode 100644 test/number/minus_operand_not_num.wren
 delete mode 100644 test/number/minus_wrong_argument_type.wren
 create mode 100644 test/number/mod_operand_not_num.wren
 delete mode 100644 test/number/mod_wrong_argument_type.wren
 create mode 100644 test/number/multiply_operand_not_num.wren
 delete mode 100644 test/number/multiply_wrong_argument_type.wren
 create mode 100644 test/number/plus_operand_not_num.wren
 delete mode 100644 test/number/plus_wrong_argument_type.wren

diff --git a/src/wren_common.h b/src/wren_common.h
index 402056d9..ef0c6112 100644
--- a/src/wren_common.h
+++ b/src/wren_common.h
@@ -1,8 +1,6 @@
 #ifndef wren_common_h
 #define wren_common_h
 
-// TODO: Would be good to use "const" on pointer params when possible.
-
 // This header contains macros and defines used across the entire Wren
 // implementation. In particular, it contains "configuration" defines that
 // control how Wren works. Some of these are only used while hacking on
diff --git a/src/wren_core.c b/src/wren_core.c
index 1cfca300..73f046c5 100644
--- a/src/wren_core.c
+++ b/src/wren_core.c
@@ -87,6 +87,80 @@ 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)
+{
+  if (IS_NUM(args[index])) return true;
+
+  char message[100];
+  snprintf(message, 100, "%s must be a number.", argName);
+  args[0] = wrenNewString(vm, message, strlen(message));
+  return false;
+}
+
+// Validates that the given argument in [args] is an integer. Returns true if
+// it is. If not, reports an error and returns false.
+static bool validateInt(WrenVM* vm, Value* args, int index, const char* argName)
+{
+  // Make sure it's a number first.
+  if (!validateNum(vm, args, index, argName)) return false;
+
+  double value = AS_NUM(args[index]);
+  if (trunc(value) == value) return true;
+
+  char message[100];
+  snprintf(message, 100, "%s must be an integer.", argName);
+  args[0] = wrenNewString(vm, message, strlen(message));
+  return false;
+}
+
+// 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. If invalid, reports an error and returns -1.
+static int validateIndex(WrenVM* vm, Value* args, int count, int argIndex,
+                         const char* argName)
+{
+  if (!validateInt(vm, args, argIndex, argName)) return -1;
+
+  int index = (int)AS_NUM(args[argIndex]);
+
+  // Negative indices count from the end.
+  if (index < 0) index = count + index;
+
+  // Check bounds.
+  if (index >= 0 && index < count) return index;
+
+  char message[100];
+  snprintf(message, 100, "%s out of bounds.", argName);
+  args[0] = wrenNewString(vm, message, strlen(message));
+
+  return -1;
+}
+
 DEF_NATIVE(bool_not)
 {
   RETURN_BOOL(!AS_BOOL(args[0]));
@@ -106,28 +180,6 @@ DEF_NATIVE(bool_toString)
 
 DEF_NATIVE(fn_call) { return PRIM_CALL; }
 
-// 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 validateIndex(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;
-}
-
 DEF_NATIVE(list_add)
 {
   ObjList* list = AS_LIST(args[0]);
@@ -156,10 +208,8 @@ DEF_NATIVE(list_insert)
   ObjList* list = AS_LIST(args[0]);
 
   // count + 1 here so you can "insert" at the very end.
-  int index = validateIndex(args[2], list->count + 1);
-  // 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, 2, "Index");
+  if (index == -1) return PRIM_ERROR;
 
   wrenListInsert(vm, list, args[1], index);
   RETURN_VAL(args[1]);
@@ -170,12 +220,13 @@ DEF_NATIVE(list_iterate)
   // If we're starting the iteration, return the first index.
   if (IS_NULL(args[1])) RETURN_NUM(0);
 
-  ObjList* list = AS_LIST(args[0]);
-  double index = AS_NUM(args[1]);
-  // TODO: Handle arg not a number or not an integer.
+  if (!validateInt(vm, args, 1, "Iterator")) return PRIM_ERROR;
 
-  // Stop if we're out of elements.
-  if (index >= list->count - 1) RETURN_FALSE;
+  ObjList* list = AS_LIST(args[0]);
+  int index = (int)AS_NUM(args[1]);
+
+  // Stop if we're out of bounds.
+  if (index < 0 || index >= list->count - 1) RETURN_FALSE;
 
   // Otherwise, move to the next index.
   RETURN_NUM(index + 1);
@@ -184,15 +235,16 @@ DEF_NATIVE(list_iterate)
 DEF_NATIVE(list_iteratorValue)
 {
   ObjList* list = AS_LIST(args[0]);
-  double index = AS_NUM(args[1]);
-  // TODO: Handle index out of bounds or not integer.
-  RETURN_VAL(list->elements[(int)index]);
+  int index = validateIndex(vm, args, list->count, 1, "Iterator");
+  if (index == -1) return PRIM_ERROR;
+
+  RETURN_VAL(list->elements[index]);
 }
 
 DEF_NATIVE(list_removeAt)
 {
   ObjList* list = AS_LIST(args[0]);
-  int index = validateIndex(args[1], list->count);
+  int index = validateIndexOld(args[1], list->count);
   // TODO: Instead of returning null here, should signal an error explicitly
   // somehow.
   if (index == -1) RETURN_NULL;
@@ -204,7 +256,7 @@ DEF_NATIVE(list_subscript)
 {
   ObjList* list = AS_LIST(args[0]);
 
-  int index = validateIndex(args[1], list->count);
+  int index = validateIndexOld(args[1], list->count);
   // TODO: Instead of returning null here, should signal an error explicitly
   // somehow.
   if (index == -1) RETURN_NULL;
@@ -216,7 +268,7 @@ DEF_NATIVE(list_subscriptSetter)
 {
   ObjList* list = AS_LIST(args[0]);
 
-  int index = validateIndex(args[1], list->count);
+  int index = validateIndexOld(args[1], list->count);
   // TODO: Instead of returning null here, should signal an error explicitly
   // somehow.
   if (index == -1) RETURN_NULL;
@@ -249,45 +301,34 @@ DEF_NATIVE(num_negate)
   RETURN_NUM(-AS_NUM(args[0]));
 }
 
-// Validates that the second argument in [args] is a Num. Returns true if it is.
-// If not, sets a type error and returns false.
-static bool checkNumType(WrenVM* vm, Value* args)
-{
-  if (IS_NUM(args[1])) return true;
-
-  const char* msg = "TypeError: Right operand must be Num.";
-  args[0] = wrenNewString(vm, msg, strlen(msg));
-  return false;
-}
-
 DEF_NATIVE(num_minus)
 {
-  if (!checkNumType(vm, args)) return PRIM_ERROR;
+  if (!validateNum(vm, args, 1, "Right operand")) return PRIM_ERROR;
   RETURN_NUM(AS_NUM(args[0]) - AS_NUM(args[1]));
 }
 
 DEF_NATIVE(num_plus)
 {
-  if (!checkNumType(vm, args)) return PRIM_ERROR;
+  if (!validateNum(vm, args, 1, "Right operand")) return PRIM_ERROR;
   // TODO: Handle coercion to string if RHS is a string.
   RETURN_NUM(AS_NUM(args[0]) + AS_NUM(args[1]));
 }
 
 DEF_NATIVE(num_multiply)
 {
-  if (!checkNumType(vm, args)) return PRIM_ERROR;
+  if (!validateNum(vm, args, 1, "Right operand")) return PRIM_ERROR;
   RETURN_NUM(AS_NUM(args[0]) * AS_NUM(args[1]));
 }
 
 DEF_NATIVE(num_divide)
 {
-  if (!checkNumType(vm, args)) return PRIM_ERROR;
+  if (!validateNum(vm, args, 1, "Right operand")) return PRIM_ERROR;
   RETURN_NUM(AS_NUM(args[0]) / AS_NUM(args[1]));
 }
 
 DEF_NATIVE(num_mod)
 {
-  if (!checkNumType(vm, args)) return PRIM_ERROR;
+  if (!validateNum(vm, args, 1, "Right operand")) return PRIM_ERROR;
   if (!IS_NUM(args[1])) RETURN_NULL;
   RETURN_NUM(fmod(AS_NUM(args[0]), AS_NUM(args[1])));
 }
diff --git a/test/list/insert.wren b/test/list/insert.wren
index 83585540..9a306a58 100644
--- a/test/list/insert.wren
+++ b/test/list/insert.wren
@@ -1,6 +1,3 @@
-// TODO: What happens if the index isn't an integer?
-// TODO: Out of bounds.
-
 // Add to empty list.
 var a = []
 a.insert(1, 0)
diff --git a/test/list/insert_index_not_int.wren b/test/list/insert_index_not_int.wren
new file mode 100644
index 00000000..f382cc4e
--- /dev/null
+++ b/test/list/insert_index_not_int.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.insert("value", 1.5) // expect runtime error: Index must be an integer.
diff --git a/test/list/insert_index_not_num.wren b/test/list/insert_index_not_num.wren
new file mode 100644
index 00000000..d046d3a2
--- /dev/null
+++ b/test/list/insert_index_not_num.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.insert("value", "2") // expect runtime error: Index must be a number.
diff --git a/test/list/insert_index_too_large.wren b/test/list/insert_index_too_large.wren
new file mode 100644
index 00000000..a193f8ad
--- /dev/null
+++ b/test/list/insert_index_too_large.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.insert("value", 4) // expect runtime error: Index out of bounds.
diff --git a/test/list/insert_index_too_small.wren b/test/list/insert_index_too_small.wren
new file mode 100644
index 00000000..433d2f4d
--- /dev/null
+++ b/test/list/insert_index_too_small.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.insert("value", -5) // expect runtime error: Index out of bounds.
diff --git a/test/list/iterate.wren b/test/list/iterate.wren
new file mode 100644
index 00000000..40cd3c93
--- /dev/null
+++ b/test/list/iterate.wren
@@ -0,0 +1,7 @@
+var a = ["one", "two", "three", "four"]
+IO.write(a.iterate(null)) // expect: 0
+IO.write(a.iterate(0)) // expect: 1
+IO.write(a.iterate(1)) // expect: 2
+IO.write(a.iterate(2)) // expect: 3
+IO.write(a.iterate(3)) // expect: false
+IO.write(a.iterate(-1)) // expect: false
diff --git a/test/list/iterate_iterator_not_int.wren b/test/list/iterate_iterator_not_int.wren
new file mode 100644
index 00000000..0fdf558d
--- /dev/null
+++ b/test/list/iterate_iterator_not_int.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.iterate(1.5) // expect runtime error: Iterator must be an integer.
diff --git a/test/list/iterate_iterator_not_num.wren b/test/list/iterate_iterator_not_num.wren
new file mode 100644
index 00000000..a93312da
--- /dev/null
+++ b/test/list/iterate_iterator_not_num.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.iterate("2") // expect runtime error: Iterator must be a number.
diff --git a/test/list/iterator_value.wren b/test/list/iterator_value.wren
new file mode 100644
index 00000000..116ce374
--- /dev/null
+++ b/test/list/iterator_value.wren
@@ -0,0 +1,5 @@
+var a = ["one", "two", "three", "four"]
+IO.write(a.iteratorValue(0)) // expect: one
+IO.write(a.iteratorValue(1)) // expect: two
+IO.write(a.iteratorValue(2)) // expect: three
+IO.write(a.iteratorValue(3)) // expect: four
diff --git a/test/list/iterator_value_iterator_not_int.wren b/test/list/iterator_value_iterator_not_int.wren
new file mode 100644
index 00000000..11383612
--- /dev/null
+++ b/test/list/iterator_value_iterator_not_int.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.iteratorValue(1.5) // expect runtime error: Iterator must be an integer.
diff --git a/test/list/iterator_value_iterator_not_num.wren b/test/list/iterator_value_iterator_not_num.wren
new file mode 100644
index 00000000..fcbef904
--- /dev/null
+++ b/test/list/iterator_value_iterator_not_num.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.iteratorValue("2") // expect runtime error: Iterator must be a number.
diff --git a/test/list/iterator_value_iterator_too_large.wren b/test/list/iterator_value_iterator_too_large.wren
new file mode 100644
index 00000000..9afe3f11
--- /dev/null
+++ b/test/list/iterator_value_iterator_too_large.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.iteratorValue(4) // expect runtime error: Iterator out of bounds.
diff --git a/test/list/iterator_value_iterator_too_small.wren b/test/list/iterator_value_iterator_too_small.wren
new file mode 100644
index 00000000..788b88c8
--- /dev/null
+++ b/test/list/iterator_value_iterator_too_small.wren
@@ -0,0 +1,2 @@
+var a = [1, 2, 3]
+a.iteratorValue(-5) // expect runtime error: Iterator out of bounds.
diff --git a/test/number/divide_operand_not_num.wren b/test/number/divide_operand_not_num.wren
new file mode 100644
index 00000000..0cb27f0e
--- /dev/null
+++ b/test/number/divide_operand_not_num.wren
@@ -0,0 +1 @@
+1 / false // expect runtime error: Right operand must be a number.
diff --git a/test/number/divide_wrong_argument_type.wren b/test/number/divide_wrong_argument_type.wren
deleted file mode 100644
index 10187f86..00000000
--- a/test/number/divide_wrong_argument_type.wren
+++ /dev/null
@@ -1 +0,0 @@
-1 / false // expect runtime error: TypeError: Right operand must be Num.
diff --git a/test/number/minus_operand_not_num.wren b/test/number/minus_operand_not_num.wren
new file mode 100644
index 00000000..9b00bd7b
--- /dev/null
+++ b/test/number/minus_operand_not_num.wren
@@ -0,0 +1 @@
+1 - false // expect runtime error: Right operand must be a number.
diff --git a/test/number/minus_wrong_argument_type.wren b/test/number/minus_wrong_argument_type.wren
deleted file mode 100644
index 0b1791d5..00000000
--- a/test/number/minus_wrong_argument_type.wren
+++ /dev/null
@@ -1 +0,0 @@
-1 - false // expect runtime error: TypeError: Right operand must be Num.
diff --git a/test/number/mod_operand_not_num.wren b/test/number/mod_operand_not_num.wren
new file mode 100644
index 00000000..3f6132cf
--- /dev/null
+++ b/test/number/mod_operand_not_num.wren
@@ -0,0 +1 @@
+1 % false // expect runtime error: Right operand must be a number.
diff --git a/test/number/mod_wrong_argument_type.wren b/test/number/mod_wrong_argument_type.wren
deleted file mode 100644
index bc2dd5df..00000000
--- a/test/number/mod_wrong_argument_type.wren
+++ /dev/null
@@ -1 +0,0 @@
-1 % false // expect runtime error: TypeError: Right operand must be Num.
diff --git a/test/number/multiply_operand_not_num.wren b/test/number/multiply_operand_not_num.wren
new file mode 100644
index 00000000..d5b3db43
--- /dev/null
+++ b/test/number/multiply_operand_not_num.wren
@@ -0,0 +1 @@
+1 * false // expect runtime error: Right operand must be a number.
diff --git a/test/number/multiply_wrong_argument_type.wren b/test/number/multiply_wrong_argument_type.wren
deleted file mode 100644
index 423a3ffe..00000000
--- a/test/number/multiply_wrong_argument_type.wren
+++ /dev/null
@@ -1 +0,0 @@
-1 * false // expect runtime error: TypeError: Right operand must be Num.
diff --git a/test/number/plus_operand_not_num.wren b/test/number/plus_operand_not_num.wren
new file mode 100644
index 00000000..dfb05a4d
--- /dev/null
+++ b/test/number/plus_operand_not_num.wren
@@ -0,0 +1,3 @@
+1 + false // expect runtime error: Right operand must be a number.
+
+// TODO: What about a string on the RHS?
diff --git a/test/number/plus_wrong_argument_type.wren b/test/number/plus_wrong_argument_type.wren
deleted file mode 100644
index d44e09b0..00000000
--- a/test/number/plus_wrong_argument_type.wren
+++ /dev/null
@@ -1,3 +0,0 @@
-1 + false // expect runtime error: TypeError: Right operand must be Num.
-
-// TODO: What about a string on the RHS?