Closed Bug 609756 Opened 9 years ago Closed 3 years ago

exact order of operations in evaluating fun(...)++ not handled per ES5

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(1 file)

Per ES5 (probably ES3 too, but I haven't checked), the function call should happen, a little more work should be done to add 1 to that, and then an attempt (doomed to failure) must be made to assign that value to the result of eval() -- which, as a guaranteed non-Reference in our implementation, must throw.  But the side effects of the function call (and of the addition) must happen regardless -- ES5 is clear on this point.

It's possible other setcall-style situations (assignment and so on) may need similar adjustments.

See also bug 609617, most specifically bug 609617 comment 7 (and 10, 11, 13).  We're definitely punting this specific issue until after Firefox 4 because changing code generation this late in the game is a recipe for failure, but perhaps afterward we can circle back to it.
(In reply to comment #0)
> Per ES5 (probably ES3 too, but I haven't checked),

ES1. This goes all the way back.

/be
ES5 Clause 16 defines early errors and then mandates, among other cases, that

"Attempts to call PutValue on any value for which an early determination can be made that the value is not a Reference (for example, executing the assignment statement 3=4)."

must be early errors.

Since we no longer support built-in functions returning Reference type instances (JS_HAS_LVALUE_RETURN was the macro, no longer in jsversion.h), all call expressions in lvalue contexts should be early errors by ES5.

We believe we can't do what ES5 says because it is not web-compatible. See bug 509098, although that does not have the full back-story. I will try to find the webkit bug or mail from Maciej about how web content JS has

  if (document.all) {
    // use d.item(i) = j; here
  } else {
    // use d.item[i] = j;
  }

We can try to find out if this is still a problem at leisure, and this bug would be better used for that purpose. Removing JSOP_SETCALL, the silly op that always throws, is much better than elaborating it to call the function on the left of assignment first, then throw. We could do that, for sure -- but removing trumps elaborating when it comes to VB-compatible cruft, and we'd conform to ES5 too.

/be
(In reply to comment #2)
> ES5 Clause 16 defines early errors and then mandates, among other cases, that
> 
> "Attempts to call PutValue on any value for which an early determination can be
> made that the value is not a Reference (for example, executing the assignment
> statement 3=4)."
> 
> must be early errors.

Interesting; I hadn't realized this.  This seems like an ES5 spec bug to me -- the spec mandates two different behaviors for an implementation not supporting References.  I'll bring it up on the list.
What's the other mandated behavior?

If you kill all calls as lvalues via early errors, there's no runtime semantics to follow that might increment x before a throw, given eval('x++') = 42.

/be
Hm, you're right, I read overhastily.  I still think it worth bringing up on the list for the compatibility-trap concern, in any case:

https://mail.mozilla.org/pipermail/es5-discuss/2010-November/003834.html

And I imagine, with some mental gymnastics (whether plausible or dubious, not sure, haven't thought it through), you could read that to refer only to ES5 capabilities, not those of the implementation, regarding when to produce an early error for bad PutValue.
(In reply to comment #2)
> We can try to find out if this is still a problem at leisure, and this bug
> would be better used for that purpose. Removing JSOP_SETCALL, the silly op that
> always throws, is much better than elaborating it to call the function on the
> left of assignment first, then throw. We could do that, for sure -- but
> removing trumps elaborating when it comes to VB-compatible cruft, and we'd
> conform to ES5 too.

JSOP_SETCALL already does call, and then throw, unless I'm totally misreading it.
What this bug needs is a testcase!

/be
js> function f(){++x;}
js> x=0
0
js> f() = 42
typein:3: SyntaxError: invalid assignment left-hand side
js> x
1

This bug is WFM, except for the eval case, which Jim already tested at bug 609617 comment 7:

js> function g() { print("bleah") }
js> function f() { function g() { throw('food'); } eval('g()')++; } 
js> f()
bleah
typein:6: SyntaxError: invalid assignment left-hand side

The eval should be direct but it is indirect. I'll fix that bug over there (bug 609617). Marking this one WFM.

We can have a bug about getting rid of JSOP_SETCALL altogether after Firefox 4. Waldo's es-discuss post should lead to helpful pre-bug-filing discussion. I will poke the list about it if no one follows up soon.

/be
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
(In reply to comment #8)
> We can have a bug about getting rid of JSOP_SETCALL altogether after Firefox 4.
> Waldo's es-discuss post should lead to helpful pre-bug-filing discussion. I
> will poke the list about it if no one follows up soon.

See bug 509098 comment 6 and 7, though.
To nitpick, our behavior right now isn't *exactly* compliant, because:

    var x = 'ok';
    function f() {
        return {valueOf: function() {x = 'ok';}};
    }
    try {
        eval("x = 'bad'; f()++;");
        throw "expected an error";
    } catch (exc) {
        assert(exc instanceof ReferenceError);
    }
    assertEq(x, 'ok');  // fails

My reading of ES5 is that two different behaviors are allowed:

  * "reporting" an early error (the standard does not precisely define
    this, but I guess it means throwing a ReferenceError, or maybe a
    SyntaxError, from eval without having executed any eval code); or

  * calling f, adding 1 to the result, and *then* throwing a
    ReferenceError.

We do something slightly different--call f but then throw before adding 1.
Ok, then this bug serves its summarized purpose. Thanks,

/be
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Status: REOPENED → NEW
(In reply to comment #9)
> (In reply to comment #8)
> > We can have a bug about getting rid of JSOP_SETCALL altogether after Firefox 4.
> > Waldo's es-discuss post should lead to helpful pre-bug-filing discussion. I
> > will poke the list about it if no one follows up soon.
> 
> See bug 509098 comment 6 and 7, though.

Right, I cited that bug in comment 2 ;-). This was about taking another run at the web, in the hope that we can get away with an early error. But that is yet another bug from this one, and one to defer as much as this one or more.

/be
Summary: fun(...)++ not handled per ES5 → exact order of operations in evaluating fun(...)++ not handled per ES5
Assignee: general → nobody
See Also: → 1169894
ESmumble sort of kind of has its head in the sand about whether function calls are valid assignment targets, so this bug's validity is a little vague.

In any event, it's easy to have call-increment code perform the call, then ToNumber, then throw.  In the process of doing that, I discovered that PNX_SETCALL, once we've made the necessary changes for call-increment, can die.  So in the end this all turned into a net code reduction of +48,-55.  Not bad.
Attachment #8787821 - Flags: review?(arai.unmht)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #8787821 - Flags: review?(arai.unmht) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/947563d9be79
{Pre,Post}{in,de}crements on function calls must ToNumber after evaluating the call, in case ToNumber's effects are observable via toString/valueOf.  r=arai
https://hg.mozilla.org/mozilla-central/rev/947563d9be79
Status: ASSIGNED → RESOLVED
Closed: 9 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.