Belt.Int.fromString silently throws away part of input

[RasmusKlett]

I encountered some very surprising behaviour using Belt.Int.fromString. If given the string representation of a floating point number, it will silently ignore anything beyond the decimal point.

Looking at the source, it turns out fromString is simply wrapping the Javascript parseInt. This is a very quirky Javascript-esque function, and in my opinion does not deserve a place in Belt.

Examples of surprising behaviour:

"15.9" -> 15      // The ".9" is ignored
"15e99" -> 15     // The "e99" is ignored
"   15" -> 15     // The whitespace is ignored - maybe fine?

I suggest that at least the first two should result in None, since the full string could not be parsed. I find it convenient that leading whitespace is automatically stripped, but some might prefer strict None behaviour in this case as well.

Please - let us not repeat the mistakes of Javascript just because it is convenient to implement.

[bobzhang]

@RasmusKlett looks reasonable, do you have a clean alternative in mind?

[RasmusKlett]

I've tried a few versions, but haven't been able to maintain the performance of the builtin parseInt. I'm not sure how much we care about performance, but maybe someone smarter than me can help?

I've tried simply wrapping the OCaml int_of_string, this has a 3x performance penalty on my machine (Excuse the reason syntax):

let intFromString = str =>
  switch (int_of_string(str)) {
  | number => Some(number)
  | exception (Failure(_msg)) => None
};

Note this does not allow leading whitespace - I'm not sure what the correct behavour here is?

MDN suggests simply doing a regex validation. This performs slightly better than int_of_string, and can easily be made to allow leading whitespace.

function filterInt(value) {
  if (/^[-+]?(\d+)$/.test(value)) {
    return Number(value);
  } else {
    return undefined;
  }
}

I've also tried implementing it in terms of an explicit for-loop over code points. This performs terribly for some reason. If this was native code, I would expect this to perform best, since it skips a lot of checks not necessary with static types, but that was definitely not the case.

function codePointInt(value) {
  var result = 0;
  var length = value.length;
  for (var i=0; i < length; i++) {
    var code = value.codePointAt(i);
    result += (code - 48) * 10**(length - 1 - i);
  }
  return result;
}

So there are two questions here: Do we allow leading whitespace? And can we live with a 2-3x performance hit? If not, I think we need someone smarter than me to do some tuning.

Here's the stuff I've benched, if anyone wants to try.

[bobzhang]

I don't think performance is critical here, but not introducing additional deps is quite important.
Would you mind send a PR?

[RasmusKlett]

It will take me some time, I haven't tried committing to the project before. I will give it a shot next week.

Unless someone disagrees, I will go ahead and use the Mozilla-suggested version of validating the input using a regex. This has the lowest performance hit, and it can allow leading whitespace.

[bobzhang]

@RasmusKlett There is one thing annoying that in latest js, 3_23 is legal, how shall we handle it properly

[TheSpyder]

I agree the default should be more reliable than parseInt - and performance probably doesn't matter - but I'd like parseInt to still remain somewhere. In some circumstances I want the quirky behaviour.

Perhaps parseFloat + Math.round would be a better basis for Int.fromString?

[amiralies]

another case happening to me:

let x = Int.fromString("22 foo bar") // 22

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[glennsl]

See discussion about this issue in rescript-lang/rescript-core#86 and proposed solution in rescript-lang/rescript-core#93

Probably better to fix this in rescript-core now than in the standard library.