Relax [[DefineOwnProperty]] on module namespace exotic objects. #858

GeorgNeis · 2017-03-27T05:53:52Z

[[DefineOwnProperty]] on module namespace exotic objects used to always
return false. With this change, it returns true if, intuitively,
applying the given property descriptor wouldn't change anything. This
is consistent with how [[DefineOwnProperty]] behaves for other kinds of
objects.

…ects. [[DefineOwnProperty]] on module namespace exotic objects used to always return false. With this change, it returns true if, intuitively, applying the given property descriptor wouldn't change anything. This is consistent with how [[DefineOwnProperty]] behaves for other kinds of objects. - Special case for symbols. - Make OrdinaryDefineOwnProperty use IsExtensible.

GeorgNeis · 2017-03-27T05:55:22Z

Affected test262 tests: language/module-code/namespace/internals/define-own-property

GeorgNeis · 2017-03-27T06:08:36Z

Note: Since [[GetOwnProperty]] can throw (if the property has not yet been initialized), it matters in which step we call it. In my change it's the very first step, but it may also make sense to do it as late as possible.

ljharb · 2017-03-27T06:51:29Z

spec.html

+          1. Let _current_ be ? _O_.[[GetOwnProperty]](_P_).
+          1. If _current_ is *undefined*, return *false*.
+          1. If IsAccessorDescriptor(_Desc_) is *true*, return *false*.
+          1. If _Desc_.[[Writable]] is present and has value *false*, return *false*.


Would this perhaps be cleaner (and independently useful) if there was an IsSameDescriptor abstract operation? It might also be helpful in cleaning up ValidateAndApplyPropertyDescriptor as well.

It wasn't clear to me how to simplify things but in the meantime I think I found a way to factor out the common logic (something like IsSameDescriptor doesn't cut it, I believe). Will update the PR soon.

I had started writing this down but then felt that it doesn't really help a lot.

¯\_(ツ)_/¯ up to you, but it feels like being able to have an algorithm step that's effectively "if descriptorA == descriptorB, then" would be valuable - even if it's only used in a few places.

littledan · 2017-04-27T14:15:45Z

This change seems reasonable to me.

rwaldron · 2017-08-04T15:18:02Z

@GeorgNeis this change has consensus, but is blocked on tests. Are you planning to make the relevant updates to language/module-code/namespace/internals/define-own-property.js?

GeorgNeis · 2017-08-04T15:20:57Z

I can do that.

rwaldron · 2017-08-04T18:12:36Z

@GeorgNeis excellent!

See tc39/ecma262#858.

anba · 2017-08-19T15:40:09Z

spec.html

@@ -8274,7 +8274,14 @@
        <h1>[[DefineOwnProperty]] ( _P_, _Desc_ )</h1>
        <p>When the [[DefineOwnProperty]] internal method of a module namespace exotic object _O_ is called with property key _P_ and Property Descriptor _Desc_, the following steps are taken:</p>
        <emu-alg>
-          1. Return *false*.
+          1. Let _current_ be ? _O_.[[GetOwnProperty]](_P_).


The first step needs to be If Type(P) is Symbol, return OrdinaryDefineOwnProperty(O, P, Desc)., because the symbol properties of module namespace objects have different property descriptor attributes compared to the export properties.

Right, thanks.

Meh, I just noticed there is an issue when the current definition of OrdinaryDefineOwnProperty is directly called:
OrdinaryDefineOwnProperty(...) directly accesses the [[Extensible]] internal slot instead of calling [[IsExtensible]], and module namespace objects don't have an [[Extensible]] internal slot.

So we've got two choices:

Either change OrdinaryDefineOwnProperty(...) to call [[IsExtensible]] instead of directly accessing [[Extensible]].

Or add an [[Extensible]] internal slot to module namespace objects.

I thing the first alternative is better. Providing alternative essential internal method definitions was intended to be the manner to encode such values that apply to all instance of a specific kind of exotic object.

I dislike the direction the second alternative is going. The [[Extensible]] internal slot is one of the defining characteristics of an ordinary object. Namespace objects are about as unordinary as you can get. Adding slots simply to allow use of Ordinary* internal methods on such non-ordinary objects weakens the concept of an ordinary object within the spec.

In ES6, Module namespace objects originally did not have a [[Prototype]] slot for similar reasons. Instead, it had a [[GetPrototypeOf]] internal method that always returned null. For some reason, this has changed to eliminate eliminate the exotic [[GetPrototypeOf]] and add a [[Prototype]] slot that always "contains the value null". I suspect this was done for a similar reason to choice 2 above. That change should probably be reverted.

In both cases, these sounds like changes that were simply made to facilitate pseudo-code reuse, but I think such changes detract rather than add to the clarity of the spec. Internal slots are instance specific, stateful and generally mutable. Their presence should imply at the very least that the state may differ among different instances of that kind of exotic object. When a value is immutable and constant that is no need to have instance specific slot to hold the constant value. The better way is to provide exotic internal methods (such as [[isExtensible]] or [[GetPrototypeOf]]) that directly use the constant. Such internal methods are conceptually shared among of instance of the exotic object type.

littledan · 2017-10-26T21:55:01Z

What more does this need to be able to merge into the spec?

anba · 2017-10-31T17:09:18Z

What more does this need to be able to merge into the spec?

At least the issue noted in https://github.com/tc39/ecma262/pull/858/files/5de505fcb88ceb61ba3778c18edb0ce2cb3e55ee..0c201ef1b82bd744e10dd5c7c8341d2175442002#r136673470 still needs to be fixed.

GeorgNeis · 2017-11-02T08:40:06Z

Sorry, I had missed that. Diff now includes the (tiny) generalization of OrdinaryDefineOwnProperty.

anba · 2018-01-11T23:25:01Z

Anything still missing before this change can be applied?

FWIW Firefox implements the proposed semantics since Firefox 57 (https://bugzilla.mozilla.org/show_bug.cgi?id=1391773).

littledan · 2018-03-05T11:01:50Z

Now that we have tests, consensus on spec text, and a shipping implementation, it seems like time to land this patch.

bmeck · 2018-03-05T19:29:43Z

LGTM but might want to match existing spec wording : https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-defineownproperty-p-desc

littledan · 2018-07-02T09:42:42Z

OK, is this change ready to merge now?

ljharb reviewed Mar 27, 2017

View reviewed changes

ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Mar 27, 2017

leobalter added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label May 25, 2017

rwaldron added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels May 25, 2017

GeorgNeis added a commit to GeorgNeis/test262 that referenced this pull request Aug 7, 2017

Relax [[DefineOwnProperty]] on module namespace exotic objects.

51c536f

See tc39/ecma262#858.

GeorgNeis mentioned this pull request Aug 7, 2017

Relax [[DefineOwnProperty]] on module namespace exotic objects. tc39/test262#1170

Merged

rwaldron pushed a commit to tc39/test262 that referenced this pull request Aug 7, 2017

Relax [[DefineOwnProperty]] on module namespace exotic objects.

fd5054f

See tc39/ecma262#858.

rwaldron added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Aug 7, 2017

anba reviewed Aug 19, 2017

View reviewed changes

ljharb approved these changes Mar 5, 2018

View reviewed changes

ljharb requested review from bterlson and bmeck March 5, 2018 19:20

bmeck approved these changes Mar 5, 2018

View reviewed changes

ljharb assigned bterlson Apr 16, 2018

GeorgNeis mentioned this pull request May 29, 2018

Object.keys throws for uninitialized module namespace object #1209

Open

bterlson approved these changes Jul 18, 2018

View reviewed changes

ljharb assigned ljharb and unassigned bterlson Jul 18, 2018

ljharb force-pushed the DefineOwnProperty branch from 7858e9b to e4bfa86 Compare July 18, 2018 18:48

ljharb merged commit e4bfa86 into tc39:master Jul 18, 2018

shvaikalesh mentioned this pull request Mar 1, 2021

Editorial: Define Module Namespace's [[GetPrototypeOf]] method #2335

Merged

shvaikalesh mentioned this pull request Mar 12, 2021

Named properties object: allow symbols in [[DefineOwnProperty]] and [[Delete]] whatwg/webidl#963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax [[DefineOwnProperty]] on module namespace exotic objects. #858

Relax [[DefineOwnProperty]] on module namespace exotic objects. #858

GeorgNeis commented Mar 27, 2017

GeorgNeis commented Mar 27, 2017

GeorgNeis commented Mar 27, 2017

ljharb Mar 27, 2017

GeorgNeis Mar 29, 2017

GeorgNeis Aug 7, 2017

ljharb Aug 7, 2017

littledan commented Apr 27, 2017

rwaldron commented Aug 4, 2017

GeorgNeis commented Aug 4, 2017

rwaldron commented Aug 4, 2017

anba Aug 19, 2017

GeorgNeis Aug 21, 2017

anba Sep 1, 2017

allenwb Sep 1, 2017

littledan commented Oct 26, 2017

anba commented Oct 31, 2017

GeorgNeis commented Nov 2, 2017

anba commented Jan 11, 2018

littledan commented Mar 5, 2018

bmeck commented Mar 5, 2018

littledan commented Jul 2, 2018

Relax [[DefineOwnProperty]] on module namespace exotic objects. #858

Relax [[DefineOwnProperty]] on module namespace exotic objects. #858

Conversation

GeorgNeis commented Mar 27, 2017

GeorgNeis commented Mar 27, 2017

GeorgNeis commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan commented Apr 27, 2017

rwaldron commented Aug 4, 2017

GeorgNeis commented Aug 4, 2017

rwaldron commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan commented Oct 26, 2017

anba commented Oct 31, 2017

GeorgNeis commented Nov 2, 2017

anba commented Jan 11, 2018

littledan commented Mar 5, 2018

bmeck commented Mar 5, 2018

littledan commented Jul 2, 2018