Last Comment Bug 492844 - ES5: Implement Object.freeze, Object.isFrozen
: ES5: Implement Object.freeze, Object.isFrozen
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Jim Blandy :jimb
:
Mentors:
Depends on: 558451 600128 600142
Blocks: 430133 es5 595081
  Show dependency treegraph
 
Reported: 2009-05-13 13:40 PDT by Jim Blandy :jimb
Modified: 2010-10-05 01:13 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
Implement Object.freeze, Object.isFrozen (10.63 KB, patch)
2010-09-16 15:05 PDT, Jim Blandy :jimb
brendan: review+
Details | Diff | Splinter Review
Implement Object.freeze, Object.isFrozen (9.80 KB, patch)
2010-09-20 09:53 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2009-05-13 13:40:22 PDT
+++ This bug was initially created as a clone of Bug #492840 +++

Object.freeze and Object.isFrozen are specified by 15.2.3.9 and 15.2.3.12 in the ES5 draft:
http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-18 15:25:17 PDT
This bug depends on Brendan's scope-removal work in bug 558451.
Comment 2 Brendan Eich [:brendan] 2010-08-18 16:08:31 PDT
See bug 492849 comment 9.

/be
Comment 3 Jim Blandy :jimb 2010-09-16 15:05:08 PDT
Created attachment 476044 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen
Comment 4 Brendan Eich [:brendan] 2010-09-17 02:22:06 PDT
Comment on attachment 476044 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

The logic is wrong here, see ES5 15.2.3.9 Step 2, noted in bug 492849 comment 38.

/be
Comment 5 Brendan Eich [:brendan] 2010-09-17 03:05:43 PDT
Comment on attachment 476044 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

I'm clearly too tired to review, or do much of anything else. Sorry about that. To make up for it, some nits and an r+.
 
>+        /* If no change is necessary, avoid the setAttributes call. */
>         if ((attrs & JSPROP_PERMANENT) &&
>-            (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)) || (attrs & JSPROP_READONLY))) {
>+            ((attrs & JSPROP_READONLY) || (attrs & (JSPROP_GETTER | JSPROP_SETTER))))
>             continue;
>-        }

Keep the parens per house style (multiline condition is enough to trigger them).

Combine all the tests: (attrs & (JSPROP_READONLY | JSPROP_GETTER | JSPROP_SETTER)).

/be
Comment 6 Jim Blandy :jimb 2010-09-20 09:53:09 PDT
Created attachment 476839 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

Revised per comments.
Comment 7 Brendan Eich [:brendan] 2010-09-20 10:20:20 PDT
Comment on attachment 476839 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

Shouldn't deepEqual use === instead of ==? Even === will equate -0 and 0, and not equate NaN with itself. See

http://wiki.ecmascript.org/doku.php?id=strawman:egal

where the fix is coded in JS:

 Object.eq = function(x, y) {
    if (x === y) {
      // 0 === -0, but they are not identical
      return x !== 0 || 1/x === 1/y;
    }
 
    // NaN !== NaN, but they are identical.
    // NaNs are the only non-reflexive value, i.e., if x !== x,
    // then x is a NaN.
    // isNaN is broken: it converts its argument to number, so
    // isNaN("foo") => true
    return x !== x && y !== y;
  }

/be
Comment 8 Jason Orendorff [:jorendorff] 2010-09-20 11:18:37 PDT
FWIW, I agree deepEqual should handle -0 and NaN specially so that

   deepEqual(NaN, NaN) ===>  true
   deepEqual(0, -0)    ===>  false

and I think it should check that Object.prototype.toString.call produces the same result for both operands (that is, that they have the same [[Class]]).

(Admittedly you eventually should stop and go work on other important stuff...)
Comment 9 Jason Orendorff [:jorendorff] 2010-09-20 12:25:49 PDT
Comment on attachment 476839 [details] [diff] [review]
Implement Object.freeze, Object.isFrozen

In jsobj.cpp, in GetFirstArgumentAsObject:
>    if (v.isPrimitive()) {

Uber-micro-nit: it would be nice for this to say instead:

     if (!v.isObject()) {

Exactly the same meaning, but this goes nicer with the .toObject() call that follows.

In jsobj.cpp:
>+static JSBool
>+obj_freeze(JSContext *cx, uintN argc, Value *vp)
>+{
>+    JSObject *obj;
>+    if (!GetFirstArgumentAsObject(cx, argc, vp, "Object.preventExtensions", &obj))

The string there should say "Object.freeze".

I could stand to see a few more tests:

 - calling freeze() and isFrozen() without arguments or with non-object arguments should throw

 - freezing one object shouldn't freeze a separate object with the same (empty or non-empty) shape, of course

 - after freezing an Array, .length and its elements, should appear on it, and they should all be read-only -- you shouldn't be able to set .length afterwards. (Actually I may not correctly understand 15.4.5.1 on this point, but whatever it says, we should do...)

 - likewise, freezing a Function should cause .arity and .name properties to appear.

 - and freezing an arguments object should do whatever ES5 says, even in a non-strict mode function (shudder) and even after the corresponding function call has returned

 - freezing an E4X object and then poking at it shouldn't crash

 - freezing the global object, so help us, should do whatever ES5 says

 - freezing the global object on trace should deep-bail:

    var arr = [{}, {}, {}, {}, this];
    for (var i = 0; i < arr.length; i++)   // once we go on trace, this.i isn't updated
        Object.freeze(arr[i]);             // needs to freeze this.i == 4
    assertEq(this.i, arr.indexOf(this));

 - freezing shouldn't change [[Class]] (as observable by Object.prototype.toString.call(obj))

 - freezing an object while it's the target of an active with-block, and then trying to write to one of its properties, should fail as expected

 - freezing an object that has methods (in the sense of js::Shape::METHOD), and then getting a method out, should work as expected (basically I think freezing needs to trip the method read barrier and force all the methods to be really truly created)

 - freezing a proxy should do something (throw, if that's what you've got implemented at this point)

 - the ES5 spec says that an inextensible object's [[Prototype]] may not change; we should test that. (I don't see a test for it in bug 492849; sorry if I'm missing something!)

How is freezing supposed to interact with resolve/newResolve hooks? How does it work now; is that likely to break existing JSAPI code (of which we have a bunch)?

r=me with the nits picked and as many tests as you think wise.
Comment 10 Jason Orendorff [:jorendorff] 2010-09-20 12:42:13 PDT
(In reply to comment #9)
>     var arr = [{}, {}, {}, {}, this];
>     for (var i = 0; i < arr.length; i++)   // once we go on trace, this.i isn't
> updated
>         Object.freeze(arr[i]);             // needs to freeze this.i == 4
>     assertEq(this.i, arr.indexOf(this));

Oops! The loop will never terminate. But you get the idea I hope. :)
Comment 11 Jason Orendorff [:jorendorff] 2010-09-20 12:56:11 PDT
I meant to mention: in one of the tests, a function getPropertyOf is defined but never called.
Comment 12 Brendan Eich [:brendan] 2010-09-20 13:06:09 PDT
> How is freezing supposed to interact with resolve/newResolve hooks? How does it
> work now; is that likely to break existing JSAPI code (of which we have a bunch)?

Resolve is just a way of defining (as in JS_DefineProperty) properties on demand, lazily. So API tests that exercise JS_Define* and JS_Set* from a resolve hook could be enough, if we don't already have tests using ES5 Object.defineProperty etc. on frozen objects. I recall some of those, though.

Point is, resolve can't do anything not already doable via the JS API if not via ES5's in-language meta-programming API.

/be
Comment 13 Jim Blandy :jimb 2010-09-21 11:26:03 PDT
(In reply to comment #7)
> Shouldn't deepEqual use === instead of ==? Even === will equate -0 and 0, and
> not equate NaN with itself. See

Fixed.
Comment 14 Jim Blandy :jimb 2010-09-21 11:32:01 PDT
(In reply to comment #9)
> In jsobj.cpp, in GetFirstArgumentAsObject:
> >    if (v.isPrimitive()) {
> 
> Uber-micro-nit: it would be nice for this to say instead:
> 
>      if (!v.isObject()) {
> 
> Exactly the same meaning, but this goes nicer with the .toObject() call that
> follows.

Fixed.

> In jsobj.cpp:
> >+static JSBool
> >+obj_freeze(JSContext *cx, uintN argc, Value *vp)
> >+{
> >+    JSObject *obj;
> >+    if (!GetFirstArgumentAsObject(cx, argc, vp, "Object.preventExtensions", &obj))
> 
> The string there should say "Object.freeze".

Fixed, thanks.

> I could stand to see a few more tests:

I agree, but given the time pressure, I've filed this as bug 598390.
Comment 15 Jim Blandy :jimb 2010-09-21 11:42:31 PDT
http://hg.mozilla.org/tracemonkey/rev/1a49e9c79d5a
Comment 18 Dave Townsend [:mossop] 2010-10-04 10:37:18 PDT
(In reply to comment #17)
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/freeze
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/isFrozen

My understanding is that changing properties on a frozen object will only throw an exception in strict mode but that isn't mentioned in the docs. Am I mistaken?
Comment 19 Fabian (Crash) Grutschus 2010-10-04 11:04:28 PDT
(In reply to comment #18)
> My understanding is that changing properties on a frozen object will only throw
> an exception in strict mode but that isn't mentioned in the docs. Am I
> mistaken?

I can't find anything about that, even not in ECMA-262. Object.freeze loops over all properties of the object and sets writable and configurable to false and I can't also find any documentation that setting, deleting such properties only throw an exception in strict mode.
Comment 20 Brendan Eich [:brendan] 2010-10-04 14:27:12 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > My understanding is that changing properties on a frozen object will only throw
> > an exception in strict mode but that isn't mentioned in the docs. Am I
> > mistaken?
> 
> I can't find anything about that, even not in ECMA-262. Object.freeze loops
> over all properties of the object and sets writable and configurable to false
> and I can't also find any documentation that setting, deleting such properties
> only throw an exception in strict mode.

11.4.1 The delete Operator

The production UnaryExpression : delete UnaryExpression is evaluated as follows:

1. Let ref be the result of evaluating UnaryExpression.
2. If Type(ref) is not Reference, return true.
3. If IsUnresolvableReference(ref) then,
a. If IsStrictReference(ref) is true, throw a SyntaxError exception.
b. Else, return true.
4. If IsPropertyReference(ref) is true, then
a. Return the result of calling the [[Delete]] internal method on ToObject(GetBase(ref) providing GetReferencedName(ref) and IsStrictReference(ref) as the arguments.
5. Else, ref is a Reference to an Environment Record binding, so
a. If IsStrictReference(ref) is true, throw a SyntaxError exception.
b. Let bindings be GetBase(ref).
c. Return the result of calling the DeleteBinding concrete method of bindings, providing GetReferencedName(ref) as the argument.

NOTE When a delete operator occurs within strict mode code, a SyntaxError exception is thrown if its UnaryExpression is a direct reference to a variable, function argument, or function name. In addition, if a delete operator occurs within strict mode code and the property to be deleted has the attribute { [[Configurable]]: false }, a TypeError exception is thrown.

For assignment, see 8.7.2 PutValue (V, W), IsStrictReference, etc. It's all there, as Mossop said.

/be
Comment 21 Fabian (Crash) Grutschus 2010-10-05 01:13:56 PDT
Added it to the documentation, also at Object.seal, Object.preventExtensions and here: https://developer.mozilla.org/en/JavaScript/Strict_mode

Note You need to log in before you can comment on or make changes to this bug.