688646 - Update tests/js1_5/Function/15.3.4.4.js to conform to ES5

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Edition 5 specifies two changes to Function.prototype.call over edition 3:

 1) thisArg is passed directly as this, without coercion with toObject
 2) thisArg is required and is not implicitly set to the global object

Comment 1

[Jeff Walden [:Waldo]]

Why is #2 true?  The steps of the Function.prototype.call algorithm say nothing about |thisArg| being required.  We implement that behavior, treating non-specification of |thisArg| as |undefined|, as all methods in chapter 15 do by default when incomplete arguments are specified:

    Value *argv = vp + 2;
    Value thisv;
    if (argc == 0) {
        thisv.setUndefined();
    } else {
        thisv = argv[0];

        argc--;
        argv++;
    }

And don't we already do #1?

    /* Allocate stack space for fval, obj, and the args. */
    InvokeArgsGuard args;
    if (!cx->stack.pushInvokeArgs(cx, argc, &args))
        return JS_FALSE;

    /* Push fval, thisv, and the args. */
    args.calleev() = fval;
    args.thisv() = thisv;
    memcpy(args.argv(), argv, argc * sizeof *argv);

    bool ok = Invoke(cx, args);
    *vp = args.rval();
    return ok;

There's no boxing or coercion here, beyond that which the function being called itself does, and that's beyond the purview of Function.prototype.call itself.  What am I missing?

Comment 2

[Terrence Cole [:terrence]]

On further analysis, you are completely correct for #1.  I also agree with your interpretation of the code for #2, however, tests/js1_5/Function/15.3.4.4.js (ref bug 290488) passes.  If I am reading the code and the spec properly, it should not, so I think I am missing something.

The first half just calls 'call' with no params and asserts that this is set to the globals.  This passes.

The second half is a very strange test that is explicitly exercising the weird edge case in Function.prototype.call in edition 3.  If you have a function with a specific parent (it calls its_bindMethod to do this) and *call* it with no args, then it gets the globals as this, rather than its parent (or undefined).  This test also passes currently.  The test fails in its_bindMethod if you pass a function object that is compile-n-go (replacing globals in this case is obviously wrong, so I get the failure).

What I don't get is why this test is still present/passing.

Comment 3

[Jeff Walden [:Waldo]]

So the |this| in the frame is |undefined| (lazy this) until the JSOP_THIS is hit.  When you hit that, you call ComputeThis.  That goes to BoxNonStrictThis.  In there, you hit the isNullOrUndefined() case.  That goes to |call.callee().getGlobal()| for a global object.  |call.callee()| is the function with the mutated parent.  But since the mutated parent chains back to the same global object as its unadulterated parent, just at one more hop, getGlobal() just walking up the parent chain until it hits an object whose parent is NULL.  (To verify: |parent(Function("return this")) !== parent(it.bindMethod("boundMethod", Function("return this"))) && parent(Function("return this")) === parent(parent(it.bindMethod("boundMethod", Function("return this"))))|.)  So you should get the same behavior in either case.  Right?

I think we should remove this it.bindMethod stuff; it's old, and setting the parent of a function is really, really dodgy behavior that will go away eventually.

Comment 4

[Terrence Cole [:terrence]]

Thank you for the detailed explanation of what is going on!  For some reason it did not occur to me that undefined this would be equivalent to lazy this.  Now that I've looked it up, it's very clearly spelled out in 10.4.3: Entering Function Code.  In general, the interaction between the parent pointer, the globals, and this seems quite complex and frequently unintuitive to me.

I can agree wholeheartedly that mucking with the parent chain should not happen.  What I'm not yet clear on is the role of the parent chain in determining the global object.  I had thought that most of the global pointers we track are simple optimizations to keep us from having to walk the parent chain, but reading the spec this morning has confused my (probably overly simplistic) understanding of how globals are determined for a callee.

I'll format a patch to rip out #2 and reword #1 to align it with es5.  There is probably more that can be done to test 15.3.4.4 as well -- I will give it some thought.

Comment 5

[Jeff Walden [:Waldo]]

ECMAScript itself doesn't say anything about multiple globals at all, at least not yet, which makes the parent/global thing a bit obscure.

Parent itself is roughly analogous to name lookup scope.  (Why is it present in all objects, not just functions or something?  Hysterical raisins, more or less.  There's a bug on file to remove it from all but functions, and perhaps only interpreted functions, at that.  Bound functions [via Function.prototype.bind] don't need it, for example.)  Such scopes chain (lexically, if there's no |with| statement involved) from scope to scope, and parent records that.  The final name lookup scope is the global object, so that's where the chain ends.  We could use some other scheme than the parent chain to figure out globals -- there's no particular reason to use parent except that it's what we've done in the past.  When there's a compartment per global object, we could store the global in the compartment or something, say.

As far as tests go, note that they've generally sprung up more or less organically, so there's a bit of disorganization about it.  As far as Function.prototype.call goes, you should check whether ecma_5/Function/function-call.js covers any tests you might have thought to write.

Comment 6

[Terrence Cole [:terrence]]

Attachment: Removes its.bindMethod and migrates its user to more recent test suite.

Thanks for the help getting this oddity sorted out.  Let me know if this is a good enough way to migrate/update this test or if there is something better I could do here.  Once I get 646597 finished, Functions will be compile-n-go and the question of parent pointers will be moot, or at the very least, crash hard if we do misuse them like its.bindMethod was doing.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 562473 [details] [diff] [review]
Removes its.bindMethod and migrates its user to more recent test suite.

Review of attachment 562473 [details] [diff] [review]:
-----------------------------------------------------------------

Note that requesting review from no one is not guaranteed to get as quick a response as requesting review from a particular person.  Generally it's not a good idea to put an unadorned r? out.  For figuring out who to ask, look at who else has reviewed substantive changes to the file in question, or ask on IRC.  (This isn't a very friendly process, and whenever barriers to contribution get discussed this comes up, with various ideas for how to fix it.  I'm sure it'll change, somehow, eventually.)

As far as the patch header goes, I'd refer to files as js/src/tests/js1_5/Function/15.3.4.4.js -- full qualification, that is.

Post a version of the patch with these tweaks and I'll happily push it for you.

::: js/src/shell/js.cpp
@@ +4636,2 @@
>      {NULL,NULL,0,0}
>  };

Since |its_methods| doesn't even have anything in it now, I think you can just remove |its_methods| entirely.  I think whatever this fed into would null-check its argument, so you can just swap out |its_methods| for |NULL|.

::: js/src/tests/ecma_3/Function/15.3.4.4-1.js
@@ +154,5 @@
>  
> +/*
> + * Function.prototype.call gets lexical globals, not caller globals
> + */
> +status = inSection(11);

Just noting -- not asking for a change or anything -- but this awkward way of writing tests isn't done much except in tests that already use the style.  Most people these days write tests more like, say, ecma_5/JSON/stringify-call-toJSON-once.js -- do all tests with assertEq, then put one trailing |reportCompare(true, true)| so that the test harness sees that at least one test happened.  (reportCompare was the old-school testing thing, and unlike assertEq it doesn't throw an exception.  The test harness has semantics such that if no tests happen, it treats it as a probably-unintended failure.)

Comment 8

[Terrence Cole [:terrence]]

(In reply to Jeff Walden (remove +bmo to email) from comment #7)
> Note that requesting review from no one is not guaranteed to get as quick a
> ...
> it.  I'm sure it'll change, somehow, eventually.)

Huh, I am ~100% sure that I flagged you for review specifically, because I remember looking you up in phonebook to make sure I got the right address.  Maybe it was the network problem I had earlier today, maybe I just fatfingered it.  In any case, thanks for picking it up!

> Just noting -- not asking for a change or anything -- but this awkward way
> ...
> probably-unintended failure.)

Ah, I was wondering which of those styles is the "correct" one.

Comment 9

[Terrence Cole [:terrence]]

Attachment: With reviewer feedback applied.

I went ahead and removed its_methods and its user completely.

Comment 10

[Jeff Walden [:Waldo]]

Re comment 9, exactly what I had in mind.

http://hg.mozilla.org/integration/mozilla-inbound/rev/c4b5ba89b4b7

Comment 11

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d691de3932b

Followup fix to make sure the test doesn't try to call newGlobal when the tests run in the browser -- newGlobal is shell-only.  These shell-only-helper things crop up way too often.  :-\  Fortunately this particular one, at least, will disappear with compartment-per-global.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c4b5ba89b4b7

Followup: https://hg.mozilla.org/mozilla-central/rev/6d691de3932b