super.prop assignment can silently overwrite non-writable properties
On Apr 20, 2015, at 9:38 AM, Jason Orendorff wrote:
We're implementing
superin Firefox, and ran across this surprising behavior:class X { constructor() { Object.defineProperty(this, "prop", { configurable: true, writable: false, value: 1}); } f() { super.prop = 2; // overwrites non-writable property! } }
var x = new X(); x.f(); assertEq(x.prop, 2); // passes
In the spec, 9.1.9 step 4.d.i. is where
super.prop = 2ends up, with O=X.prototype.
4.d.1 doesn't set the property, it just comes up with the property descriptor to use, if the Receiver does not already have a corresponding own property.
5.c+5.e checks if the corresponding own property actually exists on the Receiver. If it already exits then it does a [[DefineOwnProperty]] that only specifies the value attribute. This should respect the current writable attribute of the property and hence reject the attempt to change the value.
So, sounds like a FF bug to me. Of course, there might also be a problem in the spec. that I'm blind to.
V8 has implemented the same behavior that Jason has reported.
If it's determined that this behaviour is undesirable, implementations could agree to do something like:
14.5.1 Static Semantics: Early Errors
(extend with...)
ClassTail : ClassHeritage_opt { ClassBody }
ClassBody : ClassElementList
ClassElementList : ClassElement
ClassElement : MethodDefinition
ClassElement : static MethodDefinition
- It is a Syntax Error if ClassHeritage is not present and HasSuperProperty of MethodDefinition is true.
Static Semantics: HasSuperProperty
MethodDefinition : PropertyName ( StrictFormalParameters ) { FunctionBody }
1. If StrictFormalParameters Contains SuperProperty is true, return true.
2. Return FunctionBody Contains SuperProperty.
MethodDefinition : get PropertyName ( ) { FunctionBody }
1. Return FunctionBody Contains SuperProperty.
MethodDefinition : set PropertyName ( PropertySetParameterList ) {
FunctionBody }
1. If PropertySetParameterList Contains SuperProperty is true, return true.
2. Return FunctionBody Contains SuperProperty.
Which can then be added to ES2016. Note that the above is adapted from HasDirectSuper.
Jason Orendorff schrieb:
[…]
super.prop = 2ends up with O=X.prototype.
Uh, yes. And it should set X.prototype.prop to 2, because that still
is writable and not affected by the attributes of x.prop. So I would
expect x.prop to still have the value 1, shadowing the prototype
property.
(at least that's how I understood super references)
Bergi
On Apr 20, 2015, at 11:11 AM, Rick Waldron wrote:
V8 has implemented the same behavior that Jason has reported.
If it's determined that this behaviour is undesirable, implementations could agree to do something like:
What!? The behavior Jason described looks like it is contrary to the specification. Implementations that are not following the spec. have a bug. Also, since apparently at least two implementations have this bug, it sounds like we really need to have test262 tests for the specified behavior.
FWIW, I believe that once upon the time the spec. may have produced the results Jason is observing. But that behavior was pretty clearly wrong, as a [[Set]] operations should not be ignoring the writability of the property whose value it is modifying.
- It is a Syntax Error if ClassHeritage is not present and HasSuperProperty of MethodDefinition is true.
This is nothing to do with class definition syntax. This is about the semantics of the [[Set]] internal method.
Uh, yes. And it should set
X.prototype.propto2, because that still is writable and not affected by the attributes ofx.prop. So I would expectx.propto still have the value1, shadowing the prototype property.
I made this mistake as well, jorendorff@ pointed out that SuperProperty references use “this” as the receiver for [[Set]], so it will actually overwrite the value rather than setting it on X.prototype.
In the example provided, 5.e.ii is reached, and OrdinaryDefineOwnProperty() should return false and throw. This appears to be V8’s behaviour when testing in Canary (44.0.2375.0)
On Mon, Apr 20, 2015 at 2:31 PM Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:
This is nothing to do with class definition syntax. This is about the semantics of the [[Set]] internal method.
I understand that, but it struck me as very strange that class with no class heritage could be allowed to have super.foo(), super.foo=2, but not super(). Did I miss something obvious? What is wrote is certainly heavy handed, but not more so than what the spec already does with HasDirectSuper when ClassHeritage is not present. Having just written many tests* for the early errors that will result when HasDirectSuper returns true, it seemed subjectively strange to me that this syntax would even be allowed. I suspect I'm missing something that explains why SuperProperty is allowed where SuperCall is not.
- tc39/test262/pull/224/files#diff-f4c5a205149ca3f002d9f865b1465f15 (and following files)
On Mon, Apr 20, 2015 at 12:44 PM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:
4.d.1 doesn't set the property, it just comes up with the property descriptor to use, if the
Receiverdoes not already have a corresponding own property.5.c+5.e checks if the corresponding own property actually exists on the
Receiver.If it already exits then it does a [[DefineOwnProperty]] that only specifies the
valueattribute. This should respect the currentwritableattribute of the property and hence reject the attempt to change the value.
I agree with all of this, except I don't see where the attempt is rejected. Since the property is configurable, I think [[DefineOwnProperty]] succeeds.
The property is still non-writable afterwards. Only the value changes.
So this isn't breaking the object invariants: the property in question is configurable, so it's OK (I guess) to change the value. It's just surprising for assignment syntax to succeed in doing it.
Oh — he’s right, ValidateAndApplyPropertyDescriptor won’t throw in the example case, because the old descriptor is configurable. That’s kind of weird.
On Mon, Apr 20, 2015 at 2:42 PM, Caitlin Potter <caitpotter88 at gmail.com> wrote:
Oh — he’s right, ValidateAndApplyPropertyDescriptor won’t throw in the example case, because the old descriptor is configurable. That’s kind of weird.
Yes, that's it. 9.1.6.3 step 8.a says that writability is checked only if the existing property is non-configurable.
On Apr 20, 2015 12:42 PM, Caitlin Potter <caitpotter88 at gmail.com> wrote:
Oh — he’s right, ValidateAndApplyPropertyDescriptor won’t throw in the example case, because the old descriptor is configurable. That’s kind of weird
Hmm...I'm not at my machine right now. When I am I'll to make sure thus isn't a regression from ES5
On Apr 20, 2015, at 12:42 PM, Caitlin Potter wrote:
Oh — he’s right, ValidateAndApplyPropertyDescriptor won’t throw in the example case, because the old descriptor is configurable. That’s kind of weird.
It is kind of weird, but that was what TC39 decided on back when ES5 was being developed. The logic was that if a property is configurable then it is possible to change all of its attributes by performing a [[DefineOwnProperty]] with a complete property description. Because of that possibility, all changes made via a partial property descriptor are also accepted. In other words:
var o = Object.create(null, {x:{value: 0, writable: false, enumerable: true, configurable:true}});
Object.defineProperty(o,' x', {value:2});
console.log(o.x); //2
The define property above is allowed because it could have been replaced with the sequence :
Object.defineProperty(o,'x', {writable: true});
Object.defineProperty(o,'x', {value: 2, writable: false});
or even by:
delete o.x;
Object.defineProperty(o,'x', {value: 2, writable: false, enumerable: true, configurable: true};)
hence, we might as well accept the single line version.
In retrospect, perhaps not such a good idea.
It makes perfect sense for Object.defineProperty, but maybe not so much sense for PutValue(). One idea was to just add an return false if existingDescriptor.[[Writable]] is false. Before receiver.[[DefineOwnProperty]]().
On Apr 20, 2015, at 5:28 PM, Caitlin Potter wrote:
It makes perfect sense for Object.defineProperty, but maybe not so much sense for PutValue(). One idea was to just add an
return false if existingDescriptor.[[Writable]] is false.Before receiver.[DefineOwnProperty]`.
yes, something like that. I'm working on the fix right now. But it's probably more complicated then that. Consider what happens if the Receiver already has an like-named own accessor property...
I think in that case it needs to fail. Otherwise, the current algorithm will turn the accessor property into a data property, which seems even more bogus then the ignore writable behavior.
Yes. The problem is not that DefineOwnProperty is not acting more like assignment. The problem is that super.prop = x; is acting too much like DefineOwnProperty and too little like assignment.
On Apr 20, 2015, at 12:39 PM, Jason Orendorff wrote:
I agree with all of this, except I don't see where the attempt is rejected. Since the property is configurable, I think [[DefineOwnProperty]] succeeds.
The property is still non-writable afterwards. Only the value changes.
So this isn't breaking the object invariants: the property in question is configurable, so it's OK (I guess) to change the value. It's just surprising for assignment syntax to succeed in doing it.
I think it's bogus and needs to be corrected. Not only does it allow (in weird cases for [[Set]] (ie, assignment) to change the value of a non-writable property. It also means there are cases where [[Set]] will convert an accessor property to a data property.
In combination, I think this is a serious bug that needs to be fix in the final published ES6 spec. The fix I propose is in 9.1.9 to replace Set 5.e as follows:
5.e If existingDescriptor is not undefined, then
i. If IsAccessorDescript(existingDescript), return false.
ii. If existingDescriptor.[[Writable]] is false, return false.
iii. Let valueDesc be the PropertyDescriptor{[[Value]]: V}.
iv. Return Receiver.[[DefineOwnProperty]](P, valueDesc).
Lines 5.e.i and 5.e.ii are new additions.
Thoughts?
If the prop property accessed by super.prop is an accessor, super.prop = x; should invoke its setter. super.prop should invoke its getter.
It does. This is about what happens when that property is a data property doesn't exist. What happens when we do [[HomeObject]].[[GetPrototypeOf]]().[[Set]]('prop', x, this)
I don’t think the accessor case does work. ownDesc never refers to the property descriptor of the receiver when O[P] is a SuperReference, so if there’s an this.prop is an accessor, and super.prop doesn’t exist, the data descriptor path is taken.
On Apr 20, 2015, at 11:55 AM, Rick Waldron wrote:
I understand that, but it struck me as very strange that class with no class heritage could be allowed to have super.foo(), super.foo=2, but not super(). Did I miss something obvious? What is wrote is certainly heavy handed, but not more so than what the spec already does with HasDirectSuper when ClassHeritage is not present. Having just written many tests* for the early errors that will result when HasDirectSuper returns true, it seemed subjectively strange to me that this syntax would even be allowed. I suspect I'm missing something that explains why SuperProperty is allowed where SuperCall is not.
remember, super.x (LHS or RHS) is a completely different operations than super(). The former is semantically a [[Get]] or [[Set]] operation while the latter is a [[Construct]]. Completely different things that happen to both use the same keywords
'super.x' is allowed in any concise method, including those defined in object literals. super() is only allowed in class constructors of classes that include a ClassHeritage.
On Apr 20, 2015, at 6:52 PM, Caitlin Potter wrote:
I don’t think the accessor case does work.
ownDescnever refers to the property descriptor of the receiver when O[P] is a SuperReference, so if there’s anthis.propis an accessor, andsuper.propdoesn’t exist, the data descriptor path is taken.
ownDexc refers to the property descriptor of the [[Prototype]] in this case and if "super.prop is an accessor" that will be an accessor property descriptor. That falls throw steps 4 and 5 and eventually invokes its setter in step 9. The Receiver (the original this value only is involved as the this value passed in the call to the setter.
Right, good point. No need to care about existingDescriptor.[[Get]] or existingDescriptor.[[Set]]
5.e If *existingDescriptor* is not *undefined*, then i. If IsAccessorDescriptor(*existingDescriptor*), return *false*. ii. If *existingDescriptor*.[[Writable]] is *false*, return *false*. iii. Let *valueDesc* be the PropertyDescriptor{[[Value]]: *V*}. iv. Return *Receiver*.[[DefineOwnProperty]](*P*, *valueDesc*).Lines 5.e.i and 5.e.ii are new additions.
Looks good to me.
Thanks, Jason, for bringing this up!
See also ecmascript#3246#c3 I think the behavior where a data property can be installed accidentally under an accessor property is crazy, but looks like I was overruled.
FWIW, you can reproduce this test case without reference to the new super
syntax:
var parent = {};
var x = Object.create(parent, {
prop: { value: 1, configurable: true, writable: false }
});
Reflect.set(parent, "prop", 2, x); // expected false, but under current semantics will return true
However, I'm not sure the new step 5.e.i. is correct: why abort the [[Set]] when the existing descriptor is an accessor? If it has a setter, it seems to me the setter should be run. Testcase:
var parent = {};
var v = 1;
var x = Object.create(parent, {
prop: { get: function() { return v; }, set: function(n) { v = n; }, configurable: true }
});
Reflect.set(parent, "prop", 2, x); // under Allen's proposed changes, this will return false while I think it should just call the setter?
(You don't end up in 9.1.9 step 7 in the above case, because 4.d.i first sets ownDesc to a data descriptor. This falls into step 5. In step 5.c. the existingDescriptor will be the accessor. The new step 5.e.i then returns false rather than checking whether existingDescriptor.[[Set]] is callable)
var parent = {};
var v = 1;
var x = Object.create(parent, {
prop: { get: function() { return v; }, set: function(n) { v = n; }, configurable: true }
});
Reflect.set(parent, "prop", 2, x); // under Allen's proposed changes, this will return false while I think it should just call the setter?
Another interpretation might be that since you've explicitly stated an intention to bypass the receiver's setter, falling back to that setter would be a bug.
Maybe?
On Apr 21, 2015, at 12:31 PM, Tom Van Cutsem wrote:
However, I'm not sure the new step 5.e.i. is correct: why abort the [[Set]] when the existing descriptor is an accessor? If it has a setter, it seems to me the setter should be run. Testcase:
Yes, I considered that possibility in deciding upon the proposed change. The reason I error out if the Receiver property is an accessor is because I think the most likely way this scenario will occur is when that that access includes a super.prop assignment. That would result in an infinite [[Set]] recursion. For example:
var y = {
__proto__: x,
set prop(v) {
// ...
super.prop = v;
}
};
y.prop = 42;
2015-04-21 22:15 GMT+02:00 Allen Wirfs-Brock <allen at wirfs-brock.com>:
That would result in an infinite [[Set]] recursion. For example:
Assuming x here is the object where "prop" is bound to an accessor, can
you clarify how this would lead to an infinite recursion?
y.prop = 42; // 9.1.9 step 2 binds ownDesc to y's accessor, and calls it in step 9
super.prop = v; // 9.1.9 step 2 binds ownDesc to x's accessor, and calls it in step 9
x's setter then runs v = n; and the assignment terminates
I must be missing something?
2015-04-21 21:52 GMT+02:00 Kevin Smith <zenparsing at gmail.com>:
Another interpretation might be that since you've explicitly stated an intention to bypass the receiver's setter, falling back to that setter would be a bug.
No, that wouldn't be right. To clarify, Reflect.set takes the following
params: Reflect.set(lookupObject, propertyName, propertyValue, receiverObject)
It states that we want to start the lookup of propertyName in lookupObject, but eventually, the object to be updated is receiverObject. The reason for the split between lookupObject and receiverObject is so that Reflect.set (or the [[Set]] internal method, which mirrors it) can encode a prototype chain walk. For a regular property assignment, we always start at the receiverObject, that is, we initially call:
Reflect.set(receiverObject, propertyName, propertyValue, receiverObject)
As long as we don't find propertyName, we recurse:
Reflect.set(receiverObject.__proto__, propertyName, propertyValue, receiverObject)
Reflect.set(receiverObject.__proto__.__proto__, propertyName, propertyValue, receiverObject)
...
and so on until we hit the root.
The only reason why [[Set]] wants to climb the prototype chain in this way is to see if there are non-writable inherited properties. However, if the receiverObject has overridden that property (either with an accessor or a writable property), then the inherited property takes precedence.
In normal ES5 code, since lookup is guaranteed to start at receiverObject,
this precedence is implicitly guaranteed. In ES6, with both super.prop
syntax and Reflect.set, the programmer can explicitly start the lookup
higher up the prototype chain. The implicit precedence of the
receiverObject's property is now no longer guaranteed, because we've
skipped looking for the property in receiverObject first.
That's why I think in step 5.c and onward, we must now explicitly ensure that existingDescriptor takes precedence over the newly defined descriptor.
Assuming
xhere is the object where "prop" is bound to an accessor, can you clarify how this would lead to an infinite recursion?
Assume let x = {}; (i.e. no property definition for "prop" on x.)
"y" is the receiver, so if you fall back to calling the receiver's accessor somewhere in 5.e, you get infinite recursion.
Falling back to the receiver's accessor would be a bug in this case.
Thanks, got it.
We're implementing
superin Firefox, and ran across this surprising behavior:In the spec, 9.1.9 step 4.d.i. is where
super.prop = 2ends up, with O=X.prototype.Options:
Do nothing. Allow overwriting. It's a little weird, but this code is pretty weird.
(post-ES6 of course) Add more "code" in 4.d.i. to perform a [[GetOwnProperty]] and check.
We're implementing `super` in Firefox, and ran across this surprising behavior: class X { constructor() { Object.defineProperty(this, "prop", { configurable: true, writable: false, value: 1}); } f() { super.prop = 2; // overwrites non-writable property! } } var x = new X(); x.f(); assertEq(x.prop, 2); // passes In the spec, 9.1.9 step 4.d.i. is where `super.prop = 2` ends up, with O=X.prototype. Options: 1. Do nothing. Allow overwriting. It's a little weird, but this code is pretty weird. 2. (post-ES6 of course) Add more "code" in 4.d.i. to perform a [[GetOwnProperty]] and check. -j