When translating analyzer/fasta token streams, match up begin/end tokens.

When translating analyzer/fasta token streams, match up begin/end tokens.

This required adding some scanner tests in order to validate the translation.

Note that the Fasta scanner considers "<", ">", and ">>" to be
grouping tokens (with special rules to avoid problems when "<" is used
to mean "less than"); analyzer does not.  The translation does not
handle this correctly yet.

R=ahe@google.com

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

[Paul Berry, 2017-02-13 22:15:36]

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

[Paul Berry, 2017-02-13 22:38:59]

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/test/scanner_...
File pkg/front_end/test/scanner_fasta_test.dart (right):

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/test/scanner_...
pkg/front_end/test/scanner_fasta_test.dart:96: // Figure out which recovery
technique we want the front end to use.
FWIW, my intuition is that Fasta's error recovery is better here, because when a
user is typing in a file, they are likely to type openers first and then close
them, so when there is a mismatch between an opener and a closer, it's probably
better to drop the opener.

[ahe, 2017-02-14 11:28:32]

lgtm

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/token_utils.dart (right):

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:83: token.endGroup != null
&& token.endGroup.charOffset != token.charOffset) {
Long line.

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:150: if (translatedToken
is BeginGroupToken && analyzerToken is analyzer.BeginToken &&
Long line.

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:438: return new
analyzer.BeginTokenWithComment(tokenType, token.charOffset, commentToken);
Long line.

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/test/scanner_...
File pkg/front_end/test/scanner_fasta_test.dart (right):

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/test/scanner_...
pkg/front_end/test/scanner_fasta_test.dart:96: // Figure out which recovery
technique we want the front end to use.
On 2017/02/13 22:38:59, Paul Berry wrote:
> FWIW, my intuition is that Fasta's error recovery is better here, because when
a
> user is typing in a file, they are likely to type openers first and then close
> them, so when there is a mismatch between an opener and a closer, it's
probably
> better to drop the opener.

That also has its downsides, but I have some ideas I want to try out in
pkg/front_end/lib/src/fasta/scanner/recover.dart in the future. I don't know
exactly when I'll find time to experiment with this, but feel free to ask me
more about it at any time.

[Paul Berry, 2017-02-14 12:40:40]

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/token_utils.dart (right):

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:83: token.endGroup != null
&& token.endGroup.charOffset != token.charOffset) {
On 2017/02/14 11:28:32, ahe wrote:
> Long line.

Sorry.  My usual workflow is hampered by not being able to run dartfmt on this
file (usually I edit files in an editor with wide margins, without regard to
line length, and then run dartfmt in bulk prior to sending out for review). 
Hopefully we will run dartfmt on the fasta codebase soon and then that won't be
an issue anymore.  Fixed.

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:150: if (translatedToken
is BeginGroupToken && analyzerToken is analyzer.BeginToken &&
On 2017/02/14 11:28:32, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:438: return new
analyzer.BeginTokenWithComment(tokenType, token.charOffset, commentToken);
On 2017/02/14 11:28:32, ahe wrote:
> Long line.

Done.

[Paul Berry, 2017-02-14 12:46:41]

Description was changed from

==========
When translating analyzer/fasta token streams, match up begin/end tokens.

This required adding some scanner tests in order to validate the translation.

Note that the Fasta scanner considers "<", ">", and ">>" to be
grouping tokens (with special rules to avoid problems when "<" is used
to mean "less than"); analyzer does not.  The translation does not
handle this correctly yet.
==========

to

==========
When translating analyzer/fasta token streams, match up begin/end tokens.

This required adding some scanner tests in order to validate the translation.

Note that the Fasta scanner considers "<", ">", and ">>" to be
grouping tokens (with special rules to avoid problems when "<" is used
to mean "less than"); analyzer does not.  The translation does not
handle this correctly yet.

R=ahe@google.com

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

[Paul Berry, 2017-02-14 12:46:42]

Committed patchset #1 (id:1) manually as
cf6a01be1976a4707e493df21923ee053b0d1881 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-02-15 00:23:10]

lgtm

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/token_utils.dart (right):

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:72: // If this token
closes a group, set the corresponding opener token
super optional nit - (IMO not worth making a change for this).

I'd consider moving some of these algorithm comments out above beginTokenStack
and turn them into some general technique documentation as a whole. In other
words, right now I read this as a description of the implementation, but it's
not as obvious what's the underlying goal.

On a related note, we could also introduce some helper variables which may make
the code more self-explanatory further below. For example:

// Both fasta and analyzer have links from a `BeginToken` to 
// it's matching `EndToken` in a group (like parenthesis and braces).
// However, fasta may contain synthetic tokens from error-recovery 
// that are not mapped to the analyzer token stream. We use these
// stacks to create the appropriate links for non-synthetic tokens in
// the way analyzer expects them to be.

// Note: null is a sentinel value to avoid empty checks below.
var beginTokenStack = <analyzer.BeginToken>[null];
var endTokenStack = <Token>[null];

void matchGroups(Token token, analyzer.Token translatedToken) {
  if (identical(endTokenStack.last, token)) {
    beginTokenStack.last.endToken = translatedToken;
    beginTokenStack.removeLast();
    endTokenStack.removeLast();
  }

  if (translatedToken is analyzer.BeginToken && token is BeginGroupToken) {
    // Synthetic end-tokens use the same offset as the begin token.
    var isSynthetic = token.endGroup?.charOffset = token.charOffset;
    if (!isSynthetic) {
      beginStack.add(translatedToken);
      endStack.add(token.endGroup);
    }
  }

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:79: // If this token opens
a group, and there is a matching closer that's not
minor terminology nit here: consider using closing-token, end-token,. or even
closer-token, instead of plain `closer`. For a flip-second I started thinking of
closer in the sense of distance (as opposed to farther), which was a possible
interpretation given that we are also looking into charOffsets

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/test/scanner_...
File pkg/front_end/test/scanner_test.dart (right):

https://codereview.chromium.org/2690073003/diff/1/pkg/front_end/test/scanner_...
pkg/front_end/test/scanner_test.dart:786: // always matche the preceding
unmatched `{`, even if there are intervening
matche => match