The rev16 changes to EvaluateCall (11.2.3) seem to have broken (backwards compatible) direct eval detection.
This is visible when using the Withstatement. In short, code like:
---
with (obj) { eval(s) }
---
is per the latest changes equivalent to:
---
obj.eval(s)
---
The first code sample is a (possible) direct eval per 15.1.2.1.1, whereas the latter code sample is never a direct eval call.
I don't see how it is possible to perform backwards compatible direct eval detection and support EvaluateMethodCall / [[Invoke]] at the same time. Unless EvaluateMethodCall is restricted to pure property references, that means without automatic property reference conversion per EvaluateCall, step 1.b.ii.
Full test case:
---
(function(global) {
var x = 1;
var p = new Proxy({}, {
has: (_, pk) => {
print("has: " + pk);
return (pk == "eval");
},
get: (_, pk) => {
print("get: " + pk);
return global.eval;
},
invoke: (_, pk, args) => {
print("invoke: " + pk);
}
});
with(p) {
eval("eval('var x = 0')");
}
return x;
})(this);
---
Output with rev15:
---
has: eval
has: eval
get: eval
has: eval
has: eval
get: eval
has: x
0
---
Output with rev16: unspecified
(In reply to comment #0)
> The rev16 changes to EvaluateCall (11.2.3) seem to have broken (backwards
> compatible) direct eval detection.
>
> This is visible when using the Withstatement. In short, code like:
> ---
> with (obj) { eval(s) }
> ---
>
> is per the latest changes equivalent to:
> ---
> obj.eval(s)
> ---
>
This equivalence (for actual call purposes) was also the case in ES5, it's just that the direct eval test needs to be made considering the actual binding model before making that transformation.
> The first code sample is a (possible) direct eval per 15.1.2.1.1, whereas the
> latter code sample is never a direct eval call.
If written that way, but we are actually talking about evals that are written like your first example. It wasn't intended that you could treat this as a simple desugaring.
>
> I don't see how it is possible to perform backwards compatible direct eval
> detection and support EvaluateMethodCall / [[Invoke]] at the same time. Unless
> EvaluateMethodCall is restricted to pure property references, that means
> without automatic property reference conversion per EvaluateCall, step 1.b.ii.
As your test shows, this is primarily an issue in that direct eval determination requires doing a getValue on the function reference which will do a [[Get]] on the with object. Ordinary [[Invoke]] also does a [[Get]] for that same property. Prior to the introduction of [[Invoke]] the result of a single [[Get]] calls could be used both for the the direct eval test and to supply the this value for the call. The extra [[Get]] call may be observable if the with object is a Proxy or if its 'eval' property is an accessor.
I think the most straight forward fix is to add another clause to the definition of direct eval that says that this case is not a direct eval when the with object is a proxy. In that case you can unobservably inline expand the [[Invoke]] for the potentially direct eval case in a manner that avoids the possibility of the extra [[Get]] tripping over an 'eval' accessor.
Messy, but direct eval is inherently a messy hack.
Hi Allen,
As a more general point, I would be very careful with introducing spec language that explicitly calls out exceptions for proxies.
This is because proxies are just one kind of exotic object, and any exception made only for proxies does not necessarily cover other exotics.
Hence, rather than adding a clause that says that "this case is not a direct eval when
the with object is a proxy", would it not be better to state "this case is not a direct eval when the with object is anything other than a normal ECMAScript object"?
That way you cover host objects too (because in general we cannot know whether the inlining is safe for such objects)
(In reply to comment #2)
> Hi Allen,
>
> As a more general point, I would be very careful with introducing spec language
> that explicitly calls out exceptions for proxies.
>
> This is because proxies are just one kind of exotic object, and any exception
> made only for proxies does not necessarily cover other exotics.
>
> Hence, rather than adding a clause that says that "this case is not a direct
> eval when
> the with object is a proxy", would it not be better to state "this case is not
> a direct eval when the with object is anything other than a normal ECMAScript
> object"?
>
> That way you cover host objects too (because in general we cannot know whether
> the inlining is safe for such objects)
I known...I did consider saying "exotic object" instead of Proxy but array instances are exotic objects and I can imagine somebody using an array as a with object. It is very unlike that there would be an 'eval' property, but not impossible.
Since what I'm trying to enable is the logical inlining of the ordinary [[Invoke]] for this one special case, I guess I could says it isn't a direct eval when the with object is an exotic object that does not use the ordinary [[Invoke]] internal method.
(mid-air collision with Allen, planned to say the same objection w.r.t exotic objects and Arrays/Strings/...)
Nevertheless, with the proposed changes I'm now getting the following output with my test implementation (https://github.com/anba/es6draft). So the [[Invoke]] trap is called on the Proxy and no direct eval call takes place. Does this sound right?
---
has: eval
invoke: eval
1
---
(In reply to comment #4)
> Does this sound right?
> ---
> has: eval
> invoke: eval
> 1
> ---
yes it does.
BTW, I really like you're tests with the MOP level trace results. These essentially provide a "fingerprint" as to whether side-effects occur in the correct order. We should probably push to get some tests like this into Test262.
(In reply to comment #3)
> Since what I'm trying to enable is the logical inlining of the ordinary
> [[Invoke]] for this one special case, I guess I could says it isn't a direct
> eval when the with object is an exotic object that does not use the ordinary
> [[Invoke]] internal method.
+1
fixed in rev17 editor's draft
fixed in rev17, August 23, 2013 draft