Fix whitespace in number parsing

[thaliaarchi]

This is two separate number parsing bug fixes.

As of the unreleased changes on master since 1.7.1, the tonumber filter has been changed to no longer strip whitespace. Assuming we want to commit to that change, this PR makes some related bug fixes. Otherwise, if we want to keep the old tonumber behavior, similar changes should be made in the other direction.

tonumber inconsistently skips leading whitespace

In commit ce0e788 (improve tonumber/0 performance by parsing input as number literal, 2024-03-02), tonumber was changed to directly parse as a number, instead of generically as a jv, and to reject leading or trailing whitespace.

Problem is, the two number backends are not consistent on their whitespace handling: jvp_strtod skips leading whitespace, but its decnum counterpart decNumberFromString (called within jv_number_with_literal) does not. Those two are called interchangeably, so it leads to inconsistent behavior depending on whether the decnum feature is enabled. (Additionally, classify, used in the token scanner, only considers [ \t\n\r] to be whitespace, but jvp_strtod consumes the larger set [ \t\n\v\f\r], so those extra characters are considered literals.)

Change jvp_strtod to not skip leading whitespace. This case was missed, since the new test was for " 4 " | tonumber, which hid that only one side would be stripped.

If we consider strtod to be a public symbol, this is deviating from the behavior of strdod in <stdlib.h> and is technically a breaking API change.

The four cases, before this PR:

$ ./configure
$ make -j8
$ ./jq -n '" 123" | tonumber'
jq: error (at <unknown>): string (" 123") cannot be parsed as a number
$ ./jq -n '"123 " | tonumber'
jq: error (at <unknown>): string ("123 ") cannot be parsed as a number

$ make clean
$ ./configure --disable-decnum
$ make -j8
$ ./jq -n '" 123" | tonumber'
123
$ ./jq -n '"123 " | tonumber'
jq: error (at <unknown>): string ("123 ") cannot be parsed as a number

--indent does not validate its input

Handle malformed and overflowing arguments. Also, forbid leading and trailing spaces to match the behavior of tonumber.

[emanuele6]

The patch is fine, but I have missed why we want to make tonumber/0, in the next version of jq, no longer ignore trailing and leading whitespace instead of just making skip whitespace before and after also when using decNum to not change the behaviour.

[emanuele6]

Because that is what gojq also does, I guess

[pkoppstein]

emanuele6 wrote:

I have missed why we want to make tonumber/0, in the next version of jq, no longer ignore trailing and leading whitespace instead of just making skip whitespace before

This is a breaking change, and in my view quite a serious one.

Since there is no underlying bug at issue (in particular, the JSON spec for numbers does not make it so), I think the existing behavior is more than acceptable, and indeed quite conventional, e.g. in DuckDB
' 12 '::INT => 12, and I'm sure there are countless others like it.

[thaliaarchi]

If the consensus is that whitespace should still be stripped, maintaining the released and earlier behavior, I'll rework this PR in the opposite direction. I personally don't see a good reason for the breaking change to forbid leading and trailing whitespace, so am in favor of this.

[wader]

Trimming leading/trailing whitespace is fine with me 👍

[pkoppstein]

@thaliaarchi wrote:

I personally don't see a good reason for the breaking change to forbid leading and trailing whitespace, so am in favor of this.

Thanks for your understanding.

It looks like there might not be absolute consensus regarding the change to tonumber/0 in the immediate future, so I hope you'll understand that in such cases where there is no compelling reason for a breaking change, the norm would be to defer it to the next "major" release, which in this case I believe would be 2.0.

If the performance difference between the two versions of tonumber/0 is judged to be so significant, then my suggestion would be to leave tonumber() alone and introduce a new filter, say tonumber/1, that would support the optimization. (E.g. tonumber(null) could disallow all extraneous characters, and tonumber(regex) could allow stripping of characters as determined by regex.)

[emanuele6]

I have noticed two problems here:

1. end == NULL doesn't make sense, it seems entirely unnecessary
2. I think there should also be end == argv[i+1] as error condition otherwise --indent '' is accepted and treated as equivalent of --indent 0. Did you do this intentionally?

[thaliaarchi]

Thanks. I've made those fixes.

[thaliaarchi]

I've tidied up the PR. If we want to commit to to tonumber stripping whitespace, this is ready.

If we want to revert that change, we'd need a different patch. Last I checked, we'd need to modify decnum, if we take that approach, and my progress stalled there.

[itchyny]

Thanks! Let's keep the tonumber/0 strictness, and mention to use trim/0 in the changelog.