Bug 381113 (expclo)

Implement ES4/JS2 expression closures for JS1.8

VERIFIED FIXED in mozilla1.9alpha5

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

Trunk
mozilla1.9alpha5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 10 obsolete attachments)

(Assignee)

Description

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

10 years ago
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; }".
(Assignee)

Comment 2

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

Updated

10 years ago
(Assignee)

Comment 3

10 years ago
Created attachment 265317 [details] [diff] [review]
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
Attachment #265317 - Flags: review?(mrbkap)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Priority: -- → P1

Comment 4

10 years ago
js> function(x) yield x
function (x) {
    return yield x;
}

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

Comment 5

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

(Assignee)

Comment 6

10 years ago
Created attachment 265324 [details] [diff] [review]
proposed implementation, v2

The decompiler hates me.

/be
Attachment #265317 - Attachment is obsolete: true
Attachment #265317 - Flags: review?(mrbkap)

Comment 7

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

Comment 8

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

10 years ago
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});

(Assignee)

Comment 10

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

Comment 11

10 years ago
Created attachment 265341 [details] [diff] [review]
proposed implementation, v3
Attachment #265324 - Attachment is obsolete: true
Attachment #265341 - Flags: review?(mrbkap)
(Assignee)

Comment 12

10 years ago
Created attachment 265342 [details] [diff] [review]
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
Attachment #265341 - Attachment is obsolete: true
Attachment #265342 - Flags: review?(mrbkap)
Attachment #265341 - Flags: review?(mrbkap)

Comment 13

10 years ago
Bogus semicolon:

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

Comment 14

10 years ago
Created attachment 265345 [details] [diff] [review]
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
Attachment #265342 - Attachment is obsolete: true
Attachment #265345 - Flags: review?(mrbkap)
Attachment #265342 - Flags: review?(mrbkap)
(Assignee)

Comment 15

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

Comment 16

10 years ago
Created attachment 265364 [details] [diff] [review]
proposed implementation, v3c

Removed a pointless null test.

/be
Attachment #265345 - Attachment is obsolete: true
Attachment #265364 - Flags: review?(mrbkap)
Attachment #265345 - Flags: review?(mrbkap)
(Assignee)

Comment 17

10 years ago
Created attachment 265368 [details] [diff] [review]
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
Attachment #265364 - Attachment is obsolete: true
Attachment #265368 - Flags: review?(mrbkap)
Attachment #265364 - Flags: review?(mrbkap)

Comment 18

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

10 years ago
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);
}

(Assignee)

Comment 20

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

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

Updated

10 years ago
Attachment #265364 - Attachment description: proposed fix to xpinstall, v3c → proposed implementation, v3c
(Assignee)

Comment 22

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

New patch soon, or my battery died.

/be
Attachment #265368 - Attachment description: proposed fix to xpinstall, v4 → proposed implementation, v4
Attachment #265368 - Attachment is obsolete: true
Attachment #265368 - Flags: review?(mrbkap)
(Assignee)

Comment 23

10 years ago
Created attachment 265774 [details] [diff] [review]
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
Attachment #265774 - Flags: review?(mrbkap)

Comment 24

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

Comment 25

10 years ago
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
Attachment #265774 - Attachment is obsolete: true
Attachment #265774 - Flags: review?(mrbkap)

Comment 26

10 years ago
I'm still seeing the problem in comment 19.

Comment 27

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

Comment 28

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

/be
(Assignee)

Comment 29

10 years ago
Created attachment 265903 [details] [diff] [review]
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
Attachment #265903 - Flags: review?(mrbkap)
(Assignee)

Comment 30

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

Updated

10 years ago
Attachment #265903 - Flags: review?(mrbkap) → review+

Comment 31

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

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

Comment 33

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

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

10 years ago
Extra parens:

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

Comment 36

10 years ago
Here too:

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

Comment 37

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

10 years ago
Round-trip paren change.

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

Comment 39

10 years ago
Same thing with the "new" operator:

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

Comment 40

10 years ago
Created attachment 266509 [details] [diff] [review]
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
Attachment #265903 - Attachment is obsolete: true
Attachment #266509 - Flags: review?(mrbkap)
(Assignee)

Comment 41

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

Updated

10 years ago
Attachment #266509 - Flags: review?(mrbkap) → review+

Comment 42

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

Comment 43

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

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

Comment 45

10 years ago
Created attachment 266533 [details] [diff] [review]
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
Attachment #266509 - Attachment is obsolete: true
Attachment #266533 - Flags: review?(mrbkap)

Updated

10 years ago
Attachment #266533 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 46

10 years ago
Created attachment 266558 [details] [diff] [review]
testsuite patch to track paren minimization

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

/be
Attachment #266558 - Flags: review?
(Assignee)

Comment 47

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 48

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

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

form looks fine.
Attachment #266558 - Flags: review? → review+

Comment 50

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

Updated

10 years ago
Depends on: 382400

Updated

10 years ago
Flags: in-testsuite+

Comment 51

10 years ago
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
Depends on: 485867
You need to log in before you can comment on or make changes to this bug.