776106 - Autocompletion on arrays include numbers

Status: RESOLVED WORKSFORME

(DevTools :: Console, defect, P3)

Description

[Thaddee Tyl [:espadrine]]

> window.a = [1, 2];
> window.a.<TAB>

The autocompletion request above yields two possibilities,
"0" and "1", corresponding to the indices of the array.
However, selecting one of those inserts a syntactically
invalid JS statement.

The best way to deal with this is to autocomplete with

> window.a[0]

instead of

> window.a.0

Comment 1

[Mihai Sucan [:msucan]]

This is probably also a problem with property names that have spaces in their names or other special characters, not just numbers in arrays.

Thanks for the report!

Comment 2

[Thaddee Tyl [:espadrine]]

Mihai: Property names that have spaces in their names should use a JS parser
to check whether the property needs to be in a string.

Then again, the use of a state machine to parse JS in the WebConsole
is not great. For starters, having spaces around the dot in a property lookup
is valid JS (see example below). Yet the state machine rejects it.

> window . a [ 0 ]

We should use a JS parser there, too. I have a patch for bug 759837,
but I have yet to write tests for it. That shouldn't take too long.

Comment 3

[Thaddee Tyl [:espadrine]]

Attachment: Don't include numbers when completing objects and arrays.

This patch does not include numbers in autocompletion of objects and arrays.

It leaves room for bracket completion (eg, completion of |window[|).

It would require a JS parser to check for properties
that are not valid identifiers.

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 649075 [details] [diff] [review]
Don't include numbers when completing objects and arrays.

Review of attachment 649075 [details] [diff] [review]:
-----------------------------------------------------------------

What should I review here?

I see the new completeAfter property which is never changed. I also see checks for +foo != +foo and specific behavior for such cases, but you have an outer if that skips those cases. Sorry, I am confused about the patch.

Do we really need a parser? Can you please check if the spec tells us what makes a valid identifier? Perhaps we could have a regex that would work for most-cases that determines an invalid identifier (it doesn't have to be 100% accurate). When the identifier is invalid we would wrap it in quotes, like ["foo identifier"].

Thanks!

Comment 5

[Thaddee Tyl [:espadrine]]

(In reply to Mihai Sucan [:msucan] from comment #4)
> I see the new completeAfter property which is never changed. I also see
> checks for +foo != +foo and specific behavior for such cases, but you have
> an outer if that skips those cases. Sorry, I am confused about the patch.

As the code comment explain, dot completion does not add plain numbers
as properties.

It checks for those string numbers by converting the string to a number,
and checking whether the conversion failed. That's what the |+foo != +foo| is.

> Do we really need a parser? Can you please check if the spec tells us what
> makes a valid identifier? Perhaps we could have a regex that would work for
> most-cases that determines an invalid identifier (it doesn't have to be 100%
> accurate). When the identifier is invalid we would wrap it in quotes, like
> ["foo identifier"].

The spec is hard to implement in JS. Esprima does it, though [1].
It achieves that by listing all valid UTF-16 characters in a huge regex,
and checking whether every character of the identifier validates against it.
Of course, it has two huge regexes, one for the first character of the identifier,
the other for the following characters of the identifier.

Then, it has a different, big string to interpret what is a valid whitespace.


  [1] https://github.com/ariya/esprima/blob/master/esprima.js#L233


This algorithm is easy to get wrong; that's why I'd rather rely on a JS parser…

Anyway, we'll get a parser for everyone soon! :)

Comment 6

[Mihai Sucan [:msucan]]

Thanks for your explanation.

Since all these patches work on the same parts of the code, please make one patch in one bug. Thanks!

Comment 7

[Rob Campbell [:rc] (:robcee)]

Filter on GUNGNIR.

Comment 8

[Mihai Sucan [:msucan]]

This bug was fixed long ago. I dont remember which bug.