fix handling of odd strings by test builtin

The `test` builtin currently has unexpected behavior with respect to
expressions such as `'' -eq 0`. That currently evaluates to true with a
return status of zero. This change addresses that oddity while also
ensuring that other unusual strings (e.g., numbers with leading and
trailing whitespace) are handled consistently.

Fixes #3346

src/builtin_test.cpp

@@ -639,10 +639,8 @@ bool parenthetical_expression::evaluate(wcstring_list_t &errors) {
 // example, should we interpret 0x10 as 0, 10, or 16? Here we use only base 10 and use wcstoll,
 // which allows for leading + and -, and leading whitespace. This matches bash.
 static bool parse_number(const wcstring &arg, long long *out) {
-    const wchar_t *str = arg.c_str();
-    wchar_t *endptr = NULL;
-    *out = wcstoll(str, &endptr, 10);
-    return endptr && *endptr == L'\0';
+    *out = fish_wcstoll(arg.c_str());
+    return !errno;
 }
 
 static bool binary_primary_evaluate(test_expressions::token_t token, const wcstring &left,

src/fish_tests.cpp

@@ -1764,15 +1764,18 @@ static bool run_one_test_test(int expected, wcstring_list_t &lst, bool bracket)
 
 static bool run_test_test(int expected, const wcstring &str) {
     using namespace std;
-    wcstring_list_t lst;
+    wcstring_list_t argv;
+    completion_list_t comps;
 
-    wistringstream iss(str);
-    copy(istream_iterator<wcstring, wchar_t, std::char_traits<wchar_t> >(iss),
-         istream_iterator<wstring, wchar_t, std::char_traits<wchar_t> >(),
-         back_inserter<vector<wcstring> >(lst));
+    // We need to tokenize the string in the same manner a normal shell would do. This is because we
+    // need to test things like quoted strings that have leading and trailing whitespace.
+    parser_t::expand_argument_list(str, 0, &comps);
+    for (completion_list_t::const_iterator it = comps.begin(), end = comps.end(); it != end; ++it) {
+        argv.push_back(it->completion);
+    }
 
-    bool bracket = run_one_test_test(expected, lst, true);
-    bool nonbracket = run_one_test_test(expected, lst, false);
+    bool bracket = run_one_test_test(expected, argv, true);
+    bool nonbracket = run_one_test_test(expected, argv, false);
     do_test(bracket == nonbracket);
     return nonbracket;
 }
@@ -1801,6 +1804,17 @@ static void test_test() {
     do_test(run_test_test(0, L"0 -eq 0"));
     do_test(run_test_test(0, L"-1 -eq -1"));
     do_test(run_test_test(0, L"1 -ne -1"));
+    do_test(run_test_test(1, L"' 2 ' -ne 2"));
+    do_test(run_test_test(0, L"' 2' -eq 2"));
+    do_test(run_test_test(0, L"'2 ' -eq 2"));
+    do_test(run_test_test(0, L"' 2 ' -eq 2"));
+    do_test(run_test_test(1, L"' 2x' -eq 2"));
+    do_test(run_test_test(1, L"'' -eq 0"));
+    do_test(run_test_test(1, L"'' -ne 0"));
+    do_test(run_test_test(1, L"'  ' -eq 0"));
+    do_test(run_test_test(1, L"'  ' -ne 0"));
+    do_test(run_test_test(1, L"'x' -eq 0"));
+    do_test(run_test_test(1, L"'x' -ne 0"));
     do_test(run_test_test(1, L"-1 -ne -1"));
     do_test(run_test_test(0, L"abc != def"));
     do_test(run_test_test(1, L"abc = def"));

src/wutil.cpp

@@ -594,6 +594,37 @@ long fish_wcstol(const wchar_t *str, const wchar_t **endptr, int base) {
     return result;
 }
 
+/// An enhanced version of wcstoll().
+///
+/// This is needed because BSD and GNU implementations differ in several ways that make it really
+/// annoying to use them in a portable fashion.
+///
+/// The caller doesn't have to zero errno. Sets errno to -1 if the int ends with something other
+/// than a digit. Leading whitespace is ignored (per the base wcstoll implementation). Trailing
+/// whitespace is also ignored.
+long long fish_wcstoll(const wchar_t *str, const wchar_t **endptr, int base) {
+    while (iswspace(*str)) ++str;  // skip leading whitespace
+    if (!*str) {  // this is because some implementations don't handle this sensibly
+        errno = EINVAL;
+        if (endptr) *endptr = str;
+        return 0;
+    }
+
+    errno = 0;
+    wchar_t *_endptr;
+    long long result = wcstoll(str, &_endptr, base);
+    while (iswspace(*_endptr)) ++_endptr;  // skip trailing whitespace
+    if (!errno && *_endptr) {
+        if (_endptr == str) {
+            errno = EINVAL;
+        } else {
+            errno = -1;
+        }
+    }
+    if (endptr) *endptr = _endptr;
+    return result;
+}
+
 file_id_t file_id_t::file_id_from_stat(const struct stat *buf) {
     assert(buf != NULL);
 

src/wutil.h

@@ -119,6 +119,7 @@ int fish_wcswidth(const wcstring &str);
 
 int fish_wcstoi(const wchar_t *str, const wchar_t **endptr = NULL, int base = 10);
 long fish_wcstol(const wchar_t *str, const wchar_t **endptr = NULL, int base = 10);
+long long fish_wcstoll(const wchar_t *str, const wchar_t **endptr = NULL, int base = 10);
 
 /// Class for representing a file's inode. We use this to detect and avoid symlink loops, among
 /// other things. While an inode / dev pair is sufficient to distinguish co-existing files, Linux