Num.fromString(" ") returns 0

[Jackie365]

Hi, I'm using wren v0.4.0 and I find

//whitespace
Num.fromString(" ") == 0

Should it be null ?

[minirop]

there probably should be a check to see if end == string->value (meaning nothing has been consumed) before skipping the whitespaces.

wren/src/vm/wren_core.c

Lines 619 to 622 in a4ae905

	double number = strtod(string->value, &end);
	// Skip past any trailing whitespace.
	while (*end != '\0' && isspace((unsigned char)*end)) end++;

[mhermier]

I don't know why the string is striped here, but it doesn't seems the sensible way to do. Instead, a strip method should be provided instead and let the user decide to strip or not the string before parsing to number.

[ruby0x1]

Trim is user space already fwiw

[PureFox48]

Considering how often Num.fromString is used, it's surprising this bug hasn't been noticed before. Perhaps people had thought that, since strtod returns 0 for invalid numeric strings, this was the expected behavior.

Anyway, on the face of it, it seems (as @minirop said) that all we need to do is to check whether any part of the string has been consumed by strtod and return NULL if it hasn't.

If we do that, there should be no need to treat an empty string as a special case or to skip past any trailing whitespace.

So that would give us:

DEF_PRIMITIVE(num_fromString)
{
  if (!validateString(vm, args[1], "Argument")) return false;

  ObjString* string = AS_STRING(args[1]);

  errno = 0;
  char* end;
  double number = strtod(string->value, &end);

  if (errno == ERANGE) RETURN_ERROR("Number literal is too large.");

  // Check we have consumed any part of the string. Otherwise, it contains non-number
  // characters and we can't parse it.
  if (end == string->value) RETURN_NULL;

  RETURN_NUM(number);
}

[CrazyInfin8]

Some time ago, I tried removing our dependency on the standard strtod and strtoll functions with #984.

wren/src/vm/wren_utils.c

Lines 232 to 417 in 3f5a16a

	bool wrenParseNum(const char* str, int base, wrenParseNumResults* results)
	{
	int i = 0, maxMant, mantDigits = 0, e = 0;
	bool hasDigits = false, neg = false;
	long long num = 0;
	char c = str[i];
	// Check sign.
	if (c == '-')
	{
	neg = true;
	c = str[++i];
	}
	else if (c == '+') c = str[++i];
	if (base == 0)
	{
	// Base unset. Check for prefix or default to base 10.
	base = 10;
	maxMant = maxMantissaDigits[10 - 2];
	if (c == '0')
	{
	switch (c = str[++i])
	{
	case 'x':
	base = 16;
	c = str[++i];
	maxMant = maxMantissaDigits[16 - 2];
	break;
	case 'o':
	base = 8;
	c = str[++i];
	maxMant = maxMantissaDigits[8 - 2];
	break;
	case 'b':
	base = 2;
	c = str[++i];
	maxMant = maxMantissaDigits[2 - 2];
	break;
	default:
	// this number is base 10 so treat the '0' as a digit.
	hasDigits = true;
	}
	}
	}
	else
	{
	// If base is set, make sure that it is valid.
	if (base > 36) return wrenParseNumError(i, "Base is to high.", results);
	else if (base < 2) return wrenParseNumError(i, "Base is to low.", results);
	// if the base may have a prefix, skip past the prefix.
	if (c == '0')
	{
	c = str[++i];
	switch (base)
	{
	case 16:
	if (c == 'x') c = str[++i];
	break;
	case 8:
	if (c == 'o') c = str[++i];
	break;
	case 2:
	if (c == 'b') c = str[++i];
	break;
	default:
	hasDigits = true;
	}
	}
	maxMant = maxMantissaDigits[base - 2];
	}
	// Parse the integer part of the number.
	for (;;)
	{
	int t;
	if (isdigit(c)) t = c - '0';
	else if (islower(c)) t = c - ('a' - 10);
	else if (isupper(c)) t = c - ('A' - 10);
	else if (c == '_')
	{
	c = str[++i];
	continue;
	}
	else break;
	if (t >= base) break;
	hasDigits = true;
	if (mantDigits < maxMant)
	{
	num = num * base + t;
	if (num > 0) mantDigits++;
	}
	else if (e < INT_MAX) e++;
	else return wrenParseNumError(i, "Too many digits.", results);
	c = str[++i];
	}
	// Only parse the fractional and the exponential part of the number if the
	// base is 10.
	if (base == 10)
	{
	// Parse the decimal part of the number. The decimal point must be followed
	// by a digit or else the decimal point may actually be a fuction call.
	if (c == '.' && isdigit(str[i + 1]))
	{
	c = str[++i];
	for (;;)
	{
	if (isdigit(c))
	{
	if (mantDigits < maxMant)
	{
	num = num * 10 + (c - '0');
	hasDigits = true;
	if (num > 0) mantDigits++;
	if (e > INT_MIN) e--;
	else return wrenParseNumError(i, "Too many digits.", results);
	}
	}
	else if (c != '_')
	break;
	c = str[++i];
	}
	}
	// We must have parsed digits from here or else this number is invalid
	if (!hasDigits) return wrenParseNumError(i, "Number has no digits.", results);
	// Parse the exponential part of the number.
	if (c == 'e' || c == 'E')
	{
	c = str[++i];
	int expNum = 0;
	bool expHasDigits = false, expNeg = false;
	if (c == '-')
	{
	expNeg = true;
	c = str[++i];
	}
	else if (c == '+') c = str[++i];
	for (;;)
	{
	if (isdigit(c))
	{
	int t = c - '0';
	if (expNum < INT_MAX / 10 ||
	(expNum == INT_MAX / 10 && t <= INT_MAX % 10))
	{
	expNum = expNum * 10 + t;
	}
	expHasDigits = true;
	}
	else if (c != '_') break;
	c = str[++i];
	}
	if (!expHasDigits)
	return wrenParseNumError(i, "Unterminated scientific literal.",
	results);
	// Before changing "e", ensure that it will not overflow.
	if (expNeg)
	{
	if (e >= INT_MIN + expNum) e -= expNum;
	else return wrenParseNumError(i, "Exponent is too small.", results);
	}
	else
	{
	if (e <= INT_MAX - expNum) e += expNum;
	else return wrenParseNumError(i, "Exponent is too large.", results);
	}
	}
	} else {
	if (!hasDigits) return wrenParseNumError(i, "Number has no digits.", results);
	}
	// Floating point math is often inaccurate. To get around this issue, we
	// calculate using long doubles to preserve as much data as possible.
	double f =
	(double)((long double)(num)*powl((long double)(base), (long double)(e)));
	// Check whether the number became infinity or 0 from being too big or too
	// small.
	if (isinf(f)) return wrenParseNumError(i, "Number is too large.", results);
	else if (f == 0 && num != 0) return wrenParseNumError(i, "Number is too small.", results);
	results->consumed = i;
	results->value.dbl = neg ? f * -1 : f;
	return true;
	}

It certainly is a very extreme solution to this specific problem but thought it could be more intuitive this way and get rid of a few weird quirks of strtoXX.

With wrenParseNum we have easier access to different error messages than strtoXX, for example: if there were no digits when parsing. fromString currently discards this information but I find it could still be helpful for the compiler or if someone else wanted to use wrenParseNum in their host C code.

---

I think the biggest issue of #984 is that the results of wrenParseNum may be less accurate to strtoXX as the double's exponents gets too high or too low.

[PureFox48]

Yes, I remember that PR.

If we introduce octal (0oxxx) and binary (0bxxx) literals, which I think we should, then strtod will always return 0.0 when passed those literals in string form even though it does currently deal properly with hexadecimal strings. For consistency, we'd therefore need something like your solution to deal with all three.