[ESnext] Parse dynamic import expression

[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in
follow on patch.

Adds a new --harmony-dynamic-import flag.

Adds a ignore_error_msg parameter to the test runner to ignore the
discrepancy in the error messages while parsing import expression with
parser and pre parser. This discrepancy will actually never happen in
real code.

BUG=v8:5785
Review-Url: https://codereview.chromium.org/2661933003
Cr-Commit-Position: refs/heads/master@{#42820}
Committed: https://chromium.googlesource.com/v8/v8/+/e791ded4cdbe8c4d3f4e4a65e62f61e2b5a1c196

[gsathya, 2017-01-30 23:52:46]

Description was changed from

==========
[ESnext] Parse import call

BUG=
==========

to

==========
[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to 
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in 
follow on patch.

BUG=v8:5785
==========

[gsathya, 2017-01-30 23:56:33]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 23:56:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-30 23:59:12]

Description was changed from

==========
[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to 
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in 
follow on patch.

BUG=v8:5785
==========

to

==========
[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in
follow on patch.

Adds a new --harmony-dynamic-import flag.

Adds a ignore_error_msg parameter to the test runner to ignore the
discrepancy in the error messages while parsing import expression with
parser and pre parser. This discrepancy will actually never happen in
real code.

BUG=v8:5785
==========

[gsathya, 2017-01-31 00:00:11]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 00:00:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-31 00:01:49]

Description was changed from

==========
[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in
follow on patch.

Adds a new --harmony-dynamic-import flag.

Adds a ignore_error_msg parameter to the test runner to ignore the
discrepancy in the error messages while parsing import expression with
parser and pre parser. This discrepancy will actually never happen in
real code.

BUG=v8:5785
==========

to

==========
[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in
follow on patch.

Adds a new --harmony-dynamic-import flag.

Adds a ignore_error_msg parameter to the test runner to ignore the
discrepancy in the error messages while parsing import expression with
parser and pre parser. This discrepancy will actually never happen in
real code.

BUG=v8:5785
==========

[gsathya, 2017-01-31 00:01:50]

gsathya@chromium.org changed reviewers:
+ adamk@chromium.org, neis@chromium.org, vogelheim@chromium.org

[gsathya, 2017-01-31 00:14:47]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 00:14:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2017-01-31 00:29:33]

https://codereview.chromium.org/2661933003/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2661933003/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:3424: int pos = position();
I think you want peek_position() here (or alternatively grab the position after
consuming Token::IMPORT).

https://codereview.chromium.org/2661933003/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:3427: ExpressionT arg_ref =
ParseAssignmentExpression(true, CHECK_OK);
Naming nit: I think I'd like "arg" better, "_ref" doesn't seem to add much?

[gsathya, 2017-01-31 00:33:12]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 00:33:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-31 00:35:09]

https://codereview.chromium.org/2661933003/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2661933003/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:3424: int pos = position();
On 2017/01/31 00:29:33, adamk wrote:
> I think you want peek_position() here (or alternatively grab the position
after
> consuming Token::IMPORT).

Done.

https://codereview.chromium.org/2661933003/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:3427: ExpressionT arg_ref =
ParseAssignmentExpression(true, CHECK_OK);
On 2017/01/31 00:29:33, adamk wrote:
> Naming nit: I think I'd like "arg" better, "_ref" doesn't seem to add much?

I was using the same identifier as the spec. Changed.

[adamk, 2017-01-31 00:36:40]

lgtm

[commit-bot: I haz the power, 2017-01-31 01:19:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 01:20:00]

Dry run: This issue passed the CQ dry run.

[neis, 2017-01-31 10:47:57]

lgtm with minor comments

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:984: }
Nit: I find this version a little nicer as it doesn't duplicate the default
case:

if (next == Token::EXPORT) {
  return ParseExportDeclaration(ok);
}

// We must be careful not to parse a dynamic import expression as an import
declaration.
if (next == Token::IMPORT &&
    (!allow_harmony_dynamic_import() || PeekAhead() != Token::LPAREN) {
  ParseImportDeclaration(CHECK_OK);
  return factory()->NewEmptyStatement(kNoSourcePosition);
}

return ParseStatementListItem(ok);

https://codereview.chromium.org/2661933003/diff/100001/src/runtime/runtime-mo...
File src/runtime/runtime-module.cc (right):

https://codereview.chromium.org/2661933003/diff/100001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:15: return isolate->heap()->undefined_value();
Maybe add a TODO here :)

https://codereview.chromium.org/2661933003/diff/100001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2661933003/diff/100001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:4181: arraysize(flags), NULL, 0, true, true);
Please add a comment here on why we ignore the error message. I actually don't
understand the comment in the description. Are you saying the discrepancy is not
observable? Why?

[neis, 2017-01-31 11:20:09]

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser-bas...
src/parsing/parser-base.h:3357: 
Please update the comment.

I'm a bit confused, though, this doesn't seem to match the spec anymore.

[gsathya, 2017-01-31 18:24:38]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 18:24:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-31 18:26:08]

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser-bas...
src/parsing/parser-base.h:3357: 
On 2017/01/31 11:20:09, neis wrote:
> Please update the comment.
> 
> I'm a bit confused, though, this doesn't seem to match the spec anymore.

This hasn't matched the spec in a while. Fixed

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2661933003/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:984: }
On 2017/01/31 10:47:57, neis wrote:
> Nit: I find this version a little nicer as it doesn't duplicate the default
> case:
> 
> if (next == Token::EXPORT) {
>   return ParseExportDeclaration(ok);
> }
> 
> // We must be careful not to parse a dynamic import expression as an import
> declaration.
> if (next == Token::IMPORT &&
>     (!allow_harmony_dynamic_import() || PeekAhead() != Token::LPAREN) {
>   ParseImportDeclaration(CHECK_OK);
>   return factory()->NewEmptyStatement(kNoSourcePosition);
> }
> 
> return ParseStatementListItem(ok);

I did this initially and found the if conditional less readable. Changed anyway

https://codereview.chromium.org/2661933003/diff/100001/src/runtime/runtime-mo...
File src/runtime/runtime-module.cc (right):

https://codereview.chromium.org/2661933003/diff/100001/src/runtime/runtime-mo...
src/runtime/runtime-module.cc:15: return isolate->heap()->undefined_value();
On 2017/01/31 10:47:57, neis wrote:
> Maybe add a TODO here :)

Done.

https://codereview.chromium.org/2661933003/diff/100001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2661933003/diff/100001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:4181: arraysize(flags), NULL, 0, true, true);
On 2017/01/31 10:47:57, neis wrote:
> Please add a comment here on why we ignore the error message. I actually don't
> understand the comment in the description. Are you saying the discrepancy is
not
> observable? Why?

Done.

[neis, 2017-01-31 18:40:28]

lgtm

[commit-bot: I haz the power, 2017-01-31 18:56:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 18:56:37]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-31 18:57:14]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2017-01-31 18:57:14]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2661933003/#ps120001
(title: "fix")

[commit-bot: I haz the power, 2017-01-31 18:57:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 18:58:54]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485889034466670,
"parent_rev": "c41f5f2de68d4cc330ed9a0034f0f1573e4aab49", "commit_rev":
"e791ded4cdbe8c4d3f4e4a65e62f61e2b5a1c196"}

[commit-bot: I haz the power, 2017-01-31 18:59:00]

Description was changed from

==========
[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in
follow on patch.

Adds a new --harmony-dynamic-import flag.

Adds a ignore_error_msg parameter to the test runner to ignore the
discrepancy in the error messages while parsing import expression with
parser and pre parser. This discrepancy will actually never happen in
real code.

BUG=v8:5785
==========

to

==========
[ESnext] Parse dynamic import expression

Rewrites import expression into a runtime call. Uses peekahead to
determine if parsing an import declaration or import expression.

The runtime call doesn't actually do the import yet, will be added in
follow on patch.

Adds a new --harmony-dynamic-import flag.

Adds a ignore_error_msg parameter to the test runner to ignore the
discrepancy in the error messages while parsing import expression with
parser and pre parser. This discrepancy will actually never happen in
real code.

BUG=v8:5785

Review-Url: https://codereview.chromium.org/2661933003
Cr-Commit-Position: refs/heads/master@{#42820}
Committed:
https://chromium.googlesource.com/v8/v8/+/e791ded4cdbe8c4d3f4e4a65e62f61e2b5a...
==========

[commit-bot: I haz the power, 2017-01-31 18:59:01]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/e791ded4cdbe8c4d3f4e4a65e62f61e2b5a...