search

millermedeiros / rocambole

Recursively walk and transform EcmaScript AST
171 stars 12 forks source link

Error while instrumenting arrays literals with empty or null values #15

Closed twolfson closed 10 years ago

twolfson commented 10 years ago

I ran into a snag while trying out esformatter, it cannot format:

function hai() {
    return [,];
}
// or
function hai() {
    return [,null];
}

The stack trace in both cases:

/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:77
        node.parent = parent;
                    ^
TypeError: Cannot set property 'parent' of null
    at instrumentNodes (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:77:21)
    at recursiveWalk (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:347:10)
    at recursiveWalk (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:372:17)
    at recursiveWalk (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:368:13)
    at recursiveWalk (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:372:17)
    at recursiveWalk (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:368:13)
    at recursiveWalk (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:372:17)
    at Object.parse (/usr/local/lib/node_modules/esformatter/node_modules/rocambole/rocambole.js:93:5)
    at Object.format (/usr/local/lib/node_modules/esformatter/lib/esformatter.js:43:23)
    at formatToConsole (/usr/local/lib/node_modules/esformatter/lib/cli.js:156:28)

For reference, I was beautifying minified source code which is probably how that got introduced.

http://www.kimonolabs.com/js/kimono.min.js

twolfson commented 10 years ago

Boiled this down and found the cause of the issue but I don't actually know what the expected behavior should be =(

https://github.com/twolfson/rocambole/compare/bug;cannot.parse.comma.arrays

The root of the problem is esprima returns the following AST for our regression:

// Part of `esprima.parse('a = [,]`);`
{ type: 'ArrayExpression', elements: [ null ], range: [ 4, 7 ] }

The specific part that is in reference to is our array [,]. When we walk over this array, the element is null. As a result, when instrumentNodes attempts to set a value on null, it errors out.

https://github.com/millermedeiros/rocambole/blob/v0.3.2/rocambole.js#L371-L373

However, the rub is I am not sure what the expected behavior for that is. Do we skip it? Do we upcast it to the equivalent of a = [undefined]?

twolfson commented 10 years ago

This issue has been stale for a month and the problem persists. Can we decide what action to take?

skeggse commented 10 years ago

I use this enough in my tests that this makes esformatter-diff crash :frowning:

millermedeiros commented 10 years ago

I'm also not sure on what to do.. but I guess the best thing is to keep the AST as is, and just skip the null during the instrumentation for now.

sorry for not replying to this before, was swamped with other tasks and totally forgot about it.

millermedeiros commented 10 years ago

this is still failing on the moonwalk call since null doesn't have a depth..

millermedeiros commented 10 years ago

esformatter v0.3.2 should fix this issue \o/

twolfson commented 10 years ago

Thanks @millermedeiros ! =D