I did a thorough revision of Allen's Rev12 and Rev13 drafts which
incorporate (among many other things) the refactored MOP, symbols, proxies
and the Reflect module in the ES6 draft.
Below is a long list of detailed comments, both editorial and more
substantial in nature.
It's too cumbersome for me to file bugs for every individual comment.
Allen, please let me know what comments are worth filing a bug for.
Before delving into the details, two more things:
First, thank you Allen for the great spec work. Overall I'm very happy with
the way proxies have been incorporated into the ES6 draft.
Second, if people want to comment on a particular issue, I suggest to
rename the topic and fork off a separate thread. That will be more
manageable, and will allow people that don't want to read through this
whole list to follow the separate discussions.
Cheers,
Tom
== Comments based on ES6 draft Rev. 13 (21 dec. 2012) ==
=== General remarks ===
[[GetP]]/[[SetP]] => rename these to just [[Get]] and [[Set]]?
[[GetInheritance]]/[[SetInheritance]] => why not
[[GetProto]]/[[SetProto]]?
More familiar to ECMAScript programmers
No risk of confusion with function "prototype" property
For ordinary objects, [[GetInheritance]] returns the value of the
[[Prototype]] field
[[SetInheritance]]/Reflect.setPrototypeOf: I'm not sure this was agreed
upon. Especially since proto is currently specified as a data property.
This means there is no setter that separately reifies the ability to set
the prototype. Thus, it's perfectly possible to just exclude
[[SetInheritance]] and Reflect.setPrototypeOf from the spec.
I'm a bit uncomfortable with the removal of property descriptor
normalization in the getOwnPropertyDescriptor/defineProperty traps.
Especially for getOwnPropertyDescriptor I think it's a breaking change
w.r.t. ES5.1.
=== Chapter 8 ===
8.1.6.2
"Unless explicitly specified otherwise, internal data properties may be
dynamically added to ECMAScript objects."
Isn't it safer to err on the side of safety and specify the opposite? (i.e.
"properties cannot be dynamically added unless specified otherwise")?
Table 8:
[[PreventExtensions]] internal method: should return a Boolean success
value
[[Delete]]: I would remove "because its [[Configurable]] attribute is
false." from the description. Proxies and host objects may return false for
other reasons as well (cf. the recent discussions about DontDelete vs.
configurable:false)
[[Enumerate]]: typo: "Returns an iterator object that {iterates} over
the string values"
[[Keys]] should be removed
[[HasProperty]] should probably be added to this table again
[[OwnPropertyKeys]]: the return type should be changed to Object
(iterator)
8.3.4 Object [PreventExtensions]
This method should return a Boolean (true).
8.3.7.3 ValidateAndApplyPropertyDescriptor
Typo: "… and no{t} object updates are {pre}formed"
The note: "However, if O has an [[BuiltinBrand]] internal data property
whose value is BuiltinArray O also has a more elaborate
[[DefineOwnProperty]] internal method defined in 15.4.5.1." is probably
more appropriately placed at the top of section 8.3.7 (closer to the
definition of [[DefineOwnProperty]])
The note: "NOTE Step 10.b allows any field…" should probably refer to Step
10.a (there is no longer a step 10.b in the new
ValidateAndApplyPropertyDescriptor algorithm)
8.7.8 Object [[HasProperty]] ( P )
The section number should probably be 8.3.8
"When the [[GetProperty]] internal method" => should probably be
[[HasProperty]].
8.3.19 Ordinary Function Objects
Table 13: Description for [[Home]]
"…this is the object whose [[Inheritance]] provides the object…"
should probably be [[GetInheritance]].
8.4 Built-in Exotic Object Internal Methods and Data Fields
A few typos in introductory text:
"This specification define{s} several kinds of built-in exotic objects.
These objects generally behave similar to ordinary objects except for a
few specific situ{t}ations. The following exotic objects use the ordinary
object internal methods except where it is explicitly specified {other}wise
below:"
8.4.4 Exotic Symbol Objects
Typo: "A{n} Symbol object is an exotic object…"
"Exotic String objects have {the} a..."
In the introduction, it's mentioned that "Symbol exotic objects are unique
in that they are always immutable and never observably reference any other
object."
Yet, as currently specified, evaluating aSymbol.toString yields a reference
to the global Object.prototype.toString function (which is mutable by
default).
Shouldn't aSymbol.toString just be undefined?
I notice that Object.prototype.toString special-cases Symbols anyway, so
Object.prototype.toString.call(aSymbol) continues to work fine.
In case aSymbol.toString should continue to return
Object.prototype.toString, I would advise to modify [[HasProperty]] for
Symbols to answer 'true' for the string "toString", and [[Delete]] to
answer 'false', so that [[Get]],[[HasProperty]] and [[Delete]] remain
internally consistent.
8.4.5 Exotic Arguments Objects
Typo: "They also have a [[Par{a}meterMap]] internal data property"
=== 8.5 Proxies ===
Introduction:
"Every proxy object also has an internal data property called
[[ProxyTarget]] whose value is usually an object."
The word "usually" is unnecessarily vague. It would be more precise to say:
"whose value is always an object for an unrevoked proxy, or null for a
revoked proxy."
Second paragraph, last sentence, typo: "on the proxy’s target object i{f}
the handler object does not have a method corresponding to the internal
trap."
Third paragraph, typo: "Some proxy objects are created in a manner that
permits them to be subsequent{ly} revoked"
Fourth paragraph: as written currently, it reads as if the invariants from
Section 8.1.6.2 can effectively be violated by proxies. I would rephrase
the first sentence to make it clearer that proxies have this potential, but
this potential is not realized thanks to the invariant checks:
"Because proxies permit arbitrary ECMAScript code to be used to implement
internal methods, they have the potential to violate the invariants defined
in 8.1.6.2."
8.5.1 Proxy [[GetInheritance]]
In the NOTE at the bottom:
"[[GetInheritance] applied to the proxy object must return the same value
as [[GetInheritance] applied to the proxy object’s handler object."
=> should be "… applied to the proxy object's target object."
(also, [[GetInheritance] should have two closing brackets)
The same comment applies to the NOTE in 8.5.2 and 8.5.3.
8.5.2 Proxy [[SetInheritance]]
Reply to Comment [50]: the invariant on [[GetInheritance]] is needed
because otherwise proxies can emulate mutable prototype chains even when
UnderScoreProtoEnabled is false.
8.5.4 Proxy [[PreventExtensions]]
The steps starting from step 8 are new to me.
I don't understand why you changed the invariant checks here.
This is also the first and only place where a single intercepted operation
("preventExtensions") gives rise to two trap invocations on the handler
("preventExtensions" and "isExtensible")
The only important invariant for [[PreventExtensions]] is that, if the
"preventExtensions" trap returns true, then the [[ProxyTarget]] must be
non-extensible. There's no need to invoke the extra "isExtensible" trap to
verify this.
In step 17, [[PreventExtensions]] currently returns no value. It's more
useful to return the boolean "proxyIsExtensible" success value (like
[[DefineOwnProperty]] and [[Delete]]), and have user-facing methods that
call [[PreventExtensions]] convert a false return value into a thrown
exception (see below for my related comments on Object.preventExtensions).
8.5.5 Proxy [[HasOwnProperty]]
Typo: "NOTE [[HasOwnPro{p}erty] for proxy objects enforces the following
invariants"
8.5.6 Proxy [[GetOwnProperty]]
The invariant check made in step 11 of the [[GetOwnProperty]] algorithm
as specified on the wiki has been dropped. Is that because it is subsumed
by a similar test made in IsCompatiblePropertyDescriptor?
(I think it is correct to drop the redundant check, I just want to make
sure this change was intentional.)
Step 13.a of [[GetOwnProperty]] on the wiki tests:
"If targetDesc is undefined or targetDesc.[[Configurable]] is true, throw a
TypeError"
In the ES6Rev13 algorithm, step 21.a:
"If targetDesc is not undefined and targetDesc.[[Configurable]] is true,
then Throw a TypeError exception."
I think this misses the case where resultDesc.[[Configurable]] is false and
targetDesc is undefined: this should result in a TypeError per the wiki
spec, but will work fine per the ES6Rev13 draft.
This test is to uphold the 5th invariant listed in the NOTE:
"A property cannot be reported as non-configurable, if it does not exist{s}
as a{n} own property of the target"
Step 18 / Comment 51: noted. Any specific reason why you changed this?
To anyone following along, quick summary: if the "getOwnPropertyDescriptor"
trap returns an incomplete property descriptor, the descriptor will be
completed based on the values from the target property descriptor, rather
than the default attribute values from Table 7 (unless the property doesn't
exist on the target).
Step 22 / Comment [52]: I noticed the changes in 8.2.5.4
FromPropertyDescriptor(Desc) and 8.2.5.5 ToPropertyDescriptor, which use a
separate [[Origin]] field to avoid unnecessary conversions.
While this approach does preserve custom property attributes without
copying, it also circumvents the "normalization" process by which the
return value of the trap got converted into a "normal" property descriptor.
This means that the return value of Object.getOwnPropertyDescriptor can now
be any object, including objects that define properties like "writable" and
"configurable" as accessors that can modify from one moment to the next.
This puts the burden of normalizing property descriptors on clients of
Object.getOwnPropertyDescriptor. Hence, this change is a potential security
issue for extant ES5.1 code that relies on Object.getOwnPropertyDescriptor
performing the normalization.
Typo: "NOTE [[GetOwnPro{p}erty] for proxy objects enforces the following
invariants"
8.5.7 Proxy [[DefineOwnProperty]]
Step 7-8: as for [[GetOwnProperty]], this is an important change from the
wiki spec, as it means the defineProperty trap will get the original
descriptor object passed into Object.defineProperty as argument, without
normalization. That means it's now up to the handler to manually normalize
the descriptor object.
This is less of an issue than the symmetrical case for [[GetOwnProperty]],
because handlers are new in ES6, so there's no breaking change compared to
ES5.1. It is a breaking change compared to existing Proxy prototype
implementations. That's not a deal breaker, but I just want to point it out
explicitly since it may impact existing code.
Typo: "NOTE [[{DefineOwnProp}erty]] for proxy objects enforces the
following invariants:"
8.5.10 Proxy [[SetP]]
Typo: NOTE, first invariant: "Cannnot change the value of a property to
be different from the value of the corres{o}ponding…"
8.5.12 Proxy [[Enumerate]]
I think it may be possible to waive the extra invariant checks for
[[Enumerate]]. It's not a crucial primitive.
My reasoning is that [[Enumerate]] deals with both own and inherited
properties, and we don't really enforce any invariants on inherited
properties. So I guess it's ok if the invariants for [[Enumerate]] are
weakened.
Do note that this is a bit inconsistent with the way we treat internal
methods like [[HasProperty]], [[GetP]] and [[SetP]]: these also deal with
own and inherited properties, but still enforce invariants on own
properties.
8.5.13 Proxy [[OwnPropertyKeys]]
For [[OwnPropertyKeys]], I maintain that it's essential that this
iterator at least enumerates all non-configurable own properties of the
target.
Otherwise, a proxy might "hide" properties from reflective code that tries
to inspect all of its properties.
Ideally, the iterator should also not enumerate any non-existent properties
on a non-extensible target. Although if this invariant is violated, the
invariants defined on getOwnPropertyDescriptor etc. should prevent the
proxy from revealing any useful value associated with these "made-up"
properties.
As "ownPropertyKeys" returns an iterator rather than an array of strings,
it's no longer symmetric to getOwnPropertyNames anyway, so I agree with the
name change. However, to ensure consistency of user-facing names, either
the trap should be named "ownKeys" (for consistency with Reflect.ownKeys),
or Reflect.ownKeys should be renamed Reflect.ownPropertyKeys.
I think I'm ready to give up on all of these (and so drop these methods
from the internal MOP and the Reflect module altogether).
(although I don't agree with your Comment[56-57-58] that keeping them would
entail a lot of additional invariant checks: the checks are still there
even without these internal methods, they're just performed implicitly by
the Object.freeze algorithm when it loops over all the properties and calls
[[DefineOwnProperty]] for each property)
8.5.19 Proxy [[Construct]]
Step 7, typo: "Return the result of calling{v} trap with handler…"
=== Chapter 9 ===
9.3.9 MakeObjectSecure
9.3.10 TestIfSecureObject
I would prefer a name that doesn't feature the word "Secure" (I'm sure Mark
would too :)
Mark and I have previously used the term "tamper-proof" to abstract over
preventExtensions/seal/freeze (so: MakeTamperProof/IsTamperProof). Or, in
the earlier Proxy API there was the "fix" trap (so perhaps: Fix/IsFixed)
=== Chapter 15 ===
15.2.3.8 Object.seal
15.2.3.9 Object.freeze
15.2.3.10 Object.preventExtensions
In all of these algorithms, if status is false, that should probably be
turned into an explicit TypeError. This is consistent with the design of
[[DefineOwnProperty]].
Neither of these operations should ever fail silently.
15.3.4.2 Function.prototype.toString
Perhaps this is still on your TODO list, but I would advise to specify what
this method should do on a Proxy. My suggested semantics:
"When called on a Proxy that is callable (i.e. whose [[ProxyTarget]] has a
[[Call]] internal method), return the result of applying the built-in
Function.prototype.toString on the Proxy’s [[ProxyTarget]]. Otherwise,
throw a TypeError. When applied to a revoked proxy, throw a TypeError."
15.17 The Reflect Module
In looking at the spec now, I realize the following inconsistency:
On the wiki, I specced all of the Reflect.* methods to perform a ToObject
conversion on the target argument, but the corresponding Object.* methods
instead perform an explicit typecheck and throw if the target is not an
object.
I think it makes sense to adopt the Object.* behavior and change all
ToObject conversions into explicit type checks, because
a) it's more consistent
b) trying to call e.g. Reflect.preventExtensions on a primitive value is
most certainly a programming error (it doesn't make sense to prevent
extensions on a boxed primitive), and implicit conversions hide such errors.
15.17.1.5 Reflect.has
Typo: "When the has{Own} function…"
15.17.1.12 Reflect.enumerate
Steps 3 and 4 can be merged since itr is never used.
15.17.1.13 Reflect.ownKeys
Either rename to Reflect.ownPropertyKeys, or rename the "ownPropertyKeys"
trap to "ownKeys", to ensure consistency of user-facing names.
15.17.1.14,15,16,17: can be dropped if [[Freeze]] and friends are dropped.
15.18 Proxy Objects
I presume this section will host the Proxy and Proxy.revocable constructors.
Hi,
I did a thorough revision of Allen's Rev12 and Rev13 drafts which
incorporate (among many other things) the refactored MOP, symbols, proxies
and the Reflect module in the ES6 draft.
Below is a long list of detailed comments, both editorial and more
substantial in nature.
It's too cumbersome for me to file bugs for every individual comment.
Allen, please let me know what comments are worth filing a bug for.
Before delving into the details, two more things:
First, thank you Allen for the great spec work. Overall I'm very happy with
the way proxies have been incorporated into the ES6 draft.
Second, if people want to comment on a particular issue, I suggest to
rename the topic and fork off a separate thread. That will be more
manageable, and will allow people that don't want to read through this
whole list to follow the separate discussions.
Cheers,
Tom
== Comments based on ES6 draft Rev. 13 (21 dec. 2012) ==
=== General remarks ===
* [[GetP]]/[[SetP]] => rename these to just [[Get]] and [[Set]]?
* [[GetInheritance]]/[[SetInheritance]] => why not
[[GetProto]]/[[SetProto]]?
- More familiar to ECMAScript programmers
- No risk of confusion with function "prototype" property
- For ordinary objects, [[GetInheritance]] returns the value of the
[[Prototype]] field
* [[SetInheritance]]/Reflect.setPrototypeOf: I'm not sure this was agreed
upon. Especially since __proto__ is currently specified as a data property.
This means there is no setter that separately reifies the ability to set
the prototype. Thus, it's perfectly possible to just exclude
[[SetInheritance]] and Reflect.setPrototypeOf from the spec.
* I'm a bit uncomfortable with the removal of property descriptor
normalization in the getOwnPropertyDescriptor/defineProperty traps.
Especially for getOwnPropertyDescriptor I think it's a breaking change
w.r.t. ES5.1.
=== Chapter 8 ===
8.1.6.2
"Unless explicitly specified otherwise, internal data properties may be
dynamically added to ECMAScript objects."
Isn't it safer to err on the side of safety and specify the opposite? (i.e.
"properties cannot be dynamically added unless specified otherwise")?
Table 8:
* [[PreventExtensions]] internal method: should return a Boolean success
value
* [[Delete]]: I would remove "because its [[Configurable]] attribute is
false." from the description. Proxies and host objects may return false for
other reasons as well (cf. the recent discussions about DontDelete vs.
configurable:false)
* [[Enumerate]]: typo: "Returns an iterator object that {iterates} over
the string values"
* [[Keys]] should be removed
* [[HasProperty]] should probably be added to this table again
* [[OwnPropertyKeys]]: the return type should be changed to Object
(iterator)
8.3.4 Object [[PreventExtensions]]()
This method should return a Boolean (true).
8.3.7.3 ValidateAndApplyPropertyDescriptor
Typo: "… and no{t} object updates are {pre}formed"
The note: "However, if O has an [[BuiltinBrand]] internal data property
whose value is BuiltinArray O also has a more elaborate
[[DefineOwnProperty]] internal method defined in 15.4.5.1." is probably
more appropriately placed at the top of section 8.3.7 (closer to the
definition of [[DefineOwnProperty]])
The note: "NOTE Step 10.b allows any field…" should probably refer to Step
10.a (there is no longer a step 10.b in the new
ValidateAndApplyPropertyDescriptor algorithm)
8.7.8 Object [[HasProperty]] ( P )
- The section number should probably be 8.3.8
"When the [[GetProperty]] internal method" => should probably be
[[HasProperty]].
8.3.19 Ordinary Function Objects
Table 13: Description for [[Home]]
"…this is the object whose [[Inheritance]] provides the object…"
should probably be [[GetInheritance]].
8.4 Built-in Exotic Object Internal Methods and Data Fields
A few typos in introductory text:
"This specification define{s} several kinds of built-in exotic objects.
These objects generally behave similar to ordinary objects except for a
few specific situ{t}ations. The following exotic objects use the ordinary
object internal methods except where it is explicitly specified {other}wise
below:"
8.4.4 Exotic Symbol Objects
Typo: "A{n} Symbol object is an exotic object…"
"Exotic String objects have {the} a..."
In the introduction, it's mentioned that "Symbol exotic objects are unique
in that they are always immutable and never observably reference any other
object."
Yet, as currently specified, evaluating aSymbol.toString yields a reference
to the global Object.prototype.toString function (which is mutable by
default).
Shouldn't aSymbol.toString just be undefined?
I notice that Object.prototype.toString special-cases Symbols anyway, so
Object.prototype.toString.call(aSymbol) continues to work fine.
In case aSymbol.toString should continue to return
Object.prototype.toString, I would advise to modify [[HasProperty]] for
Symbols to answer 'true' for the string "toString", and [[Delete]] to
answer 'false', so that [[Get]],[[HasProperty]] and [[Delete]] remain
internally consistent.
8.4.5 Exotic Arguments Objects
Typo: "They also have a [[Par{a}meterMap]] internal data property"
=== 8.5 Proxies ===
- Introduction:
"Every proxy object also has an internal data property called
[[ProxyTarget]] whose value is usually an object."
The word "usually" is unnecessarily vague. It would be more precise to say:
"whose value is always an object for an unrevoked proxy, or null for a
revoked proxy."
Second paragraph, last sentence, typo: "on the proxy’s target object i{f}
the handler object does not have a method corresponding to the internal
trap."
Third paragraph, typo: "Some proxy objects are created in a manner that
permits them to be subsequent{ly} revoked"
Fourth paragraph: as written currently, it reads as if the invariants from
Section 8.1.6.2 can effectively be violated by proxies. I would rephrase
the first sentence to make it clearer that proxies have this potential, but
this potential is not realized thanks to the invariant checks:
"Because proxies permit arbitrary ECMAScript code to be used to implement
internal methods, they have the potential to violate the invariants defined
in 8.1.6.2."
8.5.1 Proxy [[GetInheritance]]
- In the NOTE at the bottom:
"[[GetInheritance] applied to the proxy object must return the same value
as [[GetInheritance] applied to the proxy object’s handler object."
=> should be "… applied to the proxy object's target object."
(also, [[GetInheritance] should have two closing brackets)
The same comment applies to the NOTE in 8.5.2 and 8.5.3.
8.5.2 Proxy [[SetInheritance]]
- Reply to Comment [50]: the invariant on [[GetInheritance]] is needed
because otherwise proxies can emulate mutable prototype chains even when
UnderScoreProtoEnabled is false.
8.5.4 Proxy [[PreventExtensions]]
- The steps starting from step 8 are new to me.
I don't understand why you changed the invariant checks here.
This is also the first and only place where a single intercepted operation
("preventExtensions") gives rise to two trap invocations on the handler
("preventExtensions" and "isExtensible")
The only important invariant for [[PreventExtensions]] is that, if the
"preventExtensions" trap returns true, then the [[ProxyTarget]] must be
non-extensible. There's no need to invoke the extra "isExtensible" trap to
verify this.
- In step 17, [[PreventExtensions]] currently returns no value. It's more
useful to return the boolean "proxyIsExtensible" success value (like
[[DefineOwnProperty]] and [[Delete]]), and have user-facing methods that
call [[PreventExtensions]] convert a false return value into a thrown
exception (see below for my related comments on Object.preventExtensions).
8.5.5 Proxy [[HasOwnProperty]]
- Typo: "NOTE [[HasOwnPro{p}erty] for proxy objects enforces the following
invariants"
8.5.6 Proxy [[GetOwnProperty]]
- The invariant check made in step 11 of the [[GetOwnProperty]] algorithm
as specified on the wiki has been dropped. Is that because it is subsumed
by a similar test made in IsCompatiblePropertyDescriptor?
(I think it is correct to drop the redundant check, I just want to make
sure this change was intentional.)
- Step 13.a of [[GetOwnProperty]] on the wiki tests:
"If targetDesc is undefined or targetDesc.[[Configurable]] is true, throw a
TypeError"
In the ES6Rev13 algorithm, step 21.a:
"If targetDesc is not undefined and targetDesc.[[Configurable]] is true,
then Throw a TypeError exception."
I think this misses the case where resultDesc.[[Configurable]] is false and
targetDesc is undefined: this should result in a TypeError per the wiki
spec, but will work fine per the ES6Rev13 draft.
This test is to uphold the 5th invariant listed in the NOTE:
"A property cannot be reported as non-configurable, if it does not exist{s}
as a{n} own property of the target"
- Step 18 / Comment 51: noted. Any specific reason why you changed this?
To anyone following along, quick summary: if the "getOwnPropertyDescriptor"
trap returns an incomplete property descriptor, the descriptor will be
completed based on the values from the target property descriptor, rather
than the default attribute values from Table 7 (unless the property doesn't
exist on the target).
- Step 22 / Comment [52]: I noticed the changes in 8.2.5.4
FromPropertyDescriptor(Desc) and 8.2.5.5 ToPropertyDescriptor, which use a
separate [[Origin]] field to avoid unnecessary conversions.
While this approach does preserve custom property attributes without
copying, it also circumvents the "normalization" process by which the
return value of the trap got converted into a "normal" property descriptor.
This means that the return value of Object.getOwnPropertyDescriptor can now
be any object, including objects that define properties like "writable" and
"configurable" as accessors that can modify from one moment to the next.
This puts the burden of normalizing property descriptors on clients of
Object.getOwnPropertyDescriptor. Hence, this change is a potential security
issue for extant ES5.1 code that relies on Object.getOwnPropertyDescriptor
performing the normalization.
- Typo: "NOTE [[GetOwnPro{p}erty] for proxy objects enforces the following
invariants"
8.5.7 Proxy [[DefineOwnProperty]]
- Step 7-8: as for [[GetOwnProperty]], this is an important change from the
wiki spec, as it means the defineProperty trap will get the original
descriptor object passed into Object.defineProperty as argument, without
normalization. That means it's now up to the handler to manually normalize
the descriptor object.
This is less of an issue than the symmetrical case for [[GetOwnProperty]],
because handlers are new in ES6, so there's no breaking change compared to
ES5.1. It is a breaking change compared to existing Proxy prototype
implementations. That's not a deal breaker, but I just want to point it out
explicitly since it may impact existing code.
- Typo: "NOTE [[{DefineOwnProp}erty]] for proxy objects enforces the
following invariants:"
8.5.10 Proxy [[SetP]]
- Typo: NOTE, first invariant: "Cannnot change the value of a property to
be different from the value of the corres{o}ponding…"
8.5.12 Proxy [[Enumerate]]
I think it may be possible to waive the extra invariant checks for
[[Enumerate]]. It's not a crucial primitive.
My reasoning is that [[Enumerate]] deals with both own and inherited
properties, and we don't really enforce any invariants on inherited
properties. So I guess it's ok if the invariants for [[Enumerate]] are
weakened.
Do note that this is a bit inconsistent with the way we treat internal
methods like [[HasProperty]], [[GetP]] and [[SetP]]: these also deal with
own and inherited properties, but still enforce invariants on own
properties.
8.5.13 Proxy [[OwnPropertyKeys]]
- For [[OwnPropertyKeys]], I maintain that it's essential that this
iterator at least enumerates all non-configurable own properties of the
target.
Otherwise, a proxy might "hide" properties from reflective code that tries
to inspect all of its properties.
Ideally, the iterator should also not enumerate any non-existent properties
on a non-extensible target. Although if this invariant is violated, the
invariants defined on getOwnPropertyDescriptor etc. should prevent the
proxy from revealing any useful value associated with these "made-up"
properties.
- As "ownPropertyKeys" returns an iterator rather than an array of strings,
it's no longer symmetric to getOwnPropertyNames anyway, so I agree with the
name change. However, to ensure consistency of user-facing names, either
the trap should be named "ownKeys" (for consistency with Reflect.ownKeys),
or Reflect.ownKeys should be renamed Reflect.ownPropertyKeys.
8.5.14 Proxy [[Freeze]]
8.5.15 Proxy [[Seal]]
8.5.16 Proxy [[IsFrozen]]
8.5.17 Proxy [[IsSealed]]
I think I'm ready to give up on all of these (and so drop these methods
from the internal MOP and the Reflect module altogether).
(although I don't agree with your Comment[56-57-58] that keeping them would
entail a lot of additional invariant checks: the checks are still there
even without these internal methods, they're just performed implicitly by
the Object.freeze algorithm when it loops over all the properties and calls
[[DefineOwnProperty]] for each property)
8.5.19 Proxy [[Construct]]
Step 7, typo: "Return the result of calling{v} trap with handler…"
=== Chapter 9 ===
9.3.9 MakeObjectSecure
9.3.10 TestIfSecureObject
I would prefer a name that doesn't feature the word "Secure" (I'm sure Mark
would too :)
Mark and I have previously used the term "tamper-proof" to abstract over
preventExtensions/seal/freeze (so: MakeTamperProof/IsTamperProof). Or, in
the earlier Proxy API there was the "fix" trap (so perhaps: Fix/IsFixed)
=== Chapter 15 ===
15.2.3.8 Object.seal
15.2.3.9 Object.freeze
15.2.3.10 Object.preventExtensions
In all of these algorithms, if status is false, that should probably be
turned into an explicit TypeError. This is consistent with the design of
[[DefineOwnProperty]].
Neither of these operations should ever fail silently.
15.3.4.2 Function.prototype.toString
Perhaps this is still on your TODO list, but I would advise to specify what
this method should do on a Proxy. My suggested semantics:
"When called on a Proxy that is callable (i.e. whose [[ProxyTarget]] has a
[[Call]] internal method), return the result of applying the built-in
Function.prototype.toString on the Proxy’s [[ProxyTarget]]. Otherwise,
throw a TypeError. When applied to a revoked proxy, throw a TypeError."
15.17 The Reflect Module
In looking at the spec now, I realize the following inconsistency:
On the wiki, I specced all of the Reflect.* methods to perform a ToObject
conversion on the target argument, but the corresponding Object.* methods
instead perform an explicit typecheck and throw if the target is not an
object.
I think it makes sense to adopt the Object.* behavior and change all
ToObject conversions into explicit type checks, because
a) it's more consistent
b) trying to call e.g. Reflect.preventExtensions on a primitive value is
most certainly a programming error (it doesn't make sense to prevent
extensions on a boxed primitive), and implicit conversions hide such errors.
15.17.1.5 Reflect.has
Typo: "When the has{Own} function…"
15.17.1.12 Reflect.enumerate
Steps 3 and 4 can be merged since itr is never used.
15.17.1.13 Reflect.ownKeys
Either rename to Reflect.ownPropertyKeys, or rename the "ownPropertyKeys"
trap to "ownKeys", to ensure consistency of user-facing names.
15.17.1.14,15,16,17: can be dropped if [[Freeze]] and friends are dropped.
15.18 Proxy Objects
I presume this section will host the Proxy and Proxy.revocable constructors.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/es-discuss/attachments/20121229/8dd4591b/attachment-0001.html>
I did a thorough revision of Allen's Rev12 and Rev13 drafts which incorporate (among many other things) the refactored MOP, symbols, proxies and the Reflect module in the ES6 draft.
Below is a long list of detailed comments, both editorial and more substantial in nature. It's too cumbersome for me to file bugs for every individual comment. Allen, please let me know what comments are worth filing a bug for.
Before delving into the details, two more things:
First, thank you Allen for the great spec work. Overall I'm very happy with the way proxies have been incorporated into the ES6 draft.
Second, if people want to comment on a particular issue, I suggest to rename the topic and fork off a separate thread. That will be more manageable, and will allow people that don't want to read through this whole list to follow the separate discussions.
Cheers, Tom
== Comments based on ES6 draft Rev. 13 (21 dec. 2012) ==
=== General remarks ===
[[GetP]]/[[SetP]] => rename these to just [[Get]] and [[Set]]?
[[GetInheritance]]/[[SetInheritance]] => why not [[GetProto]]/[[SetProto]]?
[[SetInheritance]]/Reflect.setPrototypeOf: I'm not sure this was agreed upon. Especially since proto is currently specified as a data property. This means there is no setter that separately reifies the ability to set the prototype. Thus, it's perfectly possible to just exclude [[SetInheritance]] and Reflect.setPrototypeOf from the spec.
I'm a bit uncomfortable with the removal of property descriptor normalization in the getOwnPropertyDescriptor/defineProperty traps. Especially for getOwnPropertyDescriptor I think it's a breaking change w.r.t. ES5.1.
=== Chapter 8 ===
8.1.6.2 "Unless explicitly specified otherwise, internal data properties may be dynamically added to ECMAScript objects." Isn't it safer to err on the side of safety and specify the opposite? (i.e. "properties cannot be dynamically added unless specified otherwise")?
Table 8:
8.3.4 Object [PreventExtensions] This method should return a Boolean (true).
8.3.7.3 ValidateAndApplyPropertyDescriptor Typo: "… and no{t} object updates are {pre}formed"
The note: "However, if O has an [[BuiltinBrand]] internal data property whose value is BuiltinArray O also has a more elaborate [[DefineOwnProperty]] internal method defined in 15.4.5.1." is probably more appropriately placed at the top of section 8.3.7 (closer to the definition of [[DefineOwnProperty]])
The note: "NOTE Step 10.b allows any field…" should probably refer to Step 10.a (there is no longer a step 10.b in the new ValidateAndApplyPropertyDescriptor algorithm)
8.7.8 Object [[HasProperty]] ( P )
8.3.19 Ordinary Function Objects Table 13: Description for [[Home]] "…this is the object whose [[Inheritance]] provides the object…" should probably be [[GetInheritance]].
8.4 Built-in Exotic Object Internal Methods and Data Fields
A few typos in introductory text: "This specification define{s} several kinds of built-in exotic objects. These objects generally behave similar to ordinary objects except for a few specific situ{t}ations. The following exotic objects use the ordinary object internal methods except where it is explicitly specified {other}wise below:"
8.4.4 Exotic Symbol Objects Typo: "A{n} Symbol object is an exotic object…" "Exotic String objects have {the} a..."
In the introduction, it's mentioned that "Symbol exotic objects are unique in that they are always immutable and never observably reference any other object."
Yet, as currently specified, evaluating aSymbol.toString yields a reference to the global Object.prototype.toString function (which is mutable by default).
Shouldn't aSymbol.toString just be undefined?
I notice that Object.prototype.toString special-cases Symbols anyway, so Object.prototype.toString.call(aSymbol) continues to work fine.
In case aSymbol.toString should continue to return Object.prototype.toString, I would advise to modify [[HasProperty]] for Symbols to answer 'true' for the string "toString", and [[Delete]] to answer 'false', so that [[Get]],[[HasProperty]] and [[Delete]] remain internally consistent.
8.4.5 Exotic Arguments Objects Typo: "They also have a [[Par{a}meterMap]] internal data property"
=== 8.5 Proxies ===
Second paragraph, last sentence, typo: "on the proxy’s target object i{f} the handler object does not have a method corresponding to the internal trap."
Third paragraph, typo: "Some proxy objects are created in a manner that permits them to be subsequent{ly} revoked"
Fourth paragraph: as written currently, it reads as if the invariants from Section 8.1.6.2 can effectively be violated by proxies. I would rephrase the first sentence to make it clearer that proxies have this potential, but this potential is not realized thanks to the invariant checks: "Because proxies permit arbitrary ECMAScript code to be used to implement internal methods, they have the potential to violate the invariants defined in 8.1.6.2."
8.5.1 Proxy [[GetInheritance]]
The same comment applies to the NOTE in 8.5.2 and 8.5.3.
8.5.2 Proxy [[SetInheritance]]
8.5.4 Proxy [[PreventExtensions]]
This is also the first and only place where a single intercepted operation ("preventExtensions") gives rise to two trap invocations on the handler ("preventExtensions" and "isExtensible")
The only important invariant for [[PreventExtensions]] is that, if the "preventExtensions" trap returns true, then the [[ProxyTarget]] must be non-extensible. There's no need to invoke the extra "isExtensible" trap to verify this.
8.5.5 Proxy [[HasOwnProperty]]
8.5.6 Proxy [[GetOwnProperty]]
The invariant check made in step 11 of the [[GetOwnProperty]] algorithm as specified on the wiki has been dropped. Is that because it is subsumed by a similar test made in IsCompatiblePropertyDescriptor? (I think it is correct to drop the redundant check, I just want to make sure this change was intentional.)
Step 13.a of [[GetOwnProperty]] on the wiki tests: "If targetDesc is undefined or targetDesc.[[Configurable]] is true, throw a TypeError" In the ES6Rev13 algorithm, step 21.a: "If targetDesc is not undefined and targetDesc.[[Configurable]] is true, then Throw a TypeError exception."
I think this misses the case where resultDesc.[[Configurable]] is false and targetDesc is undefined: this should result in a TypeError per the wiki spec, but will work fine per the ES6Rev13 draft.
This test is to uphold the 5th invariant listed in the NOTE: "A property cannot be reported as non-configurable, if it does not exist{s} as a{n} own property of the target"
Step 18 / Comment 51: noted. Any specific reason why you changed this? To anyone following along, quick summary: if the "getOwnPropertyDescriptor" trap returns an incomplete property descriptor, the descriptor will be completed based on the values from the target property descriptor, rather than the default attribute values from Table 7 (unless the property doesn't exist on the target).
Step 22 / Comment [52]: I noticed the changes in 8.2.5.4 FromPropertyDescriptor(Desc) and 8.2.5.5 ToPropertyDescriptor, which use a separate [[Origin]] field to avoid unnecessary conversions.
While this approach does preserve custom property attributes without copying, it also circumvents the "normalization" process by which the return value of the trap got converted into a "normal" property descriptor. This means that the return value of Object.getOwnPropertyDescriptor can now be any object, including objects that define properties like "writable" and "configurable" as accessors that can modify from one moment to the next.
This puts the burden of normalizing property descriptors on clients of Object.getOwnPropertyDescriptor. Hence, this change is a potential security issue for extant ES5.1 code that relies on Object.getOwnPropertyDescriptor performing the normalization.
8.5.7 Proxy [[DefineOwnProperty]]
This is less of an issue than the symmetrical case for [[GetOwnProperty]], because handlers are new in ES6, so there's no breaking change compared to ES5.1. It is a breaking change compared to existing Proxy prototype implementations. That's not a deal breaker, but I just want to point it out explicitly since it may impact existing code.
8.5.10 Proxy [[SetP]]
8.5.12 Proxy [[Enumerate]]
I think it may be possible to waive the extra invariant checks for [[Enumerate]]. It's not a crucial primitive.
My reasoning is that [[Enumerate]] deals with both own and inherited properties, and we don't really enforce any invariants on inherited properties. So I guess it's ok if the invariants for [[Enumerate]] are weakened.
Do note that this is a bit inconsistent with the way we treat internal methods like [[HasProperty]], [[GetP]] and [[SetP]]: these also deal with own and inherited properties, but still enforce invariants on own properties.
8.5.13 Proxy [[OwnPropertyKeys]]
Otherwise, a proxy might "hide" properties from reflective code that tries to inspect all of its properties.
Ideally, the iterator should also not enumerate any non-existent properties on a non-extensible target. Although if this invariant is violated, the invariants defined on getOwnPropertyDescriptor etc. should prevent the proxy from revealing any useful value associated with these "made-up" properties.
8.5.14 Proxy [[Freeze]] 8.5.15 Proxy [[Seal]] 8.5.16 Proxy [[IsFrozen]] 8.5.17 Proxy [[IsSealed]]
I think I'm ready to give up on all of these (and so drop these methods from the internal MOP and the Reflect module altogether).
(although I don't agree with your Comment[56-57-58] that keeping them would entail a lot of additional invariant checks: the checks are still there even without these internal methods, they're just performed implicitly by the Object.freeze algorithm when it loops over all the properties and calls [[DefineOwnProperty]] for each property)
8.5.19 Proxy [[Construct]] Step 7, typo: "Return the result of calling{v} trap with handler…"
=== Chapter 9 ===
9.3.9 MakeObjectSecure 9.3.10 TestIfSecureObject I would prefer a name that doesn't feature the word "Secure" (I'm sure Mark would too :)
Mark and I have previously used the term "tamper-proof" to abstract over preventExtensions/seal/freeze (so: MakeTamperProof/IsTamperProof). Or, in the earlier Proxy API there was the "fix" trap (so perhaps: Fix/IsFixed)
=== Chapter 15 ===
15.2.3.8 Object.seal 15.2.3.9 Object.freeze 15.2.3.10 Object.preventExtensions In all of these algorithms, if status is false, that should probably be turned into an explicit TypeError. This is consistent with the design of [[DefineOwnProperty]].
Neither of these operations should ever fail silently.
15.3.4.2 Function.prototype.toString
Perhaps this is still on your TODO list, but I would advise to specify what this method should do on a Proxy. My suggested semantics:
"When called on a Proxy that is callable (i.e. whose [[ProxyTarget]] has a [[Call]] internal method), return the result of applying the built-in Function.prototype.toString on the Proxy’s [[ProxyTarget]]. Otherwise, throw a TypeError. When applied to a revoked proxy, throw a TypeError."
15.17 The Reflect Module
In looking at the spec now, I realize the following inconsistency: On the wiki, I specced all of the Reflect.* methods to perform a ToObject conversion on the target argument, but the corresponding Object.* methods instead perform an explicit typecheck and throw if the target is not an object.
I think it makes sense to adopt the Object.* behavior and change all ToObject conversions into explicit type checks, because a) it's more consistent b) trying to call e.g. Reflect.preventExtensions on a primitive value is most certainly a programming error (it doesn't make sense to prevent extensions on a boxed primitive), and implicit conversions hide such errors.
15.17.1.5 Reflect.has Typo: "When the has{Own} function…"
15.17.1.12 Reflect.enumerate Steps 3 and 4 can be merged since itr is never used.
15.17.1.13 Reflect.ownKeys Either rename to Reflect.ownPropertyKeys, or rename the "ownPropertyKeys" trap to "ownKeys", to ensure consistency of user-facing names.
15.17.1.14,15,16,17: can be dropped if [[Freeze]] and friends are dropped.
15.18 Proxy Objects I presume this section will host the Proxy and Proxy.revocable constructors.
Hi, I did a thorough revision of Allen's Rev12 and Rev13 drafts which incorporate (among many other things) the refactored MOP, symbols, proxies and the Reflect module in the ES6 draft. Below is a long list of detailed comments, both editorial and more substantial in nature. It's too cumbersome for me to file bugs for every individual comment. Allen, please let me know what comments are worth filing a bug for. Before delving into the details, two more things: First, thank you Allen for the great spec work. Overall I'm very happy with the way proxies have been incorporated into the ES6 draft. Second, if people want to comment on a particular issue, I suggest to rename the topic and fork off a separate thread. That will be more manageable, and will allow people that don't want to read through this whole list to follow the separate discussions. Cheers, Tom == Comments based on ES6 draft Rev. 13 (21 dec. 2012) == === General remarks === * [[GetP]]/[[SetP]] => rename these to just [[Get]] and [[Set]]? * [[GetInheritance]]/[[SetInheritance]] => why not [[GetProto]]/[[SetProto]]? - More familiar to ECMAScript programmers - No risk of confusion with function "prototype" property - For ordinary objects, [[GetInheritance]] returns the value of the [[Prototype]] field * [[SetInheritance]]/Reflect.setPrototypeOf: I'm not sure this was agreed upon. Especially since __proto__ is currently specified as a data property. This means there is no setter that separately reifies the ability to set the prototype. Thus, it's perfectly possible to just exclude [[SetInheritance]] and Reflect.setPrototypeOf from the spec. * I'm a bit uncomfortable with the removal of property descriptor normalization in the getOwnPropertyDescriptor/defineProperty traps. Especially for getOwnPropertyDescriptor I think it's a breaking change w.r.t. ES5.1. === Chapter 8 === 8.1.6.2 "Unless explicitly specified otherwise, internal data properties may be dynamically added to ECMAScript objects." Isn't it safer to err on the side of safety and specify the opposite? (i.e. "properties cannot be dynamically added unless specified otherwise")? Table 8: * [[PreventExtensions]] internal method: should return a Boolean success value * [[Delete]]: I would remove "because its [[Configurable]] attribute is false." from the description. Proxies and host objects may return false for other reasons as well (cf. the recent discussions about DontDelete vs. configurable:false) * [[Enumerate]]: typo: "Returns an iterator object that {iterates} over the string values" * [[Keys]] should be removed * [[HasProperty]] should probably be added to this table again * [[OwnPropertyKeys]]: the return type should be changed to Object (iterator) 8.3.4 Object [[PreventExtensions]]() This method should return a Boolean (true). 8.3.7.3 ValidateAndApplyPropertyDescriptor Typo: "… and no{t} object updates are {pre}formed" The note: "However, if O has an [[BuiltinBrand]] internal data property whose value is BuiltinArray O also has a more elaborate [[DefineOwnProperty]] internal method defined in 15.4.5.1." is probably more appropriately placed at the top of section 8.3.7 (closer to the definition of [[DefineOwnProperty]]) The note: "NOTE Step 10.b allows any field…" should probably refer to Step 10.a (there is no longer a step 10.b in the new ValidateAndApplyPropertyDescriptor algorithm) 8.7.8 Object [[HasProperty]] ( P ) - The section number should probably be 8.3.8 "When the [[GetProperty]] internal method" => should probably be [[HasProperty]]. 8.3.19 Ordinary Function Objects Table 13: Description for [[Home]] "…this is the object whose [[Inheritance]] provides the object…" should probably be [[GetInheritance]]. 8.4 Built-in Exotic Object Internal Methods and Data Fields A few typos in introductory text: "This specification define{s} several kinds of built-in exotic objects. These objects generally behave similar to ordinary objects except for a few specific situ{t}ations. The following exotic objects use the ordinary object internal methods except where it is explicitly specified {other}wise below:" 8.4.4 Exotic Symbol Objects Typo: "A{n} Symbol object is an exotic object…" "Exotic String objects have {the} a..." In the introduction, it's mentioned that "Symbol exotic objects are unique in that they are always immutable and never observably reference any other object." Yet, as currently specified, evaluating aSymbol.toString yields a reference to the global Object.prototype.toString function (which is mutable by default). Shouldn't aSymbol.toString just be undefined? I notice that Object.prototype.toString special-cases Symbols anyway, so Object.prototype.toString.call(aSymbol) continues to work fine. In case aSymbol.toString should continue to return Object.prototype.toString, I would advise to modify [[HasProperty]] for Symbols to answer 'true' for the string "toString", and [[Delete]] to answer 'false', so that [[Get]],[[HasProperty]] and [[Delete]] remain internally consistent. 8.4.5 Exotic Arguments Objects Typo: "They also have a [[Par{a}meterMap]] internal data property" === 8.5 Proxies === - Introduction: "Every proxy object also has an internal data property called [[ProxyTarget]] whose value is usually an object." The word "usually" is unnecessarily vague. It would be more precise to say: "whose value is always an object for an unrevoked proxy, or null for a revoked proxy." Second paragraph, last sentence, typo: "on the proxy’s target object i{f} the handler object does not have a method corresponding to the internal trap." Third paragraph, typo: "Some proxy objects are created in a manner that permits them to be subsequent{ly} revoked" Fourth paragraph: as written currently, it reads as if the invariants from Section 8.1.6.2 can effectively be violated by proxies. I would rephrase the first sentence to make it clearer that proxies have this potential, but this potential is not realized thanks to the invariant checks: "Because proxies permit arbitrary ECMAScript code to be used to implement internal methods, they have the potential to violate the invariants defined in 8.1.6.2." 8.5.1 Proxy [[GetInheritance]] - In the NOTE at the bottom: "[[GetInheritance] applied to the proxy object must return the same value as [[GetInheritance] applied to the proxy object’s handler object." => should be "… applied to the proxy object's target object." (also, [[GetInheritance] should have two closing brackets) The same comment applies to the NOTE in 8.5.2 and 8.5.3. 8.5.2 Proxy [[SetInheritance]] - Reply to Comment [50]: the invariant on [[GetInheritance]] is needed because otherwise proxies can emulate mutable prototype chains even when UnderScoreProtoEnabled is false. 8.5.4 Proxy [[PreventExtensions]] - The steps starting from step 8 are new to me. I don't understand why you changed the invariant checks here. This is also the first and only place where a single intercepted operation ("preventExtensions") gives rise to two trap invocations on the handler ("preventExtensions" and "isExtensible") The only important invariant for [[PreventExtensions]] is that, if the "preventExtensions" trap returns true, then the [[ProxyTarget]] must be non-extensible. There's no need to invoke the extra "isExtensible" trap to verify this. - In step 17, [[PreventExtensions]] currently returns no value. It's more useful to return the boolean "proxyIsExtensible" success value (like [[DefineOwnProperty]] and [[Delete]]), and have user-facing methods that call [[PreventExtensions]] convert a false return value into a thrown exception (see below for my related comments on Object.preventExtensions). 8.5.5 Proxy [[HasOwnProperty]] - Typo: "NOTE [[HasOwnPro{p}erty] for proxy objects enforces the following invariants" 8.5.6 Proxy [[GetOwnProperty]] - The invariant check made in step 11 of the [[GetOwnProperty]] algorithm as specified on the wiki has been dropped. Is that because it is subsumed by a similar test made in IsCompatiblePropertyDescriptor? (I think it is correct to drop the redundant check, I just want to make sure this change was intentional.) - Step 13.a of [[GetOwnProperty]] on the wiki tests: "If targetDesc is undefined or targetDesc.[[Configurable]] is true, throw a TypeError" In the ES6Rev13 algorithm, step 21.a: "If targetDesc is not undefined and targetDesc.[[Configurable]] is true, then Throw a TypeError exception." I think this misses the case where resultDesc.[[Configurable]] is false and targetDesc is undefined: this should result in a TypeError per the wiki spec, but will work fine per the ES6Rev13 draft. This test is to uphold the 5th invariant listed in the NOTE: "A property cannot be reported as non-configurable, if it does not exist{s} as a{n} own property of the target" - Step 18 / Comment 51: noted. Any specific reason why you changed this? To anyone following along, quick summary: if the "getOwnPropertyDescriptor" trap returns an incomplete property descriptor, the descriptor will be completed based on the values from the target property descriptor, rather than the default attribute values from Table 7 (unless the property doesn't exist on the target). - Step 22 / Comment [52]: I noticed the changes in 8.2.5.4 FromPropertyDescriptor(Desc) and 8.2.5.5 ToPropertyDescriptor, which use a separate [[Origin]] field to avoid unnecessary conversions. While this approach does preserve custom property attributes without copying, it also circumvents the "normalization" process by which the return value of the trap got converted into a "normal" property descriptor. This means that the return value of Object.getOwnPropertyDescriptor can now be any object, including objects that define properties like "writable" and "configurable" as accessors that can modify from one moment to the next. This puts the burden of normalizing property descriptors on clients of Object.getOwnPropertyDescriptor. Hence, this change is a potential security issue for extant ES5.1 code that relies on Object.getOwnPropertyDescriptor performing the normalization. - Typo: "NOTE [[GetOwnPro{p}erty] for proxy objects enforces the following invariants" 8.5.7 Proxy [[DefineOwnProperty]] - Step 7-8: as for [[GetOwnProperty]], this is an important change from the wiki spec, as it means the defineProperty trap will get the original descriptor object passed into Object.defineProperty as argument, without normalization. That means it's now up to the handler to manually normalize the descriptor object. This is less of an issue than the symmetrical case for [[GetOwnProperty]], because handlers are new in ES6, so there's no breaking change compared to ES5.1. It is a breaking change compared to existing Proxy prototype implementations. That's not a deal breaker, but I just want to point it out explicitly since it may impact existing code. - Typo: "NOTE [[{DefineOwnProp}erty]] for proxy objects enforces the following invariants:" 8.5.10 Proxy [[SetP]] - Typo: NOTE, first invariant: "Cannnot change the value of a property to be different from the value of the corres{o}ponding…" 8.5.12 Proxy [[Enumerate]] I think it may be possible to waive the extra invariant checks for [[Enumerate]]. It's not a crucial primitive. My reasoning is that [[Enumerate]] deals with both own and inherited properties, and we don't really enforce any invariants on inherited properties. So I guess it's ok if the invariants for [[Enumerate]] are weakened. Do note that this is a bit inconsistent with the way we treat internal methods like [[HasProperty]], [[GetP]] and [[SetP]]: these also deal with own and inherited properties, but still enforce invariants on own properties. 8.5.13 Proxy [[OwnPropertyKeys]] - For [[OwnPropertyKeys]], I maintain that it's essential that this iterator at least enumerates all non-configurable own properties of the target. Otherwise, a proxy might "hide" properties from reflective code that tries to inspect all of its properties. Ideally, the iterator should also not enumerate any non-existent properties on a non-extensible target. Although if this invariant is violated, the invariants defined on getOwnPropertyDescriptor etc. should prevent the proxy from revealing any useful value associated with these "made-up" properties. - As "ownPropertyKeys" returns an iterator rather than an array of strings, it's no longer symmetric to getOwnPropertyNames anyway, so I agree with the name change. However, to ensure consistency of user-facing names, either the trap should be named "ownKeys" (for consistency with Reflect.ownKeys), or Reflect.ownKeys should be renamed Reflect.ownPropertyKeys. 8.5.14 Proxy [[Freeze]] 8.5.15 Proxy [[Seal]] 8.5.16 Proxy [[IsFrozen]] 8.5.17 Proxy [[IsSealed]] I think I'm ready to give up on all of these (and so drop these methods from the internal MOP and the Reflect module altogether). (although I don't agree with your Comment[56-57-58] that keeping them would entail a lot of additional invariant checks: the checks are still there even without these internal methods, they're just performed implicitly by the Object.freeze algorithm when it loops over all the properties and calls [[DefineOwnProperty]] for each property) 8.5.19 Proxy [[Construct]] Step 7, typo: "Return the result of calling{v} trap with handler…" === Chapter 9 === 9.3.9 MakeObjectSecure 9.3.10 TestIfSecureObject I would prefer a name that doesn't feature the word "Secure" (I'm sure Mark would too :) Mark and I have previously used the term "tamper-proof" to abstract over preventExtensions/seal/freeze (so: MakeTamperProof/IsTamperProof). Or, in the earlier Proxy API there was the "fix" trap (so perhaps: Fix/IsFixed) === Chapter 15 === 15.2.3.8 Object.seal 15.2.3.9 Object.freeze 15.2.3.10 Object.preventExtensions In all of these algorithms, if status is false, that should probably be turned into an explicit TypeError. This is consistent with the design of [[DefineOwnProperty]]. Neither of these operations should ever fail silently. 15.3.4.2 Function.prototype.toString Perhaps this is still on your TODO list, but I would advise to specify what this method should do on a Proxy. My suggested semantics: "When called on a Proxy that is callable (i.e. whose [[ProxyTarget]] has a [[Call]] internal method), return the result of applying the built-in Function.prototype.toString on the Proxy’s [[ProxyTarget]]. Otherwise, throw a TypeError. When applied to a revoked proxy, throw a TypeError." 15.17 The Reflect Module In looking at the spec now, I realize the following inconsistency: On the wiki, I specced all of the Reflect.* methods to perform a ToObject conversion on the target argument, but the corresponding Object.* methods instead perform an explicit typecheck and throw if the target is not an object. I think it makes sense to adopt the Object.* behavior and change all ToObject conversions into explicit type checks, because a) it's more consistent b) trying to call e.g. Reflect.preventExtensions on a primitive value is most certainly a programming error (it doesn't make sense to prevent extensions on a boxed primitive), and implicit conversions hide such errors. 15.17.1.5 Reflect.has Typo: "When the has{Own} function…" 15.17.1.12 Reflect.enumerate Steps 3 and 4 can be merged since itr is never used. 15.17.1.13 Reflect.ownKeys Either rename to Reflect.ownPropertyKeys, or rename the "ownPropertyKeys" trap to "ownKeys", to ensure consistency of user-facing names. 15.17.1.14,15,16,17: can be dropped if [[Freeze]] and friends are dropped. 15.18 Proxy Objects I presume this section will host the Proxy and Proxy.revocable constructors. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.mozilla.org/pipermail/es-discuss/attachments/20121229/8dd4591b/attachment-0001.html>