Loose parser fails to parse ArrayExpression on multiple lines

[mrennie]

Using the latest from master parse the following:

var a = [1,
2,
3];

The resulting AST has three root nodes in the body (I would expect only a variableDeclaration node) and has only one element in the elements array of the ArrayExpression node (I would expect three Literal nodes).

Acorn.parse(...) properly parses the same expression correctly.

[RReverser]

This is kind of by design:

The expected use for this is to first try acorn.parse, and only
if that fails switch to parse_dammit. The loose parser might
parse badly indented code incorrectly, so don't use it as
your default parser.

Loose parser expects that code is already corrupted, so it doesn't rely strictly on token list, but rather on indentation in order to complete same-indented expressions as incomplete lists, i.e.:

var a = [1,

is treated as single-line array (which seems to be the most valid assumption based on same-styled code) but

var a = [
  1,

would be treated as incomplete multiline array.

If you specifically mess up style by mixing indentations, it of course gets confused and can't determine what did you want to write. So better to follow advice from comment - first try to parse your code with valid parser, and only when it fails, fallback to acorn_loose for style-based approximations.

[mrennie]

This is kind of by design:

Thanks for the pointer, but this feels like a serious flaw. I assumed (obviously incorrectly) that both parsers could parse valid JavaScript code, just that parse_dammit could recover in certain failure cases.

So better to follow advice from comment - first try to parse your code with valid parser, and only when it > fails, fallback to acorn_loose for style-based approximations.

This would not be ideal. In Orion 99% of parsing is done on files that are not complete (i.e. for content assist, marking occurrences, etc) and some of the files are very large - the performance impact of having to parse the same file twice would not be good, especially if I get a different result based on formatting.

[marijnh]

I did at some point sketch out a design for a loose parser that would parse in two passes, first trying to match brackets, and then doing the actual parsing. I never got around to implementing it because it is a lot of work. If someone (IBM?) is willing to invest money in this and pay me to build it, get in contact.

[mrennie]

I would be more than happy to work on ensuring parse_dammit can parse valid JavaScript source if you would be interested in such a contribution.

[marijnh]

Feel free to take a stab at it. But keep performance in mind -- the loose parser should also remain fast.

[mrennie]

I was playing around with this the other day, and I added the following to the loose parser in parseExprList (which fixed up the array case I originally reported):

var blockHeuristic = this.last && this.last.type === acorn.tokTypes.bracketL;
while (!this.closes(close, indent + 1, line, blockHeuristic)) {
...

Does this fix make sense for the array case? If so I can open a PR for it.

[marijnh]

Why would treating parenthesized and bracketed expression lists differently be a good idea?

[RReverser]

@marijnh Not going into the details of the suggested fix, in general one reason could be that a(1, 2, 3) is pretty common so it's reasonable to expect continuation of arguments, while a[1, 2, 3] is rather not and it's more reasonable to expect that it's an array and not indexing?

[mrennie]

The main reason for this change was to fix Orion bug 518330 - so that we could have completions (from Tern) for requirejs regardless of whitespace around the imports.

I also did consider that it is far more likely than not the starting '[' would delimit an array (as @RReverser pointed out).