search

higgsjs / Higgs

Higgs JavaScript Virtual Machine
876 stars 62 forks source link

Fix issue #192 with cyclic object references and JSON formatting #194

Closed sbstp closed 9 years ago

sbstp commented 9 years ago

This fixes the bug where JSON was not properly formatted (#192). However, it doesn't change the way cyclic references are dealt with, (which is print the first one and ignore the subsequent ones).

maximecb commented 9 years ago

I don't think this is a correct fix though. Those are not actually cyclic references.

V8 behaves this way:

d8> var a = {qux: 'baz'}; undefined d8> var b = [a, a, a]; undefined d8> JSON.stringify(b) "[{"qux":"baz"},{"qux":"baz"},{"qux":"baz"}]" d8>

Which seems more appropriate.

sbstp commented 9 years ago

Ok here's my plan of action. Circular references are only problematic if an object contains itself or a parent.

     a
    / \
   b   c
  /\    \
 d  e    g

Here's it's perfectly, fine for d to contain a reference to, c or g. It's only problematic if d contains a reference to a parent, in this case a, b, or d (d is a parent to its children elements).

Now if g contained a reference to d, it would be fine as long as d does not contain a reference to a, b, c, g and d.

The algorithm I have in mind is basically propagating a list of the parents inside all of the nodes. The list has to be shallow-copied (via .slice()) everytime we "go deeper" so the branches don't interfere with each other. Then all the children (that are objects) of the node are checked against the list of parents.

maximecb commented 9 years ago

Seems acceptable. Slice does not currently do a shallow copy in Higgs though. Not sure if we have any code for which this might be a performance issue. Possibly, you could use an array as a stack? Pushing nodes when going down, popping them when going up?

sbstp commented 9 years ago

Is there still an issue with this? I'm not clear on the status.