1250589 - Remove the must-be-parenthesized requirement from yield expressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Our pre-standard implementation requires yield expressions be parenthesized, or appear inside syntactic parentheses not associated with an expression (e.g. |switch (...)|).  The spec didn't do this, instead making |foo(1, yield 2, 3)| a three-argument function call the user just has to know is such, without benefit of disambiguating parentheses.  We should do the spec thing.

This is good-first-bug territory, IMO.  Probably will require changing some jstests/jit-tests that check the parentheses requirement, otherwise straightforward.

Comment 1

[Daniel Näslund [:dannas]]

Attachment: wip-1250589.patch

Can I be assigned to this issue, please?

Attaching a WIP patch.

Comment 2

[Jeff Walden [:Waldo]]

(In reply to Daniel Näslund from comment #1)
> Can I be assigned to this issue, please?

Planeteer, The Power Is Yours!

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8723201 [details] [diff] [review]
wip-1250589.patch

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

::: js/src/js.msg
@@ +206,5 @@
>  MSG_DEF(JSMSG_LET_STARTING_FOROF_LHS,  0, JSEXN_SYNTAXERR, "an expression X in 'for (X of Y)' must not start with 'let'")
>  MSG_DEF(JSMSG_BAD_GENERATOR_RETURN,    1, JSEXN_TYPEERR,   "generator function {0} returns a value")
> +
> +// TODO(dannas): Can we remove fields from this file? I get a static_assert, complaining that the
> +// expected length of the number of messages do not match.

Yes!  The reason for the static_assert, is that we cache compiled code from browser run to browser run.  This cached code is tagged with a "bytecode version".  The JS engine and the cached bytecode must speak the same language -- the same bytecode version -- for the cache to be usable, else we'll recompile from scratch.

The bytecode version the JS engine recognizes must change whenever the "meaning" of the cached compiled code might be interpreted differently by the JS engine.  One way is if a new JS opcode is added or removed -- you might change the numbering of all the other ops, and then cached code asking for opcode 237 would refer to a different op, if interpreted naively.  Another way is if an opcode's meaning *changes*.  And one way meaning can change, is if the builtin list of errors changes.

The reason is that our cached bytecode includes self-hosted code's bytecode.  And in self-hosted code, we use literal JSMSG_FOO_BAR_BAZ via #defines.  This burns into the self-hosted code the literal numeric value of the error.  So when self-hosted code says

  ThrowTypeError(JSMSG_MISSING_FUN_ARG, 0, 'Array.indexOf');

it really is viewed by the JS engine as if it said

  ThrowTypeError(17, 0, 'Array.indexOf');

where |17| is some integer literal.  Adding or removing an error number, like JS opcodes, will change the meaning of every number above the addition/removal.

What to do, then, to add/remove an error number?  Perform the removal.  Increase the bytecode-version subtrahend in Xdr.h (the one that's been touched a zillion times, if you look at that file's revision history).  Then update the 455 or whatever in the static_assert to the new number of error messages.  This will ensure the JS engine, when interpreting cached bytecode, will version-mismatch on it and not misinterpret any error numbers in cached self-hosted code.

Comment 4

[Daniel Näslund [:dannas]]

Attachment: 1250589.diff

Thank you for your quick review of the previous wip patch.

This patch passes the tests under tests/ and jit-tests/.

BUT in the error-messages.js test, I've replaced the previous - now removed MISSING_PARENS - with PAREN_IN_PAREN. I'm not sure that's correct.

So a top level |(yield 1)| will print... 
    
    "missing ) in parenthetical" 

... Which I find non-intuitive; what parenthesis are missing, they look balanced to me?

To understand why it was marked as PAREN_IN_PAREN, I called gdb -ex "rbreak yieldExpression" --args js. 

From that I inferred that assignExpr() calls yieldExpressionSupported() which returns false. What's going on? Surely the shell supports yield expressions?

So, even though I'm uncertain about how the parsing of parenthesized yield expressions is done, I request a review to keep the feedback-loop alive. Perhaps I've overlooked something very obvious?

Comment 5

[Daniel Näslund [:dannas]]

Can someone help me push to try, please?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

As this is relaxing syntax requirements, I assume only jsreftests / jit-tests need to be tested:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe25ab25907

Comment 7

[Daniel Näslund [:dannas]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> As this is relaxing syntax requirements, I assume only jsreftests /
> jit-tests need to be tested:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe25ab25907

I see one testfailure, something about an addon dir that could not be created. I assume that's not related to my patch.

https://treeherder.mozilla.org/logviewer.html#?job_id=17410720&repo=try#L26037

Can someone commit and push this patch for me, please? Could that someone add a r=Waldo too? I'm afraid to touch the patch after it has passed the try test - I might screw something up.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0e7a6b6f10

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e0e7a6b6f10