From 058c2606b02a41c8a1a05e99b5b7faa6dad13ded Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Fri, 3 Jan 2014 10:47:26 -0800 Subject: [PATCH] Better argument validation for core methods. --- runtests | 4 +- src/wren_core.c | 75 ++++++------------- test/list/remove_at.wren | 5 -- test/list/remove_at_index_not_int.wren | 2 + test/list/remove_at_index_not_num.wren | 2 + test/list/remove_at_index_too_large.wren | 2 + test/list/remove_at_index_too_small.wren | 2 + test/list/subscript.wren | 13 ---- test/list/subscript_not_int.wren | 2 + test/list/subscript_not_num.wren | 2 + test/list/subscript_setter_not_int.wren | 2 + test/list/subscript_setter_not_num.wren | 2 + test/list/subscript_setter_too_large.wren | 2 + test/list/subscript_setter_too_small.wren | 2 + test/list/subscript_too_large.wren | 2 + test/list/subscript_too_small.wren | 2 + test/string/contains.wren | 2 - test/string/contains_argument_not_string.wren | 1 + test/string/subscript.wren | 13 ---- test/string/subscript_not_int.wren | 2 + test/string/subscript_not_num.wren | 2 + test/string/subscript_too_large.wren | 2 + test/string/subscript_too_small.wren | 2 + 23 files changed, 58 insertions(+), 87 deletions(-) create mode 100644 test/list/remove_at_index_not_int.wren create mode 100644 test/list/remove_at_index_not_num.wren create mode 100644 test/list/remove_at_index_too_large.wren create mode 100644 test/list/remove_at_index_too_small.wren create mode 100644 test/list/subscript_not_int.wren create mode 100644 test/list/subscript_not_num.wren create mode 100644 test/list/subscript_setter_not_int.wren create mode 100644 test/list/subscript_setter_not_num.wren create mode 100644 test/list/subscript_setter_too_large.wren create mode 100644 test/list/subscript_setter_too_small.wren create mode 100644 test/list/subscript_too_large.wren create mode 100644 test/list/subscript_too_small.wren create mode 100644 test/string/contains_argument_not_string.wren create mode 100644 test/string/subscript_not_int.wren create mode 100644 test/string/subscript_not_num.wren create mode 100644 test/string/subscript_too_large.wren create mode 100644 test/string/subscript_too_small.wren diff --git a/runtests b/runtests index 6e2e970a..d5310e62 100755 --- a/runtests +++ b/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: diff --git a/src/wren_core.c b/src/wren_core.c index 73f046c5..e30bd378 100644 --- a/src/wren_core.c +++ b/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. diff --git a/test/list/remove_at.wren b/test/list/remove_at.wren index d3624cb8..ff1eed51 100644 --- a/test/list/remove_at.wren +++ b/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 diff --git a/test/list/remove_at_index_not_int.wren b/test/list/remove_at_index_not_int.wren new file mode 100644 index 00000000..72bbe588 --- /dev/null +++ b/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. diff --git a/test/list/remove_at_index_not_num.wren b/test/list/remove_at_index_not_num.wren new file mode 100644 index 00000000..af16e4dd --- /dev/null +++ b/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. diff --git a/test/list/remove_at_index_too_large.wren b/test/list/remove_at_index_too_large.wren new file mode 100644 index 00000000..2f3e64aa --- /dev/null +++ b/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. diff --git a/test/list/remove_at_index_too_small.wren b/test/list/remove_at_index_too_small.wren new file mode 100644 index 00000000..57e1c6aa --- /dev/null +++ b/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. diff --git a/test/list/subscript.wren b/test/list/subscript.wren index d66852ed..9b541575 100644 --- a/test/list/subscript.wren +++ b/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 diff --git a/test/list/subscript_not_int.wren b/test/list/subscript_not_int.wren new file mode 100644 index 00000000..204022d4 --- /dev/null +++ b/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. diff --git a/test/list/subscript_not_num.wren b/test/list/subscript_not_num.wren new file mode 100644 index 00000000..3784239c --- /dev/null +++ b/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. diff --git a/test/list/subscript_setter_not_int.wren b/test/list/subscript_setter_not_int.wren new file mode 100644 index 00000000..06de558e --- /dev/null +++ b/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. diff --git a/test/list/subscript_setter_not_num.wren b/test/list/subscript_setter_not_num.wren new file mode 100644 index 00000000..313f54a3 --- /dev/null +++ b/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. diff --git a/test/list/subscript_setter_too_large.wren b/test/list/subscript_setter_too_large.wren new file mode 100644 index 00000000..1518fb86 --- /dev/null +++ b/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. diff --git a/test/list/subscript_setter_too_small.wren b/test/list/subscript_setter_too_small.wren new file mode 100644 index 00000000..51870da8 --- /dev/null +++ b/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. diff --git a/test/list/subscript_too_large.wren b/test/list/subscript_too_large.wren new file mode 100644 index 00000000..869cb73a --- /dev/null +++ b/test/list/subscript_too_large.wren @@ -0,0 +1,2 @@ +var a = [1, 2, 3] +a[4] // expect runtime error: Subscript out of bounds. diff --git a/test/list/subscript_too_small.wren b/test/list/subscript_too_small.wren new file mode 100644 index 00000000..e5b92535 --- /dev/null +++ b/test/list/subscript_too_small.wren @@ -0,0 +1,2 @@ +var a = [1, 2, 3] +a[-5] // expect runtime error: Subscript out of bounds. diff --git a/test/string/contains.wren b/test/string/contains.wren index 3863649a..cc813808 100644 --- a/test/string/contains.wren +++ b/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. diff --git a/test/string/contains_argument_not_string.wren b/test/string/contains_argument_not_string.wren new file mode 100644 index 00000000..ba6817bc --- /dev/null +++ b/test/string/contains_argument_not_string.wren @@ -0,0 +1 @@ +"foo".contains(1) // expect runtime error: Argument must be a string. diff --git a/test/string/subscript.wren b/test/string/subscript.wren index 5034b91d..c1cba0d4 100644 --- a/test/string/subscript.wren +++ b/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 diff --git a/test/string/subscript_not_int.wren b/test/string/subscript_not_int.wren new file mode 100644 index 00000000..3fecb234 --- /dev/null +++ b/test/string/subscript_not_int.wren @@ -0,0 +1,2 @@ +var a = "123" +a[1.5] // expect runtime error: Subscript must be an integer. diff --git a/test/string/subscript_not_num.wren b/test/string/subscript_not_num.wren new file mode 100644 index 00000000..3e04ac50 --- /dev/null +++ b/test/string/subscript_not_num.wren @@ -0,0 +1,2 @@ +var a = "123" +a["2"] // expect runtime error: Subscript must be a number. diff --git a/test/string/subscript_too_large.wren b/test/string/subscript_too_large.wren new file mode 100644 index 00000000..3c674b38 --- /dev/null +++ b/test/string/subscript_too_large.wren @@ -0,0 +1,2 @@ +var a = "123" +a[4] // expect runtime error: Subscript out of bounds. diff --git a/test/string/subscript_too_small.wren b/test/string/subscript_too_small.wren new file mode 100644 index 00000000..c841c926 --- /dev/null +++ b/test/string/subscript_too_small.wren @@ -0,0 +1,2 @@ +var a = "123" +a[-5] // expect runtime error: Subscript out of bounds.