509098 - Remove JS_HAS_LVALUE_RETURN support

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

See the URL. It's no longer the '90s, and VBScript's influence is on the wane.

Rob, could you please find an owner for this. Thanks,

/be

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

This is rather naively done, but the result passes all the regression tests I expected it to pass (more on that in a moment).

Comment 2

[Jason Orendorff [:jorendorff]]

Since this deletes a language feature, some tests fail.

These tests *only* test SETCALL and can be marked as known failures.

  js1_5/decompilation/regress-352453.js
  js1_5/decompilation/regress-350670.js
  js1_5/Regress/regress-350253.js
  js1_5/Regress/regress-462292.js
  js1_5/Regress/regress-319391.js
  js1_7/decompilation/regress-352079.js
  js1_7/decompilation/regress-352272.js
  js1_8_1/decompilation/regress-352022.js

These tests look like they also test other things, so we don't want to just
mark them as failures; we want to do something so that we retain the useful
parts of these tests.

  js1_5/Regress/regress-321874.js
      The last two tests are conditionally executed. The condition is
      `if (typeof it != 'undefined')`.  It could be changed to
      `if (typeof it != 'undefined' && it.item)`.
  js1_7/extensions/regress-346642-06.js
      function () { [y([a]=b)] = z }  // test 5 of 7 is no good anymore
  js1_8_1/decompilation/regress-346642-01.js
      f = function () { ({x: a(b)}) = window; }  // test 8 of 28
  js1_8_1/decompilation/regress-380237-04.js
      Two lines are affected:
      needParens(32, "xx() = 3;");  // can just be deleted
      rejectLHS(61, "delete (xx);"); // change to doesNotNeedParens(...)

One test actually changed behavior without exactly breaking:

  *-* Testcase js1_5/decompilation/regress-354910.js failed:
  Bug Number 354910
  STATUS: decompilation of (delete r(s)).t = a;
  Failure messages were:
   FAILED! [reported from test()] decompilation of (delete r(s)).t = a; :
     Expected value ' function ( ) { ( delete r ( s ) ) . t = a ; } ',
     Actual value   ' function ( ) { ( r ( s ) , true ) . t = a ; } ' 

The new behavior is acceptable.

Hope this saves you some work, bc!

Comment 3

[Jason Orendorff [:jorendorff]]

Oh, before this can land, we need to get rid of the use in xpconnect/src/XPCDispObject.cpp.  Filed bug 509103.

Comment 4

[Bob Clary [:bc] (inactive)]

(In reply to comment #2)

> Hope this saves you some work, bc!

Yes, Thank you!

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 393255 [details] [diff] [review]
v1

Nit: no need for ; after end_call: label in any case (whether JS_HAS_LVALUE_RETURN was defined or not -- if not, then end_call: labeled the END_CASE macro call, which was and is fine).

Thanks for the fast surgical removal work!

/be

Comment 6

[Jason Orendorff [:jorendorff]]

Do you think it might be necessary, for web compatibility, to accept this syntax, provided the assignment-expr is never actually evaluated? I'm thinking of stuff like:

  if (msie) {
      // never runs in firefox
      Frob.Controls.Item(0) = Frob.InstantiateWhizBangControl();
  }

We could still remove JS_SetCallReturnValue2 (and IDispatch in bug 509103), but would have to retain much of the rest.  JSOP_SETCALL would just throw a TypeError instead of trying to do something useful.

Comment 7

[Brendan Eich [:brendan]]

Now that you mention it, yeah -- the Bison grammar for WebKit/JavaScriptCore does follow ECMA-262 in producting a() = b; and similar sentences.

Should be a smaller patch. We still should remove the JS_HAS_LVALUE_RETURN configurable of course.

/be

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v2 - Retain parser/decompiler support, drop runtime support

bc: The v2 patch only breaks three tests.

> *-* Testcase js1_5/decompilation/regress-354910.js failed:
> Bug Number 354910
> STATUS: decompilation of (delete r(s)).t = a;
> Failure messages were:
>  FAILED! [reported from test()] decompilation of (delete r(s)).t = a; : Expected value ' function ( ) { ( delete r ( s ) ) . t = a ; } ', Actual value ' function ( ) { ( r ( s ) , true ) . t = a ; } ' 

The new decompilation is acceptable.

> *-* Testcase js1_7/decompilation/regress-352079.js failed:
> Bug Number 352079
> STATUS: decompilation of various operators
> Failure messages were:
>  FAILED! [reported from test()] decompilation of various operators : Expected value ' function ( ) { delete p ( 3 ) ; } ', Actual value ' function ( ) { p ( 3 ) , true ; } ' 

Same here.

> *-* Testcase js1_5/Regress/regress-321874.js failed:
> Bug Number 321874
> [...]
> Failure messages were:
>  FAILED! lhs must be a reference in (for lhs in rhs) ...: for(it.item(0) in b); : Expected value 'foo', Actual value 'error' 
>  FAILED! lhs must be a reference in (for lhs in rhs) ...: function foo(){for(it.item(0) in b);};foo(); : Expected value 'foo', Actual value 'error' 

it.item is gone.  The test should probably just be weakened so that throwing an error here is acceptable.  (That is, we still want to test that this doesn't crash.)

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 393535 [details] [diff] [review]
v2 - Retain parser/decompiler support, drop runtime support

>+            if (js_Invoke(cx, argc, vp, 0))
>+                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_LEFTSIDE_OF_ASS);
>+            goto error;
>           END_CASE(JSOP_SETCALL)

No need for END_CASE after goto error; -- other ops don't do it, and impending jsops.cpp change will need to handle this case other than via goto error anyway. Not sure if the END_MACRO call hurts in that light, but other cases that end their bodies with goto don't use it currently.

Looks good otherwise, r=me -- thanks.

/be

Comment 10

[Brendan Eich [:brendan]]

Should we keep the API entry point but have it fail with an internal error? That might help embedders get up and limping.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

I dunno, we could, but I think it's kinder to make their embedding not link/load than to have it abort or start throwing SyntaxErrors at run time.

Comment 12

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/64674b9f45a5
http://hg.mozilla.org/tracemonkey/rev/270901f18b6d

Comment 13

[Bob Clary [:bc] (inactive)]

update tests
http://hg.mozilla.org/tracemonkey/rev/6c578c9096e8 
js/tests/js1_5/Regress/regress-321874.js
js/tests/js1_5/decompilation/regress-354910.js
js/tests/js1_7/decompilation/regress-352079.js
js/tests/spidermonkey-n-1.9.3.tests

fix incorrectly committed changes to spidermonkey-n-1.9.3.tests.
http://hg.mozilla.org/tracemonkey/rev/304aa5d0cae0
js/tests/spidermonkey-n-1.9.3.tests

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/64674b9f45a5

Comment 15

[Bob Clary [:bc] (inactive)]

(In reply to comment #13)
> update tests
> http://hg.mozilla.org/tracemonkey/rev/6c578c9096e8 

landed on 1.9.2 without the code patch

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/463234ba925b

> http://hg.mozilla.org/tracemonkey/rev/304aa5d0cae0
> js/tests/spidermonkey-n-1.9.3.tests

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/edb042c0e5fe