Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

how does for-in test for presence of a property in the enumerated object? #1281

Open
michaelficarra opened this issue Aug 7, 2018 · 17 comments

Comments

@michaelficarra
Copy link
Member

Consider the program

let a = {
  0: 0,
  1: 1,
};

let b = {
  1: 1,
  2: 2,
};

function name(x) {
  return x === a ? 'a' : (x === b ? 'b' : 'unknown');
}

let handlers = {
  has(target, propertyName) {
    console.log(name(target) + " has: " + propertyName);
    return Reflect.has(target, propertyName);
  },
  getOwnPropertyDescriptor(target, propertyName) {
    console.log(name(target) + " getOwnPropertyDescriptor: " + propertyName);
    return Reflect.getOwnPropertyDescriptor(target, propertyName);
  },
};

let pa = new Proxy(a, handlers);
let pb = new Proxy(b, handlers);

Object.setPrototypeOf(a, pb);
  1. What does this print?

    for (let x in pa) {
      console.log(x);
    }
  2. What does this print?

    for (let x in pa) {
      console.log(x);
      delete a[1];
    }
  3. What does this print?

    for (let x in pa) {
      console.log(x);
      delete b[2];
      a[2] = 2;
    }

The relevant spec text is in §13.7.5.15 and answers these questions ambiguously. Implementations also do not match the informative implementation given in the specification.

@ljharb
Copy link
Member

ljharb commented Aug 7, 2018

I think there’s an open bug on v8 about this.

@michaelficarra
Copy link
Member Author

Well we should probably figure out how we expect it to work before they change their implementation. Anyway, do you have a link?

@littledan
Copy link
Member

IIRC the ambiguity here is deliberate, to permit multiple strategies for executing for-of loops that different implementations take (sorry, not sure where to find a reference). Do you think we should nail down one interpretation or another? Any suggestion as to which one?

@ljharb
Copy link
Member

ljharb commented Aug 7, 2018

Looks like the one I was thinking of is https://bugs.chromium.org/p/v8/issues/detail?id=705, and it's been fixed.

In a non-Proxy situation

I would expect wrt delete of a key on one object shadowing the same key higher up in the prototype chain, whether it's own, inherited, or both: that not-yet-visited keys are retrieved with in (naturally) and Get, so you'd end up seeing the shadowed key (ie, that if there's N keys, and the Nth leaf-most is deleted, the (N-1)th leaf-most would show up). Visited keys would ofc not be revisited.

I'd also think it reasonable for engines to be allowed to treat delete as a signal that that key should be marked as visited, and then you'd see none of them (it'd be nice for the spec to force one or the other, but as long as an engine always picked the same thing, i'd think it's reasonable)

with a Proxy as one of the objects in the chain

I would expect that the Proxy would have its list of own keys retrieved once at the beginning of the loop, and that after that, has would be invoked only when the key was eligible to appear in the iteration; getOwnPropertyDescriptor would be invoked when has returned true.

I wrote this without reading the spec text first; how does my interpretation differ from yours, or from what specific engines do? If it doesn't differ, what specific ambiguities do you see?

@michaelficarra
Copy link
Member Author

michaelficarra commented Aug 7, 2018

@littledan I believe this text was written in a time before proxies, where the difference between an in test and getOwnPropertyDescriptor with manual prototype traversal was less observable. Additionally, we may want to specify the short-circuiting of the prototype traversal or the interleaving of the tests for presence and the loop body evaluation.

@michaelficarra
Copy link
Member Author

michaelficarra commented Aug 7, 2018

@ljharb They're completely different (except XS matches SpiderMonkey in all cases 🤔)

1

Chakra

a getOwnPropertyDescriptor: 0
0
a getOwnPropertyDescriptor: 1
1
b getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 2
2

JavaScriptCore

0
a getOwnPropertyDescriptor: 1
1
a getOwnPropertyDescriptor: 2
b getOwnPropertyDescriptor: 2
2

SpiderMonkey, XS

a getOwnPropertyDescriptor: 0
a getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 2
0
1
2

V8

a getOwnPropertyDescriptor: 0
0
a getOwnPropertyDescriptor: 1
1
a getOwnPropertyDescriptor: 2
b getOwnPropertyDescriptor: 2
2

2

Chakra

a getOwnPropertyDescriptor: 0
0
a getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 1
1
b getOwnPropertyDescriptor: 2
2

JavaScriptCore

0
a getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 1
1
a getOwnPropertyDescriptor: 2
b getOwnPropertyDescriptor: 2
2

SpiderMonkey, XS

a getOwnPropertyDescriptor: 0
a getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 2
0
1
2

V8

a getOwnPropertyDescriptor: 0
0
a getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 1
1
a getOwnPropertyDescriptor: 2
b getOwnPropertyDescriptor: 2
2

3

Chakra

a getOwnPropertyDescriptor: 0
0
a getOwnPropertyDescriptor: 1
1
b getOwnPropertyDescriptor: 1

JavaScriptCore

0
a getOwnPropertyDescriptor: 1
1
a getOwnPropertyDescriptor: 2
2

SpiderMonkey, XS

a getOwnPropertyDescriptor: 0
a getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 1
b getOwnPropertyDescriptor: 2
0
1
2

V8

a getOwnPropertyDescriptor: 0
0
a getOwnPropertyDescriptor: 1
1
a getOwnPropertyDescriptor: 2
2

edit: Interestingly, JSC and V8 agree except JSC never triggers the first getOwnPropertyDescriptor trap.

@allenwb
Copy link
Member

allenwb commented Aug 7, 2018 via email

@michaelficarra
Copy link
Member Author

@allenwb

Do any of the implementation violate the requirements given in prose section?

The reason I opened this issue is because I can't tell. I've posted the implementation results. You tell me if any violate the requirements.

For example,

A property that is deleted before it is processed by the iterator's next method is ignored.

If a property is being enumerated because it was present on the prototype, but is deleted from the prototype and added to the target object (or elsewhere in the prototype chain), does that count as a deleted property? Because everybody but Chakra thinks it is not deleted.

@bakkot
Copy link
Contributor

bakkot commented Aug 7, 2018

I think it would definitely be a shame to forbid JavaScriptCore's optimization here (i.e., you don't need to check if the very first property exists, if you're not worried about proxy traps, since there's no way for it to get deleted between the enumeration and the first pass of the loop). Though I guess it could just have a slow path for proxies.


@allenwb, the problem is that the prose is not sufficiently specific to tell which implementations violate it. For example, it is not clear to me

  • which proxy traps are permitted to be invoked and when

  • whether "a property that is deleted before it is processed is ignored" requires ignoring properties which are removed and then added back, or added to the object's prototype prototype

  • whether "a property that is deleted before it is processed is ignored" requires ignoring properties which are present on both an object and its prototype and deleted only from one

  • whether the sentence "If new properties are added to the target object during enumeration, the newly added properties are not guaranteed to be processed in the active enumeration" is intended to apply to properties added to the object's prototype

  • what "a property that is deleted" means in the context of proxies

  • whether for-in should invoke proxy traps at all

  • etc.

@allenwb
Copy link
Member

allenwb commented Aug 7, 2018

All that is not forbidden is allowed.

More tomorrow...

@allenwb
Copy link
Member

allenwb commented Aug 7, 2018

Ok, first some background. For-in enumeration order has always been specified to be implementation dependent and there have also been desires to precisely specify it going back to the development days of ES3. In practice that has never been possible, both for pragmatic (implementations want freedom to optimize/aren't willing to deoptimize existing implementations) and technical reasons (the desire to interleave traversal of a complex meta-mutable (ie, includes exotic objects/proxies) data structure with user provided, potentially mutating code). Starting with ES3 it was concluded that the best that was possible was to impose some basic nominal constraints on the stream of keys produced by the enumeration. That, itself, proved to be challenging.

13.7.5.15 is the 4th generation attempt at defining those constraints. Every word of the ES6 equivalent of 13.7.5.15 was carefully chosen and scrutinized during the latter part of ES2015 development and it reflects the consensus of the committee at that time. The changes made subsequent to ES2015 (ES2016??) don't appear to reflect any significant changes of that consensus. Is it possible that the prose might be improved? Probably. Is it possible to get agreement on more constraints or a stronger ordering requirement? Maybe, but it will be hard. A starting point should probably be researching the ES6 era meeting notes and bugs.ecmascript.org.

Also, note that the word "target" as used in the second prose paragraph is not related to Proxy target objects. The wording should be changed if it is being interpreted as implying some special processing requirements for Proxies. There is nothing special about Proxies for this operation. Proxies are just exotic objects and everything "bad" that a proxy might do could also be done by a host provided exotic object.

The most important normative requirements are those stated by this prose:

A property that is deleted before it is processed by the iterator's next method is ignored. If new properties are added to the target object during enumeration, the newly added properties are not guaranteed to be processed in the active enumeration. A property name will be returned by the iterator's next method at most once in any enumeration.

The the importance proceeds in reverse order of the sentences:

  1. No duplicate names. An enumeration of an objects property most not yield any specific name more than once. Historically, some implementations did not conform to this
  2. An implementation may ignore properties that are "added" (meaning discussed below) after the start of an enumeration but is not required to do so.
  3. A key must provably exist as a own or inherited property of the object immediately before the enumeration returns that key.

These requirements are intend to be sufficient such that an implementation, when enumerating an ordinary object whose prototypes are also all ordinary objects, has the option of either precomputing a complete list of enumerate property keys (but still subject to existence filtering) before yielding any keys or may interleave the determination of the list of keys with yielding keys and client processing of keys.

What does "adding" and "deleting" properties mean. For this purpose, we only need to define it for own properties because the second prose paragraph says that the same implementation defined enumeration algorithm must be used when climbing the prototype chain. An operation "adds" a own property to an object if the result of a call to to that object's [[OwnPropertyKeys]] immediately before the operation does not include the property's key and a call to that object's [[OwnPropertyKeys]] immediately after the operation does include the key. "delete" can be defined similarly, except that the ordering of "does not include" and "does include" are reversed. Note that the operations in question are not just [[DefineOwnProperty]] and [[Delete]]. Any operation that has this observable effect is an add or delete. It could even be an operation that does nothing if the object's [[OwnPropertyKeys]] returns random results on each call.

@bakkot
Copy link
Contributor

bakkot commented Aug 7, 2018

@allenwb, thanks, that clarifies a lot and I think we should probably try to expand the normative prose to include some of that. Can you also say what "a key must provably exist as a own or inherited property of the object" means, in the context of proxies? In particular, does that imply a requirement to trigger any particular traps? Implementations seem to rely on getOwnPropertyDescriptor in practice, but has is as appropriate for that language, and there's no requirement that the getOwnPropertyDescriptor and has traps report the same results.

This is particularly tricky because proxies are permitted to violate invariants which can otherwise be assumed; for example, if you define "an operation 'deletes' an own property to an object if the result of a call to to that object's [[OwnPropertyKeys]] immediately before the operation includes the property's key and a call to that object's [[OwnPropertyKeys]] immediately after the operation does not include the key", by a strict reading that implies that satisfying the "A property that is deleted before it is processed by the iterator's next method is ignored" constraint requires triggering the ownKeys trap immediately before and after any operation touching the object, since the results of that trap are not necessarily consistent with the results of any other.

The basic problem I'm running into is that the prose is given in terms of "the properties of an object", and since proxies have at least three different traps (has, ownKeys, getOwnPropertyDescriptor) which might be understood to answer the question of whether an object has a property, and these traps are not required be to be consistent with each other or over time, I don't know how to interpret that. For a really specific example, it's not clear to me whether JavaScriptCore's optimization (of skipping the getOwnPropertyDescriptor check for the first pass of the loop, trusting that it must be consistent with the ownKeys trap at that point) is legal.

I would be happier if we could nail this down more consistently in terms of the MOP operations, rather than relying on words like "delete" or "has" whose meaning, in a world with proxies, is imprecise.

@allenwb
Copy link
Member

allenwb commented Aug 7, 2018

which proxy traps are permitted to be invoked and when
The spec requires: " EnumerateObjectProperties must obtain the own property keys of the target object by calling its [[OwnPropertyKeys]] internal method. Property attributes of the target object must be obtained by calling its [[GetOwnProperty]] internal method.". All other invocation of internal methods (eg, proxy traps) is implementation dependent and permitted (ie, it isn't forbidden). The ES2015 version qualified those calls with "as if by" but I'm comfortable with the change because in the only case it makes a difference (Proxies) the actual call really is necessary.

whether "a property that is deleted before it is processed is ignored" requires ignoring properties which are removed and then added back, or added to the object's prototype prototype

That's a delete and a add, so the delete and add rules apply independently.

whether "a property that is deleted before it is processed is ignored" requires ignoring properties which are present on both an object and its prototype and deleted only from one

Implementation dependent because the order to prototype traversal isn't specified. But use of the same EnumerateObjectProperties algorithm is required at each level and information from a (lower/higher) level are passed among those calls. But the intent is that starting EnumerateObjectProperties invocation make the ultimate determination of what it yields. II'm pretty sure that we assume recursion would proceed up the prototype chain.; Maybe that needs to be made explicit.

whether the sentence "If new properties are added to the target object during enumeration, the newly added properties are not guaranteed to be processed in the active enumeration" is intended to apply to properties added to the object's prototype

Additions up the chain would not show up in a well behaved implementation of [[OwnPropertyKeys]]. But an ill mannered [[OwnPropertyKeys]] or any other ill mannered behaviors could. In such cases, the basic constraints apply "any property addition after the start may be ignored by an implementation and never yield the same key more than once.

what "a property that is deleted" means in the context of proxies

see #1281 (comment)

whether for-in should invoke proxy traps at all
Absolutely, invoking the internal methods are the only valid way to validly access the state and behavior of an arbitrarily defined Proxy or other exotic object that you don't have implementation level knowledge about.

But in the presence of Proxy it is impossible to guarantee global consistency of an enumeration algorithm that encounters them. All you can do is make sure that your basic enumeration algorithm algorithm does not violate the yield no duplicates requirements and otherwise acts reasonably with ordinary object equivalent trap behavior.

In general, all you can expect from a Proxy is enforcement of the essential invariants and stepwise consistency. If successive calls to a Proxy's traps yields crazy results you only need to maintain you own internal invariants as you progress through each call. You aren't required to produce sane results when dealing with a crazy proxy. You can decide to bail out and throw something or you can yield your own crazy results as long as you don't violate you own requirements such as the no duplicates rule.

@allenwb
Copy link
Member

allenwb commented Aug 7, 2018

@bakkot

I would be happier if we could nail this down more consistently in terms of the MOP operations, rather than relying on words like "delete" or "has" whose meaning, in a world with proxies, is imprecise.

As an implementor, would you be ok with the spec. saying you are only allowed to use [[OwnPropertyKeys]], [[GetOwnProperty]], and [[GetPrototypeOf]] in your enumeration algorithm? Is that too restrictive? Of course, if you know you are dealing with ordinary objects you always get to use the "as if" exemption.

@bakkot
Copy link
Contributor

bakkot commented Aug 28, 2018

@allenwb, I would personally be OK with that and consider it an improvement, certainly. AFAIK no major implementation deviates that.

But I think it is worth nailing this down more precisely, at least for some cases. I've just come across four novel bugs in three engines relating to this part of the spec - in JavaScriptCore (x, x), SpiderMonkey (x), and XS (x). These are actual bugs according to the spec as it stands, not just cases where their behavior differs from other engines but is allowed by the spec, but I think this sort of bug is much more difficult to avoid when trying to conform to the sort of long-form prose in this section rather than the more usual algorithm steps.

Edit: also an inconsistency in v8; I can't actually tell whether or not it's a bug.

@bakkot
Copy link
Contributor

bakkot commented Feb 19, 2020

https://github.com/tc39/proposal-for-in-order improved this situation for many objects, but it is still unclear for some objects, especially proxies. We'll need to make a (normative) decision about what we do or do not want to allow for the remaining cases, and then write that down more clearly.

@bakkot bakkot removed the editor call to be discussed in the next editor call label Feb 19, 2020
@michaelficarra
Copy link
Member Author

For the record, I care quite a bit that the proxy cases are well defined. I want to be able to rewrite a for-in of a proxy to a combination of other JavaScript primitives in a spec-compliant way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants