Last Comment Bug 1185106 - Implement async functions (ES 2017 proposal)
: Implement async functions (ES 2017 proposal)
Status: RESOLVED FIXED
[DocArea=JS]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: Unspecified Unspecified
: -- normal with 16 votes (vote)
: ---
Assigned To: mkierski
:
: Jason Orendorff [:jorendorff]
Mentors: Eric Faust [:efaust]
Depends on: 1329129 1204024 1208067 1212656 1305333 1313764 1314128 1314380 1316166 1320697
Blocks: html5test 1298286 1313956
  Show dependency treegraph
 
Reported: 2015-07-17 14:14 PDT by mkierski
Modified: 2017-01-17 11:28 PST (History)
45 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
52+

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Part 0: Stub implementation of Promise (12.12 KB, patch)
2015-07-19 12:04 PDT, Till Schneidereit [till]
no flags Details | Diff | Splinter Review
Basic implementation of promises (16.03 KB, patch)
2015-07-21 14:53 PDT, mkierski
no flags Details | Diff | Splinter Review
fix_unified_bustage-v0.diff (2.86 KB, patch)
2015-07-28 11:50 PDT, Terrence Cole [:terrence]
efaustbmo: review+
Details | Diff | Splinter Review
Implemented a polyfill for promises (24.80 KB, patch)
2015-07-28 14:10 PDT, mkierski
no flags Details | Diff | Splinter Review
Fixed (rebased) implementation of promises (21.86 KB, patch)
2015-07-28 16:03 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 1: (terrence) fixes for the StoreBuffer (1.44 KB, patch)
2015-07-29 12:50 PDT, mkierski
mkierski: review+
Details | Diff | Splinter Review
Part 2: (till) Promises boilerplate (11.04 KB, patch)
2015-07-29 12:50 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Part 3: Promises implementation (15.90 KB, patch)
2015-07-29 12:51 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 1: Exclude StoreBuffer.cpp from unified build to prevent build bustage (1.73 KB, patch)
2015-07-30 02:40 PDT, Till Schneidereit [till]
till: review+
Details | Diff | Splinter Review
Part 2: Stub implementation of Promise (10.05 KB, patch)
2015-07-30 02:56 PDT, Till Schneidereit [till]
till: review+
Details | Diff | Splinter Review
Part 3: Promises implementation (19.10 KB, patch)
2015-08-04 15:26 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 4: Updated some parser grammar rules (51.46 KB, patch)
2015-08-04 16:07 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 3: Promises implementation (19.47 KB, patch)
2015-08-05 10:12 PDT, mkierski
no flags Details | Diff | Splinter Review
Refactor DEFFUN in jits (5.85 KB, patch)
2015-08-19 14:54 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Refactor DEFFUN in jits (5.96 KB, patch)
2015-08-21 11:19 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Part 4: Parser grammar rules (64.30 KB, patch)
2015-08-21 14:10 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 5: Bytecode and self-hosted wrapper code (43.21 KB, patch)
2015-08-21 14:12 PDT, mkierski
efaustbmo: feedback+
Details | Diff | Splinter Review
Part 6: Refactored JIT DEFFUN (by efaust) (5.96 KB, patch)
2015-08-21 14:19 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 2: (till) Promises boilerplate (10.95 KB, patch)
2015-09-03 15:50 PDT, mkierski
mkierski: review+
Details | Diff | Splinter Review
Part 3: Promises implementation (21.71 KB, patch)
2015-09-03 16:00 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 4: Parser grammar rules (99.25 KB, patch)
2015-09-03 16:01 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 5: Bytecode and self-hosted wrapper code (31.51 KB, patch)
2015-09-03 16:02 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Part 6: Refactored JIT DEFFUN (by efaust) (5.96 KB, patch)
2015-09-03 16:02 PDT, mkierski
jdemooij: review+
Details | Diff | Splinter Review
Part 2: (till) Promises boilerplate (10.95 KB, patch)
2015-09-17 15:17 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 3: Promises implementation (25.71 KB, patch)
2015-09-17 15:17 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 4: Parser grammar rules (101.42 KB, patch)
2015-09-17 15:18 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Part 5: Bytecode and self-hosted wrapper code (28.74 KB, patch)
2015-09-17 15:19 PDT, mkierski
efaustbmo: review+
till: feedback+
Details | Diff | Splinter Review
Part 6: Refactored JIT DEFFUN (by efaust) (5.96 KB, patch)
2015-09-17 15:20 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Rolled up testing patch (162.72 KB, patch)
2015-09-23 16:05 PDT, mkierski
no flags Details | Diff | Splinter Review
Part 1: (terrence) fixes for the StoreBuffer (11.02 KB, patch)
2015-09-24 12:33 PDT, mkierski
mkierski: review+
Details | Diff | Splinter Review
Part 1: (till) Promises boilerplate (11.02 KB, patch)
2015-09-24 12:34 PDT, mkierski
till: review+
Details | Diff | Splinter Review
Part 2: Promises implementation (28.92 KB, patch)
2015-09-24 12:35 PDT, mkierski
till: review+
Details | Diff | Splinter Review
Part 3: Parser grammar rules (100.12 KB, patch)
2015-09-24 12:56 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Part 4: Bytecode and self-hosted wrapper code (24.94 KB, patch)
2015-09-24 12:57 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Part 5: Refactored JIT DEFFUN (by efaust) (5.96 KB, patch)
2015-09-24 12:57 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Part 2: Promises implementation (29.65 KB, patch)
2015-09-24 15:44 PDT, mkierski
efaustbmo: review+
Details | Diff | Splinter Review
Part 6: Clean up the NIGHTLY_ONLY guards for the feature, and allow tests to uplift cleanly (22.57 KB, patch)
2015-10-01 15:27 PDT, Eric Faust [:efaust]
jwalden+bmo: review+
Details | Diff | Splinter Review
Fix busted serviceworkers interfaces test for adding ShellPromise (974 bytes, patch)
2015-10-07 15:27 PDT, Eric Faust [:efaust]
ehsan: review+
Details | Diff | Splinter Review
Part 1: Refactor JSOP_DEFFUN. (11.41 KB, patch)
2016-05-16 21:14 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 2: Add parser support for Async functions. r=efaust (96.59 KB, patch)
2016-05-16 21:15 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 4: Fix function name for export default async function. (2.43 KB, patch)
2016-05-16 21:16 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3: Implement async functions. r=efaust,jandem (28.52 KB, patch)
2016-05-16 21:17 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 4: Fix function name for export default async function. (2.43 KB, patch)
2016-05-16 21:20 PDT, Tooru Fujisawa [:arai]
efaustbmo: review+
Details | Diff | Splinter Review
Part 5: Treat yield as an identifier in async function parameters and body. (22.22 KB, patch)
2016-05-16 21:20 PDT, Tooru Fujisawa [:arai]
efaustbmo: review+
Details | Diff | Splinter Review
Part 6: Support async arrow function. (26.65 KB, patch)
2016-05-16 21:26 PDT, Tooru Fujisawa [:arai]
efaustbmo: review+
Details | Diff | Splinter Review
Part 7: Add parser test for async/await. (6.68 KB, patch)
2016-05-16 21:29 PDT, Tooru Fujisawa [:arai]
efaustbmo: review+
Details | Diff | Splinter Review
Part 8: Add Reflect.parse test for async/await. (5.71 KB, patch)
2016-05-16 21:31 PDT, Tooru Fujisawa [:arai]
efaustbmo: review+
Details | Diff | Splinter Review
Part 4: Fix function name for export default async function. r=efaust (2.44 KB, patch)
2016-05-21 13:17 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 5: Treat yield as an identifier in async function parameters and body. r=efaust (22.51 KB, patch)
2016-05-21 13:17 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 6: Support async arrow function. r=efaust (28.52 KB, patch)
2016-05-21 13:18 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 6.1: Fix yield handling in async function. (31.87 KB, patch)
2016-05-21 13:25 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6.2: Fix await handling in async function. (24.34 KB, patch)
2016-05-21 13:33 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7: Add parser test for async/await. r=efaust (6.69 KB, patch)
2016-05-21 13:34 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 8: Add Reflect.parse test for async/await. r=efaust (5.72 KB, patch)
2016-05-21 13:34 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 9: Fix syntax priority of async function expression. (3.71 KB, patch)
2016-05-23 19:08 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10: Add AsyncFunction constructor and prototype. (27.53 KB, patch)
2016-05-23 19:15 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11: Set length of async function. (2.98 KB, patch)
2016-05-23 19:16 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 12: Support async function in Function.prototype.toString. (10.62 KB, patch)
2016-05-23 19:23 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10: Add AsyncFunction constructor and prototype. (39.78 KB, patch)
2016-05-24 08:54 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11: Support async function in Function.prototype.toString. (3.69 KB, patch)
2016-05-24 08:55 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 1: Refactor JSOP_DEFFUN. r=efaust,jandem (11.10 KB, patch)
2016-07-17 02:48 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 2: Add parser support for Async functions. r=efaust,jwalden (96.41 KB, patch)
2016-07-17 02:49 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 3: Implement async functions. r=efaust,jandem (29.89 KB, patch)
2016-07-17 02:49 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 4: Fix function name for export default async function. r=efaust (2.50 KB, patch)
2016-07-17 02:50 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 5: Treat yield as an identifier in async function parameters and body. r=efaust (22.71 KB, patch)
2016-07-17 02:50 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 6: Support async arrow function. r=efaust (28.58 KB, patch)
2016-07-17 02:50 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 6.1: Fix yield handling in async function. (32.03 KB, patch)
2016-07-17 02:51 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6.2: Fix await handling in async function. (24.26 KB, patch)
2016-07-17 02:52 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7: Add parser test for async/await. r=efaust (6.69 KB, patch)
2016-07-17 02:52 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 8: Add Reflect.parse test for async/await. r=efaust (5.72 KB, patch)
2016-07-17 02:52 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 9: Fix syntax priority of async function expression. (3.73 KB, patch)
2016-07-17 02:53 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10: Add AsyncFunction constructor and prototype. (40.19 KB, patch)
2016-07-17 02:55 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11: Support async function in Function.prototype.toString. (3.73 KB, patch)
2016-07-17 02:55 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 0.1: Refactor JSOP_DEFFUN. r=efaust,jandem (11.32 KB, patch)
2016-08-29 01:52 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 0.2: Pass GeneratorKind to Parser::functionFormalParametersAndBody and calculate YieldHandling inside it. r?efaust (5.61 KB, patch)
2016-08-29 01:54 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 0.3: Pass YieldHandling to Parser::checkYieldNameValidity for later use. r?efaust (9.23 KB, patch)
2016-08-29 01:54 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 0.4: Pass YieldHandling to Parser::functionFormalParametersAndBody for later use. r?efaust (16.55 KB, patch)
2016-08-29 01:55 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 1: Add AsyncFunction flag in FunctionBox, JSScript, and LazyScript. r=efaust (20.10 KB, patch)
2016-08-29 01:56 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 2: Add FunctionAsyncKind parameter to Parser methods that receives GeneratorKind. r=efaust (26.72 KB, patch)
2016-08-29 01:56 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 3: Add await token. r=efaust (4.70 KB, patch)
2016-08-29 01:57 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 4: Add AutoAwaitIsKeyword class to set TokenStream.awaitIsKeyword. r?efaust (2.55 KB, patch)
2016-08-29 01:58 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.1: Support async function declaration in Parser. r=efaust,jwalden (12.38 KB, patch)
2016-08-29 01:58 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 5.2: Support async function declaration in Reflect.parse. r=efaust (4.87 KB, patch)
2016-08-29 01:59 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 5.3: Support await expression in Parser. r?efaust (25.43 KB, patch)
2016-08-29 02:00 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.4: Support await expression in Reflect.parse. r=efaust (2.61 KB, patch)
2016-08-29 02:00 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 5.5: Add parser test for async function declaration. r=efaust (13.15 KB, patch)
2016-08-29 02:01 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 5.6: Add parser test for yield in async function declaration. r?efaust (2.14 KB, patch)
2016-08-29 02:02 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6.1: Support async function expression in Parser. r?efaust (4.61 KB, patch)
2016-08-29 02:04 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6.2: Add parser test for async function expression. r=efaust (7.64 KB, patch)
2016-08-29 02:05 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 6.3: Add parser test for yield in async function expression. r?efaust (2.53 KB, patch)
2016-08-29 02:05 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7.1: Support async method in Parser. r=efaust,jwalden (7.64 KB, patch)
2016-08-29 02:06 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 7.2: Add parser test for async method. r=efaust (2.86 KB, patch)
2016-08-29 02:07 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 7.3: Add parser test for yield in async method. r?efaust (3.53 KB, patch)
2016-08-29 02:09 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 8.1: Treat await as keyword in module. r=efaust (917 bytes, patch)
2016-08-29 02:09 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 8.2: Add parser test for await in module. r=efaust (1.11 KB, patch)
2016-08-29 02:10 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 9.1: Support async function statement in export default in Parser. r=efaust (1.77 KB, patch)
2016-08-29 02:11 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 9.2: Add parser test for async function statement in export default. r=efaust (1.84 KB, patch)
2016-08-29 02:11 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 9.3: Add parser test for yield in async function statement in export default. r?efaust (2.69 KB, patch)
2016-08-29 02:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10.1: Support async arrow function in Parser. r?efaust (15.69 KB, patch)
2016-08-29 02:14 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10.2: Support async arrow function in Reflect.parse. r=efaust (3.24 KB, patch)
2016-08-29 02:14 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 10.3: Add parser test for async arrow function. r=efaust (9.53 KB, patch)
2016-08-29 02:15 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 11.1: Implement async functions. r?efaust (49.54 KB, patch)
2016-08-29 02:16 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.2: Add helper functions for async/await test. r?efaust. (1.52 KB, patch)
2016-08-29 02:17 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.3: Add semantics test for async/await. r=efaust. (7.08 KB, patch)
2016-08-29 02:17 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 11.4: Add function length test for async function. r?efaust. (956 bytes, patch)
2016-08-29 02:18 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.5: Add async function constructor and prototype. r?efaust. (1.64 KB, patch)
2016-08-29 02:18 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.6: Add test for async function expression binding identity. r?efaust (1.18 KB, patch)
2016-08-29 02:19 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 12: Return wrapped function for arguments.callee. r?efaust (5.12 KB, patch)
2016-08-29 02:19 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 13: Support async function in Function.prototype.toString. r?efaust (3.91 KB, patch)
2016-08-29 02:21 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 0.1: Refactor JSOP_DEFFUN. r=efaust,jandem (11.32 KB, patch)
2016-09-26 20:00 PDT, Tooru Fujisawa [:arai]
arai.unmht: review+
Details | Diff | Splinter Review
Part 0.2: Pass GeneratorKind to Parser::functionFormalParametersAndBody and calculate YieldHandling inside it (5.77 KB, patch)
2016-09-26 20:03 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 0.3: Pass YieldHandling to Parser::functionFormalParametersAndBody for later use (20.53 KB, patch)
2016-09-26 20:06 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 1: Add AsyncFunction flag in FunctionBox, JSScript, and LazyScript (21.50 KB, patch)
2016-09-26 20:08 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 2: Add FunctionAsyncKind parameter to Parser methods that receives GeneratorKind (26.41 KB, patch)
2016-09-26 20:08 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 3: Add await token (4.55 KB, patch)
2016-09-26 20:09 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 4: Add AutoAwaitIsKeyword class to set TokenStream.awaitIsKeyword (2.55 KB, patch)
2016-09-26 20:09 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.1: Support async function declaration in Parser (14.46 KB, patch)
2016-09-26 20:10 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.2: Support async function declaration in Reflect.parse (4.87 KB, patch)
2016-09-26 20:11 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.3: Support await expression in Parser (25.32 KB, patch)
2016-09-26 20:11 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.4: Support await expression in Reflect.parse (2.61 KB, patch)
2016-09-26 20:11 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.5: Add parser test for async function declaration (13.15 KB, patch)
2016-09-26 20:11 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 5.6: Add parser test for yield in async function declaration (2.14 KB, patch)
2016-09-26 20:11 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6.1: Support async function expression in Parser (6.61 KB, patch)
2016-09-26 20:11 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6.2: Add parser test for async function expression (7.64 KB, patch)
2016-09-26 20:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 6.3: Add parser test for yield in async function expression (2.53 KB, patch)
2016-09-26 20:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7.1: Support async method in Parser (7.66 KB, patch)
2016-09-26 20:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7.2: Add parser test for async method (2.86 KB, patch)
2016-09-26 20:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 7.3: Add parser test for yield in async method (3.53 KB, patch)
2016-09-26 20:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 8.1: Treat await as keyword in module (920 bytes, patch)
2016-09-26 20:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 8.2: Add parser test for await in module (1.11 KB, patch)
2016-09-26 20:12 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 9.1: Support async function statement in export default in Parser (1.77 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 9.2: Add parser test for async function statement in export default (1.84 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 9.3: Add parser test for yield in async function statement in export default (2.69 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10.1: Support async arrow function in Parser (14.82 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10.2: Support async arrow function in Reflect.parse (3.24 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 10.3: Add parser test for async arrow function (9.53 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.1: Implement async functions (52.53 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.2: Add helper functions for async/await test (1.52 KB, patch)
2016-09-26 20:13 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.3: Add semantics test for async/await (7.08 KB, patch)
2016-09-26 20:14 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.4: Add function length test for async function (956 bytes, patch)
2016-09-26 20:14 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.5: Add async function constructor and prototype (1.64 KB, patch)
2016-09-26 20:14 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 11.6: Add test for async function expression binding identity (1.18 KB, patch)
2016-09-26 20:14 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 12: Return wrapped function for arguments.callee (5.12 KB, patch)
2016-09-26 20:14 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
Part 13: Support async function in Function.prototype.toString (3.91 KB, patch)
2016-09-26 20:14 PDT, Tooru Fujisawa [:arai]
no flags Details | Diff | Splinter Review
a.txt (5 bytes, text/plain)
2016-10-08 13:47 PDT, Tooru Fujisawa [:arai]
no flags Details
Bug 1185106 - Part 0.1: Refactor JSOP_DEFFUN. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
hv1989: review+
Details | Review
Bug 1185106 - Part 1: Add AsyncFunction flag in FunctionBox, JSScript, and LazyScript. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 2: Add FunctionAsyncKind parameter to Parser methods that receives GeneratorKind. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 3: Add await token. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 4: Add AutoAwaitIsKeyword class to set TokenStream.awaitIsKeyword. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 5.1: Support async function declaration in Parser. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 5.2: Support async function declaration in Reflect.parse. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 5.3: Support await expression in Parser. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 5.4: Support await expression in Reflect.parse. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 5.5: Add parser test for async function declaration. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 5.6: Add parser test for yield in async function declaration. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 6.1: Support async function expression in Parser. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 6.2: Add parser test for async function expression. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 6.3: Add parser test for yield in async function expression. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 7.1: Support async method in Parser. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 7.2: Add parser test for async method. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 7.3: Add parser test for yield in async method. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 8.1: Treat await as keyword in module. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 8.2: Add parser test for await in module. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 9.1: Support async function statement in export default in Parser. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 9.2: Add parser test for async function statement in export default. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 9.3: Add parser test for yield in async function statement in export default. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 10.1: Support async arrow function in Parser. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 10.2: Support async arrow function in Reflect.parse. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 10.3: Add parser test for async arrow function. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 11.1: Implement async functions. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 11.2: Add helper functions for async/await test. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 11.3: Add semantics test for async/await. .,till (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
no flags Details | Review
Bug 1185106 - Part 11.4: Add function length test for async function. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 11.5: Add async function constructor and prototype. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 11.6: Add test for async function expression binding identity. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 12: Return wrapped function for arguments.callee. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Bug 1185106 - Part 13: Support async function in Function.prototype.toString. (58 bytes, text/x-review-board-request)
2016-10-08 13:50 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Review
Part 14: Add AsyncFunction.prototype[@@toStringTag]. (1.98 KB, patch)
2016-10-28 06:29 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Diff | Splinter Review
Part 7.4: Fix property name parsing with async name. (2.95 KB, patch)
2016-10-28 11:40 PDT, Tooru Fujisawa [:arai]
till: review+
Details | Diff | Splinter Review

Description User image mkierski 2015-07-17 14:14:18 PDT
Proposal: https://tc39.github.io/ecmascript-asyncawait/
Comment 1 User image Till Schneidereit [till] 2015-07-17 14:17:51 PDT
ni? for myself because I promised to provide a bare-bones self-hosted constructor for Promise, to be used as the implementation vehicle here.
Comment 2 User image Till Schneidereit [till] 2015-07-19 12:04:01 PDT
Created attachment 8635822 [details] [diff] [review]
Part 0: Stub implementation of Promise

This implements the Promise object as an empty stub. It also has stubs for all the (static) methods, but doesn't store the callbacks anywhere. I guess converting it to an extended function and using the reserved slots might just work.
Comment 3 User image mkierski 2015-07-21 14:53:07 PDT
Created attachment 8636818 [details] [diff] [review]
Basic implementation of promises
Comment 4 User image Eric Faust [:efaust] 2015-07-22 14:55:25 PDT
Comment on attachment 8636818 [details] [diff] [review]
Basic implementation of promises

Punting to till, the selfhosting guru.
Comment 5 User image Till Schneidereit [till] 2015-07-23 10:01:02 PDT
Comment on attachment 8636818 [details] [diff] [review]
Basic implementation of promises

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

For future reference, it's best to keep changes in separate patches, at least while reviewing. Much easier to separate out what you changed on top of my changes. As it is now, I'm effectively reviewing my own changes, too.

Please make sure that all indentation is using spaces, not tabs. Promise.js contains a mix of both.

I added lots of additional comments below that need to be addressed. In general, you should read the self-hosting documentation once again to see what the restrictions are compared to normal JS code. I didn't note all issues in Promise.js because most are repeats of the same issues already mentioned.

Also, the implementation isn't spec-compliant in various ways. I don't know how important that is for your project, you should discuss that with efaust. At the very least, I'd like to see comments about deviations from the spec, so we're aware of them.

::: js/src/builtin/Promise.cpp
@@ +20,5 @@
> +static const JSFunctionSpec promise_static_methods[] = {
> +    JS_SELF_HOSTED_FN("all", "Promise_all", 1, 0),
> +    JS_SELF_HOSTED_FN("race", "Promise_race", 1, 0),
> +    JS_SELF_HOSTED_FN("reject", "Promise_reject", 1, 0),
> +    JS_SELF_HOSTED_FN("init", "Promise_init", 2, 0),

You only need to install functions on an object if they're exposed to content and should be called on that object. That's not the case for Promise_init: it's neither supposed to be exposed to content (but is, by this line), nor should you ever call it via Promise.init. (But see below for more issues with this function.)

@@ +34,5 @@
> +    HandleValue fn = args.get(0);
> +
> +    if (!IsCallable(fn)) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_NOT_FUNCTION,
> +                             "Promise constructor argument");

Please test which errors the current implementation in Gecko throws here and match those. (Hint, test for too few arguments in addition to IsCallable, and fix the error message.)

@@ +43,5 @@
> +    JSObject* obj = NewBuiltinClassInstance(cx, &PromiseObject::class_);
> +    if (!obj)
> +        return false;
> +
> +    Rooted<JSObject*> rt(cx, obj);

This can be RootedObject. Also, you can get rid of the JSObject* above and just use
RootedObject obj(cx, NewBuiltinClassInstance(cx, &PromiseObject::class_));
if (!obj)
    return false;

(and change all usages of rt to obj below.)

@@ +45,5 @@
> +        return false;
> +
> +    Rooted<JSObject*> rt(cx, obj);
> +    JSObject* promiseConstructor = JS_GetConstructor(cx, rt);
> +    Rooted<JSObject*> rtPromiseConstructor(cx, promiseConstructor);

Same here: RootedObject, and no intermediary JSObject*.

@@ +56,5 @@
> +    JS::AutoValueVector argValues(cx);
> +    argValues.append(promise);
> +    argValues.append(fn);
> +    HandleValueArray arr(argValues);
> +    JS_CallFunctionName(cx, rtPromiseConstructor, "init",

There are some problems with this: the most important one is that content could just change the way promises are initialized by doing `Promise.init = myEvilFunction`. However, it's also quite a bit slower than it'd need to be.

Instead, the instance should be fully initialized in C++ code. The way things are set up right now, that'd be quite annoying: defining properties on JSObjects from C++ sucks a bit. However, we shouldn't do that anyway: right now, the internal state of a promise could just be changed by content code by doing assigning to the _state, _value, or _deferreds properties. (Or changing their internal values.)

So how should we do this instead, you ask. By using reserved slots. A good example for how to use those and initialize instances with enough of them is the ArrayIterator in builtin/Array.js. Take a look at CreateArrayIteratorAt in that file: it calls the intrinsic NewArrayIterator (defined and installed in SelfHosting.cpp). That returns an object with 3 reserved slots. Check ArrayIteratorObject::class_ in jsiter.cpp for how that works. Then observe how the UnsafeSetReservedSlot intrinsic is used to set the slots. Reserved slots can also be set from C++, which is what you want to do in the constructor. Try to understand how the ArrayIterator implementation works: it'll help you understand how to use the reserved slots from both C++ and self-hosted JS code.

::: js/src/builtin/Promise.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function Promise_all(promises) {
> +		return new Promise(function (resolve, reject) {

As described above, this needs to use an intrinsic function for calling the C++-implemented constructor, similar to how things are done for ArrayIterator.

@@ +5,5 @@
> +function Promise_all(promises) {
> +		return new Promise(function (resolve, reject) {
> +			if (promises.length === 0) return resolve([]);
> +			var remaining = promises.length;
> +			function res(i, val) {

Please give all functions and variables speaking names. If they're a bit longer, that's ok.

@@ +7,5 @@
> +			if (promises.length === 0) return resolve([]);
> +			var remaining = promises.length;
> +			function res(i, val) {
> +				try {
> +					if (val && (typeof val === 'object' || typeof val === 'function')) {

There's an `IsCallable` intrinsic which should be used here. Somewhat irritatingly, not all callables are functions.

@@ +10,5 @@
> +				try {
> +					if (val && (typeof val === 'object' || typeof val === 'function')) {
> +						var then = val.then;
> +						if (typeof then === 'function') {
> +							then.call(val, function (val) { res(i, val) }, reject);

In self-hosted code, using .call and .apply (and pretty much all other method calls) is forbidden in 99.9% of all cases. This code would break if content code overrode Function.prototype.call. Use `callFunction` instead.

@@ +23,5 @@
> +					reject(ex);
> +				}
> +			}
> +			for (var i = 0; i < promises.length; i++) {
> +				res(i, promises[i]);

Can't you just inline the contents of `res` into this loop?

@@ +28,5 @@
> +			}
> +		});
> +}
> +
> +function Promise_race(values) {

This doesn't really implement Promise.prototype.race, right? It's somewhat ok if the implementation is just enough to get async/await working, but if so, please add a comment explaining why it's done this way. If race isn't needed to implement async/await at all, I'd much rather see this replaced with a stub that just throws an exception saying that it's not really implemented.

@@ +31,5 @@
> +
> +function Promise_race(values) {
> +    return new Promise(function (resolve, reject) {
> +        for (var i = 0, len = values.length; i < len; i++) {
> +            values[i].then(resolve, reject);

Same here: no method calls without callFunction.

@@ +43,5 @@
> +    });
> +}
> +
> +function Promise_resolve(value) {
> +    if (value && typeof value === 'object' && value.constructor === Promise) {

`value.constructor === Promise` probably isn't the right check: it can be fooled by changing value.__proto__, Promise.prototype.constructor, or value.constructor. Check the specification for how this check is supposed to work.

@@ +57,5 @@
> +  return this.then(null, onRejected);
> +}
> +
> +function asap(cb) {
> +  cb();

This implementation of asap will definitely not be sufficient for your needs. It's going to be very important to delay running `cb` until the next turn of the event loop. That's probably the most annoying part of doing all this in SpiderMonkey. (Though it might also be easier than I think.) Talk to efaust or Waldo about how to approach this.

@@ +93,5 @@
> +
> +MakeConstructible(Promise, {
> +  then: Promise_then,
> +  catch: Promise_catch
> +});

As explained above, you can't use this constructor but have to use an intrinsic as is done for ArrayIterator.

There's another reason for this that I didn't explain before: a promise created this way, using `new Promise` in self-hosted code will not have the same constructor as one created using `new Promise` in content code. That's because the `Promise` constructor available to content code isn't the one you have here. Instead it's the one created from PromiseObject::class_. One consequence here is that the `constructor` properties would be different, als would be __proto__.

::: js/src/gc/StoreBuffer.cpp
@@ +105,5 @@
>  
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::ValueEdge>;
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::CellPtrEdge>;
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::SlotsEdge>;
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::WholeCellEdges>;

This needs review from someone else. I commented out these lines to get things to compile, but really, that's not the right solution. My guess is that adding a new source file changed which cpp files get compiled together, causing issues here. Talk to efaust or waldo about how best to deal with this.
Comment 6 User image Terrence Cole [:terrence] 2015-07-28 11:50:24 PDT
Created attachment 8640005 [details] [diff] [review]
fix_unified_bustage-v0.diff

This patch fixes the unified bustage.

The issue here is that we may or may not need the template instantiations depending one what code the compiler sees in the translation unit containing the instantiations.
Comment 7 User image Eric Faust [:efaust] 2015-07-28 12:21:23 PDT
Comment on attachment 8640005 [details] [diff] [review]
fix_unified_bustage-v0.diff

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

Seems like it should fix the problem. There's no way commenting out those specializations can be correct, so I'm willing to believe this should be the way forward.
Comment 8 User image mkierski 2015-07-28 14:10:01 PDT
Created attachment 8640126 [details] [diff] [review]
Implemented a polyfill for promises
Comment 9 User image Eric Faust [:efaust] 2015-07-28 14:49:55 PDT
Comment on attachment 8640126 [details] [diff] [review]
Implemented a polyfill for promises

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

::: js/src/moz.build
@@ +343,4 @@
>  SOURCES += [
>      'builtin/RegExp.cpp',
>      'frontend/Parser.cpp',
> +    'gc/StoreBuffer.cpp',

At the very least, the patch that terrence wrote should land seperately. This fix has nothing to do with the patch for the polyfill.
Comment 10 User image mkierski 2015-07-28 16:03:07 PDT
Created attachment 8640212 [details] [diff] [review]
Fixed (rebased) implementation of promises

Note: It is dependent on terrence's patch (does not include it)
Comment 11 User image Till Schneidereit [till] 2015-07-29 03:36:37 PDT
Comment on attachment 8640212 [details] [diff] [review]
Fixed (rebased) implementation of promises

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

I've been thinking about this some more and would really prefer for this to not include my original patch, either. The reason is that I'm deeply uncomfortable with reviewing my own code.

The way this works is that you first apply terrence's patch, then mine, then yours. Change the descriptions to "Bug 1185106 - Part N: description". That way, it's clear which order they have to be applied in without additional comments.

Clearing review for now, please re-request for the new patch.
Comment 12 User image mkierski 2015-07-29 12:50:04 PDT
Created attachment 8640668 [details] [diff] [review]
Part 1: (terrence) fixes for the StoreBuffer
Comment 13 User image mkierski 2015-07-29 12:50:48 PDT
Created attachment 8640670 [details] [diff] [review]
Part 2: (till) Promises boilerplate
Comment 14 User image mkierski 2015-07-29 12:51:37 PDT
Created attachment 8640672 [details] [diff] [review]
Part 3: Promises implementation
Comment 15 User image Eric Faust [:efaust] 2015-07-29 15:16:44 PDT
Comment on attachment 8640670 [details] [diff] [review]
Part 2: (till) Promises boilerplate

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

I believe this.

::: js/src/gc/StoreBuffer.cpp
@@ +105,5 @@
>  
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::ValueEdge>;
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::CellPtrEdge>;
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::SlotsEdge>;
> +// template struct StoreBuffer::MonoTypeBuffer<StoreBuffer::WholeCellEdges>;

We should not still do this. We even have a patch in this series so that it's not necessary.
Comment 17 User image Till Schneidereit [till] 2015-07-30 02:40:46 PDT
Created attachment 8640988 [details] [diff] [review]
Part 1: Exclude StoreBuffer.cpp from unified build to prevent build bustage

Fixed up the patch format for part 1 and landed:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48a35f84fb9d
Comment 18 User image Till Schneidereit [till] 2015-07-30 02:56:16 PDT
Created attachment 8640998 [details] [diff] [review]
Part 2: Stub implementation of Promise

Removed the StoreBuffer.cpp changes from part 2, carrying review.

I didn't land this because it doesn't make any sense whatsoever. We might fold it in with part 3 in the end, not sure yet.
Comment 19 User image Till Schneidereit [till] 2015-07-30 04:54:03 PDT
Comment on attachment 8640672 [details] [diff] [review]
Part 3: Promises implementation

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

Thanks for the new patch. This is getting closer, but still has quite a few issues. Please read through the comments carefully and try to address them all. I'm still particularly concerned with the asap stuff and don't fully understand how it works. For the parts of the code that aren't straight-forward, comments would be really important.

::: js/src/builtin/Promise.cpp
@@ +7,5 @@
>  #include "builtin/Promise.h"
>  
>  #include "jscntxt.h"
>  
> +#define PROMISE_STATE_SLOT 0

You can add these defines to builtin/SelfHostingDefines.h, include that in this cpp file, and remove them from here and from Promise.js. That way they're not duplicated and can't accidentally change in only one place.

@@ +33,5 @@
>  PromiseConstructor(JSContext* cx, unsigned argc, Value* vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    HandleValue fn = args.get(0);

Move this to below the args length check.

@@ +37,5 @@
> +    HandleValue fn = args.get(0);
> +
> +    if (args.length() == 0) {
> +      JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
> +          "Promise.constructor", "0", "s");

Nit: check the indentations and general formatting in this file, too.

@@ +53,5 @@
>          return false;
> +
> +    RootedObject promiseConstructor(cx, JS_GetConstructor(cx, promise));
> +
> +    JS_SetReservedSlot(promise, PROMISE_STATE_SLOT, NullValue()); // state

See the comment for reject in Promise.js.

@@ +55,5 @@
> +    RootedObject promiseConstructor(cx, JS_GetConstructor(cx, promise));
> +
> +    JS_SetReservedSlot(promise, PROMISE_STATE_SLOT, NullValue()); // state
> +    JS_SetReservedSlot(promise, PROMISE_VALUE_SLOT, NullValue()); // value
> +    JS_SetReservedSlot(promise, PROMISE_DEFERREDS_SLOT, ObjectValue(*JS_NewArrayObject(cx, 0))); // deferreds

The result of calling JS_NewArrayObject needs to be checked: it could fail, just as NewBuiltinClassInstance above could.

However, we really don't want to call JS_NewArrayObject here: it creates an array in the content compartment, with the behavior

@@ +56,5 @@
> +
> +    JS_SetReservedSlot(promise, PROMISE_STATE_SLOT, NullValue()); // state
> +    JS_SetReservedSlot(promise, PROMISE_VALUE_SLOT, NullValue()); // value
> +    JS_SetReservedSlot(promise, PROMISE_DEFERREDS_SLOT, ObjectValue(*JS_NewArrayObject(cx, 0))); // deferreds
> +    JS_SetReservedSlot(promise, PROMISE_CONSTRUCTOR_SLOT, ObjectValue(*promiseConstructor)); // constructor

As explained in Promise.js, I don't think you need this slot. If that's correct, please remove it and change the slot count to 3.

@@ +64,5 @@
> +    argValues.append(ObjectValue(*promise));
> +    argValues.append(fn);
> +    HandleValueArray arr(argValues);
> +
> +    RootedPropertyName name(cx, JS_AtomizeAndPinString(cx, "Promise_init")->asAtom().asPropertyName());

Use Atomize(cx, "Promise_init", 12) here. You also need to check the result because the allocation might fail.

@@ +70,5 @@
> +
> +    if (!GlobalObject::getIntrinsicValue(cx, cx->global(), name, &selfHostedFun))
> +        return false;
> +
> +    JS_CallFunctionValue(cx, promise, selfHostedFun, arr, &initRval);

I *think* this should check for the result of calling the function and return false without setting the return value. Then you can also just return true in the last line.

::: js/src/builtin/Promise.h
@@ +23,5 @@
>  
>  } // namespace js
>  
> +bool
> +PromiseConstructor(JSContext* cx, unsigned argc, Value* vp);

Move this into the js namespace.

::: js/src/builtin/Promise.js
@@ +1,2 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this

Nit: undo this change.

@@ +8,2 @@
>  
> +/* This implementation is by no means complete and is using a polyfill.

Nit: please change the comment format to the following:
/**
 * Text
 * more text
 */

@@ +24,3 @@
>  }
>  
> +MakeConstructible(Handler, {});

Use std_Object_create(null) instead of {} here.

@@ +24,5 @@
>  }
>  
> +MakeConstructible(Handler, {});
> +
> +function Promise_setState(state) {

For all these internal utility functions (the Promise_getFoo, etc ones), I think it'd be much better to explicitly pass in the promise and then just call them: `Promise_getState(promise)`

Also, this "state" is whether the promise was resolved or not, right? In that case, change it to "Promise_setIsResolved", and the getter to "Promise_getIsResolved". (Note: I think I misunderstood this, so the name is probably ok as-is. See the comment for Promise_reject below.)

@@ +30,4 @@
>  }
>  
> +function Promise_getState() {
> +  return UnsafeGetReservedSlot(this, PROMISE_STATE_SLOT);

Use UnsafeGetObjectFromReservedSlot for all object slots and UnsafeGetBooleanFromReservedSlot for bools.

@@ +53,5 @@
> +  return UnsafeGetReservedSlot(this, PROMISE_CONSTRUCTOR_SLOT);
> +}
> +
> +function Promise_init(promise, fn) {
> +  doResolve(fn, resolve.bind(promise), reject.bind(promise));

I don't think using "bind" is ok here. If content changes Function.prototype.bind, that'll change what happens here. Wrap the functions into closures instead: `function() {callFunction(resolve, promise}`. That's better for performance, too.

@@ +56,5 @@
> +function Promise_init(promise, fn) {
> +  doResolve(fn, resolve.bind(promise), reject.bind(promise));
> +}
> +
> +function Promise_all(promises) {

This is supposed to take an iterable, not just arrays. I think it's ok for now to only deal with arrays (and array-likes), but please add a comment mentioning this.

@@ +57,5 @@
> +  doResolve(fn, resolve.bind(promise), reject.bind(promise));
> +}
> +
> +function Promise_all(promises) {
> +  var results = [];

This code is unsafe: it creates an Array with the __proto__ being the content compartment's Array.prototype. If there's a setter defined for an index, that will be called instead of the value being stored in the index.

Also, I think it's best to only call promises.length once, so use something like
var length = promises.length;
var results = NewDenseArray(length);

And change all usages of promises.length below to use length.

@@ +60,5 @@
> +function Promise_all(promises) {
> +  var results = [];
> +  return NewPromise(function (resolve, reject) {
> +    if (promises.length === 0)
> +			return resolve([]);

Change the indentation to spaces, here and everywhere else. In general, check the formatting of all code once again: there are various alignment issues, which I didn't explicitly call out below.

@@ +68,5 @@
> +        if (valueOrPromise && (typeof valueOrPromise === 'object' || IsCallable(valueOrPromise))) {
> +        	var then = valueOrPromise.then;
> +          if (IsCallable(then)) {
> +            callFunction(then, valueOrPromise,
> +              function (valueOrPromise) { resolveChain(index, valueOrPromise); }, reject);

Nit: horizontally align this argument with the beginning of the first argument.

@@ +72,5 @@
> +              function (valueOrPromise) { resolveChain(index, valueOrPromise); }, reject);
> +            return;
> +          }
> +        }
> +        results[index] = valueOrPromise;

Is it ok to not store the result if an exception was thrown? It probably is because it looks like we're not doing anything with the results in that case, but I'm not sure.

@@ +73,5 @@
> +            return;
> +          }
> +        }
> +        results[index] = valueOrPromise;
> +        if (--remaining === 0) {

Can't you just use `index === length - 1` here and get rid of `remaining`?

@@ +80,5 @@
> +      } catch (ex) {
> +        reject(ex);
> +      }
> +    }
> +    for (var i = 0; i < promises.length; i) {

Nit: no braces needed for single-line bodies.

@@ +87,5 @@
> +  });
> +}
> +
> +function Promise_race(values) {
> +  throw new Error("Promise.race is not implemented");

I'm pretty sure this doesn't actually work: the "Error" constructor almost certainly isn't available. Use ThrowTypeError for now, I would say.

@@ +100,5 @@
> +function Promise_resolve(value) {
> +  if (value && typeof value === 'object' &&
> +    callFunction(Promise_getConstructor, value) === this) {
> +    return value;
> +  }

This doesn't do what (IIUC) it should be doing: Promise_getConstructor would only work on actual instances of Promise. For everything else, it would either crash if no reserved slot with that index exists, or return some bogus value. Both are bad.

Also, if you do something like `MyObject.resolve = Promise.resolve` and then call `MyObject.resolve(someValue)`, you're checking if the result of `Promise_getConstructor(someValue)` equals `MyObject`.

I think it'd be best to implement the IsPromise spec function as an intrinsic. The IsTypedArray intrinsic is a good example for how that'd work. Check the definition of that in SelfHosting.cpp and usages in builtin/TypedArray.js.

We don't currently support subclassing of builtins (but efaust is on the case!), so it's probably ok to just return the value of the IsPromise check returns true.

@@ +109,4 @@
>  }
>  
>  function Promise_catch(onRejected) {
> +  return this.then(null, onRejected);

Per spec, this should use `undefined`, not `null`.

@@ +112,5 @@
> +  return this.then(null, onRejected);
> +}
> +
> +function asap(cb) {
> +  cb();

I still don't understand how this works and actually defers execution. Can you add an explanation? Also, it still executes in the same event loop turn instead of in the next one, right? If that's the case, it definitely needs to change, otherwise the semantics of async/await won't be what we want them to be at all.

@@ +116,5 @@
> +  cb();
> +}
> +
> +function handle(deferred) {
> +  var me = this;

Change this to `promise`, and use it everywhere below instead of `this`.

@@ +117,5 @@
> +}
> +
> +function handle(deferred) {
> +  var me = this;
> +  if (callFunction(Promise_getState, this) === null) {

This check will never succeed because the state is always a boolean. Change `null` to `false`.

@@ +140,5 @@
> +    deferred.resolve(returnValue);
> +  });
> +}
> +
> +function doResolve(fn, onFulfilled, onRejected) {

It's not ideal that we have Promise_resolve, doResolve, and resolve. I don't have good suggestions, but names that make it more clear what each of these does would be good. If that turns out to be hard, at the very least we need comments explaining their roles.

@@ +172,5 @@
> +        return;
> +      }
> +    }
> +    callFunction(Promise_setState, this, true);
> +    callFunction(Promise_setValue, this, newValue);

Can't these two calls be moved into "finale", with the values passed in?

@@ +179,5 @@
> +		callFunction(reject, this, ex);
> +	}
> +}
> +
> +function reject(newValue) {

Same issue was with resolve: multiple functions with very similar names. Please either change the names or add comments.

@@ +180,5 @@
> +	}
> +}
> +
> +function reject(newValue) {
> +  callFunction(Promise_setState, this, false);

Hmm, maybe I'm misunderstanding what the state stores. It looks like it starts out with `null`, indicating that it's unresolved, and can change to `true` for resolved and `false` for rejected? If that's the case, please change it to use enum values. Well, the poor-man's version of them, because we don't have enums in JS. Define values in SelfHostingDefines.h like this and use them here and in Promise.cpp:
#define PROMISE_STATE_UNRESOLVED 0
#define PROMISE_STATE_RESOLVED   1
#define PROMISE_STATE_REJECTED   2

@@ +185,5 @@
> +  callFunction(Promise_setValue, this, newValue);
> +  callFunction(finale, this);
> +}
> +
> +function finale() {

I'm not sure I understand what "finale" means. Perhaps "finalize"? That would still not be an ideal name, as it sounds like some destructor thing, which it isn't, really. I don't have a good suggestion, but this name certainly isn't right.

@@ +187,5 @@
> +}
> +
> +function finale() {
> +  var deferreds = callFunction(Promise_getDeferreds, this);
> +  for (var i = 0, len = deferreds.length; i < len; i) {

Nit: no braces needed.

@@ +188,5 @@
> +
> +function finale() {
> +  var deferreds = callFunction(Promise_getDeferreds, this);
> +  for (var i = 0, len = deferreds.length; i < len; i) {
> +    callFunction(handle, this, deferreds[i]);

Is this guaranteed to not throw? I think so, but am not sure. If it is, add a comment noting that so readers can immediately tell that nothing bad can happen here.

@@ +195,4 @@
>  }
>  
>  function Promise_then(onFulfilled, onRejected) {
> +  var me = this;

Change this to "promise".

::: js/src/vm/SelfHosting.cpp
@@ +1387,5 @@
>            CallNonGenericSelfhostedMethod<Is<LegacyGeneratorObject>>, 2, 0),
>      JS_FN("CallStarGeneratorMethodIfWrapped",
>            CallNonGenericSelfhostedMethod<Is<StarGeneratorObject>>, 2, 0),
>  
>      JS_FN("IsWeakSet",               intrinsic_IsWeakSet,               1,0),

Nit: undo this change.

@@ +1392,2 @@
>      JS_FN("NewDenseArray",           intrinsic_NewDenseArray,           1,0),
> +    JS_FN("NewPromise",              intrinsic_NewPromise, 1, 0),

Nit: please align the arguments as in the lines above.

::: js/src/vm/SelfHosting.h
@@ -17,5 @@
>   * Check whether the given JSFunction is a self-hosted function whose
>   * self-hosted name is the given name.
>   */
>  bool IsSelfHostedFunctionWithName(JSFunction* fun, JSAtom* name);
> -

Nit: undo this change.
Comment 20 User image Ryan VanderMeulen [:RyanVM] 2015-07-30 13:12:54 PDT
https://hg.mozilla.org/mozilla-central/rev/48a35f84fb9d
Comment 21 User image mkierski 2015-08-04 12:35:39 PDT
Till, I agree with most of the comments and am about to send a new, fixed patch. I also did a few things to refactor the code. It is actually based on a polyfill taken from GitHub, and it has a few of wrong design patterns (like using null/true/false as tri-state).

I also introduced a quasi-eventloop for deferring calls to setTimeout. I will also want to make setTimeout available to the user as soon as this promise implementation gets accepted.

Moreover, I added some basic test cases for promises.

> Can't you just use `index === length - 1` here and get rid of `remaining`?

I don't think so - there is no guarantee that the promise with the last index in the array will resolve as last. Or maybe I don't understand something?

> This doesn't do what (IIUC) it should be doing: Promise_getConstructor would only work
> on actual instances of Promise. For everything else, it would either crash
> if no reserved slot with that index exists, or return some bogus value. Both are bad.

As far as I read, accessing an unregistered slot just returns an `undefined` value. My implementation of `isPromise` closely follows the one provided in the spec, and it works... are we sure we want to change this?
For now I'm leaving this one as-is.
Comment 22 User image mkierski 2015-08-04 15:26:42 PDT
Created attachment 8643323 [details] [diff] [review]
Part 3: Promises implementation

This fixes all of mentioned issues, except for those which I don't understand/fully agree with. Also I did some work to make code more readable, hopefully enough :)
Comment 23 User image mkierski 2015-08-04 16:07:30 PDT
Created attachment 8643351 [details] [diff] [review]
Part 4: Updated some parser grammar rules

Currently only a part of async/await grammar is supported:
- Async function statements are supported.
- Await expressions are supported (as regular unary expressions).
All other parts of proposal are probably not supported.
Even the supported parts of implementation may not be 100% compliant with
the grammar. This is to be considered a proof-of-concept implementation.
Comment 24 User image mkierski 2015-08-05 10:12:27 PDT
Created attachment 8643790 [details] [diff] [review]
Part 3: Promises implementation

I found a bug, fixed that and added a test case.
Comment 25 User image Eric Faust [:efaust] 2015-08-19 14:54:21 PDT
Created attachment 8650150 [details] [diff] [review]
Refactor DEFFUN in jits
Comment 26 User image mkierski 2015-08-19 15:05:15 PDT
Comment on attachment 8640988 [details] [diff] [review]
Part 1: Exclude StoreBuffer.cpp from unified build to prevent build bustage

Already landed separately.
Comment 27 User image Eric Faust [:efaust] 2015-08-21 11:19:41 PDT
Created attachment 8651161 [details] [diff] [review]
Refactor DEFFUN in jits

This should fix the baseline stack problems.
Comment 28 User image mkierski 2015-08-21 14:10:50 PDT
Created attachment 8651233 [details] [diff] [review]
Part 4: Parser grammar rules
Comment 29 User image mkierski 2015-08-21 14:12:29 PDT
Created attachment 8651235 [details] [diff] [review]
Part 5: Bytecode and self-hosted wrapper code
Comment 30 User image mkierski 2015-08-21 14:19:13 PDT
Created attachment 8651237 [details] [diff] [review]
Part 6: Refactored JIT DEFFUN (by efaust)
Comment 31 User image Eric Faust [:efaust] 2015-08-21 14:39:49 PDT
Comment on attachment 8651237 [details] [diff] [review]
Part 6: Refactored JIT DEFFUN (by efaust)

This is not yet r+, if it needs review, we should get it from Jan.
Comment 32 User image Eric Faust [:efaust] 2015-08-28 13:02:26 PDT
Comment on attachment 8651235 [details] [diff] [review]
Part 5: Bytecode and self-hosted wrapper code

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

Cancelling review for now. This patch needs to get the right hunks into the right other patches where it should live. Please repost when that happens.

The parser stuff should go to the parser. The promise stuff should go to the promise patch. Please make a new patch for the DEFFUN refactor, as well.

The emitter sections, however, look pretty good! The self-hosted stuff also seems fine. So the async/await parts of this look fine, but it is parts of several patches sewn together.

::: js/src/builtin/AsyncFunctions.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function AsyncFunction_wrap(genFunction) {
> +  var fun = function() {
> +    return AsyncFunction_start(callFunction(std_Function_apply, genFunction, null, arguments));

we talked about adding the try catch here. We should do that.

::: js/src/builtin/Promise.js
@@ +135,2 @@
>      return value;
> +  } // FIXME this does not properly check if 'value' is a Promise, which it should do.

These changes should split out and go with the ones that till is supposed to review, right?

::: js/src/frontend/BytecodeEmitter.cpp
@@ -998,5 @@
>  bool
>  BytecodeEmitter::emitIndex32(JSOp op, uint32_t index)
>  {
>      MOZ_ASSERT(checkStrictOrSloppy(op));
> -

This blank line and the one above where there intentionally. Let's leave them.

@@ +5887,5 @@
>  bool
> +BytecodeEmitter::emitAsyncWrapper(unsigned index) {
> +    JSAtom* atom = Atomize(cx, "AsyncFunction_wrap", 18);
> +    return (atom != nullptr &&
> +        emitAtomOp(atom, JSOP_GETINTRINSIC) &&

This formatting can't stand. We want more whitespace.

if (!atom)
    return false;

return emitAtomOp(...) &&
       ...;

@@ -7995,5 @@
> -      // so currently we just return "true" as a placeholder.
> -      case PNK_AWAIT:
> -        if(!emit1(JSOP_TRUE))
> -            return false;
> -        break;

This seems like it should not have been in a previous patch, but it's fine.

::: js/src/frontend/Parser.cpp
@@ +2290,5 @@
>  {
>      MOZ_ASSERT_IF(kind == Statement, funName);
>  
> +    if (asyncKind == AsyncFunction)
> +      generatorKind = StarGenerator;

4 spaces indenture, please.

@@ +2785,5 @@
>          !report(ParseStrictError, pc->sc->strict(), null(), JSMSG_STRICT_FUNCTION_STATEMENT))
>          return null();
>  
> +    if (asyncKind == AsyncFunction)
> +      generatorKind = StarGenerator;

and here.

@@ +6308,5 @@
>        case TOK_NAME: {
>          TokenKind next;
> +        TokenKind nextSameLine;
> +
> +        #ifdef NIGHTLY_BUILD

These changes should be in the parser patch, and are stale, right?

::: js/src/vm/Interpreter.cpp
@@ +3493,5 @@
>          goto error;
>  }
>  END_CASE(JSOP_DEFVAR)
>  
> +CASE(JSOP_DEFFUN) {

Please leave this on the next line, like it is everywhere else in this file.
Comment 33 User image Eric Faust [:efaust] 2015-08-28 13:05:05 PDT
Comment on attachment 8651233 [details] [diff] [review]
Part 4: Parser grammar rules

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

Cancelling review in the face of having to rewrite most of this, as per our discussion. The spec changed.
Comment 34 User image mkierski 2015-09-03 15:50:04 PDT
Created attachment 8656853 [details] [diff] [review]
Part 2: (till) Promises boilerplate
Comment 35 User image mkierski 2015-09-03 16:00:39 PDT
Created attachment 8656862 [details] [diff] [review]
Part 3: Promises implementation
Comment 36 User image mkierski 2015-09-03 16:01:26 PDT
Created attachment 8656864 [details] [diff] [review]
Part 4: Parser grammar rules
Comment 37 User image mkierski 2015-09-03 16:02:05 PDT
Created attachment 8656865 [details] [diff] [review]
Part 5: Bytecode and self-hosted wrapper code
Comment 38 User image mkierski 2015-09-03 16:02:32 PDT
Created attachment 8656866 [details] [diff] [review]
Part 6: Refactored JIT DEFFUN (by efaust)
Comment 39 User image Eric Faust [:efaust] 2015-09-11 15:58:41 PDT
Comment on attachment 8656864 [details] [diff] [review]
Part 4: Parser grammar rules

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

OK, a lot of things went right here:

1) The tests look phenomenal. Very careful, with good coverage.
2) Much of the parsing is good and robust.
3) It all mostly does what it claims to do

But a few things went very wrong.

1) There are tons of little stylistic things. The patch needs some more polish before it can go in. I have tried to note as many as I saw below.
2) There are a few small refactoring things that need to be taken care of (the default await throwing case, removing asyncKind from FunctionBox)
3) We cannot increase the size of JSFunction by a word. We have got to find a way to pack the FunctionAsyncKind.
4) I am dubious that we cannot lazily parse an async function, and I'm not seeing a place where we stash that information in the lazyScript, so I suspect we are getting this wrong.

Because of 3 and 4, I have to cancel review for now. We can talk about how to fix these things on Tuesday, when I'm back in the office. Until then, 1 and 2 seem fairly tractable, if somewhat boring, to churn through and fix.

A more complete review follows.

::: js/src/builtin/ReflectParse.cpp
@@ +1798,5 @@
>                     "defaults", defarray,
>                     "body", body,
>                     "rest", rest,
>                     "generator", isGeneratorVal,
> +                   "async", isAsyncVal,

this will do for now. We may well have to revisit this when esprima writes a spec for async/await support. It is likely to look a lot like this, though, so this should be good.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +312,5 @@
>      RootedFunction fun(cx, evalCaller->functionOrCallerFunction());
>      MOZ_ASSERT_IF(fun->strict(), options.strictOption);
>      Directives directives(/* strict = */ options.strictOption);
>      ObjectBox* funbox = parser->newFunctionBox(/* fn = */ nullptr, fun, &parseContext,
> +                                              directives, fun->generatorKind(), fun->asyncKind());

pre-existing nit: directives should be aligned  with the /*, not the (

@@ +863,5 @@
>          return false;
>  
>      Rooted<JSFunction*> fun(cx, lazy->functionNonDelazifying());
>      MOZ_ASSERT(!lazy->isLegacyGenerator());
> +    ParseNode* pn = parser.standaloneLazyFunction(fun, lazy->strict(), lazy->generatorKind(), SyncFunction);

It's not at all clear to me that this is right. What keeps us from lazily compiling an async function?

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8089,5 @@
>              return false;
>          break;
>  
> +      // PNK_AWAIT handling is not yet implemented,
> +      // so currently we just return "true" as a placeholder.

This is replaced later in this stack. OK.

::: js/src/frontend/FoldConstants.cpp
@@ +1738,5 @@
>          MOZ_ASSERT(!pn->pn_kid->maybeExpr());
>          return true;
>  
> +      case PNK_AWAIT:
> +        return true;

We can do better than this. At least we can Fold() the expression, right?

::: js/src/frontend/FullParseHandler.h
@@ +435,2 @@
>          TokenPos pos(begin, value ? value->pn_pos.end : begin + 1);
> +        return new_<BinaryNode>(isAwait ? PNK_AWAIT : PNK_YIELD, op, pos, value, gen);

Why not just add a new one, newAwaitExpression()? Maybe I will understand why below.

::: js/src/frontend/Parser.cpp
@@ +824,5 @@
>      if (!pc->sc->needStrictChecks())
>          return true;
>  
> +    if (name == context->names().eval || name == context->names().arguments ||
> +      (IsKeyword(name) && name != context->names().await)) {

nit: align (IsKeyword( with name above, and move opening brace to next line.

@@ +860,2 @@
>      ParseNode* pn = statements(YieldIsKeyword);
> +    tokenStream.setAwaitIsKeyword(awaitIsKeyword);

Man, this would have been ugly, otherwise, huh?

@@ +1451,5 @@
>          return nullptr;
>      if (options().selfHostingMode)
>          fun->setIsSelfHostedBuiltin();
> +
> +    fun->setAsyncKind(asyncKind);

If the function holds the asyncKind, then the FunctionBox needn't do so. We can just access it though funbox->function()->asyncKind().

@@ +2411,5 @@
>  {
>      MOZ_ASSERT_IF(kind == Statement, funName);
>  
> +    if (asyncKind == AsyncFunction)
> +        generatorKind = StarGenerator;

Shouldn't this be an assertion instead? functionDef has very specific callers, looks like.

@@ +2453,5 @@
>      tokenStream.tell(&start);
>  
>      while (true) {
> +        if (functionArgsAndBody(inHandling, pn, fun, kind, generatorKind, asyncKind,
> +           directives, &newDirectives))

nit: alignment

@@ +2698,5 @@
>  template <>
>  ParseNode*
>  Parser<FullParseHandler>::standaloneLazyFunction(HandleFunction fun, bool strict,
> +                                                 GeneratorKind generatorKind,
> +                                                 FunctionAsyncKind asyncKind)

but isn't this always SyncFunction, above? I don't get it.

@@ +2819,5 @@
>  #endif
>      }
>  
> +    bool beforeAwaitIsKeyword = tokenStream.getAwaitIsKeyword();
> +    tokenStream.setAwaitIsKeyword(fun->isAsync());

This approach is a little kludgey, but nowhere near as bad as what we do for yield. Nice design.

@@ +2869,5 @@
>  }
>  
>  template <typename ParseHandler>
>  typename ParseHandler::Node
> +Parser<ParseHandler>::functionStmt(YieldHandling yieldHandling, DefaultHandling defaultHandling, FunctionAsyncKind asyncKind)

nit: suspected line length. Ensure that it's <100 characters

@@ +2910,5 @@
>          !report(ParseStrictError, pc->sc->strict(), null(), JSMSG_STRICT_FUNCTION_STATEMENT))
>          return null();
>  
> +    if (asyncKind == AsyncFunction)
> +      generatorKind = StarGenerator;

Right, this supports the assertion claims above.

@@ +5806,5 @@
>      if (!noteNameUse(context->names().dotGenerator, generator))
>          return null();
>      if (isYieldStar)
>          return handler.newYieldStarExpression(begin, expr, generator);
> +    return handler.newYieldExpression(begin, expr, generator, JSOP_YIELD, isAwait);

yeah, this is a mistake. we should instead

if (isAwait)
    return handler.newAwaitExpression(...);
return handler.newYieldExpression(...);

@@ +6426,5 @@
>                  report(ParseError, false, propName, JSMSG_BAD_METHOD_DEF);
>                  return null();
>              }
> +            if (propType == PropertyType::AsyncMethod) {
> +                report(ParseError, false, null(), JSMSG_ASYNC_CONSTRUCTOR);

Uhm...Surely the error above will always be thrown if propType == PropertyType::AsyncMethod, so this code is dead.

@@ +6711,5 @@
>  typename ParseHandler::Node
>  Parser<ParseHandler>::expr(InHandling inHandling, YieldHandling yieldHandling,
>                             InvokedPrediction invoked)
>  {
> +    Node pn = assignExpr(inHandling, yieldHandling, true, invoked);

this |true| needs a /* foo = */ for readability.

@@ +6983,5 @@
>      if (tt == TOK_NAME) {
> +        TokenKind nextSameLine;
> +
> +        #ifdef NIGHTLY_BUILD
> +        if (tokenStream.currentName() == context->names().async &&

nit: alignment of if clauses.

@@ +6997,5 @@
>          if (endsExpr)
>              return identifierName(yieldHandling);
>      }
>  
> +    if (tt == TOK_AWAIT && !awaitAllowed) {

This feels out of place here. It should find a way to live in assignExprWithoutYieldOrAwait, or functionArguments().

@@ +7306,5 @@
>          return handler.newDelete(begin, expr);
>        }
>  
> +      case TOK_AWAIT:
> +          TokenKind nextSameLine;

Does this declaration not warn? Normally, you need a new block. See the default block below.

@@ +7315,5 @@
> +              if (nextSameLine != TOK_EOL) {
> +                 Node kid = unaryExpr(yieldHandling);
> +                  if (!kid)
> +                      return null();
> +                  return newYieldExpression(begin, kid, false, true);

These too need /* isYieldStar = */ and so on.

@@ +8241,3 @@
>      if (res && pc->lastYieldOffset != startYieldOffset) {
>          reportWithOffset(ParseError, false, pc->lastYieldOffset,
>                           msg, js_yield_str);

maybe change that to res->isKind(PNK_AWAIT) ? awaitMsg : yieldMsg, or something? Here is infinitely better than inside assignExpression for the await in default message

@@ +8844,5 @@
>          TokenKind tt;
>          if (!tokenStream.getToken(&tt, TokenStream::KeywordIsName))
>              return null();
> +
> +        if (isAsync && (tt == TOK_NAME || tt == TOK_STRING || tt == TOK_NUMBER || tt == TOK_LB)) {

We can do it cleaner than this. Not looking for accessor syntax on async is enough, right?

Then

async get name() { }

is still a syntax error? It looks like it should be.

@@ +8852,3 @@
>          if (tt == TOK_NAME) {
>              propAtom.set(tokenStream.currentName());
> +

nit: This new line is probably not needed.

@@ +8875,3 @@
>              return newNumber(tokenStream.currentToken());
>          }
> +        if (tt == TOK_LB) {

nit: Please remove the braces. Single line ifs do not need them. The blank line is also unnecessary.

@@ +8932,5 @@
>      }
>  
>      if (tt == TOK_LP) {
>          tokenStream.ungetToken();
> +        *propType = isGenerator ? PropertyType::GeneratorMethod : (

nit: indenting here, with ( on end of previous line is really ugly. Indent it one indenture past the start of isGenerator with the entire last ternary parenthesized.

@@ +9185,2 @@
>          return identifierName(yieldHandling);
> +      }

nit: no need for these braces.

::: js/src/frontend/Parser.h
@@ +127,5 @@
>      bool isGenerator() const { return generatorKind() != NotGenerator; }
>      bool isLegacyGenerator() const { return generatorKind() == LegacyGenerator; }
>      bool isStarGenerator() const { return generatorKind() == StarGenerator; }
>  
> +    bool isAsync() const { return sc->isFunctionBox() && sc->asFunctionBox()->function()->isAsync(); }

This confirms that we can not store the async kind in the function box, as we are already not consulting it.

@@ +519,5 @@
>      }
>  
>      // Use when the funbox should be linked to the outerpc's innermost scope.
>      FunctionBox* newFunctionBox(Node fn, HandleFunction fun, ParseContext<ParseHandler>* outerpc,
> +                                Directives directives, GeneratorKind generatorKind, FunctionAsyncKind asyncKind = SyncFunction)

The last parameter should flow to a third line.

@@ +650,5 @@
>       * Some parsers have two versions:  an always-inlined version (with an 'i'
>       * suffix) and a never-inlined version (with an 'n' suffix).
>       */
> +    Node functionStmt(YieldHandling yieldHandling,
> +      DefaultHandling defaultHandling, FunctionAsyncKind asyncKind);

Please align params after the (

::: js/src/frontend/SharedContext.h
@@ +298,5 @@
>      bool            usesApply:1;      /* contains an f.apply() call */
>      bool            usesThis:1;       /* contains 'this' */
>  
>      FunctionContextFlags funCxFlags;
> +    FunctionAsyncKind asyncKind_;

Unnecessary as discussed.

@@ +335,5 @@
> +    }
> +
> +    bool isAsync() {
> +      return asyncKind() == AsyncFunction;
> +    }

These seem dead. Are they? If so, please remove.

::: js/src/js.msg
@@ +513,5 @@
>  MSG_DEF(JSMSG_CANT_DELETE_SUPER, 0, JSEXN_REFERENCEERR, "invalid delete involving 'super'")
> +
> +// Promise
> +MSG_DEF(JSMSG_PROMISE_RESOLVED_WITH_ITSELF, 0, JSEXN_TYPEERR, "A promise cannot be resolved with itself")
> +MSG_DEF(JSMSG_SETTIMEOUT_INTERVAL_NONZERO, 0, JSEXN_TYPEERR, "Intervals other than 0 are not supported")

These don't belong in this patch, still, probably.

::: js/src/jsfun.h
@@ +139,5 @@
>          } i;
>          void*           nativeOrScript;
>      } u;
>      js::HeapPtrAtom  atom_;       /* name for diagnostics and decompiling */
> +    js::FunctionAsyncKind   asyncKind_;

OK, there's no way we can afford to do this. There are /way/ too many functions in the world to make them all a word larger. It's one bit. We need to find a way to fit it into the union above.

::: js/src/tests/ecma_7/AsyncFunctions/syntax.js
@@ +5,5 @@
> +/**
> + * Currently only a part of async/await grammar is supported:
> + * - Async function statements are supported.
> + * - Await expressions are supported (as regular unary expressions).
> + * All other parts of proposal are probably not supported.

This isn't even true! :)

@@ +53,5 @@
> +assertThrows(() => Reflect.parse("async function a() { return await; }"), SyntaxError);
> +
> +// Yield is not allowed in an async function / treated as identifier
> +assertThrows(() => Reflect.parse("async function a() { yield 3; }"), SyntaxError);
> +assertThrows(() => Reflect.parse("async function a() { return yield 3; }"), SyntaxError);

also throw in a test for var yield = 5, just for completeness

@@ +62,5 @@
> +
> +// Await is treated differently depending on context. Various cases.
> +Reflect.parse("var await = 3; async function a() { await 4; }");
> +Reflect.parse("async function a() { await 4; } var await = 5");
> +Reflect.parse("async function a() { function b() { return await; } }");

I can't believe this nonsense is specced to work. Blech. Nice test.

::: js/src/vm/Xdr.h
@@ +32,5 @@
>  static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 306;
>  static const uint32_t XDR_BYTECODE_VERSION =
>      uint32_t(0xb973c0de - XDR_BYTECODE_VERSION_SUBTRAHEND);
>  
> +static_assert(JSErr_Limit == 416,

When this goes up, the subtrahend must also. Please increment the number above.
Comment 40 User image Eric Faust [:efaust] 2015-09-11 16:00:13 PDT
Comment on attachment 8656865 [details] [diff] [review]
Part 5: Bytecode and self-hosted wrapper code

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

Looks good! r=me with questions below answered, and tests guarded.

::: js/src/Makefile.in
@@ +285,5 @@
>  
>  selfhosting_srcs := \
>    $(srcdir)/builtin/Utilities.js \
>    $(srcdir)/builtin/Array.js \
> +	$(srcdir)/builtin/AsyncFunctions.js \

nit: alignment.

::: js/src/builtin/AsyncFunctions.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function AsyncFunction_wrap(genFunction) {
> +  var fun = function() {
> +    try {

Please add a comment here that the try/catch is for function defaults throwing.

@@ +24,5 @@
> +function AsyncFunction_resume(gen, v, method) {
> +    let result;
> +    try {
> +      result = callFunction(method, gen, v);
> +      // get back into async function, run to next await point

nit: This should move to above the line of code it describes.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2002,5 @@
>          return true;
>  
>        case PNK_YIELD_STAR:
>        case PNK_YIELD:
> +      case PNK_AWAIT:

Oooops. Yes. Should this go in the previous patch?

@@ +5970,5 @@
> +BytecodeEmitter::emitAsyncWrapper(unsigned index, bool needsHomeObject) {
> +    JSAtom* atom = Atomize(cx, "AsyncFunction_wrap", 18);
> +    if (!atom)
> +        return false;
> +    if (needsHomeObject && !emitIndex32(JSOP_LAMBDA, index))

Please add a comment here describing the handshake with propertyList() and why we are doing it in that order.

@@ +7176,5 @@
>              propdef->pn_right->pn_funbox->needsHomeObject())
>          {
>              MOZ_ASSERT(propdef->pn_right->pn_funbox->function()->allowSuperProperty());
> +            bool isAsync = propdef->pn_right->pn_funbox->function()->isAsync();
> +            if (isAsync && !emit1(JSOP_SWAP))

Maybe the comment should actually live here, and above should be "see comment in propertyList()", or visa versa. Either way.

::: js/src/shell/js.cpp
@@ +1582,5 @@
> +        return false;
> +
> +    args.rval().set(rval);
> +    return true;
> +}

Are these impls not mandatory to getting the promise code working? Maybe just testing functions?

::: js/src/tests/ecma_7/AsyncFunctions/1.1.1.js
@@ +12,5 @@
> +assertThrowsSE("async function a() { super.prop(); }");
> +assertThrowsSE("async function a() { super(); }");
> +
> +// FIXME this should work
> +// assertThrowsSE("async function a(k = await 3) {}");

Why doesn't this work? Does it work now?

::: js/src/tests/ecma_7/AsyncFunctions/1.1.2.js
@@ +6,5 @@
> +
> +var anon = async function() { }
> +
> +assertEq(test.name, "test");
> +assertEq(anon.name, "");

Nice test.

::: js/src/tests/ecma_7/AsyncFunctions/methods.js
@@ +46,5 @@
> +  assertEventuallyEq(y.getBaseClassName(), 'X'),
> +]).then(() => {
> +  if (typeof reportCompare === "function")
> +      reportCompare(true, true);
> +});

All these tests need some kind of mechanism guarding them against failure when the code is no longer on nightly. When the nightly branch merges to Aurora (dev edition), these will start to fail, because neither classes nor async functions will parse.

::: js/src/tests/ecma_7/AsyncFunctions/semantics.js
@@ +91,5 @@
> +  return n !== 0 && await isEven(n - 1);
> +}
> +
> +// recursion, take three!
> +// FIXME this example doesn't work...

I thought you had showed me this working? It's certainly not commented out, so...

::: js/src/vm/GeneratorObject.cpp
@@ +61,5 @@
>  bool
>  GeneratorObject::suspend(JSContext* cx, HandleObject obj, AbstractFramePtr frame, jsbytecode* pc,
>                           Value* vp, unsigned nvalues)
>  {
> +    MOZ_ASSERT(*pc == JSOP_INITIALYIELD || *pc == JSOP_YIELD || *pc == JSOP_GENERATOR);

How can /that/ happen? Can it?

::: js/src/vm/ScopeObject.cpp
@@ +237,5 @@
>       */
>      if (callee->isNamedLambda()) {
> +        if (callee->isAsync()) {
> +            RootedFunction fun(cx, &callee->getExtendedSlot(1).toObject().as<JSFunction>());
> +            scopeChain = DeclEnvObject::create(cx, scopeChain, fun);

A beautiful hack :P
Comment 41 User image Eric Faust [:efaust] 2015-09-11 16:01:26 PDT
Comment on attachment 8656866 [details] [diff] [review]
Part 6: Refactored JIT DEFFUN (by efaust)

This is really part of part 5, but I wrote it, so I can't review it.

The purpose is to make JSOP_DEFFUN take the function value from the stack, instead of from the bytecode stream.
Comment 42 User image Jan de Mooij [:jandem] 2015-09-11 16:31:20 PDT
Comment on attachment 8656866 [details] [diff] [review]
Part 6: Refactored JIT DEFFUN (by efaust)

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

::: js/src/jit/BaselineCompiler.cpp
@@ +2473,5 @@
>  
>  bool
>  BaselineCompiler::emit_JSOP_DEFFUN()
>  {
> +

Uber nit: remove this empty line.
Comment 43 User image mkierski 2015-09-17 15:17:01 PDT
Created attachment 8662650 [details] [diff] [review]
Part 2: (till) Promises boilerplate
Comment 44 User image mkierski 2015-09-17 15:17:45 PDT
Created attachment 8662651 [details] [diff] [review]
Part 3: Promises implementation
Comment 45 User image mkierski 2015-09-17 15:18:55 PDT
Created attachment 8662652 [details] [diff] [review]
Part 4: Parser grammar rules
Comment 46 User image mkierski 2015-09-17 15:19:45 PDT
Created attachment 8662653 [details] [diff] [review]
Part 5: Bytecode and self-hosted wrapper code
Comment 47 User image mkierski 2015-09-17 15:20:42 PDT
Created attachment 8662654 [details] [diff] [review]
Part 6: Refactored JIT DEFFUN (by efaust)
Comment 48 User image Till Schneidereit [till] 2015-09-18 12:23:03 PDT
Comment on attachment 8662651 [details] [diff] [review]
Part 3: Promises implementation

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

My deepest apologies for sitting on this for so long! Given that you're leaving, please do the barest minimum of what I'm asking below, and I'll pick it up from there.

The most important part is documentation of how things work. Mostly, that means explanations of the roles of the SLOTs on Promise instances: what do they do, how are they used? Also, much of the code isn't exactly self-explanatory, and it would be a great help to have some targeted comments in there.

Next is making this all tamper-proof. Right now, content code can change Promises' behavior by changing methods on Array instances. It's very important to ensure that whatever content code does, the behavior of builtins stays as it is intended to be.

I didn't look at the patches that actually introduce async/await, so ignore this if it doesn't apply, but: if the async/await implementation retrieves the Promise constructor from the current global and uses its methods, then it isn't tamper-proof in the same way. The best thing to do there would be to somehow stash the original value somewhere (for both the constructor and the set of methods used, which then should be invoked using callFunction), or use an internal version of promises that isn't affected by things being done to window.Promise.

Finally, I think it'd make sense to only expose this with an environment var set or something like that. Unconditionally landing it even in Nightly seems a bit risky, given that it's not tamper-proof right now. But again, I'm willing to pick it up and drive it in.

::: js/src/builtin/Promise.cpp
@@ +6,5 @@
>  
>  #include "builtin/Promise.h"
>  
>  #include "jscntxt.h"
> +#include "SelfHostingDefines.h"

This needs to be "builtin/SelfHostingDefines.h", and be moved up to below the Promise.h include.

::: js/src/builtin/Promise.js
@@ +20,5 @@
> + * this setTimeout implementation must be manually triggered by calling
> + * runEvents().
> + */
> +
> +var eventQueue = [];

Use `new List()` here. That way, the queue's behavior can't be changed by modifying Array.prototype. With the current implementation, content code could derail the Promise implementation by changing Array.prototype.push/pop.

@@ +24,5 @@
> +var eventQueue = [];
> +
> +function runEvents() {
> +  while (eventQueue.length > 0) {
> +    //var evt = eventQueue.pop();

Remove this line. (And the empty one below it.)

@@ +100,5 @@
> +}
> +
> +function Promise_all(promises) {
> +  var length = promises.length;
> +  var results = NewDenseArray(length);

This should just use `[]` instead of NewDenseArray. Below, the `results[index] = ..` should be replaced with `_DefineDataProperty(results, index, valueOrPromise)`. Then just remove the `NewDenseArray` function.

Right now, changing Array.prototype.push would again break Promise's behavior.

@@ +135,5 @@
> +    reject(value);
> +  });
> +}
> +
> +function Promise_resolve(value) {

What happens if you do this?

function Foo() {}
Foo.prototype.resolve = Promise.prototype.resolve;
var foo = new Foo();
foo.resolve();

I *think* it should assert in a debug build.
Comment 49 User image mkierski 2015-09-21 10:30:17 PDT
> Foo.prototype.resolve = Promise.prototype.resolve;
It's a static function Promise.resolve - not Promise.prototype.resolve. You can change it but it's all right, isn't it?

I addressed all of your comments and you're right - it's possible to break the implementation of async functions by tampering with Promise prototype. Fortunately it looks like it's easy to fix.
Comment 50 User image Eric Faust [:efaust] 2015-09-21 12:04:03 PDT
Comment on attachment 8662652 [details] [diff] [review]
Part 4: Parser grammar rules

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

Much better, just a few little nits.

::: js/src/builtin/ReflectParse.cpp
@@ +3113,5 @@
>        case PNK_INSTANCEOF:
>          return leftAssociate(pn, dst);
>  
>        case PNK_POW:
> +	      return rightAssociate(pn, dst);

nit: remove bogus indent

::: js/src/frontend/BytecodeCompiler.cpp
@@ +312,5 @@
>      RootedFunction fun(cx, evalCaller->functionOrCallerFunction());
>      MOZ_ASSERT_IF(fun->strict(), options.strictOption);
>      Directives directives(/* strict = */ options.strictOption);
>      ObjectBox* funbox = parser->newFunctionBox(/* fn = */ nullptr, fun, &parseContext,
> +                                              directives, fun->generatorKind(), fun->asyncKind());

nit: 'd' under '/', please

@@ +863,5 @@
>          return false;
>  
>      Rooted<JSFunction*> fun(cx, lazy->functionNonDelazifying());
>      MOZ_ASSERT(!lazy->isLegacyGenerator());
> +    ParseNode* pn = parser.standaloneLazyFunction(fun, lazy->strict(), lazy->generatorKind(), SyncFunction);

This is still wrong, right? this should be lazy->asyncKind()

::: js/src/frontend/Parser.cpp
@@ +1456,5 @@
>                   : JSFunction::INTERPRETED_GENERATOR);
>      }
>  
> +    if (asyncKind == AsyncFunction) {
> +        allocKind = gc::AllocKind::FUNCTION_EXTENDED;

A comment here being like

// We store the async wrapper in a slot for later access

would be nice

@@ -2654,5 @@
>          return false;
>  
>      if (!leaveFunction(pn, outerpc, kind))
>          return false;
> -

nit: please don't remove this line

@@ +2927,5 @@
>          !report(ParseStrictError, pc->sc->strict(), null(), JSMSG_STRICT_FUNCTION_STATEMENT))
>          return null();
>  
> +    if (asyncKind == AsyncFunction)
> +      generatorKind = StarGenerator;

nit: 4 space indent

@@ +7542,5 @@
> +      case TOK_AWAIT:
> +      {
> +          TokenKind nextSameLine;
> +
> +          #ifdef NIGHTLY_BUILD

nit: #ifdef always fully unindented to the far left

@@ +7550,5 @@
> +                 Node kid = unaryExpr(yieldHandling);
> +                  if (!kid)
> +                      return null();
> +                  return newYieldExpression(begin, kid, /* isYieldStar = */ false, /* isAwait = */ true);
> +            } else {

nit: indenture here makes little sense.

@@ +8475,5 @@
> +    if (!tokenStream.peekToken(&tt, TokenStream::Operand))
> +        return null();
> +
> +    if (tt == TOK_AWAIT) {
> +        report(ParseError, false, null(), JSMSG_AWAIT_IN_DEFAULT);

Yeah. This is much better. Thanks :)

::: js/src/frontend/Parser.h
@@ +657,5 @@
>       */
> +    Node functionStmt(YieldHandling yieldHandling,
> +      DefaultHandling defaultHandling, FunctionAsyncKind asyncKind);
> +    Node functionExpr(InvokedPrediction invoked = PredictUninvoked,
> +      FunctionAsyncKind asyncKind = SyncFunction);

Indenture here still wrong

::: js/src/frontend/SyntaxParseHandler.h
@@ +282,5 @@
>      bool addPropertyDefinition(Node literal, Node name, Node expr) { return true; }
>      bool addShorthand(Node literal, Node name, Node expr) { return true; }
>      bool addObjectMethodDefinition(Node literal, Node name, Node fn, JSOp op) { return true; }
>      bool addClassMethodDefinition(Node literal, Node name, Node fn, JSOp op, bool isStatic) { return true; }
> +    Node newYieldExpression(uint32_t begin, Node value, Node gen, JSOp op) { return NodeUnparenthesizedYieldExpr; }

why did this grow a JSOp?

::: js/src/jsfun.h
@@ +285,5 @@
>          flags_ |= RESOLVED_NAME;
>      }
>  
> +    void setAsyncKind(js::FunctionAsyncKind asyncKind) {
> +        if (isInterpretedLazy())

We talked about your fix here. That's fine.
Comment 51 User image Eric Faust [:efaust] 2015-09-21 12:06:45 PDT
Comment on attachment 8662653 [details] [diff] [review]
Part 5: Bytecode and self-hosted wrapper code

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

Looks good to me. Going to ask Till to look over AsyncFunctions.js, just to be sure his fears from his last review are assuaged.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +863,5 @@
>          return false;
>  
>      Rooted<JSFunction*> fun(cx, lazy->functionNonDelazifying());
>      MOZ_ASSERT(!lazy->isLegacyGenerator());
> +    ParseNode* pn = parser.standaloneLazyFunction(fun, lazy->strict(), lazy->generatorKind(), lazy->asyncKind());

Ah, this fix is here! OK. That's fine, then.

::: js/src/frontend/Parser.cpp
@@ +2639,5 @@
>          return false;
>  
>      if (!functionArgsAndBodyGeneric(inHandling, yieldHandling, pn, fun, kind))
>          return false;
> +    

nit: whitespace on empty line

::: js/src/shell/js.cpp
@@ +998,5 @@
>  static bool
>  CacheEntry_setBytecode(JSContext* cx, HandleObject cache, uint8_t* buffer, uint32_t length)
>  {
>      MOZ_ASSERT(CacheEntry_isCacheEntry(cache));
> +  

nit: whitespace on empty line
Comment 52 User image Eric Faust [:efaust] 2015-09-21 12:07:09 PDT
Comment on attachment 8662654 [details] [diff] [review]
Part 6: Refactored JIT DEFFUN (by efaust)

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

Carrying Jandem's review. I cannot review this, as I wrote it.
Comment 53 User image Till Schneidereit [till] 2015-09-21 12:57:34 PDT
(In reply to mkierski from comment #49)
> > Foo.prototype.resolve = Promise.prototype.resolve;
> It's a static function Promise.resolve - not Promise.prototype.resolve. You
> can change it but it's all right, isn't it?

Oh, right. In that case: what happens if you pass in an arbitrary object, like {}? Normal objects shouldn't have 4 reserved slots, so this should assert.

> I addressed all of your comments and you're right - it's possible to break
> the implementation of async functions by tampering with Promise prototype.
> Fortunately it looks like it's easy to fix.

Excellent. Self-hosted JS is simple on the surface, and quite finicky in the details, unfortunately.
Comment 54 User image Till Schneidereit [till] 2015-09-21 13:14:22 PDT
Comment on attachment 8662653 [details] [diff] [review]
Part 5: Bytecode and self-hosted wrapper code

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

Looks good except for the comments below. Plus: this now always uses the self-hosted implementation of Promises. That's fine, as long as that doesn't interfere with the browser's implementation. What's more problematic is that even the SpiderMonkey-implementation relies on a browser-provided builtin: setTimeout. That can again be changed by content, wreaking havoc on async/await, IINM. To verify: what happens if you do `window.setTimeout = function() {throw "bazinga"}` and then use async/await in that window? If that works fine, great. If not, we need to somehow store the original value of `setTimeout` - or use the same fake implementation as in the shell. In the latter case, move it from a shell builtin to a self-hosting intrinsic and just use it like Promise_foo. That means async/await don't work any better in the browser than in the shell, but at least it should work exactly the same.

f+ with this feedback addressed.

::: js/src/builtin/AsyncFunctions.js
@@ +36,5 @@
> +      return Promise_resolve(result.value);
> +    }
> +
> +    // If we get here, `await` occurred. `gen` is paused at a yield point.
> +    return Promise_resolve(result.value).then(

This still isn't right: Promise.prototype.then can be change by content, changing the behavior here. `return callFunction(Promise_then, Promise_resolve(result.value), ...)` should fix that.

Note that I'm not well-versed enough in the details of promises to know whether the `result.value` and similar things are ok. It seems so, but I'm not sure.

::: js/src/vm/SelfHosting.cpp
@@ +1239,5 @@
> +intrinsic_SetFunctionExtendedSlot(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 3);
> +    MOZ_ASSERT(args[0].isObject());

Uber-nit: this assert isn't required, because it's contained in `toObject` in the line below. Feel free to ignore.
Comment 55 User image mkierski 2015-09-23 16:05:33 PDT
Created attachment 8665172 [details] [diff] [review]
Rolled up testing patch

STR

open dev console

async function() a { }
run |a().then(console.log)| until assertion failure
Comment 56 User image Eric Faust [:efaust] 2015-09-23 18:22:02 PDT
(In reply to mkierski from comment #55)
> Created attachment 8665172 [details] [diff] [review]
> Rolled up testing patch
> 
> STR
> 
> open dev console
> 
> async function() a { }
> run |a().then(console.log)| until assertion failure

I pared this down to |a()| until failure, just to simplify.

The problem is that the clone of AsyncFunction_Wrap in the content compartment is relazified, despite having an internal function, which escapes.

I have the following evidence for this:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffe7d7ef1e in JSFunction::needsCallObject (this=0x7fffb130ebf0) at /home/efaust/m-classes/js/src/jsfun.h:148
148	        MOZ_ASSERT(!isInterpretedLazy());
(gdb) where
#0  0x00007fffe7d7ef1e in JSFunction::needsCallObject() const (this=0x7fffb130ebf0) at /home/efaust/m-classes/js/src/jsfun.h:148
#1  0x00007fffe7d91c01 in js::StaticScopeIter<(js::AllowGC)0>::hasSyntacticDynamicScopeObject() const (this=0x7fffffff18b0) at /home/efaust/m-classes/js/src/vm/ScopeObject-inl.h:107
#2  0x00007fffe7f052a5 in AssertDynamicScopeMatchesStaticScope(JSContext*, JSScript*, JSObject*) (cx=0x7fffd7e44500, script=0x7fffc16af8b0, scope=0x7fffb1306600) at /home/efaust/m-classes/js/src/vm/Stack.cpp:150
#3  0x00007fffe7f058bb in js::InterpreterFrame::prologue(JSContext*) (this=0x7fffd486e4e8, cx=0x7fffd7e44500) at /home/efaust/m-classes/js/src/vm/Stack.cpp:218
#4  0x00007fffe7e5352b in Interpret(JSContext*, js::RunState&) (cx=0x7fffd7e44500, state=...) at /home/efaust/m-classes/js/src/vm/Interpreter.cpp:1952
#5  0x00007fffe7e4f282 in js::RunScript(JSContext*, js::RunState&) (cx=0x7fffd7e44500, state=...) at /home/efaust/m-classes/js/src/vm/Interpreter.cpp:709
#6  0x00007fffe7e4f733 in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (cx=0x7fffd7e44500, args=..., construct=js::NO_CONSTRUCT) at /home/efaust/m-classes/js/src/vm/Interpreter.cpp:786
BACKTRACE CONTINUES

(gdb) up
#2  0x00007fffe7f052a5 in AssertDynamicScopeMatchesStaticScope (cx=0x7fffd7e44500, script=0x7fffc16af8b0, scope=0x7fffb1306600) at /home/efaust/m-classes/js/src/vm/Stack.cpp:150
150	        } else if (i.hasSyntacticDynamicScopeObject()) {
(gdb) p enclosingScope.get()->as<JSFunction>().isInterpretedLazy()
$13 = true
(gdb) p enclosingScope.get()
$14 = (JSObject * const&) @0x7fffffff18d0: 0x7fffb130ebf0
(gdb) p i.obj()
Invalid data type for function to be called.
(gdb) p i.obj
$15 = {<js::RootedBase<JSObject*>> = {<No data fields>}, ptr = 0x7fffb130ebf0}
(gdb) p i.obj.get()
$16 = (JSObject * const&) @0x7fffffff18b0: 0x7fffb130ebf0

|enclosingScope| here is args.callee()->nonLazyScript()->enclosingStaticScope(), in terms of the Invoke call.

So it's the immediately enclosing function to the one being called that got delazified, which is AsyncFunction_Wrap in this case.

One fix, which Mariusz found, was to make a function CreateAsyncWrapper() that's called by AsyncFunction_Wrap(). This will ensure that the version in the selfhosted compartment is always delazified and avoid this bug, but it seems bad to evade it.

All told, it seems way more likely that we want to do some checks and respect the doNotRelazify_ flag here in some way, but it's late and I'm not going to find the exact right spot.

ni? till as he said he would take up the torch tomorrow to fix the rest of this. It should be fairly easy for someone familiar with the code, I would think.
Comment 57 User image Till Schneidereit [till] 2015-09-24 03:02:30 PDT
Shell test case:

var g = newGlobal();
g.eval("async function a(){ }");
gc();
g.a();
Comment 58 User image mkierski 2015-09-24 10:42:22 PDT
Tested, this one works.
Comment 59 User image mkierski 2015-09-24 12:33:36 PDT
Created attachment 8665559 [details] [diff] [review]
Part 1: (terrence) fixes for the StoreBuffer
Comment 60 User image mkierski 2015-09-24 12:34:26 PDT
Created attachment 8665561 [details] [diff] [review]
Part 1: (till) Promises boilerplate
Comment 61 User image mkierski 2015-09-24 12:35:01 PDT
Created attachment 8665562 [details] [diff] [review]
Part 2: Promises implementation
Comment 62 User image mkierski 2015-09-24 12:56:18 PDT
Created attachment 8665572 [details] [diff] [review]
Part 3: Parser grammar rules
Comment 63 User image mkierski 2015-09-24 12:57:10 PDT
Created attachment 8665574 [details] [diff] [review]
Part 4: Bytecode and self-hosted wrapper code
Comment 64 User image mkierski 2015-09-24 12:57:50 PDT
Created attachment 8665575 [details] [diff] [review]
Part 5: Refactored JIT DEFFUN (by efaust)
Comment 65 User image Till Schneidereit [till] 2015-09-24 12:58:04 PDT
Comment on attachment 8665561 [details] [diff] [review]
Part 1: (till) Promises boilerplate

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

I think we can just carry efaust's r+ here, as it's just a straight rename. Please fix the author information and append r=efaust to the description before landing.
Comment 66 User image Till Schneidereit [till] 2015-09-24 13:52:31 PDT
Comment on attachment 8665562 [details] [diff] [review]
Part 2: Promises implementation

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

Looks good by and large. r=me with the feedback addressed.

The {Set,Get}FunName intrinsics introduced here (and presumably used in the next patches) shouldn't be required: _DefineDataProperty(myFunction, "name", "myName") *should* work. And just using `var name = myFunction.name` for getting the name should, too.

::: js/src/builtin/Promise.cpp
@@ +33,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
>      JSObject* obj = NewBuiltinClassInstance(cx, &ShellPromiseObject::class_);
>      if (!obj)
>          return false;

These three lines snuck back in: the last version of the patch correctly removed them.

@@ +55,5 @@
> +        return false;
> +
> +    JSObject* deferredsArray = JS_NewArrayObject(cx, 0);
> +    if (!deferredsArray)
> +        return false;

Oh, I didn't see that this was wrong in the last version, good catch.

@@ +57,5 @@
> +    JSObject* deferredsArray = JS_NewArrayObject(cx, 0);
> +    if (!deferredsArray)
> +        return false;
> +
> +    JS_SetReservedSlot(promise, PROMISE_STATE_SLOT, NumberValue(PROMISE_STATE_PENDING)); // state

The comments for these three lines should be dropped: they just repeat what the code says.

@@ +59,5 @@
> +        return false;
> +
> +    JS_SetReservedSlot(promise, PROMISE_STATE_SLOT, NumberValue(PROMISE_STATE_PENDING)); // state
> +    JS_SetReservedSlot(promise, PROMISE_VALUE_SLOT, NullValue()); // value
> +    JS_SetReservedSlot(promise, PROMISE_DEFERREDS_SLOT, ObjectValue(*deferredsArray)); // deferreds

I just realize that this slot is set in the self-hosted Promise_init, too. Given that, just replace this with `NullValue()` (and the creation of `deferredsArray` above). We could do it in either place, except the self-hosted version can use the internal `List` class which content can't modify.

@@ +94,5 @@
>      return cx->global()->createBlankPrototype(cx, &ShellPromiseObject::protoClass_);
>  }
>  
>  const Class ShellPromiseObject::class_ = {
> +   "ShellPromise",

Nit: revert this change.

@@ +108,5 @@
>      nullptr, /* finalize */
>      nullptr, /* call */
>      nullptr, /* hasInstance */
>      nullptr, /* construct */
> +    nullptr, /* trace  */

Nit: revert this change.

::: js/src/builtin/Promise.h
@@ +17,5 @@
>  {
>    public:
> +     static const unsigned RESERVED_SLOTS = 3;
> +     static const Class class_;
> +     static const Class protoClass_;

These three lines were somehow indented one space further. Please revert.

::: js/src/builtin/Promise.js
@@ +24,5 @@
> +var eventQueue = new List();
> +
> +function runEvents() {
> +  while (eventQueue.length > 0) {
> +    var evt = callFunction(std_Array_pop, eventQueue);

With `eventQueue` being a list now, .pop can just be called normally. However, it's probably clearer to still use callFunction, so leave this as-is.

@@ +71,5 @@
> +  return UnsafeGetReservedSlot(promise, PROMISE_VALUE_SLOT);
> +}
> +
> +function Promise_init(promise, fn) {
> +  Promise_setDeferreds(promise, []);

Use `new List()` here instead of [].

@@ +125,5 @@
> +}
> +
> +function Promise_resolve(value) {
> +  if (value && typeof value === 'object' &&
> +    IsPromise(value)) {

This should all fit into one line. Then, the braces can be left off.

::: js/src/vm/SelfHosting.cpp
@@ +1241,5 @@
>      return true;
>  }
>  
> +bool
> +intrinsic_SetFunctionExtendedSlot(JSContext* cx, unsigned argc, Value* vp)

This intrinsic isn't used in the patch. Is it used in a consecutive patch? If so, it's ok to keep, but if not, please remove.
Comment 67 User image Till Schneidereit [till] 2015-09-24 13:53:51 PDT
Oh, one thing I forgot: I still would really like to see documentation of how all this works. Either in one big block comment in Promise.js, or spread out over various comments in the complicated places.
Comment 68 User image mkierski 2015-09-24 15:44:31 PDT
Created attachment 8665678 [details] [diff] [review]
Part 2: Promises implementation
Comment 69 User image Eric Faust [:efaust] 2015-09-25 11:55:44 PDT
Comment on attachment 8665575 [details] [diff] [review]
Part 5: Refactored JIT DEFFUN (by efaust)

Carrying jandem's r+
Comment 70 User image Eric Faust [:efaust] 2015-09-25 11:56:07 PDT
Comment on attachment 8665678 [details] [diff] [review]
Part 2: Promises implementation

Carrying till's r+
Comment 71 User image Eric Faust [:efaust] 2015-09-25 15:23:02 PDT
Comment on attachment 8665572 [details] [diff] [review]
Part 3: Parser grammar rules

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

Carrying. This did not require additional review.
Comment 72 User image Eric Faust [:efaust] 2015-09-25 15:24:46 PDT
Comment on attachment 8665574 [details] [diff] [review]
Part 4: Bytecode and self-hosted wrapper code

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

Carrying. This did not require additional review.
Comment 73 User image Eric Faust [:efaust] 2015-10-01 15:27:43 PDT
Created attachment 8668698 [details] [diff] [review]
Part 6: Clean up the NIGHTLY_ONLY guards for the feature, and allow tests to uplift cleanly

Lots of context, by request
Comment 74 User image Jeff Walden [:Waldo] (remove +bmo to email) 2015-10-01 15:36:42 PDT
Comment on attachment 8668698 [details] [diff] [review]
Part 6: Clean up the NIGHTLY_ONLY guards for the feature, and allow tests to uplift cleanly

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

I'd prefer putting JS_HAS_ASYNC_FUNCS directly into code as a condition, so that async-handling code always builds and can't be accidentally broken for an uplift where it gets turned off again, but this is somewhat of a nit.

::: js/src/tests/ecma_7/AsyncFunctions/methods.js
@@ +59,5 @@
> +
> +`;
> +
> +if (classesEnabled() && asyncFunctionsEnabled())
> +    eval(test);

inb4philor

::: js/src/tests/ecma_7/AsyncFunctions/shell.js
@@ +30,5 @@
> +    try {
> +        eval("async function f()  { }");
> +        return true;
> +    } catch (e if e instanceof SyntaxError) {
> +        return false;

Use

  if (e instanceof SyntaxError)
    return false;
  throw e;

to avoid non-standard syntax

::: js/src/tests/shell.js
@@ +871,5 @@
>      fullmsg += " - " + msg;
>    throw new Error(fullmsg);
>  };
>  
> +function classesEnabled(testCode = "class Foo { constructor() {} }") {

Hmm, doesn't this want to be removed from ecma_6/shell.js or wherever it is?
Comment 79 User image Wes Kocher (:KWierso) 2015-10-06 09:13:14 PDT
You're also failing this mulet mochitest-4 test: https://treeherder.mozilla.org/logviewer.html#?job_id=15244267&repo=mozilla-inbound
Comment 80 User image Wes Kocher (:KWierso) 2015-10-06 10:01:20 PDT
There's actually several things failing: 
https://treeherder.mozilla.org/logviewer.html#?job_id=15247970&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=15248057&repo=mozilla-inbound


I'd suggest a try run that runs pretty much everything at this point before relanding.
Comment 83 User image Eric Faust [:efaust] 2015-10-07 15:27:06 PDT
Created attachment 8671058 [details] [diff] [review]
Fix busted serviceworkers interfaces test for adding ShellPromise
Comment 84 User image Wes Kocher (:KWierso) 2015-10-07 16:38:43 PDT
JSReftests, too: https://treeherder.mozilla.org/logviewer.html#?job_id=15339033&repo=mozilla-inbound
Comment 86 User image Xidorn Quan [:xidorn] (UTC+10) 2016-05-11 22:47:01 PDT
What's the status of this project now? It seems Blink started working on this now: https://groups.google.com/a/chromium.org/d/msg/blink-dev/jlSoAmWa1i4/7xowBIOSBAAJ
Comment 87 User image Eric Faust [:efaust] 2016-05-11 23:00:39 PDT
These patches are waiting on the spidermonkey promises work that till is doing. There is a lot of interest in getting it finished an in, and that will happen as soon as we can.
Comment 88 User image Tooru Fujisawa [:arai] 2016-05-13 01:01:43 PDT
will take this over :)
now rebasing and testing
Comment 89 User image Tooru Fujisawa [:arai] 2016-05-13 03:34:42 PDT
looks like some parts of the spec are not implemented or doesn't match.
  * async arrow functions
  * export default async function

I guess the spec had been updated since then.
now fixing those parts
Comment 90 User image Tooru Fujisawa [:arai] 2016-05-13 03:49:56 PDT
efaust, do you know the purpose of the following Early Error rule?

https://tc39.github.io/ecmascript-asyncawait/#async-decls-exprs-static-semantics-early-errors
> * It is a Syntax Error if ContainsUseStrict of AsyncFunctionBody is true and IsSimpleParameterList of FormalParameters is false.

IIUC, it means the following code throws SyntaxError, but I'm not sure why it should.

  async function f(a = 1) { `use strict`; }

as following doesn't throw SyntaxError:

  `use strict`; async function f(a = 1) {}
Comment 91 User image Tooru Fujisawa [:arai] 2016-05-13 03:53:45 PDT
(In reply to Tooru Fujisawa [:arai] from comment #90)
> efaust, do you know the purpose of the following Early Error rule?
> 
> https://tc39.github.io/ecmascript-asyncawait/#async-decls-exprs-static-
> semantics-early-errors
> > * It is a Syntax Error if ContainsUseStrict of AsyncFunctionBody is true and IsSimpleParameterList of FormalParameters is false.
> 
> IIUC, it means the following code throws SyntaxError, but I'm not sure why
> it should.
> 
>   async function f(a = 1) { `use strict`; }
> 
> as following doesn't throw SyntaxError:
> 
>   `use strict`; async function f(a = 1) {}

Sorry, just noticed that it's just same as sync function's case, and SpiderMonkey haven't yet implemented it.
https://tc39.github.io/ecma262/#sec-function-definitions-static-semantics-early-errors
Comment 92 User image Tooru Fujisawa [:arai] 2016-05-16 21:14:40 PDT
Created attachment 8753179 [details] [diff] [review]
Part 1: Refactor JSOP_DEFFUN.

Separated JSOP_DEFFUN refactoring from previous Part 3, and merged with Part 5.
Comment 93 User image Tooru Fujisawa [:arai] 2016-05-16 21:15:20 PDT
Created attachment 8753180 [details] [diff] [review]
Part 2: Add parser support for Async functions. r=efaust

Rebased previous Part 3, and merged parser part of previous Part 6.
JS_HAS_ASYNC_FUNCS is defined only if SPIDERMONKEY_PROMISE is defined.

> +#ifdef SPIDERMONKEY_PROMISE
> +/* Support for ES7 Async Functions. */
> +#define JS_HAS_ASYNC_FUNCS 1
> +#endif // SPIDERMONKEY_PROMISE
Comment 94 User image Tooru Fujisawa [:arai] 2016-05-16 21:16:15 PDT Comment hidden (obsolete)
Comment 95 User image Tooru Fujisawa [:arai] 2016-05-16 21:17:10 PDT
Comment on attachment 8753181 [details] [diff] [review]
Part 4: Fix function name for export default async function.

oops, this was wrong one.
Comment 96 User image Tooru Fujisawa [:arai] 2016-05-16 21:17:48 PDT
Created attachment 8753182 [details] [diff] [review]
Part 3: Implement async functions. r=efaust,jandem

Rebased previous Part 4, and merged remaining part of previous Part 6, and also merged sel-hosting intrinsic from previous Part 2 (SetFunctionExtendedSlot, SetFunName, GetFunName).
Comment 97 User image Tooru Fujisawa [:arai] 2016-05-16 21:20:02 PDT
Created attachment 8753184 [details] [diff] [review]
Part 4: Fix function name for export default async function.

`export default async function() {}` should be named '*default*', and it needs separated handling than other expression after `export default`.

https://tc39.github.io/ecmascript-asyncawait/#async-decls-exprs-static-semantics-BoundNames
Comment 98 User image Tooru Fujisawa [:arai] 2016-05-16 21:20:43 PDT
Created attachment 8753185 [details] [diff] [review]
Part 5: Treat yield as an identifier in async function parameters and body.

`yield` is always an identifier in async function parameters and body. (but not function name).

https://tc39.github.io/ecmascript-asyncawait/#async-function-definitions

Removed JSMSG_YIELD_IN_ASYNC, as it's not valid error.
Comment 99 User image Tooru Fujisawa [:arai] 2016-05-16 21:26:05 PDT
Created attachment 8753190 [details] [diff] [review]
Part 6: Support async arrow function.

Added async arrow support.

Async arrow function with expression body |async () => 1| is converted into STATEMENTLIST body, to insert initial yield, and converted back to expression body in Reflect.parse.

then, in ecma_7/AsyncFunctions/arrow.js, there are some disabled tests for `yield` and `await` binding in strict mode.  is there any bug already filed for the `yield` binding issue?  (if so, I'll add the bug number there.  if not, I'll file it)


Also, added comment about the stack convention in emitAsyncWrapper.
Comment 100 User image Tooru Fujisawa [:arai] 2016-05-16 21:29:12 PDT
Created attachment 8753191 [details] [diff] [review]
Part 7: Add parser test for async/await.

Added parser test for modifier and error handling.
Comment 101 User image Tooru Fujisawa [:arai] 2016-05-16 21:31:30 PDT
Created attachment 8753193 [details] [diff] [review]
Part 8: Add Reflect.parse test for async/await.

Added Reflect.parse test.

failure cases are already tested in Part 2, so this tests whole structure for successful cases.
Comment 102 User image Eric Faust [:efaust] 2016-05-20 15:56:18 PDT
Comment on attachment 8753185 [details] [diff] [review]
Part 5: Treat yield as an identifier in async function parameters and body.

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

Nice. A few nits below.

::: js/src/frontend/Parser.cpp
@@ +2827,5 @@
>  }
>  
>  template <typename ParseHandler>
>  typename ParseHandler::Node
> +Parser<ParseHandler>::functionDef(InHandling inHandling,

oh, oops. Nice catch that this was dead.

@@ +3162,5 @@
>      else if (fun->isGetter())
>          syntaxKind = Getter;
>      else if (fun->isSetter())
>          syntaxKind = Setter;
> +    YieldHandling yieldHandling = GetYieldHandling(generatorKind, asyncKind);

nit: please insert blank line above this.

@@ +3290,5 @@
>  template <typename ParseHandler>
>  bool
>  Parser<ParseHandler>::checkYieldNameValidity()
>  {
> +    if (pc->isAsync())

Add comment being like "async functions are implemented as star generators, but not syntactically. Allow yield" or something.

@@ +3296,1 @@
>      // In star generators and in JS >= 1.7, yield is a keyword.  Otherwise in

nit: blank line before comment

@@ +8037,5 @@
>          if (endsExpr)
>              return stringLiteral();
>      }
>  
>      if (tt == TOK_YIELD && yieldExpressionsSupported()) {

nit: can remove braces from single line if.
Comment 103 User image Eric Faust [:efaust] 2016-05-20 16:23:12 PDT
Comment on attachment 8753190 [details] [diff] [review]
Part 6: Support async arrow function.

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

This is good, but we also need tests for async() being a function call. As I understand things, this should be the case with no backtracking. Is that true?

::: js/src/builtin/ReflectParse.cpp
@@ +3497,1 @@
>          builder.function(type, &pn->pn_pos, id, args, defaults, body,

can we align the builder. with the functionArgs above?

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6478,5 @@
>      /* Non-hoisted functions simply emit their respective op. */
>      if (!pn->functionIsHoisted()) {
>          /* JSOP_LAMBDA_ARROW is always preceded by a new.target */
>          MOZ_ASSERT(fun->isArrow() == (pn->getOp() == JSOP_LAMBDA_ARROW));
> +        if (needsProto) {

Does this case really need to move? It's only for class constructors, right?

@@ +6564,5 @@
>                     bi->kind() == Binding::ARGUMENT);
>          MOZ_ASSERT(bi.argOrLocalIndex() < JS_BIT(20));
>  #endif
>          if (funbox->isAsync()) {
> +            if (!emitAsyncWrapper(index, false, false))

please comment these bools with argument names, for future readers

::: js/src/frontend/Parser.cpp
@@ +1389,5 @@
>              return null();
>      } else {
>          MOZ_ASSERT(type == ExpressionBody);
>  
> +        Node stmtList = null();

A comment here about star generators being assumed to be statement lists would be nice.
Comment 104 User image Eric Faust [:efaust] 2016-05-20 16:26:09 PDT
Comment on attachment 8753193 [details] [diff] [review]
Part 8: Add Reflect.parse test for async/await.

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

Good.

::: js/src/builtin/ReflectParse.cpp
@@ +3529,5 @@
>          if (pnstart && pnstart->isKind(PNK_YIELD)) {
>              MOZ_ASSERT(pnstart->getOp() == JSOP_INITIALYIELD);
>              pnstart = pnstart->pn_next;
>          }
> +        // Async arrow with expression body is converted into STATEMENTLIST

nit: blank line before comment
Comment 105 User image Tooru Fujisawa [:arai] 2016-05-21 05:51:05 PDT
Thank you for reviewing :)

I found that the yield binding issue in strict mode is just a bug in my patch, will fix it.
Comment 106 User image Tooru Fujisawa [:arai] 2016-05-21 13:17:23 PDT
Created attachment 8755146 [details] [diff] [review]
Part 4: Fix function name for export default async function. r=efaust
Comment 107 User image Tooru Fujisawa [:arai] 2016-05-21 13:17:53 PDT
Created attachment 8755147 [details] [diff] [review]
Part 5: Treat yield as an identifier in async function parameters and body. r=efaust
Comment 108 User image Tooru Fujisawa [:arai] 2016-05-21 13:18:38 PDT
Created attachment 8755148 [details] [diff] [review]
Part 6: Support async arrow function. r=efaust
Comment 109 User image Tooru Fujisawa [:arai] 2016-05-21 13:25:26 PDT
Created attachment 8755149 [details] [diff] [review]
Part 6.1: Fix yield handling in async function.

yield handling in async arrow is inherited from enclosing script, so, `GetYieldHandling` needs some more information:
  * YieldHandling of encloding script
  * FunctionSyntaxKind for the function (whether it's arrow or not)
  * YieldHandldingContext (whether current context is arguments or body)

Also, added YieldIsNotInherited to YieldHandling, to clarify that the parameter has no meaning, in the case yield handling is not inherited.

also, yieldHandling differs for arguments and body, so moved GetYieldHandling call into functionArgsAndBodyGeneric, and call it for functionArguments and functionBody.
Comment 110 User image Tooru Fujisawa [:arai] 2016-05-21 13:33:49 PDT
Created attachment 8755150 [details] [diff] [review]
Part 6.2: Fix await handling in async function.

await should be set to keyword for whole function parameter, and also name of function expression.

destructuring parameter's default value can also contain await, and default parameter can be complex expression that contains await.
So, we should check the existence of await expression just like yield, in destructuring parameter and default parameter, by lastAwaitOffset.


Also added AutoAwaitIsKeyword, to automatically reset TokenStream's awaitIsKeyword to previous value, when leaving the scope.
Comment 111 User image Tooru Fujisawa [:arai] 2016-05-21 13:34:19 PDT
Created attachment 8755151 [details] [diff] [review]
Part 7: Add parser test for async/await. r=efaust
Comment 112 User image Tooru Fujisawa [:arai] 2016-05-21 13:34:47 PDT
Created attachment 8755152 [details] [diff] [review]
Part 8: Add Reflect.parse test for async/await. r=efaust
Comment 113 User image Tooru Fujisawa [:arai] 2016-05-23 04:57:00 PDT
so far, here are things to fix:
  * AsyncFunction constructor and prototype
  * length of the async function
  * toString
  * syntax priority for async function expression

may be some more.
some are already fixed locally.
Comment 114 User image Mark Hammond [:markh] 2016-05-23 05:51:05 PDT
(In reply to Tooru Fujisawa [:arai] from comment #113)
> so far, here are things to fix:

I'm not following this closely enough to know if this is sane, but is there any possibility of hiding the feature behind a nightly-only pref and having followup bugs that block it riding the trains?
Comment 115 User image Tooru Fujisawa [:arai] 2016-05-23 05:56:47 PDT
(In reply to Mark Hammond [:markh] from comment #114)
> (In reply to Tooru Fujisawa [:arai] from comment #113)
> > so far, here are things to fix:
> 
> I'm not following this closely enough to know if this is sane, but is there
> any possibility of hiding the feature behind a nightly-only pref and having
> followup bugs that block it riding the trains?

don't worry :)
No patches have been landed, and I won't land these patches until those issues are fixed.
also, async/await is behind two switches, NIGHTLY_BUILD and SPIDERMONKEY_PROMISE, that means even if I land them, it won't be available in Nightly.
Comment 116 User image Mark Hammond [:markh] 2016-05-23 06:08:30 PDT
(In reply to Tooru Fujisawa [:arai] from comment #115)
> don't worry :)
> No patches have been landed, and I won't land these patches until those
> issues are fixed.

I worry they *will not* land ASAP - I want to play with them!

> also, async/await is behind two switches, NIGHTLY_BUILD and
> SPIDERMONKEY_PROMISE

Right - that sounds great! I'm wondering if we can land that way before those other problems are fixed, and only remove those switches once those other issues are fixed?

> that means even if I land them, it won't be available
> in Nightly.

Oh - it looks like SPIDERMONKEY_PROMISE isn't enabled anywhere by default? I must have mis-guessed what that means...
Comment 117 User image Tooru Fujisawa [:arai] 2016-05-23 06:18:21 PDT
ah, I see.
Will try fixing remaining things shortly, now toString is the only remaining not-yet-fixed-locally issue :)
Comment 118 User image Till Schneidereit [till] 2016-05-23 06:22:44 PDT
(In reply to Mark Hammond [:markh] from comment #116)
> Oh - it looks like SPIDERMONKEY_PROMISE isn't enabled anywhere by default? I
> must have mis-guessed what that means...

It means I have to get this green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce5f80201fe1
Comment 119 User image Tooru Fujisawa [:arai] 2016-05-23 19:08:05 PDT
Created attachment 8755552 [details] [diff] [review]
Part 9: Fix syntax priority of async function expression.

async function expression was parsed as assignment expression, but should be primary expression

https://tc39.github.io/ecmascript-asyncawait/#PrimaryExpression
Comment 120 User image Tooru Fujisawa [:arai] 2016-05-23 19:15:26 PDT
Created attachment 8755554 [details] [diff] [review]
Part 10: Add AsyncFunction constructor and prototype.

Added AsyncFunction constructor, and AsyncFunctionPrototype.

wrapper's prototype is dynamically changed to AsyncFunctionPrototype in AsyncFunction_wrap.

AsyncFunction constructor and prototype are created just like GeneratorFunction constructor and prototype.

AsyncFunctionConstructor is the implementation of AsyncFunction constructor.
there is one tricky thing.  function created in FunctionConstructor is unwrapped function, so it uses GeneratorFunction as its prototype.
then, it's wrapped in AsyncFunctionConstructor, just like we do in bytecode.

Also, modified intrinsic_GetBuiltinConstructor to return AsyncFunction constructor, that has no JSProtoKey.
Comment 121 User image Tooru Fujisawa [:arai] 2016-05-23 19:16:36 PDT
Created attachment 8755555 [details] [diff] [review]
Part 11: Set length of async function.

Also, length of async function was not set to unwrapped function's one.
so, changed to define it inside AsyncFunction_wrap
Comment 122 User image Tooru Fujisawa [:arai] 2016-05-23 19:23:09 PDT
Created attachment 8755557 [details] [diff] [review]
Part 12: Support async function in Function.prototype.toString.

to support toString, we need to do following steps:
  1. check if given function is wrapper
  2. extract unwrapped function from wrapper
  3. apply toString on unwrapped function


for 1, stored "AsyncWrapper" string to wrapper's first reserved slot.
as, self-hosted function's first reserved slot is mostly used to store original function name, but it's not used for inner function.
it would be better to store name there for this case too, as we can use IsSelfHostedFunctionWithName to check it.

for 2, stored unwrapped function into wrapper's second reserved slot.


Then, just as a cleanup, moved slot handling done in CallObject::createForFunction into GetAsyncFunctionWrapper,
and added GetUnwrappedAsyncFunction for other way.
Comment 123 User image Till Schneidereit [till] 2016-05-23 19:26:28 PDT
Comment on attachment 8755554 [details] [diff] [review]
Part 10: Add AsyncFunction constructor and prototype.

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

::: js/src/builtin/AsyncFunctions.js
@@ +11,5 @@
>              var promiseCtor = GetBuiltinConstructor('Promise');
>              return callFunction(Promise_static_reject, promiseCtor, e);
>          }
>      };
> +    std_Object_setPrototypeOf(wrapper, GetBuiltinPrototype("AsyncFunction"));

It'd be great if we could somehow avoid this, because it costs us in terms of optimization potential. Specifically, every typeset the async function becomes part of will be made fully generic.

For a first implementation, that's probably fine, but we should ideally fix this before shipping.
Comment 124 User image Tooru Fujisawa [:arai] 2016-05-23 19:38:19 PDT
With all those patches applied, following tests still fails in async/await test262 ( https://github.com/tc39/test262/pull/479 ), but all of them are not async/await specific issue.


implement toStringTag (bug 1114580)
  * built-ins/AsyncFunction/AsyncFunctionPrototype-to-string.js


toString should reflect the comment inside arguments (bug 755821)
  * built-ins/Function/prototype/toString/AsyncFunction.js
  * built-ins/Function/prototype/toString/async-function-declaration.js
  * built-ins/Function/prototype/toString/async-function-expression.js
  * built-ins/Function/prototype/toString/async-method.js


"use strict" in body with default/destructuring should throw (bug 1272784)
  * language/expressions/async-arrow-function/early-errors-arrow-NSPL-with-USD.js
  * language/expressions/async-function/early-errors-expression-NSPL-with-USD.js
  * language/expressions/object/method-definition/early-errors-object-method-NSPL-with-USD.js
  * language/statements/async-function/early-errors-declaration-NSPL-with-USD.js
  * language/statements/class/definition/early-errors-class-method-NSPL-with-USD.js


redeclaration should be SyntaxError, but we're using TypeError (no bug)
  * language/expressions/async-arrow-function/early-errors-arrow-formals-body-duplicate.js
  * language/expressions/async-function/early-errors-expression-formals-body-duplicate.js
  * language/statements/async-function/early-errors-declaration-formals-body-duplicate
Comment 125 User image Tooru Fujisawa [:arai] 2016-05-23 19:39:32 PDT
(In reply to Till Schneidereit [:till] from comment #123)
> Comment on attachment 8755554 [details] [diff] [review]
> Part 10: Add AsyncFunction constructor and prototype.
> 
> Review of attachment 8755554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/AsyncFunctions.js
> @@ +11,5 @@
> >              var promiseCtor = GetBuiltinConstructor('Promise');
> >              return callFunction(Promise_static_reject, promiseCtor, e);
> >          }
> >      };
> > +    std_Object_setPrototypeOf(wrapper, GetBuiltinPrototype("AsyncFunction"));
> 
> It'd be great if we could somehow avoid this, because it costs us in terms
> of optimization potential. Specifically, every typeset the async function
> becomes part of will be made fully generic.
> 
> For a first implementation, that's probably fine, but we should ideally fix
> this before shipping.

Thank you for pointing that out :)
okay, will try finding a better solution.
Comment 126 User image Tooru Fujisawa [:arai] 2016-05-23 21:54:23 PDT
Comment on attachment 8755554 [details] [diff] [review]
Part 10: Add AsyncFunction constructor and prototype.

Okay, I got another solution without setPrototypeOf.
will post it after some more test.
Comment 127 User image Tooru Fujisawa [:arai] 2016-05-23 21:54:52 PDT
Comment on attachment 8755555 [details] [diff] [review]
Part 11: Set length of async function.

This should be fixed at once
Comment 128 User image Tooru Fujisawa [:arai] 2016-05-23 21:55:33 PDT
Comment on attachment 8755557 [details] [diff] [review]
Part 12: Support async function in Function.prototype.toString.

This will also be affected by the change.
Comment 129 User image Tooru Fujisawa [:arai] 2016-05-24 08:54:42 PDT
Created attachment 8755753 [details] [diff] [review]
Part 10: Add AsyncFunction constructor and prototype.

Moved everything into CreateAsyncFunction intrinsic.
There a new function is created with %AsyncFunctionPrototype%,
reusing the script and the environment of `wrapper` function,
and the name and the length of `unwrapped` function.

For now, I'm using 3 names for each function:
  * wrapper   -- a function defined inside AsyncFunction_wrap
  * unwrapped -- a function created by compiling actual async function
  * wrapped   -- a function created by CreateAsyncFunction
                 its script and environment are `wrapper`'s,
                 and name and length are `unwrapped`'s.

  wrapper
     |
     | script, environment
     |
     |   unwrapped
     |       |
     |       | name, length
     |       |
     v       v
CreateAsyncFunction
     |
     v
  wrapped
Comment 130 User image Tooru Fujisawa [:arai] 2016-05-24 08:55:34 PDT
Created attachment 8755754 [details] [diff] [review]
Part 11: Support async function in Function.prototype.toString.

Moved most part to Part 10.
Comment 131 User image Tooru Fujisawa [:arai] 2016-05-24 10:12:05 PDT
just as a note, |(async function f(){}).isGenerator()| returns false.
not sure how people expects the value to be tho.
(related to bug 1119777)
Comment 132 User image Tooru Fujisawa [:arai] 2016-05-24 11:31:56 PDT
Filed bug 1275240 for redeclaration of formal parameter
Comment 133 User image Tooru Fujisawa [:arai] 2016-07-17 02:48:14 PDT
Created attachment 8771755 [details] [diff] [review]
Part 1: Refactor JSOP_DEFFUN. r=efaust,jandem

rebasing on bug 911216, and some trivial fix on some parts.
Comment 134 User image Tooru Fujisawa [:arai] 2016-07-17 02:49:25 PDT
Created attachment 8771756 [details] [diff] [review]
Part 2: Add parser support for Async functions. r=efaust,jwalden
Comment 135 User image Tooru Fujisawa [:arai] 2016-07-17 02:49:52 PDT
Created attachment 8771757 [details] [diff] [review]
Part 3: Implement async functions. r=efaust,jandem
Comment 136 User image Tooru Fujisawa [:arai] 2016-07-17 02:50:09 PDT
Created attachment 8771758 [details] [diff] [review]
Part 4: Fix function name for export default async function. r=efaust
Comment 137 User image Tooru Fujisawa [:arai] 2016-07-17 02:50:29 PDT
Created attachment 8771759 [details] [diff] [review]
Part 5: Treat yield as an identifier in async function parameters and body. r=efaust
Comment 138 User image Tooru Fujisawa [:arai] 2016-07-17 02:50:49 PDT
Created attachment 8771760 [details] [diff] [review]
Part 6: Support async arrow function. r=efaust
Comment 139 User image Tooru Fujisawa [:arai] 2016-07-17 02:51:43 PDT
Created attachment 8771761 [details] [diff] [review]
Part 6.1: Fix yield handling in async function.

details in comment #109
Comment 140 User image Tooru Fujisawa [:arai] 2016-07-17 02:52:12 PDT
Created attachment 8771762 [details] [diff] [review]
Part 6.2: Fix await handling in async function.

details in comment #110
Comment 141 User image Tooru Fujisawa [:arai] 2016-07-17 02:52:35 PDT
Created attachment 8771763 [details] [diff] [review]
Part 7: Add parser test for async/await. r=efaust
Comment 142 User image Tooru Fujisawa [:arai] 2016-07-17 02:52:57 PDT
Created attachment 8771764 [details] [diff] [review]
Part 8: Add Reflect.parse test for async/await. r=efaust
Comment 143 User image Tooru Fujisawa [:arai] 2016-07-17 02:53:46 PDT
Created attachment 8771765 [details] [diff] [review]
Part 9: Fix syntax priority of async function expression.

details in comment #119
Comment 144 User image Tooru Fujisawa [:arai] 2016-07-17 02:55:10 PDT
Created attachment 8771766 [details] [diff] [review]
Part 10: Add AsyncFunction constructor and prototype.

details in comment #129
Comment 145 User image Tooru Fujisawa [:arai] 2016-07-17 02:55:39 PDT
Created attachment 8771767 [details] [diff] [review]
Part 11: Support async function in Function.prototype.toString.

details in comment #122 and comment #130
Comment 146 User image Tooru Fujisawa [:arai] 2016-08-27 01:29:53 PDT
Comment on attachment 8771761 [details] [diff] [review]
Part 6.1: Fix yield handling in async function.

will rebase all these patches and cleanup the patch order/granularity.
Comment 147 User image Tooru Fujisawa [:arai] 2016-08-28 08:20:59 PDT
with new EnvironmentObject, there seems to need some more tricks to mix wrapper/unwrapped functions.
will try finding a solution.
Comment 148 User image Tooru Fujisawa [:arai] 2016-08-28 09:00:28 PDT
The issue is that, we need pseudo NamedLambdaObject for named async function expression,
that points wrapped function, instead of unwrapped function.

  var wrapped = async function func() {
    return func;
  };
  var resolved;
  wrapped().then(r=>resolved=r);
  drainJobQueue();
  assertEq(resolved, wrapped);
Comment 150 User image Tooru Fujisawa [:arai] 2016-08-29 01:52:55 PDT
Created attachment 8785838 [details] [diff] [review]
Part 0.1: Refactor JSOP_DEFFUN. r=efaust,jandem

just rebased.
no change
Comment 151 User image Tooru Fujisawa [:arai] 2016-08-29 01:54:15 PDT
Created attachment 8785840 [details] [diff] [review]
Part 0.2: Pass GeneratorKind to Parser::functionFormalParametersAndBody and calculate YieldHandling inside it. r?efaust

Later in the patch series, we'll need to calculate YieldHandling for function arguments and body separately.
As a preparation, changed Parser::functionFormalParametersAndBody to receive GeneratorKind instead of YieldHandling, and calculate YieldHandling inside it.
Comment 152 User image Tooru Fujisawa [:arai] 2016-08-29 01:54:45 PDT
Created attachment 8785841 [details] [diff] [review]
Part 0.3: Pass YieldHandling to Parser::checkYieldNameValidity for later use. r?efaust

Also, later in the patch, we need to check yield name validity depends on current yield handling.
As a preparation, changed the signature of Parser::checkYieldNameValidity to receive YieldHandling.
Comment 153 User image Tooru Fujisawa [:arai] 2016-08-29 01:55:55 PDT
Created attachment 8785842 [details] [diff] [review]
Part 0.4: Pass YieldHandling to Parser::functionFormalParametersAndBody for later use. r?efaust

One more preparation.
Parser::functionFormalParametersAndBody was changed to receive GeneratorKind instead of YieldHandling, but actually we need another YieldHandling parameter, that is the YieldHandling for outside of the function (instead of inside the function, that was previous one), that affects the YieldHandling of async arrow arguments.

YieldHandling enum is extended to 3 states YieldIsName, YieldIsKeyword, and YieldIsNotInherited.
YieldIsNotInherited explicitly represents yield handling is not inherited for the context, like non-arrow.
Comment 154 User image Tooru Fujisawa [:arai] 2016-08-29 01:56:16 PDT
Created attachment 8785843 [details] [diff] [review]
Part 1: Add AsyncFunction flag in FunctionBox, JSScript, and LazyScript. r=efaust

Separated the change for FunctionBox, JSScript, and LazyScript.
no changes except minor styling.
Comment 155 User image Tooru Fujisawa [:arai] 2016-08-29 01:56:45 PDT
Created attachment 8785844 [details] [diff] [review]
Part 2: Add FunctionAsyncKind parameter to Parser methods that receives GeneratorKind. r=efaust

For most cases, we need FunctionAsyncKind parameter where the Parser methods receives GeneratorKind.
Separated the change for it.
no changes except rebase on different new methods from frontend rewrite.
Comment 156 User image Tooru Fujisawa [:arai] 2016-08-29 01:57:30 PDT
Created attachment 8785845 [details] [diff] [review]
Part 3: Add await token. r=efaust

Separated the change for TokenStream, for TOK_AWAIT.
Removed unused TOK_ASYNC.
No other changes except moving awaitIsKeyword to private.
Comment 157 User image Tooru Fujisawa [:arai] 2016-08-29 01:58:07 PDT
Created attachment 8785846 [details] [diff] [review]
Part 4: Add AutoAwaitIsKeyword class to set TokenStream.awaitIsKeyword. r?efaust

Previously we were directly modifying awaitIsKeyword property, but it should be better the property is changed back to original state automatically when leaving the context.
So added AutoAwaitIsKeyword class that sets and resets awaitIsKeyword.
Comment 158 User image Tooru Fujisawa [:arai] 2016-08-29 01:58:43 PDT
Created attachment 8785847 [details] [diff] [review]
Part 5.1: Support async function declaration in Parser. r=efaust,jwalden

Separated async function declaration part.
no changes.
Comment 159 User image Tooru Fujisawa [:arai] 2016-08-29 01:59:10 PDT
Created attachment 8785848 [details] [diff] [review]
Part 5.2: Support async function declaration in Reflect.parse. r=efaust

Also separated async function declaration part of Reflect.parse.
no changes.
Comment 160 User image Tooru Fujisawa [:arai] 2016-08-29 02:00:12 PDT
Created attachment 8785851 [details] [diff] [review]
Part 5.3: Support await expression in Parser. r?efaust

Await handling was changed in previous not-yet-reviewed patch.
Separated that part.

  * Added ParseHandler::newAwaitExpression, instead of reusing newYieldExpression.
  * Throw error when destructuring parameter contains await,
    just in the same way as yield
Comment 161 User image Tooru Fujisawa [:arai] 2016-08-29 02:00:36 PDT
Created attachment 8785852 [details] [diff] [review]
Part 5.4: Support await expression in Reflect.parse. r=efaust

Separated await part of Reflect.parse.
no changes.
Comment 162 User image Tooru Fujisawa [:arai] 2016-08-29 02:01:04 PDT
Created attachment 8785853 [details] [diff] [review]
Part 5.5: Add parser test for async function declaration. r=efaust

Separated basic parser test for async function declaration.
no changes except BUGNUMBER and summary for each test,
and using anonymous function in shell.js.
Comment 163 User image Tooru Fujisawa [:arai] 2016-08-29 02:02:44 PDT
Created attachment 8785856 [details] [diff] [review]
Part 5.6: Add parser test for yield in async function declaration. r?efaust

Test for yield handling in async function.
Comment 164 User image Tooru Fujisawa [:arai] 2016-08-29 02:04:33 PDT
Created attachment 8785857 [details] [diff] [review]
Part 6.1: Support async function expression in Parser. r?efaust

Async function expression's priority was fixed in previous not-yet-reviewed patch.
separated that part.
Comment 165 User image Tooru Fujisawa [:arai] 2016-08-29 02:05:00 PDT
Created attachment 8785859 [details] [diff] [review]
Part 6.2: Add parser test for async function expression. r=efaust

Separated async function expression part.
no changes.
Comment 166 User image Tooru Fujisawa [:arai] 2016-08-29 02:05:44 PDT
Created attachment 8785860 [details] [diff] [review]
Part 6.3: Add parser test for yield in async function expression. r?efaust

test for yield in async function expression.
Comment 167 User image Tooru Fujisawa [:arai] 2016-08-29 02:06:13 PDT
Created attachment 8785861 [details] [diff] [review]
Part 7.1: Support async method in Parser. r=efaust,jwalden

Separated async method handling.
no changes.
Comment 168 User image Tooru Fujisawa [:arai] 2016-08-29 02:07:08 PDT
Created attachment 8785862 [details] [diff] [review]
Part 7.2: Add parser test for async method. r=efaust

Separated async method part.
no changes.
Comment 169 User image Tooru Fujisawa [:arai] 2016-08-29 02:09:20 PDT
Created attachment 8785864 [details] [diff] [review]
Part 7.3: Add parser test for yield in async method. r?efaust

test for yield in async method.
Comment 170 User image Tooru Fujisawa [:arai] 2016-08-29 02:09:48 PDT
Created attachment 8785868 [details] [diff] [review]
Part 8.1: Treat await as keyword in module. r=efaust

Changed to use AutoAwaitIsKeyword in module body.
no other changes.
Comment 171 User image Tooru Fujisawa [:arai] 2016-08-29 02:10:17 PDT
Created attachment 8785869 [details] [diff] [review]
Part 8.2: Add parser test for await in module. r=efaust

Separated async function test for modules.
no changes.
Comment 172 User image Tooru Fujisawa [:arai] 2016-08-29 02:11:17 PDT
Created attachment 8785870 [details] [diff] [review]
Part 9.1: Support async function statement in export default in Parser. r=efaust

Separated async function in export default.
no changes.
Comment 173 User image Tooru Fujisawa [:arai] 2016-08-29 02:11:42 PDT
Created attachment 8785871 [details] [diff] [review]
Part 9.2: Add parser test for async function statement in export default. r=efaust

Separated test for async function in export default.
no changes.
Comment 174 User image Tooru Fujisawa [:arai] 2016-08-29 02:12:50 PDT
Created attachment 8785872 [details] [diff] [review]
Part 9.3: Add parser test for yield in async function statement in export default. r?efaust

test for yield in async function in export default.
Comment 175 User image Tooru Fujisawa [:arai] 2016-08-29 02:14:35 PDT
Created attachment 8785873 [details] [diff] [review]
Part 10.1: Support async arrow function in Parser. r?efaust

Now YieldHandling is calculated separately for arguments and body, especially for async arrow.
Async arrow function inherits yield handling from enclosing context.

other await/yield handling is separated to Part 5.3.
Comment 176 User image Tooru Fujisawa [:arai] 2016-08-29 02:14:59 PDT
Created attachment 8785874 [details] [diff] [review]
Part 10.2: Support async arrow function in Reflect.parse. r=efaust

Separated async arrow part of Reflect.parse.
no changes.
Comment 177 User image Tooru Fujisawa [:arai] 2016-08-29 02:15:23 PDT
Created attachment 8785875 [details] [diff] [review]
Part 10.3: Add parser test for async arrow function. r=efaust

Separated test for async arrow.
no changes.
Comment 178 User image Tooru Fujisawa [:arai] 2016-08-29 02:16:38 PDT
Created attachment 8785877 [details] [diff] [review]
Part 11.1: Implement async functions. r?efaust

Previous part 10.
No change from previous one except NamedLambdaObject.

Summary from comment 129, and some extra.

Async function instance is created by CreateAsyncFunction intrinsic, called from AsyncFunction_wrap.

There a new function is created with %AsyncFunctionPrototype%,
reusing the script and the environment of `wrapper` function,
and the name and the length of `unwrapped` function.

For now, I'm using 3 names for each function:
  * wrapper   -- a function defined inside AsyncFunction_wrap
  * unwrapped -- a function created by compiling actual async function
  * wrapped   -- a function created by CreateAsyncFunction
                 its script and environment are `wrapper`'s,
                 and name and length are `unwrapped`'s.

  wrapper
     |
     | script, environment
     |
     |   unwrapped
     |       |
     |       | name, length
     |       |
     v       v
CreateAsyncFunction
     |
     v
  wrapped


`unwrapped` function can be retrieved from `wrapped` by `GetUnwrappedAsyncFunction`,
and `wrapped` can be retrieved from `unwrapped` by `GetWrappedAsyncFunction`.
Both references are stored in 2nd extended slot.

Here, fun->isAsync() is true for `unwrapped`, not `wrapped`.
`IsWrappedAsyncFunction` can be used to detect `wrapped`, it checks self hosted name.

If unwrapped function is named lambda, NamedLambdaObject is created with wrapped function for the name binding.
NamedLambdaObject::create is modified to accept `callee` and function value separately, to support this.
Comment 179 User image Tooru Fujisawa [:arai] 2016-08-29 02:17:15 PDT
Created attachment 8785878 [details] [diff] [review]
Part 11.2: Add helper functions for async/await test. r?efaust.

With previous helper functions, it's hard to see where the test fails,
so changed it to use drainJobQueue and check instantly.
So that if one test fails, we can see the stack trace.
Comment 180 User image Tooru Fujisawa [:arai] 2016-08-29 02:17:45 PDT
Created attachment 8785879 [details] [diff] [review]
Part 11.3: Add semantics test for async/await. r=efaust.

test for semantics, no changes except BUGNUMBER/summary headers and changes from helper functions.
Comment 181 User image Tooru Fujisawa [:arai] 2016-08-29 02:18:12 PDT
Created attachment 8785880 [details] [diff] [review]
Part 11.4: Add function length test for async function. r?efaust.

Added test for length property, as we're emulating unwrapped length in wrapped function.
Comment 182 User image Tooru Fujisawa [:arai] 2016-08-29 02:18:41 PDT
Created attachment 8785886 [details] [diff] [review]
Part 11.5: Add async function constructor and prototype. r?efaust.

Added constructor/prototype tests.
Comment 183 User image Tooru Fujisawa [:arai] 2016-08-29 02:19:10 PDT
Created attachment 8785897 [details] [diff] [review]
Part 11.6: Add test for async function expression binding identity. r?efaust

named lambda now uses NamedLambdaObject to store/get the binding.
added test for it.
Comment 184 User image Tooru Fujisawa [:arai] 2016-08-29 02:19:57 PDT
Created attachment 8785899 [details] [diff] [review]
Part 12: Return wrapped function for arguments.callee. r?efaust

`arguments.callee` should return some value that makes sense.
without this patch, it returns `unwrapped` function, that is not exposed elsewhere.
changed it to return `wrapped`.

maybe we could just throw error, or return undefined, as there is no backward compat issue,
as this is implementation dependent.
Comment 185 User image Tooru Fujisawa [:arai] 2016-08-29 02:21:02 PDT
Created attachment 8785900 [details] [diff] [review]
Part 13: Support async function in Function.prototype.toString. r?efaust

async function should return string representation of `unwrapped`, for toString call on `wrapped`,
so if it detects the `this` is async, calls FunctionToString with `unwrapped`.
Comment 186 User image Tooru Fujisawa [:arai] 2016-09-24 19:44:30 PDT
Comment on attachment 8785841 [details] [diff] [review]
Part 0.3: Pass YieldHandling to Parser::checkYieldNameValidity for later use. r?efaust

checkYieldNameValidity is now removed.
Comment 187 User image Tooru Fujisawa [:arai] 2016-09-26 20:00:59 PDT
Created attachment 8795105 [details] [diff] [review]
Part 0.1: Refactor JSOP_DEFFUN. r=efaust,jandem

rebased.

will post remaining patches with bzexport.
Comment 188 User image Tooru Fujisawa [:arai] 2016-09-26 20:03:22 PDT
Created attachment 8795106 [details] [diff] [review]
Part 0.2: Pass GeneratorKind to Parser::functionFormalParametersAndBody and calculate YieldHandling inside it
Comment 189 User image Tooru Fujisawa [:arai] 2016-09-26 20:05:35 PDT
no... I wasn't about to steal assignee :/
Comment 190 User image Tooru Fujisawa [:arai] 2016-09-26 20:06:20 PDT
Created attachment 8795107 [details] [diff] [review]
Part 0.3: Pass YieldHandling to Parser::functionFormalParametersAndBody for later use
Comment 191 User image Tooru Fujisawa [:arai] 2016-09-26 20:08:40 PDT
Created attachment 8795108 [details] [diff] [review]
Part 1: Add AsyncFunction flag in FunctionBox, JSScript, and LazyScript
Comment 192 User image Tooru Fujisawa [:arai] 2016-09-26 20:08:54 PDT
Created attachment 8795109 [details] [diff] [review]
Part 2: Add FunctionAsyncKind parameter to Parser methods that receives GeneratorKind
Comment 193 User image Tooru Fujisawa [:arai] 2016-09-26 20:09:01 PDT
Created attachment 8795110 [details] [diff] [review]
Part 3: Add await token
Comment 194 User image Tooru Fujisawa [:arai] 2016-09-26 20:09:30 PDT
Created attachment 8795111 [details] [diff] [review]
Part 4: Add AutoAwaitIsKeyword class to set TokenStream.awaitIsKeyword
Comment 195 User image Tooru Fujisawa [:arai] 2016-09-26 20:10:34 PDT
Created attachment 8795112 [details] [diff] [review]
Part 5.1: Support async function declaration in Parser
Comment 196 User image Tooru Fujisawa [:arai] 2016-09-26 20:11:02 PDT
Created attachment 8795113 [details] [diff] [review]
Part 5.2: Support async function declaration in Reflect.parse
Comment 197 User image Tooru Fujisawa [:arai] 2016-09-26 20:11:09 PDT
Created attachment 8795114 [details] [diff] [review]
Part 5.3: Support await expression in Parser
Comment 198 User image Tooru Fujisawa [:arai] 2016-09-26 20:11:16 PDT
Created attachment 8795115 [details] [diff] [review]
Part 5.4: Support await expression in Reflect.parse
Comment 199 User image Tooru Fujisawa [:arai] 2016-09-26 20:11:23 PDT
Created attachment 8795116 [details] [diff] [review]
Part 5.5: Add parser test for async function declaration
Comment 200 User image Tooru Fujisawa [:arai] 2016-09-26 20:11:30 PDT
Created attachment 8795117 [details] [diff] [review]
Part 5.6: Add parser test for yield in async function declaration
Comment 201 User image Tooru Fujisawa [:arai] 2016-09-26 20:11:38 PDT
Created attachment 8795118 [details] [diff] [review]
Part 6.1: Support async function expression in Parser
Comment 202 User image Tooru Fujisawa [:arai] 2016-09-26 20:12:11 PDT
Created attachment 8795119 [details] [diff] [review]
Part 6.2: Add parser test for async function expression
Comment 203 User image Tooru Fujisawa [:arai] 2016-09-26 20:12:19 PDT
Created attachment 8795120 [details] [diff] [review]
Part 6.3: Add parser test for yield in async function expression
Comment 204 User image Tooru Fujisawa [:arai] 2016-09-26 20:12:26 PDT
Created attachment 8795121 [details] [diff] [review]
Part 7.1: Support async method in Parser
Comment 205 User image Tooru Fujisawa [:arai] 2016-09-26 20:12:33 PDT
Created attachment 8795122 [details] [diff] [review]
Part 7.2: Add parser test for async method
Comment 206 User image Tooru Fujisawa [:arai] 2016-09-26 20:12:41 PDT
Created attachment 8795123 [details] [diff] [review]
Part 7.3: Add parser test for yield in async method
Comment 207 User image Tooru Fujisawa [:arai] 2016-09-26 20:12:48 PDT
Created attachment 8795124 [details] [diff] [review]
Part 8.1: Treat await as keyword in module
Comment 208 User image Tooru Fujisawa [:arai] 2016-09-26 20:12:55 PDT
Created attachment 8795125 [details] [diff] [review]
Part 8.2: Add parser test for await in module
Comment 209 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:02 PDT
Created attachment 8795126 [details] [diff] [review]
Part 9.1: Support async function statement in export default in Parser
Comment 210 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:09 PDT
Created attachment 8795127 [details] [diff] [review]
Part 9.2: Add parser test for async function statement in export default
Comment 211 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:16 PDT
Created attachment 8795128 [details] [diff] [review]
Part 9.3: Add parser test for yield in async function statement in export default
Comment 212 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:25 PDT
Created attachment 8795129 [details] [diff] [review]
Part 10.1: Support async arrow function in Parser
Comment 213 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:32 PDT
Created attachment 8795130 [details] [diff] [review]
Part 10.2: Support async arrow function in Reflect.parse
Comment 214 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:39 PDT
Created attachment 8795131 [details] [diff] [review]
Part 10.3: Add parser test for async arrow function
Comment 215 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:48 PDT
Created attachment 8795132 [details] [diff] [review]
Part 11.1: Implement async functions
Comment 216 User image Tooru Fujisawa [:arai] 2016-09-26 20:13:56 PDT
Created attachment 8795133 [details] [diff] [review]
Part 11.2: Add helper functions for async/await test
Comment 217 User image Tooru Fujisawa [:arai] 2016-09-26 20:14:04 PDT
Created attachment 8795134 [details] [diff] [review]
Part 11.3: Add semantics test for async/await
Comment 218 User image Tooru Fujisawa [:arai] 2016-09-26 20:14:11 PDT
Created attachment 8795135 [details] [diff] [review]
Part 11.4: Add function length test for async function
Comment 219 User image Tooru Fujisawa [:arai] 2016-09-26 20:14:18 PDT
Created attachment 8795136 [details] [diff] [review]
Part 11.5: Add async function constructor and prototype
Comment 220 User image Tooru Fujisawa [:arai] 2016-09-26 20:14:26 PDT
Created attachment 8795137 [details] [diff] [review]
Part 11.6: Add test for async function expression binding identity
Comment 221 User image Tooru Fujisawa [:arai] 2016-09-26 20:14:34 PDT
Created attachment 8795138 [details] [diff] [review]
Part 12: Return wrapped function for arguments.callee
Comment 222 User image Tooru Fujisawa [:arai] 2016-09-26 20:14:43 PDT
Created attachment 8795139 [details] [diff] [review]
Part 13: Support async function in Function.prototype.toString
Comment 223 User image Tooru Fujisawa [:arai] 2016-09-26 20:16:27 PDT
Comment on attachment 8795107 [details] [diff] [review]
Part 0.3: Pass YieldHandling to Parser::functionFormalParametersAndBody for later use

Removed YieldIsNotInherited, and changed to pass actual value.
  * added outerYieldHandling parameter to Parser::functionExpr, to pass to Parser::functionDelazifying
  * added LazyScript.p_.outerYieldIsKeyword to store outerYieldHandling
Comment 224 User image Tooru Fujisawa [:arai] 2016-10-08 13:47:47 PDT
Created attachment 8799151 [details]
a.txt

Just to obsolete all patches.
will post to mozreview, to avoid attaching these patches again :P
Comment 225 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 226 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 227 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 228 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 229 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 230 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 231 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 232 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 233 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 234 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 235 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 236 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 237 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 238 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 239 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 240 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 241 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 242 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 243 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 244 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 245 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 246 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 247 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 248 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 249 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 250 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 251 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 252 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 253 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 254 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 255 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 256 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 257 User image Tooru Fujisawa [:arai] 2016-10-08 13:50:59 PDT Comment hidden (mozreview-request)
Comment 258 User image Alexandre Folle de Menezes 2016-10-24 11:15:33 PDT
This is already supported on Chrome 55 (beta).  Should a [chrome-parity] tag be added?
Comment 259 User image Till Schneidereit [till] 2016-10-26 06:01:53 PDT
Comment on attachment 8799152 [details]
Bug 1185106 - Part 0.1: Refactor JSOP_DEFFUN.

https://reviewboard.mozilla.org/r/84420/#review87616

Looks good, but I think it'd be good to have a JIT person take a look at this, too. I'm also just assuming that the change I commented on below is ok, but would like to have an explanation for it.

::: js/src/vm/Interpreter.cpp
(Diff revision 1)
>  bool
>  js::DefFunOperation(JSContext* cx, HandleScript script, HandleObject envChain,
> -                    HandleFunction funArg)
> +                    HandleFunction fun)
>  {
>      /*
> -     * If static link is not current scope, clone fun's object to link to the

Why isn't this needed anymore?
Comment 260 User image Till Schneidereit [till] 2016-10-26 06:06:50 PDT
Comment on attachment 8799152 [details]
Bug 1185106 - Part 0.1: Refactor JSOP_DEFFUN.

Hannes, can you take a look at the JIT pieces?
Comment 261 User image Till Schneidereit [till] 2016-10-26 06:17:10 PDT
Comment on attachment 8799153 [details]
Bug 1185106 - Part 1: Add AsyncFunction flag in FunctionBox, JSScript, and LazyScript.

https://reviewboard.mozilla.org/r/84422/#review87620

r=me with feedback addressed.

::: js/src/frontend/SharedContext.h:474
(Diff revision 1)
>      bool            usesThis:1;             /* contains 'this' */
>      bool            usesReturn:1;           /* contains a 'return' statement */
>  
>      FunctionContextFlags funCxFlags;
>  
> +    FunctionAsyncKind _asyncKind;

Can you turn this into `uint8_t asyncKindBits_` and move it to right after `generatorKindBits_`? That'd keep `FunctionBox` at its current size, which would be nice.

::: js/src/frontend/SharedContext.h:538
(Diff revision 1)
>      GeneratorKind generatorKind() const { return GeneratorKindFromBits(generatorKindBits_); }
>      bool isGenerator() const { return generatorKind() != NotGenerator; }
>      bool isLegacyGenerator() const { return generatorKind() == LegacyGenerator; }
>      bool isStarGenerator() const { return generatorKind() == StarGenerator; }
> +    bool isAsync() const { return _asyncKind == AsyncFunction; }
> +    FunctionAsyncKind asyncKind() const { return _asyncKind; }

... obviously that'd require introducing an `AsyncKindFromBits` helper and using it here. r=me on that if it's a straight duplication of `GeneratorKindFromBits`.
Comment 262 User image Till Schneidereit [till] 2016-10-26 06:20:19 PDT
Comment on attachment 8799154 [details]
Bug 1185106 - Part 2: Add FunctionAsyncKind parameter to Parser methods that receives GeneratorKind.

https://reviewboard.mozilla.org/r/84424/#review87624

r=me
Comment 263 User image Till Schneidereit [till] 2016-10-26 06:21:20 PDT
Comment on attachment 8799155 [details]
Bug 1185106 - Part 3: Add await token.

https://reviewboard.mozilla.org/r/84426/#review87628

r=me
Comment 264 User image Till Schneidereit [till] 2016-10-26 06:24:37 PDT
Comment on attachment 8799156 [details]
Bug 1185106 - Part 4: Add AutoAwaitIsKeyword class to set TokenStream.awaitIsKeyword.

https://reviewboard.mozilla.org/r/84428/#review87630

r=me with nit addressed.

::: js/src/frontend/TokenStream.h:1035
(Diff revision 1)
>  
> +class MOZ_STACK_CLASS AutoAwaitIsKeyword
> +{
> +private:
> +    TokenStream* ts_;
> +    bool beforeAwaitIsKeyword_;

Please rename this to `oldAwaitIsKeyword_`, which is in keeping with comparable things like `oldCompartment_` in `JSAutoCompartment`.
Comment 265 User image Till Schneidereit [till] 2016-10-26 06:30:49 PDT
Comment on attachment 8799157 [details]
Bug 1185106 - Part 5.1: Support async function declaration in Parser.

https://reviewboard.mozilla.org/r/84430/#review87632

r=me with nits addressed.

::: js/src/frontend/Parser.cpp:3249
(Diff revision 1)
>      if (!funpc.init())
>          return null();
>  
>      // Our tokenStream has no current token, so pn's position is garbage.
>      // Substitute the position of the first token in our source. If the function
> -    // is an arrow, use TokenStream::Operand to keep verifyConsistentModifier
> +    // is an not-async arrow, use TokenStream::Operand to keep

nit: s/is an/is a/

::: js/src/js.msg:185
(Diff revision 1)
>  MSG_DEF(JSMSG_ACCESSOR_WRONG_ARGS,     3, JSEXN_SYNTAXERR, "{0} functions must have {1} argument{2}")
>  MSG_DEF(JSMSG_ARRAY_COMP_LEFTSIDE,     0, JSEXN_SYNTAXERR, "invalid array comprehension left-hand side")
>  MSG_DEF(JSMSG_ARRAY_INIT_TOO_BIG,      0, JSEXN_INTERNALERR, "array initializer too large")
>  MSG_DEF(JSMSG_AS_AFTER_IMPORT_STAR,    0, JSEXN_SYNTAXERR, "missing keyword 'as' after import *")
>  MSG_DEF(JSMSG_AS_AFTER_RESERVED_WORD,  1, JSEXN_SYNTAXERR, "missing keyword 'as' after reserved word '{0}'")
> +MSG_DEF(JSMSG_ASYNC_GENERATOR,         0, JSEXN_SYNTAXERR, "generator function or method may not be async")

nit: please change this from "may not" to "can't".
Comment 266 User image Till Schneidereit [till] 2016-10-26 06:32:14 PDT
Comment on attachment 8799158 [details]
Bug 1185106 - Part 5.2: Support async function declaration in Reflect.parse.

https://reviewboard.mozilla.org/r/84432/#review87634

r=me
Comment 267 User image Till Schneidereit [till] 2016-10-26 06:41:59 PDT
Comment on attachment 8799159 [details]
Bug 1185106 - Part 5.3: Support await expression in Parser.

https://reviewboard.mozilla.org/r/84434/#review87636

r=me with nits addressed.

::: js/src/frontend/Parser.h:1158
(Diff revision 1)
>                      PossibleError* possibleError,
>                      InvokedPrediction invoked = PredictUninvoked);
>      Node assignExpr(InHandling inHandling, YieldHandling yieldHandling,
>                      TripledotHandling tripledotHandling,
>                      InvokedPrediction invoked = PredictUninvoked);
> -    Node assignExprWithoutYield(YieldHandling yieldHandling, unsigned err);
> +    Node assignExprWithoutYieldAndAwait(YieldHandling yieldHandling);

Please change this to "WithoutYieldOrAwait"

::: js/src/frontend/Parser.h:1223
(Diff revision 1)
>      Node generatorComprehension(uint32_t begin);
>  
>      bool argumentList(YieldHandling yieldHandling, Node listNode, bool* isSpread);
>      Node destructuringDeclaration(DeclarationKind kind, YieldHandling yieldHandling,
>                                    TokenKind tt);
> -    Node destructuringDeclarationWithoutYield(DeclarationKind kind, YieldHandling yieldHandling,
> +    Node destructuringDeclarationWithoutYieldAndAwait(DeclarationKind kind, YieldHandling yieldHandling,

Same here.

::: js/src/js.msg:186
(Diff revision 1)
>  MSG_DEF(JSMSG_ARRAY_COMP_LEFTSIDE,     0, JSEXN_SYNTAXERR, "invalid array comprehension left-hand side")
>  MSG_DEF(JSMSG_ARRAY_INIT_TOO_BIG,      0, JSEXN_INTERNALERR, "array initializer too large")
>  MSG_DEF(JSMSG_AS_AFTER_IMPORT_STAR,    0, JSEXN_SYNTAXERR, "missing keyword 'as' after import *")
>  MSG_DEF(JSMSG_AS_AFTER_RESERVED_WORD,  1, JSEXN_SYNTAXERR, "missing keyword 'as' after reserved word '{0}'")
>  MSG_DEF(JSMSG_ASYNC_GENERATOR,         0, JSEXN_SYNTAXERR, "generator function or method may not be async")
> +MSG_DEF(JSMSG_AWAIT_IN_DEFAULT,        0, JSEXN_SYNTAXERR, "await in default expression")

nit: change this to "await can't be used in default expressions"
Comment 268 User image Till Schneidereit [till] 2016-10-26 06:42:37 PDT
Comment on attachment 8799160 [details]
Bug 1185106 - Part 5.4: Support await expression in Reflect.parse.

https://reviewboard.mozilla.org/r/84436/#review87640

r=me
Comment 269 User image Till Schneidereit [till] 2016-10-26 06:45:15 PDT
Comment on attachment 8799161 [details]
Bug 1185106 - Part 5.5: Add parser test for async function declaration.

https://reviewboard.mozilla.org/r/84438/#review87642

Please remove the license headers from the new test files. Tests should be CC0 and are by default without a header. r=me with that.
Comment 270 User image Till Schneidereit [till] 2016-10-26 06:47:04 PDT
Comment on attachment 8799162 [details]
Bug 1185106 - Part 5.6: Add parser test for yield in async function declaration.

https://reviewboard.mozilla.org/r/84440/#review87646

r=me with nits addressed

::: js/src/tests/ecma_7/AsyncFunctions/yield.js:27
(Diff revision 1)
> +    Reflect.parse("function f() { async function yield() {} }");
> +    assertThrows(() => Reflect.parse("function* g() { async function yield() {} }"), SyntaxError);
> +    assertThrows(() => Reflect.parse("'use strict'; async function yield() {}"), SyntaxError);
> +
> +    // `yield` is treated as an identifier in an async function parameter
> +    // `yield` is not allowed as an identigier in strict code.

nit: s/identigier/identifier/

::: js/src/tests/ecma_7/AsyncFunctions/yield.js:35
(Diff revision 1)
> +    testPassArgsBody("(a = yield) {}");
> +    testErrorArgsBodyStrict("(yield 3) {}");
> +    testErrorArgsBodyStrict("(a = yield 3) {}");
> +
> +    // `yield` is treated as an identifier in an async function body
> +    // `yield` is not allowed as an identigier in strict code.

Same here.
Comment 271 User image Till Schneidereit [till] 2016-10-26 06:49:11 PDT
Comment on attachment 8799163 [details]
Bug 1185106 - Part 6.1: Support async function expression in Parser.

https://reviewboard.mozilla.org/r/84442/#review87648

r=me
Comment 272 User image Till Schneidereit [till] 2016-10-26 06:50:10 PDT
Comment on attachment 8799164 [details]
Bug 1185106 - Part 6.2: Add parser test for async function expression.

https://reviewboard.mozilla.org/r/84444/#review87652

r=me
Comment 273 User image Till Schneidereit [till] 2016-10-26 06:51:13 PDT
Comment on attachment 8799165 [details]
Bug 1185106 - Part 6.3: Add parser test for yield in async function expression.

https://reviewboard.mozilla.org/r/84446/#review87654

r=me with nit addressed.

::: js/src/tests/ecma_7/AsyncFunctions/yield.js:31
(Diff revision 1)
>      Reflect.parse("function f() { async function yield() {} }");
>      assertThrows(() => Reflect.parse("function* g() { async function yield() {} }"), SyntaxError);
>      assertThrows(() => Reflect.parse("'use strict'; async function yield() {}"), SyntaxError);
>  
> +    // `yield` is treated as an identifier in an async function expression name.
> +    // `yield` is not allowed as an identigier in strict code.

nit: s/identigier/identifier/ here and above.
Comment 274 User image Till Schneidereit [till] 2016-10-26 06:57:05 PDT
Comment on attachment 8799166 [details]
Bug 1185106 - Part 7.1: Support async method in Parser.

https://reviewboard.mozilla.org/r/84448/#review87662

r=me
Comment 275 User image Till Schneidereit [till] 2016-10-26 06:57:49 PDT
Comment on attachment 8799167 [details]
Bug 1185106 - Part 7.2: Add parser test for async method.

https://reviewboard.mozilla.org/r/84450/#review87664

r=me
Comment 276 User image Till Schneidereit [till] 2016-10-26 06:59:26 PDT
Comment on attachment 8799168 [details]
Bug 1185106 - Part 7.3: Add parser test for yield in async method.

https://reviewboard.mozilla.org/r/84452/#review87666

r=me
Comment 277 User image Till Schneidereit [till] 2016-10-26 07:00:12 PDT
Comment on attachment 8799169 [details]
Bug 1185106 - Part 8.1: Treat await as keyword in module.

https://reviewboard.mozilla.org/r/84454/#review87668

r=me
Comment 278 User image Till Schneidereit [till] 2016-10-26 07:00:56 PDT
Comment on attachment 8799170 [details]
Bug 1185106 - Part 8.2: Add parser test for await in module.

https://reviewboard.mozilla.org/r/84456/#review87670

r=me
Comment 279 User image Till Schneidereit [till] 2016-10-26 07:02:05 PDT
Comment on attachment 8799171 [details]
Bug 1185106 - Part 9.1: Support async function statement in export default in Parser.

https://reviewboard.mozilla.org/r/84458/#review87672

r=me
Comment 280 User image Till Schneidereit [till] 2016-10-26 07:02:48 PDT
Comment on attachment 8799172 [details]
Bug 1185106 - Part 9.2: Add parser test for async function statement in export default.

https://reviewboard.mozilla.org/r/84460/#review87674

r=me
Comment 281 User image Till Schneidereit [till] 2016-10-26 07:03:32 PDT
Comment on attachment 8799173 [details]
Bug 1185106 - Part 9.3: Add parser test for yield in async function statement in export default.

https://reviewboard.mozilla.org/r/84462/#review87676

r=me
Comment 282 User image Till Schneidereit [till] 2016-10-26 07:23:20 PDT
Comment on attachment 8799174 [details]
Bug 1185106 - Part 10.1: Support async arrow function in Parser.

https://reviewboard.mozilla.org/r/84464/#review87684

r=me with nit addressed.

::: js/src/frontend/Parser.cpp:7585
(Diff revision 1)
>  
> +        GeneratorKind generatorKind = NotGenerator;
> +        FunctionAsyncKind asyncKind = SyncFunction;
> +
> +#ifdef JS_HAS_ASYNC_FUNCS
> +        if (ignored == TOK_NAME) {

We're not really ignoring `ignored` anymore. Would be nice to have a better name for it.
Comment 283 User image Till Schneidereit [till] 2016-10-26 07:25:12 PDT
Comment on attachment 8799175 [details]
Bug 1185106 - Part 10.2: Support async arrow function in Reflect.parse.

https://reviewboard.mozilla.org/r/84466/#review87686

r=me
Comment 284 User image Till Schneidereit [till] 2016-10-26 07:26:42 PDT
Comment on attachment 8799176 [details]
Bug 1185106 - Part 10.3: Add parser test for async arrow function.

https://reviewboard.mozilla.org/r/84468/#review87688

r=me
Comment 285 User image Till Schneidereit [till] 2016-10-26 08:42:25 PDT
Comment on attachment 8799177 [details]
Bug 1185106 - Part 11.1: Implement async functions.

https://reviewboard.mozilla.org/r/84470/#review87696

This looks great!

I think you can ignore my comments about the self-hosted aspects for the most part. It'd be very nice to have a comment at the top of `AsyncFunctions.js` explaining how this all works and which code gets executed when.

r=me with feedback addressed.

::: js/src/builtin/AsyncFunctions.js:44
(Diff revision 1)
> +                        function(val) {
> +                            return AsyncFunction_resume(gen, val, gen.next);
> +                        }, function (err) {
> +                            return AsyncFunction_resume(gen, err, gen.throw);
> +                        });
> +}

I'm not convinced any of this code really should be JS. Are there any fundamental reasons why it's much simpler in JS? All functions here contain at least one C++ call, so we can't fully inline them anyway. And with the Promise code moving to C++, that's even more true.

It's fine to do this as a follow-up, but we should eventually do it because performance will be enough of a problem even without this overhead.

::: js/src/jsfun.cpp:1763
(Diff revision 1)
>       * and so would a call to f from another top-level's script or function.
>       */
>      RootedAtom anonymousAtom(cx, cx->names().anonymous);
>      RootedObject proto(cx);
>      if (isStarGenerator) {
> +        // isAsync should also use GeneratorFunction for unwrapped function.

I don't understand what this comment means. Can you expand it a bit?

::: js/src/jsfun.cpp:1888
(Diff revision 1)
>  {
> -    return FunctionConstructor(cx, argc, vp, StarGenerator);
> +    return FunctionConstructor(cx, argc, vp, StarGenerator, SyncFunction);
> +}
> +
> +bool
> +js::AsyncFunctionConstructor(JSContext* cx, unsigned argc, Value* vp)

It's pretty unfortunate that we call a JS function here that then calls another C++ function, so we're going through the JS/VM boundary twice. I wonder if it might not be possible to directly clone the inner function from `AsyncFunction_wrap` and synthesize and `environment` for it.

That'd be fine to do as a follow-up, though.

::: js/src/jsfun.cpp:1902
(Diff revision 1)
> +    if (!GlobalObject::getSelfHostedFunction(cx, cx->global(), shName, name, 1, &wrapFunc))
> +        return false;
> +
> +    RootedValue wrapped(cx);
> +    if (!Call(cx, wrapFunc, nullptr, args.rval(), &wrapped))
> +        return false;

All this could just be
```cpp
FixedInvokeArgs<1> args2(cx);
args2[0].set(args.rval());
return CallSelfHostedFunction(cx, cx->names().AsyncFunction_wrap, NullHandleValue, args2, args.rval()))
```
(Untested, so might contain slight errors, but you get the idea.)

::: js/src/vm/AsyncFunction.cpp:35
(Diff revision 1)
> +        return false;
> +    RootedObject proto(cx, &function.toObject());
> +    RootedAtom name(cx, cx->names().AsyncFunction);
> +    RootedObject asyncFunction(cx, NewFunctionWithProto(cx, AsyncFunctionConstructor, 1,
> +                                                        JSFunction::NATIVE_CTOR, nullptr, name,
> +                                                        proto));

This is needed because bug 1226261 isn't done, right? Assuming we keep all this as self-hosted code (see comment about that above), we really should land the patch for that bug and at least simplify and speed up this part of async functions.

::: js/src/vm/AsyncFunction.cpp:73
(Diff revision 1)
> +js::CreateAsyncFunction(JSContext* cx, HandleFunction wrapper, HandleFunction unwrapped,
> +                        MutableHandleFunction result)
> +{
> +    // Create a new function with AsyncFunctionPrototype, reusing the script
> +    // and the environment of `wrapper` function, and the name and the length
> +    // of `unwrapped` function.

Oh, I see. So the self-hosted code in `AsyncFunction_wrap` is only executed once. That makes it much more acceptable, and we can probably just leave it as-is.

::: js/src/vm/AsyncFunction.cpp:91
(Diff revision 1)
> +    // Link them each other to make GetWrappedAsyncFunction and
> +    // GetUnwrappedAsyncFunction work.
> +    unwrapped->setExtendedSlot(ASYNC_WRAPPED_SLOT, ObjectValue(*wrapped));
> +    wrapped->setExtendedSlot(ASYNC_UNWRAPPED_SLOT, ObjectValue(*unwrapped));
> +
> +    // The script od `wrapper` is self-hosted, so `wrapped` should also be

nit: s/od/of/
Comment 286 User image Till Schneidereit [till] 2016-10-26 08:47:22 PDT
Comment on attachment 8799178 [details]
Bug 1185106 - Part 11.2: Add helper functions for async/await test.

https://reviewboard.mozilla.org/r/84472/#review87714

r=me
Comment 287 User image Till Schneidereit [till] 2016-10-26 08:58:58 PDT
Comment on attachment 8799179 [details]
Bug 1185106 - Part 11.3: Add semantics test for async/await. .,till

https://reviewboard.mozilla.org/r/84474/#review87718

I didn't read these too closely, but they look ok at a glance. Would be nice to run the test262 tests for async/await, too.

My only request is to remove the license headers. r=me with that.
Comment 288 User image Till Schneidereit [till] 2016-10-26 08:59:17 PDT
Comment on attachment 8799180 [details]
Bug 1185106 - Part 11.4: Add function length test for async function.

https://reviewboard.mozilla.org/r/84476/#review87720

r=me
Comment 289 User image Till Schneidereit [till] 2016-10-26 09:00:20 PDT
Comment on attachment 8799181 [details]
Bug 1185106 - Part 11.5: Add async function constructor and prototype.

https://reviewboard.mozilla.org/r/84478/#review87722

r=me
Comment 290 User image Till Schneidereit [till] 2016-10-26 09:00:55 PDT
Comment on attachment 8799182 [details]
Bug 1185106 - Part 11.6: Add test for async function expression binding identity.

https://reviewboard.mozilla.org/r/84480/#review87724

r=me with license header removed.
Comment 291 User image Till Schneidereit [till] 2016-10-26 10:02:32 PDT
Comment on attachment 8799183 [details]
Bug 1185106 - Part 12: Return wrapped function for arguments.callee.

https://reviewboard.mozilla.org/r/84482/#review87738

r=me with feedback addressed and license header removed from test file.

::: js/src/js.msg:93
(Diff revision 1)
>  MSG_DEF(JSMSG_CANT_REDEFINE_PROP,      1, JSEXN_TYPEERR, "can't redefine non-configurable property {0}")
>  MSG_DEF(JSMSG_CANT_REDEFINE_ARRAY_LENGTH, 0, JSEXN_TYPEERR, "can't redefine array length")
>  MSG_DEF(JSMSG_CANT_DEFINE_PAST_ARRAY_LENGTH, 0, JSEXN_TYPEERR, "can't define array index property past the end of an array with non-writable length")
>  MSG_DEF(JSMSG_BAD_GET_SET_FIELD,       1, JSEXN_TYPEERR, "property descriptor's {0} field is neither undefined nor a function")
>  MSG_DEF(JSMSG_THROW_TYPE_ERROR,        0, JSEXN_TYPEERR, "'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them")
> +MSG_DEF(JSMSG_ASYNC_CALLEE,            0, JSEXN_TYPEERR, "'caller', 'callee', and 'arguments' properties may not be accessed on async function")

Nit: change "may not" to "can't"

But this isn't used, right? Is it a left-over from an earlier version of the spec draft that has since become redundant? If so, just remove it. If not, the tests below, and the behavior they test, are probably wrong.

(FWIW, I can't find any specific language about this in the spec, so our current behavior is probably correct.)
Comment 292 User image Till Schneidereit [till] 2016-10-26 10:03:46 PDT
Comment on attachment 8799184 [details]
Bug 1185106 - Part 13: Support async function in Function.prototype.toString.

https://reviewboard.mozilla.org/r/84484/#review87742

r=me
Comment 293 User image Till Schneidereit [till] 2016-10-26 10:32:57 PDT
Unsurprisingly, this is excellent work!

async/await just reached stage 4 at the last tc39 meeting, so we can land this and let it ride the trains without any feature flags or non-release-only restrictions.
Comment 294 User image Hannes Verschore [:h4writer] 2016-10-27 05:20:40 PDT
Comment on attachment 8799152 [details]
Bug 1185106 - Part 0.1: Refactor JSOP_DEFFUN.

https://reviewboard.mozilla.org/r/84420/#review88054

LGTM
Comment 295 User image Tooru Fujisawa [:arai] 2016-10-28 04:20:51 PDT
Comment on attachment 8799152 [details]
Bug 1185106 - Part 0.1: Refactor JSOP_DEFFUN.

https://reviewboard.mozilla.org/r/84420/#review87616

> Why isn't this needed anymore?

CloneFunctionObjectIfNotSingleton is already done in JSOP_LAMBDA, that is placed before JSOP_DEFFUN.
there is no need to do it once again.
Comment 296 User image Tooru Fujisawa [:arai] 2016-10-28 04:23:01 PDT
Comment on attachment 8799177 [details]
Bug 1185106 - Part 11.1: Implement async functions.

https://reviewboard.mozilla.org/r/84470/#review87696

> I'm not convinced any of this code really should be JS. Are there any fundamental reasons why it's much simpler in JS? All functions here contain at least one C++ call, so we can't fully inline them anyway. And with the Promise code moving to C++, that's even more true.
> 
> It's fine to do this as a follow-up, but we should eventually do it because performance will be enough of a problem even without this overhead.

Currently it's so because of following reasons, and I thought self-hosted JS would be simpler.
  * to check if it's wrapper, by checking if it's self-hosted function (js::IsWrappedAsyncFunction)
  * to close `unwrapped` variable in `wrapper` function
I guess, they could be simply solved in different way in native function tho...

will file another bug for it after landing this and promise in c++.

> I don't understand what this comment means. Can you expand it a bit?

I meant, unwrapped function should be a generator while wrapped function (actual async function) isn't generator.
fixed the comment to say:
        // Unwrapped function of async function should use GeneratorFunction,
        // while wrapped function isn't generator.

> It's pretty unfortunate that we call a JS function here that then calls another C++ function, so we're going through the JS/VM boundary twice. I wonder if it might not be possible to directly clone the inner function from `AsyncFunction_wrap` and synthesize and `environment` for it.
> 
> That'd be fine to do as a follow-up, though.

this will also be addressed in the new bug above.

> This is needed because bug 1226261 isn't done, right? Assuming we keep all this as self-hosted code (see comment about that above), we really should land the patch for that bug and at least simplify and speed up this part of async functions.

It's just doing the same thing as other Function/GeneratorFunction ctors,
I don't see any relation between bug 1226261.

> Oh, I see. So the self-hosted code in `AsyncFunction_wrap` is only executed once. That makes it much more acceptable, and we can probably just leave it as-is.

Yes, it's executed once per single async function instance.
Comment 297 User image Tooru Fujisawa [:arai] 2016-10-28 04:23:57 PDT
Comment on attachment 8799183 [details]
Bug 1185106 - Part 12: Return wrapped function for arguments.callee.

https://reviewboard.mozilla.org/r/84482/#review87738

> Nit: change "may not" to "can't"
> 
> But this isn't used, right? Is it a left-over from an earlier version of the spec draft that has since become redundant? If so, just remove it. If not, the tests below, and the behavior they test, are probably wrong.
> 
> (FWIW, I can't find any specific language about this in the spec, so our current behavior is probably correct.)

oops, it's from earlier version of the patch, that I was about to prohibit it (because `arguments.callee` is implementation defined thing, not specced).  but it turned out that it's easy to support, so I removed the code that throws, but forgot to remove the message :P
Comment 298 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 299 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 300 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 301 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 302 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 303 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 304 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 305 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 306 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 307 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 308 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 309 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 310 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 311 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 312 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 313 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 314 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 315 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 316 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 317 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 318 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 319 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 320 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 321 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 322 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 323 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 324 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 325 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 326 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 327 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 328 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 329 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 330 User image Tooru Fujisawa [:arai] 2016-10-28 04:35:45 PDT Comment hidden (mozreview-request)
Comment 331 User image Tooru Fujisawa [:arai] 2016-10-28 04:38:39 PDT
sorry, please ignore those additional requests :/
I was about to just update with correct reviewers list, but looks like mozreview asks extra review
Comment 332 User image Tooru Fujisawa [:arai] 2016-10-28 06:29:55 PDT
Created attachment 8805536 [details] [diff] [review]
Part 14: Add AsyncFunction.prototype[@@toStringTag].

for https://tc39.github.io/ecmascript-asyncawait/#async-function-prototype-properties-toStringTag
Comment 333 User image Till Schneidereit [till] 2016-10-28 06:31:53 PDT
Comment on attachment 8805536 [details] [diff] [review]
Part 14: Add AsyncFunction.prototype[@@toStringTag].

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

r=me
Comment 334 User image Tooru Fujisawa [:arai] 2016-10-28 11:40:39 PDT
Created attachment 8805628 [details] [diff] [review]
Part 7.4: Fix property name parsing with async name.

forgot to handle object shorthand and object destructuring, when parsing property name.
property name can end with "=" and "}".

added TOK_RC and TOK_ASSIGN to tokens to stop parsing async method.
and added tests for object and destructuring with "async" names.

also added method testcases for numeric property and computed property.
Comment 335 User image Till Schneidereit [till] 2016-10-28 12:51:27 PDT
Comment on attachment 8805628 [details] [diff] [review]
Part 7.4: Fix property name parsing with async name.

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

Great catch!
Comment 336 User image Tooru Fujisawa [:arai] 2016-10-30 13:38:03 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc85cad3e93b19ea9a09c49d93b6010711f06fff
Bug 1185106 - Part 0.1: Refactor JSOP_DEFFUN. r=efaust,jandem,till,h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a91fa1603c46e8ddeef15acc45887d74f39be21
Bug 1185106 - Part 1: Add AsyncFunction flag in FunctionBox, JSScript, and LazyScript. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/be97b8687b6a0faf23743f7d8ce44c8cd4c08050
Bug 1185106 - Part 2: Add FunctionAsyncKind parameter to Parser methods that receives GeneratorKind. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/86663fbc0899abb7782e39266df269df45eb0a21
Bug 1185106 - Part 3: Add await token. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/549055084ab012587dafa202e07888074071ce6b
Bug 1185106 - Part 4: Add AutoAwaitIsKeyword class to set TokenStream.awaitIsKeyword. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/1557532c15d1d8642115d3f1c2ac1c4685c3a48f
Bug 1185106 - Part 5.1: Support async function declaration in Parser. r=efaust,jwalden,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/0572beaf72429bbffaeede34b8ce40829e62715b
Bug 1185106 - Part 5.2: Support async function declaration in Reflect.parse. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/2465f743fb4f2b36e0a92a44a02f93472a1ad63e
Bug 1185106 - Part 5.3: Support await expression in Parser. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd9e2d62584ccfa708d322c55980015cea6df76
Bug 1185106 - Part 5.4: Support await expression in Reflect.parse. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/a467817a09def09b9b9ed5fd88f905122af0d2ff
Bug 1185106 - Part 5.5: Add parser test for async function declaration. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9efeed3b8718a6a48c0d9f94b591343d5179af
Bug 1185106 - Part 5.6: Add parser test for yield in async function declaration. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb5f24802ccbb0e44cf0448c9351755b93a15943
Bug 1185106 - Part 6.1: Support async function expression in Parser. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/d19d9f409b751a282f60b9c5d0a0a284004b6fb4
Bug 1185106 - Part 6.2: Add parser test for async function expression. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a245de062189dcefd7488ac740c66d30e5e2a65
Bug 1185106 - Part 6.3: Add parser test for yield in async function expression. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6af45aef3045102fc696e52e468da0e55e3928b
Bug 1185106 - Part 7.1: Support async method in Parser. r=efaust,jwalden,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c623f1b1404e2627fd054f648d567b1c0b02ec6
Bug 1185106 - Part 7.2: Add parser test for async method. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/f43c3c900fd72163fb8083601c2719a5c0246ed4
Bug 1185106 - Part 7.3: Add parser test for yield in async method. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/203903ea62ec88835f67ffaf938d39745461d486
Bug 1185106 - Part 7.4: Fix property name parsing with async name. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f59ff767344e7856642447f5e9a3679cc2fe4ab
Bug 1185106 - Part 8.1: Treat await as keyword in module. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/92f7082a90be77700618e8176c73ad7538ee75a3
Bug 1185106 - Part 8.2: Add parser test for await in module. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9b413f34392cd8e1bdda5cb230091a204fb5ef
Bug 1185106 - Part 9.1: Support async function statement in export default in Parser. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/e042b0dba45525afea60180c2851bee4785a1855
Bug 1185106 - Part 9.2: Add parser test for async function statement in export default. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe120f64b871c96744c63d6530f41f83022511c2
Bug 1185106 - Part 9.3: Add parser test for yield in async function statement in export default. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a03d945279843d6eaa80ff56af174e0b45abf7d
Bug 1185106 - Part 10.1: Support async arrow function in Parser. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7d781a99640e24397d87c6d31bc8de45f60a8e
Bug 1185106 - Part 10.2: Support async arrow function in Reflect.parse. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/b33af49c646c8709c004bd09191298f1153d593d
Bug 1185106 - Part 10.3: Add parser test for async arrow function. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab01476a1ccebc49962acd2737980fa6ec1f69f
Bug 1185106 - Part 11.1: Implement async functions. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/61f472bbdbc1d8a095d1375e59423027fd736e70
Bug 1185106 - Part 11.2: Add helper functions for async/await test. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bfb6261bdc98c47a4cc3b5d42ee89667e6016df
Bug 1185106 - Part 11.3: Add semantics test for async/await. r=efaust,till

https://hg.mozilla.org/integration/mozilla-inbound/rev/707c862f4d63f444a307aca6f7f5ecf1bd0cc070
Bug 1185106 - Part 11.4: Add function length test for async function. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/a141cb00c7deef08035c09de9ebd720fd74ff7a7
Bug 1185106 - Part 11.5: Add async function constructor and prototype. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dbc392a25d8a1978b5e9802561375b502d19c19
Bug 1185106 - Part 11.6: Add test for async function expression binding identity. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/0df8a2814d0c6a7cb905086ffef6c4236fa4e2cf
Bug 1185106 - Part 12: Return wrapped function for arguments.callee. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b10479b988db3c3d668611ed75791424503afd2
Bug 1185106 - Part 13: Support async function in Function.prototype.toString. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fae1fb3e02eef78e34aeafb662cbc54496521e1
Bug 1185106 - Part 14: Add AsyncFunction.prototype[@@toStringTag]. r=till
Comment 337 User image Carsten Book [:Tomcat] 2016-10-31 08:50:03 PDT
https://hg.mozilla.org/mozilla-central/rev/bc85cad3e93b
https://hg.mozilla.org/mozilla-central/rev/9a91fa1603c4
https://hg.mozilla.org/mozilla-central/rev/be97b8687b6a
https://hg.mozilla.org/mozilla-central/rev/86663fbc0899
https://hg.mozilla.org/mozilla-central/rev/549055084ab0
https://hg.mozilla.org/mozilla-central/rev/1557532c15d1
https://hg.mozilla.org/mozilla-central/rev/0572beaf7242
https://hg.mozilla.org/mozilla-central/rev/2465f743fb4f
https://hg.mozilla.org/mozilla-central/rev/7dd9e2d62584
https://hg.mozilla.org/mozilla-central/rev/a467817a09de
https://hg.mozilla.org/mozilla-central/rev/ce9efeed3b87
https://hg.mozilla.org/mozilla-central/rev/eb5f24802ccb
https://hg.mozilla.org/mozilla-central/rev/d19d9f409b75
https://hg.mozilla.org/mozilla-central/rev/1a245de06218
https://hg.mozilla.org/mozilla-central/rev/c6af45aef304
https://hg.mozilla.org/mozilla-central/rev/3c623f1b1404
https://hg.mozilla.org/mozilla-central/rev/f43c3c900fd7
https://hg.mozilla.org/mozilla-central/rev/203903ea62ec
https://hg.mozilla.org/mozilla-central/rev/0f59ff767344
https://hg.mozilla.org/mozilla-central/rev/92f7082a90be
https://hg.mozilla.org/mozilla-central/rev/0e9b413f3439
https://hg.mozilla.org/mozilla-central/rev/e042b0dba455
https://hg.mozilla.org/mozilla-central/rev/fe120f64b871
https://hg.mozilla.org/mozilla-central/rev/3a03d9452798
https://hg.mozilla.org/mozilla-central/rev/3e7d781a9964
https://hg.mozilla.org/mozilla-central/rev/b33af49c646c
https://hg.mozilla.org/mozilla-central/rev/5ab01476a1cc
https://hg.mozilla.org/mozilla-central/rev/61f472bbdbc1
https://hg.mozilla.org/mozilla-central/rev/4bfb6261bdc9
https://hg.mozilla.org/mozilla-central/rev/707c862f4d63
https://hg.mozilla.org/mozilla-central/rev/a141cb00c7de
https://hg.mozilla.org/mozilla-central/rev/4dbc392a25d8
https://hg.mozilla.org/mozilla-central/rev/0df8a2814d0c
https://hg.mozilla.org/mozilla-central/rev/6b10479b988d
https://hg.mozilla.org/mozilla-central/rev/8fae1fb3e02e
Comment 339 User image Tooru Fujisawa [:arai] 2016-11-05 02:24:10 PDT
Changing Expected Publication Year
https://github.com/tc39/proposals/blob/master/finished-proposals.md
Comment 341 User image Tooru Fujisawa [:arai] 2016-11-05 03:22:56 PDT
Also updated:
  https://developer.mozilla.org/en-US/Firefox/Releases/52
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_Next_support_in_Mozilla

I think references are ready.
would be nice to add guide documentation in bug 1305261.
Comment 343 User image Kohei Yoshino [:kohei] 2016-11-18 13:39:25 PST
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
Comment 344 User image Florian Scholz [:fscholz] (MDN) 2017-01-17 11:28:05 PST
Thanks a lot for writing the docs, arai! Let's finish them in bug 1305261.

Note You need to log in before you can comment on or make changes to this bug.