tc39 / ecma262

Status, process, and documents for ECMA-262
https://tc39.es/ecma262/
Other
14.94k stars 1.27k forks source link

Odd behavior with [[Set]] on non-frozen objects inheriting from frozen objects #1154

Open dead-claudia opened 6 years ago

dead-claudia commented 6 years ago

I can't tell if this is a spec bug or feature request. But this came up just now on ES Discuss, and it seems incredibly odd that [[Set]] doesn't seem to take into account some properties on non-frozen objects are inherited from frozen ones. Is there a reason it's checking the frozenness of the inherited object, when it's only the base object that such a property would be set if not frozen?

// Adapted example
const proto = {foo: "foo", bar: "bar"}
const o = Object.create(proto)

o.foo = 1 // works
Object.freeze(proto)
o.bar = 2 // fails?
bakkot commented 6 years ago

A bit of detail: this happens for any non-writable attribute on the parent, not just for frozen objects; it's specified in step 3.a of OrdinarySetWithOwnDescriptor (which is called by OrdinarySet which is called by the [[Set]] internal method on ordinary objects, whose specifications immediately precede the linked section). And it does not happen with preventExtensions; that is, just because an object's prototype is not extensible does not mean the object is not extensible.

Note that this is also the behavior in ES5 (step 8.b), so I doubt we can change it.

I agree this seems weird at a glance, though it matches the other way of making a property unwritable (i.e., a getter with no setter). cc @erights, maybe you can clarify?

dead-claudia commented 6 years ago

@bakkot

I agree this seems weird at a glance, though it matches the other way of making a property unwritable (i.e., a getter with no setter).

An argument could be made here that a getter with no setter isn't a simple data property, but a pair of "proxy-ish" methods that each default to effectively this: "if this is called from strict mode code, throw a TypeError, else return undefined". That would group them together with exotic proxy prototypes, which are something I specifically don't propose changing (for obvious reasons).

allenwb commented 6 years ago

This behavior for [[Set]] was standardized by ES1 in 1997. Not something that can change.

trusktr commented 6 years ago

Makes sense we can't change that existing behavior. What about adding a new feature, maybe something called similar to Object.lock, where it works very similarly to Object.freeze (it is somewhat like a subset of the functionality of Object.freeze) where properties can not be modified (just like Object.freeze) but only on the object directly, and objects that inherit (who have the locked object as prototype) can still be modified.

// example
const proto = {foo: "foo", bar: "bar"}
const o = Object.create(proto)

o.foo = 1 // works
Object.lock(proto)
o.bar = 2 // works just fine

console.log(o)
// { foo: 1, bar: 2, __proto__: {foo: "foo", bar: "bar"} }

proto.foo = 3 // fails
trusktr commented 6 years ago

I suppose one way to achieve this currently is to wrap the prototype in a Proxy. But to do this on every prototype in a chain of every class, maybe that's a little heavy?

The downside of Proxy is that it requires knowing which objects already have the prototype, so that we can assign those objects new prototypes (the Proxy-wrapped prototypes). This isn't always possible when those objects are closed in other scopes. But with Object.lock, it would be possible to apply it to a prototype without having to know which objects inherit from the prototype, just like Object.freeze.

erights commented 6 years ago

See https://web.archive.org/web/20141230041441/http://wiki.ecmascript.org/doku.php?id=strawman:fixing_override_mistake

I disagree with @allenwb . Fixing this would make an error into a non-error. It is possible for code to rely on certain cases being an error; and turning errors into non-errors can break such code. But this is rare in general. I have never come across a pattern counting on this particular error. It remains plausible that this mistake fits into the more common case, where it is safe to turn the error into a non-error.

We should have fixed this in ES5 when it would have been less risky. But we should still fix this.

getify commented 6 years ago

Fixing this would make an error into a non-error.

Note that it's not an error (fails silently) in non-strict-mode. I would guess that it's potentially more risky to change silent failures into (unexpected) "successes" than it is to remove an actual error.


FWIW, when I researched this topic while writing one of my books, I recall discovering that the reasoning for [[Set]] (or [[Put]] I believe) to consult the prototype (in cases like writable: false, a setter, etc) even though the effect of a [[Set]]] / [[Put]] would only be end on the end object, was preserving the impression that a set of objects linked in a prototype chain were the dynamic equivalent of if that same set of objects had been mixed together into a single object (the way you'd observe an instance from a derived/child class behaving in a class-oriented language like Java/C++).

IOW, if you set a property on an object that ends up shadowing that same property name on an object higher in the prototype chain, the impression of those objects being all composed into one would mean that a writable: false or a setter being present for that property would have prevented you from doing so. So that's why [[Set]] / [[Put]] consults the prototype chain and respects those things.

Given that reasoning (which I don't like or agree with, but I can at least grok), I think it suggests that Object.freeze(..) (which sets writable:false on properties) being respected by an object further down the chain is consistent with that principle and not ostensibly "a bug".

erights commented 6 years ago

I disagree with that reasoning too. I have never seen code in the wild that used inheritance that way on purpose. Does anyone know of a single example, aside from code to test or illustrate this very principle?

The agreement or not on this principle is an argument we still need to have. We should still fix the override mistake.

getify commented 6 years ago

@erights Whether end-developers in JS have ever intentionally relied on this reasoning or not, it sure seems that from a language-design standpoint, JS has gone to a lot of trouble over the years to give the impression that its dynamic prototype system is -- or can be used as -- a traditional static class system. I know we're discussing prototypes and not class here, but they're inextricably linked (pun intended).

In such a class system (like Java or C++), if an instance inherits (from anywhere in the hierarchy) something like a setter or a read-only member (for whatever that would mean), I don't think it would be expected to be able to "override" that on the instance itself. Overrides can happen, but they happen in derived classes, not instances; moreover, in some languages, they can only happen if the originating class has marked something as overridable.

Whatever is inherited or overridden down through the class hierarchy, these systems give the strong impression that all of that stuff has been copied down into the instance. What you can do with the instance is directly controlled by what it inherited.

Since JS clearly wants to be seen as a language that has classes, I'm not sure why JS wouldn't want to preserve this precedent, the notion that the instance (the leaf node in the prototype chain) has "inherited" (sorta received a copy of) all the stuff from the class/prototype hierarchy.

erights commented 6 years ago

Freeze prevents changing what would be inherited. There needs to be a way to do that without altering the semantics of those inherited things. In your abstract story, it makes as much sense to want "This prototype immutably expresses that instances should have a mutable foo property" as "This prototype immutably expresses that instances should have an immutable foo property". The frozenness of the prototype property is about the changeability of what it contributes to the instance, and therefore cannot simultaneously be about the nature of that contribution.

getify commented 6 years ago

@erights

I agree with your reasoning here.

But I'd like to clarify something: setting aside "freezing" for a moment... if I just define a single property on a prototype, and I mark it as writable: false, does that give the impression that I want that singular contribution to the inheritance to be immutable, or does it give the impression that I want the contribution to be an immutable property?

I suspect you may favor the latter, as marking a single property read-only is probably not the same semantic as marking the entire prototype object as read-only.

If that's the case, then I think what's actually "broken" here is that Object.freeze(..) accomplished its immutability of the object with the same mechanism to define read-only'ness of a single property. In conflating those two semantics, the confusion arises.

As a matter of fact, Object.freeze(..) changing a property's configurable descriptor is also a troubling conflation in the same manner as writable.

The question I think then is: could Object.freeze(..) change, or could we introduce another operation like it that's different? The changed or new behavior should be: makes an object immutable -- "I want this object's contributions to the inheritance to be treated as an immutable set" -- but doesn't use writable: false or configurable: false to do it -- doesn't say one way or the other whether any individual property in that contribution is itself intended to be immutable once inherited.

The more I think about it, the more I think this "freezing" doesn't really seem like it should have been done with property descriptors at all, since it's really a statement about the whole set of contents of an object. It's like "freezing" should just be a dedicated flag/slot on an object, without modifying any of the property descriptors on that object. Any direct (not delegated) modification of an object itself would have to check that flag and abort if set.

getify commented 6 years ago

IOW, to the OP of this thread, a non-writable or non-configurable property continues to be "inherited" as such, as it has since ES1, but freezing an object shouldn't have any effect on whether a property is writable or configurable; that's a separate action.

dead-claudia commented 6 years ago

@getify

The more I think about it, the more I think this "freezing" doesn't really seem like it should have been done with property descriptors at all, since it's really a statement about the whole set of contents of an object. It's like "freezing" should just be a dedicated flag/slot on an object, without modifying any of the property descriptors on that object. Any direct (not delegated) modification of an object itself would have to check that flag and abort if set.

I've always found that particular quirk about Object.freeze extremely odd, that it operates on the descriptors rather than simply on the object itself.


Would it be considered too breaking if you were to make these changes instead?

  1. Object.freeze does what it normally would now, but it also sets a list (say, [[FrozenWritable]], initially none) on the object of all descriptors whose [[Writable]] property was previously true. (This can be built without any extra proxy calls.)
  2. On [[Set]], if it finds the descriptor to have [[Writable]]: false and the key in the prototype's [[FrozenWritable]] list, it just sets it as if it were writable.
getify commented 6 years ago

@isiahmeadows Interesting trick but it wouldn't address the problem of inheriting configurable. Would need a separate list of [[FrozenConfigurable]] and Object.defineProperty(..) would need to consult it.

dead-claudia commented 6 years ago

@getify No, you don't. Inherited non-configurable properties have no impact on whether you can Object.defineProperty(...) them on base objects (even if the parent is frozen), and you obviously don't want descriptors modifiable directly on frozen objects.

var proto = {foo: "foo"}
var o = Object.create(proto)
Object.freeze(proto)
Object.defineProperty(o, "foo", {value: 1}) // works
Object.defineProperty(proto, "foo", {value: 1}) // obviously fails
erights commented 6 years ago

Changing Object.freeze to not modify the descriptors would be a radical change that I am confident would break actual programs.

Before we invent something complicated that would involve new semantic state, let's first ask whether we can simply fix the override mistake without breaking any actual programs. I suspect we can.

ljharb commented 6 years ago

You might be right - but I’m not convinced that we should. Currently a set effectively does: “go up the prototype chain and find the object that has an own descriptor, then write it there (call the setter or assign the value) - or finding none, define an own property”. This is true regardless of the content of the descriptor.

@erights how would we change this such that it alters Object.freeze in the way you want, but doesn’t impact all non-own property lookups and mutations? Although i don’t think @getify’s solution is tenable, his reasoning in https://github.com/tc39/ecma262/issues/1154#issuecomment-376057875 seems sound to me - that the mistake is in Object.freeze, and not in [[Set]].

getify commented 6 years ago

Changing Object.freeze to not modify the descriptors would be a radical change that I am confident would break actual programs.

Yeah, I don't think we could change it either, nor do I think we should. It's probably still useful in its current form for those who want to modify all property descriptors.

whether we can simply fix the override mistake

I don't think it's been established that it's a mistake. I don't personally prefer the way it is, but I understand it, and I'm defending that it shouldn't change because changing it complicates learning (bucks precedent), aside from whether it will break programs or not.


I think you made a good point that one might want a different kind of freezing that's communicating something about the set of contributions to an inheritance. But that should be a different mechanism. As you said, you can't simultaneously communicate two different things with the same signal.

that would involve new semantic state

What you're suggesting is a new state. It's substantively different to modify the contributions to an inheritance themselves, than it is to modify how that set of contributions are treated. If I was trying to say something about the set of contributions, I'd be annoyed that the contributions themselves were being modified to accomplish it.

I think it's a mistake to keep using Object.freeze(..) for the use-case you present, but then push for this other thing (inherited writeable) that's been true of JS since 1997 to change to fix quirks of that usage.

IMO, the more defensible approach is to champion an Object.lock(..) or whatever, specifically for that use-case. Doesn't seem like it's that "complicated" at all. Whether the use-case is useful enough to justify itself is separately debatable from if other mechanisms should be changed to accommodate it (at the expense of other use cases).

dead-claudia commented 6 years ago

@erights @ljharb BTW, I did come up with a potential solution modifying Object.freeze. Just didn't want it to get lost in the woods too early.

ljharb commented 6 years ago

@isiahmeadows tbh i think any changes like what you’ve proposed would likely overcomplicate the mental model of how properties and inheritance works in JS.

getify commented 6 years ago

@ljharb

Currently a set effectively does: “go up the prototype chain and find the object that has an own descriptor, then write it there (call the setter or assign the value) - or finding none, define an own property”. This is true regardless of the content of the descriptor.

IIUC, I don't think that's accurate. The write of a property being set never happens "there" -- on the delegated object -- it either:

screen shot 2018-03-26 at 8 47 43 am

ljharb commented 6 years ago

Yes, thanks, you’re correct :-) i think my comment still holds with a more accurate pseudo-description, however.

getify commented 6 years ago

@ljharb

Although i don’t think @getify’s solution is tenable

Which solution is not tenable?

ljharb commented 6 years ago

Changing Object.freeze (a new API might be possible).

dead-claudia commented 6 years ago

@ljharb @getify I could potentially see myself liking a potential Object.lock better than Object.freeze:

  1. Locking an object doesn't conceptually lock down inheritors. It just locks down itself.
  2. You might be able to make Object.lock proxy descriptors as non-configurable, non-writable without actually redefining properties. That would both speed up the creation process and unlock a potential method of "frozen" proxies that aren't bound to the same strict guarantees of immutability.
    • Note: this would still prevent extensions and Object.defineProperty calls, of course.
    • We should still provide a proxy trap for this like what already exists with preventExtensions.
  3. Adding syntax for it might make it a lot more attractive as a simple immutable state container. (It's better than Object.freeze, at least, and it's more forgiving. An engine could also avoid allocating heavy descriptors here if it's all data properties - it only needs the values.)

(Complete shot-in-the-dark, probably-won't-have-a-snowball's-chance-in-hell-of-happening strawman idea.)

allenwb commented 6 years ago

@erights

Before we invent something complicated that would involve new semantic state, let's first ask whether we can simply fix the override mistake without breaking any actual programs. I suspect we can.

This has been discussed multiple times by TC39 and always rejected.

There are many early JavaScript language design decision that in retrospect might be considered mistakes. We've learned that they are almost never correctable at this point in time (and those that were fixable in ES5 largely depended upon introducing a syntactic mode).

It seems pointless to run around applying the "mistake" label to such features (consider, the ASI mistake, the new mistake, the with mistake, variable hoisting mistake, the undefined/null mistake, etc).

allenwb commented 6 years ago

@getify

JS has gone to a lot of trouble over the years to give the impression that its dynamic prototype system is -- or can be used as -- a traditional static class system

Good theory, but I don't think it explains this behavior. Note that the self language, which tries hard to not present a traditional class model, has exactly this same semantics.

From the self handbook with added emphasis:

There is no separate assignment operation in Self. Instead, assignments to data slots are message sends that invoke the assignment primitive. For example, a data slot x is assignable if and only if there is a slot in the same object with the same name appended with a colon (in this case, x:), containing the assignment primitive. Therefore, assigning 17 to slot x consists of sending the message x: 17. Since this is indistinguishable from a message send that invokes a method, clients do not need to know if x and x: comprise data slot accesses or method invocations.

trusktr commented 6 years ago

About Object.freeze, in the es-discuss thread, the title was "Something like Object.freeze, but not inherited from prototypes."

Even if some of the behavior of Object.freeze was unintended or a "mistake", I believe that Object.freeze behavior is useful for designing inheritable immutability, and that it shouldn't change. I've stumbled on a few websites describing this behavior already. A new feature like the Object.lock idea would be another useful tool.

How could it work?

Let's look at some examples:

first idea

// first, without lock:
let o = {}
Object.defineProperty(o, 'foo', {
  value: 1,
  writable: false, // inheritable immutability
  configurable: true,
})

o.foo = 2 // fails

let o2 = Object.create(o)
o2.foo = 2 // fails
let o = {}
Object.defineProperty(o, 'foo', {
  value: 1,
  writable: true, // inherited writability
  configurable: true,
})

Object.lock(o)
Object.isLocked(o) // true, internal [[IsLocked]] slot

// Like Object.seal, it sets configurable to false too:
// {
//   value: 1,
//   writable: true, // inherited writability
//   configurable: false, // can be configured, because locked is false
// }

// It behaves just like Object.seal, but as if writable were false only for the affected object:
o.foo = 2 // fails

// but writable is true, so inheriting objects shadow the prototype:
let o2 = Object.create(o)
o2.foo = 2 // works

second idea, added descriptors

Maybe, there could be a new descriptor property to make it possible to lock certain properties, rather than the whole object, while Object.lock would apply to all descriptors as well as be a superset of Object.seal (therefore also a superset of Object.preventExtensions). For example,

let o = {}
Object.defineProperty(o, 'foo', {
  value: 1,
  writable: true, // inherited writability
  locked: true,
  configurable: true,
})

In that example, although configurable is true, locked would have higher priority, so if locked is true, then configurable will be automatically set to false.

With locked as false but configurable as false, it behaves just as configurable: false does today.

When locked is true, it behaves just like writable: false when trying to modify an a property on the descriptor-owning object, but if writable is true then inheriting objects can still shadow the prototype. configurable is always false in this case locked is false.

Object.lock being a superset of Object.seal would be a shortcut to locking all the descriptors (keeping their existing writable values, and they also get configurable: false) and also preventing extensions.

This is nice because a subset of Object.lock can be applied only to specific properties.

Object.freeze could be a superset of Object.lock without breaking current programs, because if writable and configurable are both false, then locked being true or false has no effect. If locked and writable are both true on some object, we can't write on that object directly, only on inheriting objects. Basically, the descriptor-owning object can not have it's property written to if locked === true regardless of the writable value.

In any case, getters/setters are still just functions called with this as the leaf of the proto chain, so even if Object.freeze as a superset of the theoretical Object.lock is applied to a prototype, the extension prevention feature of Object.lock as a superset of Object.seal as a superset of Object.preventExtensions only applies to the own object (the prototype).


There's also some features that are missing from of the Object.freeze/seal/preventExtensions/lock ideas: there isn't any tool that applies prevent extensions to inheriting objects.

For example, I thought that I could design inheritable immutability by doing this, but it doesn't work:

let p = { foo: 1 }
Object.freeze(Object.seal(p))

let o = Object.create(p)

o.foo = 2
console.log(o.foo) // 1

// o is extensible:
o.bar = 3
console.log(o.bar) // 3, but I was hoping extensibility was inherited.

It seems like we should maybe address this alongside Object.lock to make our ability to design inheritance more robust.

For example, maybe we need something like Object.sealInherited and Object.preventExtensionsInherited which do similar as their counterparts, but the [[IsExtensible]] behavior can also applies to inheriting objects.

At the moment, it isn't possible to create an inheritance chain that can enforce an end user's inheriting object to have Object.seal (or any of the others) applied.

An Object.lockInherited feature would be similar to applying Object.freeze(Object.sealInherited(p)) on a prototype (because remember from above that if writable is false and configurable is false in a descriptor, then it is effectively like locked being false on the owning object anyways). The benefit of this feature would be the ability to create new immutable objects with, for example, extending methods that call super methods, and alternate default values.

// if we had those more robust options, this could be left to user space:
Object.lockInherited = o => Object.freeze( Object.seal( o ) )

and this is taking advantage of the obscure fact that although Object.seal makes the descriptor non-configurable we can (and Object.freeze can) still set writable to false after the fact.

erights commented 6 years ago

@allenwb The changes we've most often gotten away with are when we turn errors into non-errors. This has happened over and over again. I challenge anyone to show me any code in the wild --- not written to test or illustrate this very issue --- which would break if we fixed this mistake.

As for "mistake", the only name this issue has ever had is "override mistake". It is simply terminology at this point. If you want to call it something else, coin a term and see if it catches on.

Mostly, the committee has rejected fixing this over and over again because you defended the mistake strongly and I attacked it strongly but, each time, eventually gave up. I don't recall anyone else ever caring passionately about this. Perhaps this time you'll defend it less strongly, I won't give up so easily, or others will find they also care passionately about this issue.

raulsebastianmihaila commented 6 years ago

Without wanting to derail the discussion too much, I'd like to point out that there's a more fundamental problem than whether or not the override behavior is a mistake or not. This problematic behavior is obviously due to inheritance, which, beside helping with reusing code, also has this other facet of complexity that the programmer shouldn't have to deal with, namely having to think of an object on multiple levels. In Javascript it reaches into simple basic operations such as setting a value to a property. This makes inheritance nonintuitive and less practical. It would be much better if code could be reused without inheritance. That's how composition based mixins can help.

I don't think the regular developer runs often into issues because of the existing behavior, and in practice I think this behavior can be frustrating only when using the assignment to add methods to an object. Using syntax such as class declarations or object literals to create the methods doesn't cause these issues. Therefore 'fixing' this behavior would help only in a particular style of programming. In such cases the assignment is prefered instead of Object.defineProperty because it's shorter. A new mixin syntax could be simple enough so that avoiding the assignment wouldn't be a deal breaker. It would also allow things like sharing private state with the mixin functions, which would make reusing code even more practical. And of course it would enable programmers to avoid inheritance.

I find it sad that, according to the meeting notes, TC39 is discussing a new mixins feature that is based on inheritance.

allenwb commented 5 years ago

for info on the origins of this behavior see https://github.com/tc39/ecma262/pull/1307#issuecomment-432822787

brusshamilton commented 2 years ago

Can we at least clarify the language in 10.1 to make it clear that prototype property descriptors affect set operations? Currently it reads:

"Data properties of the [[Prototype]] object are inherited (and visible as properties of the child object) for the purposes of get access, but not for set access."

If they are not visible as properties for set operations then their descriptors should also not be visible, and shouldn't affect set operations on the child. It should probably say something like:

"Data properties of the [[Prototype]] object are inherited (and visible as properties of the child object) for the purposes of get access but only the property descriptor is visible for set access."