fix #17383: json.%,to and jsonutils.formJson,toJson now works with uint|uint64

[timotheecour]

fix #17383

[arnetheduck]

this should solve the most critical issue, though it's worth noting that it breaks backwards compatibility with earlier nim versions.

an aside:

• why is the isUnquoted flag not inside the JString case?
• why is it not public? this is significant information when parsing some json files.

in general, json is hard to use because it's an odd juxtaposition of the json standard and nim - in json, there are no integers, only numbers which makes JInt vs JFloat distinction unnatural to work with, from a json perspective, and this patch throws JString into the mixture further exacerbating the issue.

also, the fix to this issue should ideally be backported to 1.2.x since the issue is present in 1.2.10 also - in some projects, this can turn into a security bug leading to remote exploits (since json data often comes from untrusted sources).

[timotheecour]

though it's worth noting that it breaks backwards compatibility with earlier nim versions.

IMO it's fine in this case:

• 1.4 was raising, now it's giving correct results
• 1.2 and 1.0 were giving incorrect result (eg: $%(uint.high) was
  -1), now it's giving correct results (18446744073709551615); code that relies on bugs will eventually break

why is the isUnquoted flag not inside the JString case?

this can be changed in future work, if needed; note that sizeof(JsonNodeObj) remains 40 with or without isUnquoted on 64bit (haven't checked on 32bit). The biggest offender is fields*: OrderedTable[string, JsonNode] which makes it 40 instead of 16 (even with isUnquoted as non-variant field). There should be a better way (out of scope for this PR though), even without packedjson

why is it not public? this is significant information when parsing some json files.

not making it public means we can change implementation, and in fact #15776 might very well be a more efficient implementation, see #15768 (comment)

we should however add an API proc jsonKind(self: JsonNode): JsonKind, such that:

  import std/json
  echo uint.high
  let j = parseJson("184467440737095516159999999")
  echo j.kind # JString
  echo j.jsonKind # Number

in general, json is hard to use because it's an odd juxtaposition of the json standard and nim - in json, there are no integers, only numbers which makes JInt vs JFloat distinction unnatural to work with, from a json perspective, and this patch throws JString into the mixture further exacerbating the issue.

see above, jsonKind would solve this.

=> see RFC nim-lang/RFCs#353

also, the fix to this issue should ideally be backported to 1.2.x since the issue is present in 1.2.10 also - in some projects, this can turn into a security bug leading to remote exploits (since json data often comes from untrusted sources).

this opens a can of worms, because it'd also require backporting #15768; people should upgrade eventually or be ok living with some bugs

[arnetheduck]

IMO it's fine in this case:

we have users depending on this particular behavior and need to notify them in turn - providing a changelog and documenting the breaking behavior is just common sense, regardless if you consider the changed/original behavior buggy or not - in the end it breaks actual software out there for actual users.

[arnetheduck]

upgrade eventually

this would be a lot easier if the json module, and similar large modules that have no relationship to the language and compiler, were published outside the standard library - as is, upgrading nim, even between point versions, brings in a host of undesired changes that break backwards compatibility.

[timotheecour]

we have users depending on this particular behavior
providing a changelog and documenting the breaking behavior

adding changelog in #17400
note that the real breaking change was in 1.2=>1.4 because 1.2 gave -1 and 1.4 gave a Defect

this would be a lot easier if the json module, and similar large modules that have no relationship to the language and compiler, were published outside the standard library - as is, upgrading nim, even between point versions, brings in a host of undesired changes that break backwards compatibility.

whether or not this would be desirable, this is not as simple as what you describe.

• Both the compiler and stdlib depend on std/json and other such modules, in several places. How would you handle that, including bootstrap and circular dependencies between compiler, stdlib and nimble?
• this would have similar issues as fusion (see Fusion and stdlib evolution RFCs#310)

This means even though Fusion is supposed to be a staging area for the stdlib, contributing to Nim's stdlib is easier than contributing to Fusion. Clearly against Fusion's design

Also, you've made the same comment recently, see my previous answer here: status-im/nim-libbacktrace#12 (comment) which has more details on this (dmd vs phobos split causing friction, monorepo benefits etc).

Instead of scattering the discussion in review comments, please create a dedicated issue / RFC to discuss what you have in mind.

[arnetheduck]

So, to provide some background - the json module was broken by #15237 which introduces a backwards-incompatible language change - introducing such patches generally requires reviewing the standard library for code that breaks, such as json - it's quite certain that there are other modules in Nim broken by this.

The fix was later backported to 1.2 - we're now trying to upgrade to 1.2.10, and are running into these bugs - they are significant from our point of view because they introduce remote exploits and require manually reviewing all conversions across the codebase - this is why, at minimum, a changelog entry should be natural to have - changes like these have implications for users of the language - I appreciate #17400!

Regarding modules, I'm pointing it out in this issue that having modules standalone is easier for downstream users like us because it's such a practical and obvious case where having things in the standard library makes things hard - it can serve as background information in a future RFC - if json is used in the compiler, the compiler would simply depend on a json package that it pulls in as part of its build - or keep its own private copy for things it needs - this would also solve issues around bootstrapping where the standard library and compiler-used-to-compile-itself - same issue as csources in general.

[timotheecour]

the json module was broken by #15237 which introduces a backwards-incompatible language change

it's often impossible to fix bugs without introducing some kind of regression, when users depend on prior behavior (which is why the 1.0 stability promise should be IMO taken with a grain of salt, as enforcing stability would prevent much needed bug fixes and improving the language); the patch vs minor distinction is useful as a general guideline but comes with this disclaimer.

But yes, #15237 could've come with a changelog entry and could still be added retroactively (past changelogs are now version controlled)

if json is used in the compiler, the compiler would simply depend on a json package that it pulls in as part of its build - or keep its own private copy for things it needs

related: please read this thread nim-lang/fusion#22

[narimiran]

I'm having a very hard time to backport this to 1.2.x because it relies on stuff introduced later (e.g., but not limited to, newJRawNumber, introduced in #15768).

@timotheecour Could you maybe make a variant of this PR catered specifically for 1.2.x, using stuff only available there, i.e. based on version-1-2 branch?

[timotheecour]

I don't think this should be backported, see note above: #17389 (comment)

[arnetheduck]

I don't think this should be backported

the reasonable thing for a backport would be to keep returning -1 probably (using cast[..]) - ie that's the observable behavior that 1.2 has, which probably shouldn't be changed in a point release.,

[narimiran]

So, to provide some background - the json module was broken by #15237 which introduces a backwards-incompatible language change

@arnetheduck How about we revert #15237 in the 1.2.x branch, so you get the old behaviour back in 1.2.12?

[arnetheduck]

@narimiran sgtm, it's a prudent solution given that other places in the standard library might have changed behavior as a side effect of that change - we're in the process of auditing our code base to deal with the new Defect but it's quite difficult since aliases, borrows and distinct types are affected as well which makes a trivial "search-and-replace" hard - the same applies to the standard library in general.