Change the calling conventions for handleUnrecoverableError.

Change the calling conventions for handleUnrecoverableError.

Instead of returning the token from which parsing should proceed, it
returns a token whose "next" pointer points to the token from which
parsing should proceed.

The rationale is that when error recovery is performed in the context
of the analyzer, the listener will need to insert synthetic tokens
into the token stream which will then be passed to listener methods;
the change in calling conventions allows the listener to return the
synthetic token to the parser.

The other listeners don't need to insert synthetic tokens into the
token stream; they just need to create a synthetic token whose "next"
pointer points to the token from which parsing should proceed.  The
convenience method "newSyntheticToken" is added for the benefit of
these listeners.

No code is affected outside of error handling, so the performance
impact when compiling correct code should be zero.

R=ahe@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/a6804167162f230bf8498289a61dc7efebf32b0c

[Paul Berry, 2017-03-08 19:08:16]

paulberry@google.com changed reviewers:
+ ahe@google.com, scheglov@google.com

[scheglov, 2017-03-08 19:12:07]

LGTM

[ahe, 2017-03-09 12:08:39]

LGTM! I really appreciate how you dig in and solve things that have been nagging
me for a long time.

Three action items:

* Would it be simpler to change skipNativeClause?

* Make sure that you have run tests/compiler/dart2js/message_kind_test.dart (in
checked mode). If this test fails, take a look at my suggestions for
element_listener.

* Consider if EOF_INFO is the right info-kind to use for newSyntheticToken, and
if you share my concern, feel free to delay to a follow-up CL.

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/element_listener.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:496: return
newSyntheticToken(next);
Perhaps this should be moved to handleError.

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:534: } else {
In this case, you could return `preceding`.

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:616: return next;
In this case, it should also be possible to return the token before `next`.

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2194: if (recover != null)
return newSyntheticToken(recover);
I think we should modify skipNativeClause instead.

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2202: return
newSyntheticToken(token);
FYI: Expect an easy conflict here. The return statement should be in the
then-block.

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/listener.dart:992: return new
SymbolToken(EOF_INFO, next.charOffset)..next = next;
Should we add a new kind? I'm worried that if we make a mistake an accidentally
parse this token, we'll report a bad error message (about unexpected end of
file).

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/diet_listener.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/diet_listener.dart:466: Token recover =
skipNativeClause(token);
Change skipNativeClause?

[Paul Berry, 2017-03-09 21:51:22]

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/element_listener.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:496: return
newSyntheticToken(next);
On 2017/03/09 12:08:39, ahe wrote:
> Perhaps this should be moved to handleError.

Done.

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:534: } else {
On 2017/03/09 12:08:39, ahe wrote:
> In this case, you could return `preceding`.

Done.

https://codereview.chromium.org/2738903002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/element_listener.dart:616: return next;
On 2017/03/09 12:08:39, ahe wrote:
> In this case, it should also be possible to return the token before `next`.

Done.

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2194: if (recover != null)
return newSyntheticToken(recover);
On 2017/03/09 12:08:39, ahe wrote:
> I think we should modify skipNativeClause instead.

Done.

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2202: return
newSyntheticToken(token);
On 2017/03/09 12:08:39, ahe wrote:
> FYI: Expect an easy conflict here. The return statement should be in the
> then-block.

Thanks for the heads up.

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/listener.dart:992: return new
SymbolToken(EOF_INFO, next.charOffset)..next = next;
On 2017/03/09 12:08:39, ahe wrote:
> Should we add a new kind? I'm worried that if we make a mistake an
accidentally
> parse this token, we'll report a bad error message (about unexpected end of
> file).

Yes, good point.  I'll take your advice and address it in a follow-up CL.

I've added a TODO comment for now.

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/diet_listener.dart (right):

https://codereview.chromium.org/2738903002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/diet_listener.dart:466: Token recover =
skipNativeClause(token);
On 2017/03/09 12:08:39, ahe wrote:
> Change skipNativeClause?

Done.

[Paul Berry, 2017-03-09 21:52:47]

Description was changed from

==========
Change the calling conventions for handleUnrecoverableError.

Instead of returning the token from which parsing should proceed, it
returns a token whose "next" pointer points to the token from which
parsing should proceed.

The rationale is that when error recovery is performed in the context
of the analyzer, the listener will need to insert synthetic tokens
into the token stream which will then be passed to listener methods;
the change in calling conventions allows the listener to return the
synthetic token to the parser.

The other listeners don't need to insert synthetic tokens into the
token stream; they just need to create a synthetic token whose "next"
pointer points to the token from which parsing should proceed.  The
convenience method "newSyntheticToken" is added for the benefit of
these listeners.

No code is affected outside of error handling, so the performance
impact when compiling correct code should be zero.
==========

to

==========
Change the calling conventions for handleUnrecoverableError.

Instead of returning the token from which parsing should proceed, it
returns a token whose "next" pointer points to the token from which
parsing should proceed.

The rationale is that when error recovery is performed in the context
of the analyzer, the listener will need to insert synthetic tokens
into the token stream which will then be passed to listener methods;
the change in calling conventions allows the listener to return the
synthetic token to the parser.

The other listeners don't need to insert synthetic tokens into the
token stream; they just need to create a synthetic token whose "next"
pointer points to the token from which parsing should proceed.  The
convenience method "newSyntheticToken" is added for the benefit of
these listeners.

No code is affected outside of error handling, so the performance
impact when compiling correct code should be zero.

R=ahe@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a6804167162f230bf8498289a61dc7efebf32b0c
==========

[Paul Berry, 2017-03-09 21:52:48]

Committed patchset #3 (id:40001) manually as
a6804167162f230bf8498289a61dc7efebf32b0c (presubmit successful).