Remove JS_HAS_LVALUE_RETURN support

RESOLVED FIXED in mozilla1.9.2

Status

()

defect
P1
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: brendan, Assigned: jorendorff)

Tracking

Trunk
mozilla1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
Flags: wanted1.9.2?
(Assignee)

Comment 1

10 years ago
Posted patch v1 (obsolete) — Splinter Review
This is rather naively done, but the result passes all the regression tests I expected it to pass (more on that in a moment).
Assignee: general → jorendorff
Attachment #393255 - Flags: review?(brendan)
(Assignee)

Comment 2

10 years ago
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!
(Assignee)

Comment 3

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

Comment 4

10 years ago
(In reply to comment #2)

> Hope this saves you some work, bc!

Yes, Thank you!
(Reporter)

Comment 5

10 years ago
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
Attachment #393255 - Flags: review?(brendan) → review+

Updated

10 years ago
Flags: wanted1.9.2? → wanted1.9.2+
(Assignee)

Comment 6

10 years ago
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.
(Reporter)

Comment 7

10 years ago
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
(Assignee)

Comment 8

10 years ago
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.)
Attachment #393255 - Attachment is obsolete: true
Attachment #393535 - Flags: review?(brendan)
(Reporter)

Comment 9

10 years ago
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
Attachment #393535 - Flags: review?(brendan) → review+
(Reporter)

Comment 10

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

/be
(Assignee)

Comment 11

10 years ago
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.

Updated

10 years ago
Depends on: 510700
(Assignee)

Updated

10 years ago
Depends on: 510568

Comment 13

10 years ago
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

10 years ago
http://hg.mozilla.org/mozilla-central/rev/64674b9f45a5
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.