Object.keys throws for uninitialized module namespace object

[jdalton]

Found in Node here and filed as a V8 bug here. It looks like Object.keys will throw an error when provided an uninitialized module namespace object while Object.getOwnPropertyNames won't.

Comments from the V8 issue:

so i was talking with ljharb about this and we came to a different conclusion about this behaviour.

Since Object.keys (and Reflect.ownKeys, for..in, all the things broken by this upgrade) don't actually expose the value, they shouldn't throw. basically everything that doesn't expose the value, or a getter/setter, should work. anything that's static and won't change after evaluation should be always accessible.

And

See https://tc39.github.io/ecma262/#sec-module-namespace-exotic-objects.

[[GetOwnProperty]] throws because [[Get]] throws:

Let value be ? O.[[Get]](P, O).

[[Get]] throws because GetBindingValue throws:

Return ? targetEnvRec.GetBindingValue(binding.[[BindingName]], true).

GetBindingValue throws because the binding, which exists, is still uninitialized:
https://tc39.github.io/ecma262/#sec-module-environment-records-getbindingvalue-n-s

The bug is that Object.keys should not error just as Object.getOwnPropertyNames does not error.

\cc @ljharb

[ljharb]

Thanks for filing!

imo this is clearly a spec bug; any reflection operations on a TDZ-d module namespace object that solely expose keys, or enumerability/configurability/writable - not values (or thus the whole descriptor) - should work at all times.

cc @allenwb to confirm this is a spec bug; @caridy @dherman @bterlson for their thoughts.

[GeorgNeis]

Besides Object.keys, this affects:

• for-in
• Object.prototype.propertyIsEnumerable
• Object.prototype.hasOwnProperty
• Object.isFrozen
• Object.isSealed
• Object.freeze
• Object.seal (assuming Relax [[DefineOwnProperty]] on module namespace exotic objects. #858 will land)

And perhaps I missed some others.

[caridy]

This is tricky! Spec looks good, at least in principle:

HasOwnProperty (7.3.11, which is a global abstract operation) relies on [[GetOwnProperty]] internal method (which for NS objects in specified in 9.4.6.4), which relies on [[Get]] internal method (which for NS objects in specified in 9.4.6.7) to get the current value needed for the descriptor. It just happen that the [[Get]] internal method is throwing when that binding is in TDZ. This behavior is not specific for modules, this is the regular behavior for all Objects.

Do we really want to change this behavior? If we change this to make [[Get]] to return undefined when the value is in TDZ, or even if we change upstream in [[GetOwnProperty]] to return value undefined in the descriptor when in TDZ, this has other problems (imagine that you're exporting a const whose descriptor value is sometimes undefined or the actual value, and all that is observed by the importer. I don't think those two options are real options.

Likewise, changing the regular mechanism that govern HasOwnProperty (7.3.11) is probably not an option, because it is not backward compatible. I don't see a path forward here.

[jdalton]

I don't see a path forward here.

Special case for namespace objects in relevant methods since its own property keys are known up-front without needing to dig deeper.

[ljharb]

I think a non-observable spec refactoring could absolutely allow special-cased internal methods on namespace objects - or, could even allow this to be “fixed” with fewer special cases (since the tdz is already a special case)

[erights]

A test in general we should apply every time we design the behavior of an exotic object: How faithfully can this behavior be emulated by a Proxy or membrane? I don't foresee a problem here, but we need to check.

[zenparsing]

If I'm understanding correctly:

1. Object.keys has to pull out a property descriptor in order to determine whether the property is enumerable or not.
2. But it can't pull out a property descriptor without also pulling out the value of the property (assuming a data property).
3. Module namespace objects return data property descriptors.
4. In general, we aren't allowed to observe the value of an "uninitialized" binding. (That's the whole point of the TDZ.)

Taking these points together, I'm not sure how we can change Object.keys to bypass the value lookup without also bypassing the MOP (which we should not do).

Perhaps moduleNamespeceObject.[[GetOwnProperty]] could in principle have returned accessor descriptors (with get/set), but I haven't thought through the implications.

[ljharb]

@zenparsing Object.keys pulls out the descriptor record - it shouldn't be accessing the record that throws, it should be reifying the value or the getter/setter into a descriptor object.

[allenwb]

This is not a spec. bug, as such. Rather it is a direct consequence of the design of the MOP and how we choose to implement the MOP API for module namespace objects. The MOP itself is not really changeable as it is the foundation for how proxy objects and all other exotic objects interface with ES language features and the built-in library.

The MOP defines the API that exotic objects can implement in order to support such features. But exotic objects are not required to support the full functionality of ordinary objects. Throwing is a valid way to implement most of the MOP API.

For ES6 we discussed at a TC39 meeting how much of the MOP API that module namespace objects should usefully support. The consensus was that they were very exotic objects that existed to support a single function --dotted access to module imported bindings -- and that all they really needed to support was [[Get]]. That was pretty much what we did in ES 6. Most MOP operations on module namespace objects threw, or otherwise returned an error state Subsequent, people have tried to do things other than get property access with them and reported the failures as bugs. Even though the failures were "by design" some of them were fixed by adding additional functionality to the module namespace MOP implementation. But, they still are very exotic and can't be expected to be fully equivalent to ordinary objects.

It doesn't bother me that Object.keys doesn't work on them as they weren't intended to be a module reflection mirror, But here is what you might do to "fix" the problem: The dotted property access functionality is provided by [[Get]] and is not dependent upon [[GetOwnProperty]]. Subject to the essential invariants,
[[GetOwnProperty]] only provides a temporal snapshot of the state of a property.. So, make it return undefined for properties whose associated binding is uninitialized at the time the prop descriptor is constructed.

[domenic]

I've always thought they would be better off as getter/setters from the mental model perspective. This kind of discussion makes me realize the ergonomics would be better too. I believe the objection was that this would allow extracting the getter/setter functions and trying to use them, which was annoying to spec/implement.

[allenwb]

Yes, module name space objects were designed such that it would be fairly easy to for an implementation to compile mod.foo into a direct reference to the binding exported by "mod". That was really why we didn't want to expose getter functions.

[GeorgNeis]

I think making [[GetOwnProperty]] return undefined for such properties will result in equally surprising behavior. For instance, it would become possible that Object.keys first returns an empty array but later returns a non-empty array, and all the while Object.isFrozen returns true.

[ljharb]

@GeorgNeis that should not be possible since the names are all known at the time of the object being available to user code - the only thing that would change over time is the values of the properties (and i think that any attempt to get the values should throw when in a tdz)

[GeorgNeis]

Please see https://tc39.github.io/ecma262/#sec-enumerableownpropertynames.

[GeorgNeis]

Maybe there's a misunderstanding here. I assumed the suggestion is to return undefined, perhaps you assumed the suggestion is to return a property descriptor whose [[Value]] component is undefined.

[ljharb]

@GeorgNeis yes, i understand that that’s how the current spec operates, but that’s calling an un observable internal method. This issue suggests that we could call GetOwnProperty and enumerate the keys even if the values are not yet available.

[GeorgNeis]

But the issue lies deeper than Object.keys, so just changing Object.keys will not be enough. See the list in my earlier post. And it's not clear to me what to do about some of the others.

[ljharb]

I agree it’s tricky.

However, i feel like it should be achievable to allow the keys-only APIs to work while continuing to throw on the APIs that reference get/set/value.

[devsnek]

would this be a proper fix for this issue?

diff --git a/spec.html b/spec.html
index 9797576..d71e3b1 100644
--- a/spec.html
+++ b/spec.html
@@ -8574,10 +8574,10 @@
         <h1>[[OwnPropertyKeys]] ( )</h1>
         <p>When the [[OwnPropertyKeys]] internal method of a module namespace exotic object _O_ is called, the following steps are taken:</p>
         <emu-alg>
-          1. Let _exports_ be a copy of _O_.[[Exports]].
-          1. Let _symbolKeys_ be ! OrdinaryOwnPropertyKeys(_O_).
-          1. Append all the entries of _symbolKeys_ to the end of _exports_.
-          1. Return _exports_.
+          1. Let _keys_ be a copy of _O_.[[Exports]].
+          1. For each own property key _P_ of _O_ that is a Symbol, in ascending chronological order of property creation, do
+            1. Add _P_ as the last element of _keys_.
+          1. Return _keys_.
         </emu-alg>
       </emu-clause>

cc @allenwb @domenic @GeorgNeis

[GeorgNeis]

No, this would not be a proper fix, because the fundamental issue is not about [[OwnPropertyKeys]].

[ljharb]

@GeorgNeis what do you suggest?

[devsnek]

bump

[zenparsing]

@devsnek Do you have anything that might move this forward? There doesn't seem to be an obvious solution: Object.keys requires a fully-realized property descriptor, which is not available when an exported binding is in a TDZ state. The workaround is to use Reflect.ownKeys in this situation.

[devsnek]

yeah i don't have any genius ideas to fix this. i was just thinking either someone will have come up with something or this can be closed.

[ljharb]

What about, change Object.keys so that if the argument is an uninitialized Module Namespace Record, it instead calls the Reflect.ownKeys algorithm, skipping over Symbol keys?

[anba]

We seem to go round in circles, special casing Object.keys isn't enough to cover all cases. If I had to guess I'd say [[Get]] needs to be changed to return undefined for bindings in TDZ instead of throwing to fix this issue. If that's not acceptable and unless we want to change the whole MOP, this issue probably needs to be closed as works-as-intended.

[devsnek]

allen's suggestion is probably the most sensical so far, so if that isn't found to be worth implementing i'd agree that this is working as intended.

[allenwb]

@ljharb
No reason for Object.key to be specified in terms of Reflect.ownKey. In the spec Object.keys is can invoke [[OwnPropteryKeys]] directly.

But I think a better fix would be to special case https://tc39.github.io/ecma262/#sec-enumerableownpropertynames for the situation where O is an namespace exotic object and kind is "key".

[ljharb]

@allenwb sure, i mente the algorithm steps, not the method itself :-)

special-casing in the way you suggest seems totally fine to me too.

[BridgeAR]

I just had a look at this.

Would this be a proper fix? I tried to apply the suggestion by @allenwb. If it is, I'd just go ahead and open a PR for it. The only changes are in the outer 2. and 3.

7.3.24 EnumerableOwnPropertyNames ( O, kind )
The abstract operation EnumerableOwnPropertyNames takes arguments O (an Object) and kind (key, value, or key+value) and returns either a normal completion containing a List or a throw completion. It performs the following steps when called:

1. Let ownKeys be ? O.[[OwnPropertyKeys]]().
2. If O is an namespace exotic object and kind is key, then return ownKeys.
3. Else,
  a. Let properties be a new empty List.
  b. For each element key of ownKeys, do
    i. If Type(key) is String, then
      1. Let desc be ? O.[[GetOwnProperty]](key).
      2. If desc is not undefined and desc.[[Enumerable]] is true, then
        a. If kind is key, append key to properties.
        b. Else,
          i. Let value be ? Get(O, key).
          ii. If kind is value, append value to properties.
          iii. Else,
            1. Assert: kind is key+value.
            2. Let entry be CreateArrayFromList(« key, value »).
            3. Append entry to properties.
  c. Return properties.

[bakkot]

@BridgeAR Modulo some minor editorial issues, that is the suggestion, yes. (Whether it constitutes a "proper fix" depends on what behavior you think is correct, which there isn't an objective answer to.) It would still need to be presented in plenary to land, though.