Strict mode object literal validation

Strict mode object literal validation (Draft)
 * Detect duplicate data properties in strict mode
 * Detect conflict between 2 accessors of same kind or accessor and data property.


BUG=
TEST=test/mjsunit/strict-mode.js

[Martin Maly, 2011-01-21 00:41:06]

Draft of object literal validation. Most of the checks are actually required
even outside of strict mode. The code is based on object literal codegen but
must run early for object literal errors are early errors.

Any feedback welcome.

Thank you!
Martin

http://codereview.chromium.org/6335010/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3016
src/parser.cc:3016: bool IsEqualString(void* first, void* second);
These 2 are defined in ast.cc, ideally I'd want to move them to some shared
location. Is there a good place? This declaration feels suboptimal.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3061
src/parser.cc:3061: void ObjectLiteralPropertyChecker::CheckProperty(
Similar code exists in ast.cc and is executed when emitting object literal. This
is too late because the object literal errors must be early errors.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3108
src/parser.cc:3108: // Data property conflicting with an accessor.
Most of these checks should execute even outside of strict mode. Only duplicate
data properties are strict mode check.

[Lasse Reichstein, 2011-01-21 12:01:13]

LGTM if non-strict code in either Safari or Firefox already throw exceptions
where we will now do it.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3037
src/parser.cc:3037: GetAccessor = 0x01,
Add a "k" in front of constants, i.e., kGetAccessor, kSetAccessor, kAccessor,
kData.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3043
src/parser.cc:3043: HashMap props;
Properties should be at the end of the class, after methods (including static
methods).

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3050
src/parser.cc:3050: default:
Move the default to the end instead.
I just think it reads better :)

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3077
src/parser.cc:3077: ASSERT(!name->AsArrayIndex(&hash));
I'm a little worried that we might have a symbol that is also a valid
ArrayIndex.
We might not, but I haven't been able to convince myself that it can't happen
yet.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3108
src/parser.cc:3108: // Data property conflicting with an accessor.
We should be careful when adding restrictions to non-strict code that isn't
there already. There is a potential to break existing code on the web.

We should at least check whether other significant browsers already fail on the
code, and not be the first to break the code.

[MarkM, 2011-01-21 17:37:55]

http://codereview.chromium.org/6335010/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3108
src/parser.cc:3108: // Data property conflicting with an accessor.
On WebKit nightly Version 5.0.1 (5533.17.8, r75891) non-strict

    ({x: 33, get x(){}});  
    SyntaxError: Parse error

On Minefield/FF 4.0b10pre (2011-01-21) non-strict

    ({x: 33, get x(){}});
    SyntaxError: property name x appears more than once in object literal

[Martin Maly, 2011-01-21 20:00:36]

Hi Lasse,

thanks for reviewing the code for me. Please take another look.

Thank you!
Martin

http://codereview.chromium.org/6335010/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3037
src/parser.cc:3037: GetAccessor = 0x01,
On 2011/01/21 12:01:13, Lasse Reichstein wrote:
> Add a "k" in front of constants, i.e., kGetAccessor, kSetAccessor, kAccessor,
> kData.

Done.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3043
src/parser.cc:3043: HashMap props;
On 2011/01/21 12:01:13, Lasse Reichstein wrote:
> Properties should be at the end of the class, after methods (including static
> methods).

Done.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3050
src/parser.cc:3050: default:
On 2011/01/21 12:01:13, Lasse Reichstein wrote:
> Move the default to the end instead.
> I just think it reads better :)

Done.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3077
src/parser.cc:3077: ASSERT(!name->AsArrayIndex(&hash));
On 2011/01/21 12:01:13, Lasse Reichstein wrote:
> I'm a little worried that we might have a symbol that is also a valid
> ArrayIndex.
> We might not, but I haven't been able to convince myself that it can't happen
> yet.


I am less concerned since the same checks are performed in ast.cc 
ObjectLiteral::CalculateEmitStore which has been around for a while.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3108
src/parser.cc:3108: // Data property conflicting with an accessor.
On 2011/01/21 17:37:55, MarkM wrote:
> On WebKit nightly Version 5.0.1 (5533.17.8, r75891) non-strict
> 
>     ({x: 33, get x(){}});  
>     SyntaxError: Parse error
> 
> On Minefield/FF 4.0b10pre (2011-01-21) non-strict
> 
>     ({x: 33, get x(){}});
>     SyntaxError: property name x appears more than once in object literal

Thank you, Mark, for doing the research. The shipping versions of Firefox,
Safari do not report error, however. Since their future versions are
implementing this I would recommend we keep the full checks in place as
implemented, although there may be good reason (like fast Chrome release cycle)
that may lead us to wait with full check until some future date and leave them
all under strict mode for now.

[Mads Ager (chromium), 2011-01-24 07:15:40]

The only places where this patch will throw exceptions where we didn't before is
in connection with accessors, right? If that is the case I think the risk is
minimal. Throwing exceptions for multiple data properties with the same name I'm
sure would lead to trouble on the other hand.

[Lasse Reichstein, 2011-01-24 07:57:01]

True. The changes are only in cases that didn't exist in ES3, so it's not a
backwards compatibility problem.
Ok, I agree to keep the non-strict-mode restrictions, but change the "symbols
are not array indices" assumption, e.g., by checking for array indices first.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3077
src/parser.cc:3077: ASSERT(!name->AsArrayIndex(&hash));
Damnit. I fixed one such place already, but it seems there are more. It's good
that you found it :)
It's NOT safe. The ASSERT gets hit by, e.g.:
var y = {get "1234"() {return 42; }};

[Martin Maly, 2011-01-24 18:09:08]

Fixed both locations with this issue, added comment and more tests.

http://codereview.chromium.org/6335010/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/1/src/parser.cc#newcode3077
src/parser.cc:3077: ASSERT(!name->AsArrayIndex(&hash));
On 2011/01/24 07:57:01, Lasse Reichstein wrote:
> Damnit. I fixed one such place already, but it seems there are more. It's good
> that you found it :)
> It's NOT safe. The ASSERT gets hit by, e.g.:
> var y = {get "1234"() {return 42; }};

Great catch. I fixed both locations and added more tests.

[Lasse Reichstein, 2011-01-25 11:19:03]

LGTM with comments addressed.

http://codereview.chromium.org/6335010/diff/6002/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/6335010/diff/6002/src/ast.cc#newcode245
src/ast.cc:245: if (handle->ToArrayIndex(&index)) {
I already committed a patch for this case.
During that I checked what happens, and this change isn't even sufficient. The
ToArrayIndex function only works on numbers (Smi and HeapNumber), not strings.
You need a second case inside the IsSymbol case to account for a string that
represents an array index.

http://codereview.chromium.org/6335010/diff/6002/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/6002/src/parser.cc#newcode3077
src/parser.cc:3077: if (handle->ToArrayIndex(&hash)) {
This still needs to handle array-index symbols.
You can see the code in my change for ast.cc:
http://codereview.chromium.org/6304016/

http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:125: 
For good measure, try with
 var x = {123: 1, "0123": 2};

 var x = {123: 1,
123.00000000000000000000000000000000000000000000000000000000000000000001 : 2}

and 

 var x = {123: 1,
"123.00000000000000000000000000000000000000000000000000000000000000000001" : 2}

The first shouldn't throw, the second should (it's the same number), and the
third shouldn't.

http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:125: 
Also try get/set properties using strings and numbers, e.g.:

 var x = {get 12(){}, get "12"(){}};
 var x = {get foo(){}, get "foo"(){}};

[Martin Maly, 2011-01-25 18:29:23]

Thanks, I addressed all the feedback.

Martin

http://codereview.chromium.org/6335010/diff/6002/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/6335010/diff/6002/src/ast.cc#newcode245
src/ast.cc:245: if (handle->ToArrayIndex(&index)) {
On 2011/01/25 11:19:03, Lasse Reichstein wrote:
> I already committed a patch for this case.
> During that I checked what happens, and this change isn't even sufficient. The
> ToArrayIndex function only works on numbers (Smi and HeapNumber), not strings.
> You need a second case inside the IsSymbol case to account for a string that
> represents an array index.

Thanks, I reverted this part of my change.

http://codereview.chromium.org/6335010/diff/6002/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6335010/diff/6002/src/parser.cc#newcode3077
src/parser.cc:3077: if (handle->ToArrayIndex(&hash)) {
On 2011/01/25 11:19:03, Lasse Reichstein wrote:
> This still needs to handle array-index symbols.
> You can see the code in my change for ast.cc:
> http://codereview.chromium.org/6304016/
> 

Done.

http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:125: 
On 2011/01/25 11:19:03, Lasse Reichstein wrote:
> For good measure, try with
>  var x = {123: 1, "0123": 2};
> 
>  var x = {123: 1,
> 123.00000000000000000000000000000000000000000000000000000000000000000001 : 2}
> 
> and 
> 
>  var x = {123: 1,
> "123.00000000000000000000000000000000000000000000000000000000000000000001" :
2}
> 
> The first shouldn't throw, the second should (it's the same number), and the
> third shouldn't.

Done.

http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:125: 
On 2011/01/25 11:19:03, Lasse Reichstein wrote:
> Also try get/set properties using strings and numbers, e.g.:
> 
>  var x = {get 12(){}, get "12"(){}};
>  var x = {get foo(){}, get "foo"(){}};

Done.