% operator creates invalid json tree that crashes in printing.

[treeform]

import json

type
  Foo = ref object
    name: string
    params: JsonNode

var foo = Foo()
echo pretty %foo

nim c

Traceback (most recent call last)
/p/andrelytics/tmp/bug2.nim(10) bug2
/Users/andre.vonhouck/.choosenim/toolchains/nim-#devel/lib/pure/json.nim(716) pretty
/Users/andre.vonhouck/.choosenim/toolchains/nim-#devel/lib/pure/json.nim(655) toPretty
/Users/andre.vonhouck/.choosenim/toolchains/nim-#devel/lib/pure/json.nim(639) toPretty
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

nim js

/p/andrelytics/tmp/bug2.js:994
                switch (node_237621.kind) {
                                   ^

TypeError: Cannot read property 'kind' of null
    at to_pretty_237617 (/p/andrelytics/tmp/bug2.js:994:22)
    at to_pretty_237617 (/p/andrelytics/tmp/bug2.js:1053:8)
    at pretty_238040 (/p/andrelytics/tmp/bug2.js:1214:3)
    at Object.<anonymous> (/p/andrelytics/tmp/bug2.js:1724:9)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
Error: execution of an external program failed: '/usr/local/Cellar/node/8.0.0_1/bin/node /p/andrelytics/tmp/bug2.js '

I expected to either:

• Complain during compilation that converting JsonNode to JSON is not possible. Or:
• Just convert it to regular JSON.

Nim Compiler Version 0.20.99 [MacOSX: amd64]
Compiled at 2019-07-17
Copyright (c) 2006-2019 by Andreas Rumpf

active boot switches: -d:release

[Vindaar]

As far as I can see, there is no real bug here. At least not in the sense of the issues' title. All that's happening is that we get a SIGSEGV when trying to convert a JsonNode to a string that's nil.

Either if we do not echo the created JsonNode:

import json

type
  Foo = ref object
    name: string
    params: JsonNode

var foo = Foo()
let foojson = %foo

or if we instantiate the params field:

import json

type
  Foo = ref object
    name: string
    params: JsonNode

var foo = Foo(params: % [5, 5])
let foojson = %foo

echo foojson.pretty

it compiles and works fine.

Of course, one could probably change the string conversion proc to check for nil.

edit: Having thought about handling this in toPretty and toUgly, I'm not sure if there's a good solution. We can:

• replace all nil nodes as nil in the string, but then we cannot parse the created json string back to a JsonNode, because the value nil is not supported
• we can replace all nil nodes by null. Then we can parse it again, but have a different problem. The resulting JsonNode will have nodes of JNull value for each previous nil node. While that seems like a decent mapping, it still feels wrong, simply because parseJson($someJsonNode) should imo be the identity.

[disruptek]

If it'd be considered, I'd offer a patch to json for better behavior given nil ref input when its nonsensical and looser acceptance of same otherwise. I think we can offer a much better experience pretty cheaply.

With regards to the issue, the code already assumes semantic identity between nil and JNull in several places, so the renderer should render it. I would offer a patch for that as well, if you want it.

[Araq]

@disruptek please go ahead. Details can be figured out during the review process.

[krux02]

I wrote in #11907 as well, but here is my reasoning. The crash does not happen in the % operator, it does happen in the printing function, because the generated JSon has an invalid nil in it. So the % operator should catch the invalid nil during the conversion to the json tree, it should not create this invalid json tree. Then with a nice error message it is clear what the problem is.

[disruptek]

Maybe I'm missing something, but I'm pretty sure that this change is designed to break this arguably incorrect but nevertheless currently valid code.

If we're going to break code, shouldn't we just go ahead and pull the nil<->Null out the way you wanted to in the first place? By supporting it in some places and not others, we invite rekindling this criticism with each new user of the module.

[krux02]

If we're going to break code, shouldn't we just go ahead and pull the nil<->Null out the way you wanted to in the first place?

Yes, that is exactly what I think should be done. But I think a few if arg == nil: raise ... wouldn't harm either. Then at least you get a nice invalid argument error instead of crashes.

[disruptek]

Well, I think I still have a branch for the arg == nil but as it turns out, there aren't so many -- it's just a few notable ones, IIRC. I'll submit a PR that deprecates nil as null and adds those exceptions unless I hear otherwise; then you can see where you'd like to cut deeper. :smiling_imp:

[treeform]

I have written a JSON library to solve this problem: https://github.com/treeform/jsony

[ringabout]

toJson provides an option to treat JsonNode as a regular ref object

import std/[json, jsonutils]

type
  Foo = ref object
    name: string
    params: JsonNode

let option = ToJsonOptions(
    enumMode: joptEnumOrd,
    jsonNodeMode: joptJsonNodeAsObject
    ) # treats JsonNode as a regular ref object

var foo = Foo()
echo pretty toJson(foo, option)