381113 - (expclo) Implement ES4/JS2 expression closures for JS1.8

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

See the URL.

Still searching for an easy to type, readable shorthand for 'function'. Greek Lambda (u03BB) is allowed as a lexical identifier by ES3, so it could be in use already. And it's not supported by keyboards around this hemisphere, AFAIK.

/be

Comment 1

[Jesse Ruderman]

Interesting thread at:

http://www.nabble.com/Expression-closures---use-cases-for-shortcut-lambda-syntax-(blocks)-t3411894.html

I'm a fan of the "x => x * x" idea, but "function (x) x * x" is definitely an improvement over "function (x) { return x * x; }".

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Interesting thread at:
> 
> http://www.nabble.com/Expression-closures---use-cases-for-shortcut-lambda-syntax-(blocks)-t3411894.html
> 
> I'm a fan of the "x => x * x" idea, but "function (x) x * x" is definitely an
> improvement over "function (x) { return x * x; }".

Igor had the last word in that thread, and I thought his arguments were convincing. To recap:

(\ formals ) expr
( formals ) => expr
function ( formals ) expr

ranked from shortest to longest are not the only issue. The examples given, including the Y combinator rewritten to use a named function for applySelf, show that it's not all about terseness.

Also, the bottom up parsing (or simulation of same) required by => is not only bad for implementors -- it strongly suggests (Igor and Lars brought this point out too) that readers will have a hard time grokking any non-trivial use of =>.

Anyway, we're sticking with 'function' being the minimum leading word required for all function forms, for JS2/ES4.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed implementation

Try to break this -- it's similar to the let expression parser, but we may have to tweak it for JS2/ES4 (later, and assuming we don't self-host the front end then).

/be

Comment 4

[Jesse Ruderman]

js> function(x) yield x
function (x) {
    return yield x;
}

Should say "generator function returns a value" or something to that effect.

Comment 5

[Jesse Ruderman]

js> function ([]) x
Assertion failure: body->pn_type == TOK_LEXICALSCOPE, at jsparse.c:1380

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v2

The decompiler hates me.

/be

Comment 7

[Jesse Ruderman]

js> (function(){ return ({x setter: function(i)i }) })
Assertion failure: rval[strlen(rval)-1] == '}', at jsopcode.c:4217

Comment 8

[Jesse Ruderman]

Not sure if we care, but there are extra parens in this case:

js> (function() { x = function (i) i; })
function () {
    x = (function (i) i);
}

Comment 9

[Jesse Ruderman]

Need parens to prevent object-literal braces from being interpreted as function body braces:

js> function () function (i) ({p:i})               
function () (function (i) {p:i});

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Not sure if we care, but there are extra parens in this case:
> 
> js> (function() { x = function (i) i; })
> function () {
>     x = (function (i) i);
> }

See the comment that starts with this sentence: "Always parenthesize expression closures." Hard to fix without separating output precedence from the op that's stacked in order for subsequent dependent ops to (a) inspect the exact bytecode or a format flag; (b) load the precedence for that op.

Thinking about how to fix this, but I'm probably not going to bother.

Thanks for the other finds -- got any more?

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v3

Comment 12

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v3a

Tweak jsparse.c per an old Brian Kernighan chestnut (don't make a fatman's gut of the then clause, with a tiny else below -- invert) combined with my general style rule to handle errors first (in then clause, not else).

/be

Comment 13

[Jesse Ruderman]

Bogus semicolon:

js> (function() { L: function() 1 })  
function () {
L:
    (function () 1;);
}

Comment 14

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v3b

That's not valid ECMA-262 code, of course, but an extension we support, although JSOPTION_ANONFUNFIX should be used to make the anonymous function statement (and an unlabeled anonymous function declaration) an error.

Fix is to treat anonymous, grammatically non-lambda expression closures as if they were truly grammatical lambdas when decompiling.

/be

Comment 15

[Brendan Eich [:brendan]]

js> (function() { L: function() 1 })  
function () {
L:
    (function () 1);
}
js> options('anonfunfix')

js> options()
anonfunfix
js> (function() { L: function() 1 })  
typein:4: SyntaxError: syntax error:
typein:4: (function() { L: function() 1 })  
typein:4: .........................^

Comment 16

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v3c

Removed a pointless null test.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v4

I need to talk to ES4 folks about this, but for now the parser must parse an AssignExpr for the body of an expression closure, not an Expr (comma expression), or object initialisers such as

let o = {get foo() this._foo, set foo(bar) this._foo = bar};

can't be parsed (the Expr eats the |, set| and then stops, leaving the rest of the initialiser as a syntax error.

/be

Comment 18

[Jesse Ruderman]

I like it parsing AssignExpr :)  It lets me do things like

x(function(i)i+1, function(i)i*2);

without being surprised by the comma being interpreted as a sequential-evaluation operator.  Few JavaScript developers know about and use the sequential-evaluation operator, and those who do already know to parenthesize to avoid their commas being interpreted as function argument separators.

Comment 19

[Jesse Ruderman]

The decompiler needs to parenthesize comma expressions at the top level of expression closures now:

js> (function() { return {get x() (x, x)}; }) 
function () {
    return {get x() x, x};
}

js> f = (function() { return (function(y) (y, y)); })            
function () {
    return (function (y) y, y);
}

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #18)
> I like it parsing AssignExpr :)  It lets me do things like
> 
> x(function(i)i+1, function(i)i*2);
> 
> without being surprised by the comma being interpreted as a
> sequential-evaluation operator.  Few JavaScript developers know about and use
> the sequential-evaluation operator, and those who do already know to
> parenthesize to avoid their commas being interpreted as function argument
> separators.

IIUC the ES4 idea is to allow unparenthesized comma expressions in expression closure and let bodies where they are not in an argument list, so we should be able to have our cake and eat it. But the formalization is unclear to me, so I'll have to talk to Jeff Dyer before whacking SpiderMonkey harder.

(In reply to comment #19)
> The decompiler needs to parenthesize comma expressions at the top level of
> expression closures now:

Of course -- I hate the decompiler back. Easy to fix, v4a patch later today.

/be

Comment 21

[Jesse Ruderman]

js> f = (function() { ({ get x() (let (w) u), y: 2 }) })
function () {
    ({get x() let (w) u, y:2});
}
js> eval("" + f)
typein:3: SyntaxError: missing } after property list:
typein:3:     ({get x() let (w) u, y:2});
typein:3: ..........................^

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 265368 [details] [diff] [review]
proposed implementation, v4

New patch soon, or my battery died.

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v4a

I'm not so keen on the proposed ES4/JS2 way of allowing unparenthesized comma expressions as let-expression and expression closure bodies *unless* the let or closure is an item directly embedded in a comma-separated list. But here's the patch that implements that parsing rule. Comments welcome.

/be

Comment 24

[Jesse Ruderman]

js> x(function () {1, 2;})
typein:1: SyntaxError: missing ; before statement:
typein:1: x(function () {1, 2;})
typein:1: ................^

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 265774 [details] [diff] [review]
proposed implementation, v4a

In TG1 meeting today we agreed to use AssignExpr for all of let expr, expr closure, and yield (which already uses it), not Expr (comma expression).

/be

Comment 26

[Jesse Ruderman]

I'm still seeing the problem in comment 19.

Comment 27

[Jesse Ruderman]

js> f(b[1, 2])
typein:1: SyntaxError: missing ] in index expression:
typein:1: f(b[1, 2])
typein:1: .....^

Comment 28

[Brendan Eich [:brendan]]

That patch is broken as well as obsolete, don't bother testing it.

/be

Comment 29

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v5

I need to run the testsuite, but this is an improvement based on the TG1 decision yesterday to make AssignmentExpression be the non-terminal used for the bodies of expression closures and let expressions, as for yield's operand.

This patch changes the decompiler so that it no longer forces parens around a let expression. It still forces parens around an expression closure, however, in order to help itself decompiler getters and setters.

/be

Comment 30

[Brendan Eich [:brendan]]

The v5 patch also forces parens around the comma expression operand of return, e.g.

js> (function(){return (x,y)})
function () {
    return (x, y);
}

Necessary without a source note on JSOP_RETURN/JSOP_SETRVAL in order to decompile the corresponding expression closure correctly:

js> (function()(x,y))
function () (x, y)

/be

Comment 31

[Jesse Ruderman]

> The v5 patch also forces parens around the comma expression operand of return

Sorta.  It leaves parens that are there, but doesn't put them in:

js> (function(){return x, y})   
function () {
    return x, y;
}

js> (function(){return (x, y)})
function () {
    return (x, y);
}

Comment 32

[Jesse Ruderman]

js>  f = (function() { (yield 2), 3; })   
function () {
    yield 2, 3;
}
js> eval("(" + f + ")")
typein:5: SyntaxError: yield expression must be parenthesized: [...]

Comment 33

[Jesse Ruderman]

> This patch changes the decompiler so that it no longer forces parens 
> around a let expression.

With the patch, I get extra parens around a let expression in this case:

js> (function() { 1 + let (x) x }) 
function () {
    1 + (let (x) x);
}

Comment 34

[Jesse Ruderman]

And I get extra parens around the inner part of some let-expressions:

js> (function() { return let (x) {} }) 
function () {
    return let (x) ({});
}

But I guess that's reasonable, because with the "return", it would need parens to avoid being interpreted as a let-block rather than a let-expression with an object literal.

Comment 35

[Jesse Ruderman]

Extra parens:

js> (function() { f(let (x) x) }) 
function () {
    f((let (x) x));
}

Comment 36

[Jesse Ruderman]

Here too:

js> (function() { [let (x) x] })  
function () {
    [(let (x) x)];
}

Comment 37

[Jesse Ruderman]

js> f = (function() { (let(x) x), 1 })
function () {
    let (x) x, 1;
}
js> eval("(" + f + ")")
typein:5: SyntaxError: missing ; before statement:
typein:5:     let (x) x, 1;
typein:5: .............^

Comment 38

[Jesse Ruderman]

Round-trip paren change.

js> f = (function() { (y && (!z)); })
function () {
    y && (!z);
}
js> eval("(" + f + ")")
function () {
    y && !z;
}

Comment 39

[Jesse Ruderman]

Same thing with the "new" operator:

js> f = (function() { (x && (new y)) })
function () {
    x && (new y);
}
js> eval("(" + f + ")");
function () {
    x && new y;
}

Comment 40

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v5a

Fix precedence levels in jsopcode.tbl, add "nextop" support to Decompile (see the comment before its declaration), and optimize away parens around any single expr in brackets or parens.

/be

Comment 41

[Brendan Eich [:brendan]]

Comment 31 and comment 33 are still accurate with the latest patch. Comment 34 is overparenthesized no matter how you look at it:

js> (function() { return let (x) {} })
function () {
    return (let (x) ({}));
}

I'll work on this nit, but 31 and 33 are likely to remain accurate.

/be

Comment 42

[Jesse Ruderman]

I made a typo in comment 34: I meant "without the return" when I typed "with the return".

Comment 43

[Jesse Ruderman]

js> f = (function() { yield (2, 3); })    
function () {
    yield 2, 3;
}
js> eval("(" + f + ")")
typein:8: SyntaxError: yield expression must be parenthesized:
typein:8:     yield 2, 3;
typein:8: ....^

Comment 44

[Jesse Ruderman]

js> f = (function() { if ((1, 1) for (x in [])) {} } )
function () {
    if (1, 1 for (x in [])) {
    }
}
js> eval("(" + f + ")");
typein:12: SyntaxError: generator expression must be parenthesized:
typein:12:     if (1, 1 for (x in [])) {
typein:12: ...........^

Comment 45

[Brendan Eich [:brendan]]

Attachment: proposed implementation, v5b

Once again we would like input vs. output precedence in js_CodeSpec. Someone with more energy than I have right now, please file a FIXME bug.

/be

Comment 46

[Brendan Eich [:brendan]]

Attachment: testsuite patch to track paren minimization

I'm likely to commit these changes with a tachyonal r+ bc message.

/be

Comment 47

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/js.c 3.144
js/src/jsconfig.h 3.45
js/src/jsfun.h 3.35
js/src/jsopcode.c 3.248
js/src/jsopcode.tbl 3.95
js/src/jsparse.c 3.285

/be

Comment 48

[Brendan Eich [:brendan]]

Test changes committed:

js/tests/js1_7/block/regress-352609.js 1.3
js/tests/js1_7/decompilation/regress-350704.js 1.3
js/tests/js1_7/decompilation/regress-352026.js 1.3
js/tests/js1_7/decompilation/regress-352079.js 1.3
js/tests/js1_7/decompilation/regress-352272.js 1.3

This means to test the 1.8 branch you'll need an older version of the suite. I hope that's ok. If not, what to do?

/be

Comment 49

[Bob Clary [:bc] (inactive)]

Comment on attachment 266558 [details] [diff] [review]
testsuite patch to track paren minimization

form looks fine.

Comment 50

[Bob Clary [:bc] (inactive)]

(In reply to comment #48)
> 
> This means to test the 1.8 branch you'll need an older version of the suite. I
> hope that's ok. If not, what to do?

This is not that much different from the other changes that have caused failures on older branches. I should be able to flag known failure patterns by branch shortly, so that we could safely ignore the 1.8 branch failures while still catching new failure modes. The other choice is to branch the tests for the 1.8 branch which I would prefer not to do.

Comment 51

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.