Connect fasta's scanner to the analyzer-derived scanner tests in front_end.

pkg/front_end/test/scanner_test.dart

Index: pkg/front_end/test/scanner_test.dart
diff --git a/pkg/front_end/test/scanner_test.dart b/pkg/front_end/test/scanner_test.dart
index 69e802d3008e3b898096106cdaafabf806b56901..10c2ad11350fe9350ef35bce7663c4bbfecfb45e 100644
--- a/pkg/front_end/test/scanner_test.dart
+++ b/pkg/front_end/test/scanner_test.dart
@@ -4,6 +4,11 @@
 
 import 'package:front_end/src/base/errors.dart';
 import 'package:front_end/src/base/jenkins_smi_hash.dart';
+import 'package:front_end/src/fasta/scanner/error_token.dart' as fasta;
+import 'package:front_end/src/fasta/scanner/keyword.dart' as fasta;
+import 'package:front_end/src/fasta/scanner/string_scanner.dart' as fasta;
+import 'package:front_end/src/fasta/scanner/token.dart' as fasta;
+import 'package:front_end/src/fasta/scanner/token_constants.dart' as fasta;
 import 'package:front_end/src/scanner/errors.dart';
 import 'package:front_end/src/scanner/reader.dart';
 import 'package:front_end/src/scanner/scanner.dart';
@@ -16,6 +21,7 @@ main() {
     defineReflectiveTests(CharSequenceReaderTest);
     defineReflectiveTests(KeywordStateTest);
     defineReflectiveTests(ScannerTest);

[Paul Berry, 2017/02/03 23:10:36]

Note that this test file now tests both the analyzer and fasta implementations
of the scanner.

If you want to exercise a single test in the debugger, an easy way to do so is
to temporarily change the name of that test from "test_foo" to "solo_test_foo"
and temporarily comment out this line, so that only the fasta version is tested.

+    defineReflectiveTests(ScannerTest_Fasta);
     defineReflectiveTests(TokenTypeTest);
   });
 }
@@ -113,7 +119,556 @@ class KeywordStateTest {
 }
 
 @reflectiveTest
-class ScannerTest {
+class ScannerTest extends ScannerTestBase {
+  @override
+  Token _scanWithListener(String source, _ErrorListener listener,
+      {bool genericMethodComments: false,
+      bool lazyAssignmentOperators: false}) {
+    Scanner scanner =
+        new _TestScanner(new CharSequenceReader(source), listener);
+    scanner.scanGenericMethodComments = genericMethodComments;
+    scanner.scanLazyAssignmentOperators = lazyAssignmentOperators;
+    return scanner.tokenize();
+  }
+}
+
+@reflectiveTest
+class ScannerTest_Fasta extends ScannerTestBase {
+  final _keywordMap = {
+    "assert": Keyword.ASSERT,
+    "break": Keyword.BREAK,
+    "case": Keyword.CASE,
+    "catch": Keyword.CATCH,
+    "class": Keyword.CLASS,
+    "const": Keyword.CONST,
+    "continue": Keyword.CONTINUE,
+    "default": Keyword.DEFAULT,
+    "do": Keyword.DO,
+    "else": Keyword.ELSE,
+    "enum": Keyword.ENUM,
+    "extends": Keyword.EXTENDS,
+    "false": Keyword.FALSE,
+    "final": Keyword.FINAL,
+    "finally": Keyword.FINALLY,
+    "for": Keyword.FOR,
+    "if": Keyword.IF,
+    "in": Keyword.IN,
+    "new": Keyword.NEW,
+    "null": Keyword.NULL,
+    "rethrow": Keyword.RETHROW,
+    "return": Keyword.RETURN,
+    "super": Keyword.SUPER,
+    "switch": Keyword.SWITCH,
+    "this": Keyword.THIS,
+    "throw": Keyword.THROW,
+    "true": Keyword.TRUE,
+    "try": Keyword.TRY,
+    "var": Keyword.VAR,
+    "void": Keyword.VOID,
+    "while": Keyword.WHILE,
+    "with": Keyword.WITH,
+    "is": Keyword.IS,
+    "abstract": Keyword.ABSTRACT,
+    "as": Keyword.AS,
+    "covariant": Keyword.COVARIANT,
+    "dynamic": Keyword.DYNAMIC,
+    "export": Keyword.EXPORT,
+    "external": Keyword.EXTERNAL,
+    "factory": Keyword.FACTORY,
+    "get": Keyword.GET,
+    "implements": Keyword.IMPLEMENTS,
+    "import": Keyword.IMPORT,
+    "library": Keyword.LIBRARY,
+    "operator": Keyword.OPERATOR,
+    "part": Keyword.PART,
+    "set": Keyword.SET,
+    "static": Keyword.STATIC,
+    "typedef": Keyword.TYPEDEF,
+    "deferred": Keyword.DEFERRED,
+  };
+
+  @override
+  @failingTest

[Paul Berry, 2017/02/03 23:10:36]

Note: these overrides indicate to the test_reflective_loader infrastructure that
the test is expected to fail.  Once the failure is fixed, the overrides can be
removed.

+  void test_ampersand_ampersand_eq() {
+    // TODO(paulberry,ahe): Fasta doesn't support `&&=` yet
+    super.test_ampersand_ampersand_eq();
+  }
+
+  @override
+  @failingTest
+  void test_bar_bar_eq() {
+    // TODO(paulberry,ahe): Fasta doesn't support `||=` yet
+    super.test_bar_bar_eq();
+  }
+
+  @override
+  @failingTest
+  void test_comment_generic_method_type_assign() {
+    // TODO(paulberry,ahe): Fasta doesn't support generic method comment syntax.

[ahe, 2017/02/06 11:01:20]

Do you think that would be necessary to support in Fasta?

[Paul Berry, 2017/02/06 13:46:54]

It's not clear at this point.  I asked Dan Grove at Friday's analyzer meeting,
and he said that there is likely to be a long tail of internal code within
Google that will continue using generic comment syntax until Dart 2.0.  So if we
wanted to cut over to Fasta prior to the Dart 2.0 fork, we would need generic
comment support.  I don't know when we are going to want to cut over.

+    super.test_comment_generic_method_type_assign();
+  }
+
+  @override
+  @failingTest
+  void test_comment_generic_method_type_list() {
+    // TODO(paulberry,ahe): Fasta doesn't support generic method comment syntax.
+    super.test_comment_generic_method_type_list();
+  }
+
+  @override
+  @failingTest
+  void test_comment_multi_unterminated() {
+    // TODO(paulberry,ahe): see UnimplementedError("distinguish unterminated errors")

[ahe, 2017/02/06 11:01:20]

Long line here and a few below.

[Paul Berry, 2017/02/06 13:46:54]

Done.

+    super.test_comment_multi_unterminated();
+  }
+
+  @override
+  @failingTest
+  void test_comment_single() {
+    // TODO(paulberry,ahe): See TODO comment below in _translateTokenInfoKind().
+    super.test_comment_single();
+  }
+
+  @override
+  @failingTest
+  void test_double_missingDigitInExponent() {
+    // TODO(paulberry,ahe): see UnimplementedError("distinguish unterminated errors")
+    super.test_double_missingDigitInExponent();
+  }
+
+  @override
+  @failingTest
+  void test_hexidecimal_missingDigit() {
+    // TODO(paulberry,ahe): see UnimplementedError("distinguish unterminated errors")
+    super.test_hexidecimal_missingDigit();
+  }
+
+  @override
+  @failingTest
+  void test_index() {
+    // TODO(paulberry,ahe): "[]" should be parsed as a single token.

[ahe, 2017/02/04 09:48:19]

Note: [] and []= are tokenized as a single token only after the built-in
identifier "operator".

See:

https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan...

[Paul Berry, 2017/02/04 14:57:55]

Interesting.  The behavior of the analyzer scanner is to lex "[]" and "[]=" as
single tokens wherever they appear.  On the downside, this complicates the
analyzer parser slightly, since it means that it must recognize both `[]` and
`[` `]` as empty list expressions.  On the upside, it makes it easier for the
analyzer parser to recover from errors like this: 

class Foo {
  int [](int index) => 0;
}

(Analyzer reports the error "Operator declarations must be preceeded by the
keyword 'operator'.").  If we wanted to get this error recovery behavior with
fasta's lexer, there would be more parser work.  On the other hand, maybe that's
ok, because the extra parser work would be all along an error handling code path
anyway.  And admittedly, this particular error recovery corner case is probably
extremely rare in practice.

In my mind this issue is simple enough that it's probably not worth discussing,
but since it's the first one that came up, let's use it as a practice run and
see how the discussion goes.  Once we get into a groove we shouldn't need to
talk through every single detail.

I can see a few possible ways to resolve this:

(1) Change analyzer's parser and scanner to be like Fasta's, and implement extra
logic in analyzer's parser to keep the error recovery behavior.  This would
carry little benefit (it would make the unit test behavior line up, but not
change the user experience other than a possible tiny performance improvement). 
And it would all be wasted effort assuming we switch to Fasta in the long term.

(2) Change analyzer's parser and scanner to be like Fasta's, and don't worry
about the error recovery behavior.  This is also wasted effort, though probably
less effort than (1).

(3) Change fasta's parser and scanner to be like analyzer's.  This gets us the
nice error recovery behavior, at the expense of some coding effort and a
possible performance drop (though my intuition is that it would unmeasurably
small).  This is probably not worth it either, given how rare the error recovery
corner case is.

(4) Keep fasta's scanner as is, and implement logic in its parser to handle the
error recovery corner case.  Again, probably not worth it, given how rare the
error recovery corner case.

(5) Change nothing.  Fasta code stays simple and efficent, and the only
user-visible effect is that this particular error recovery case stops being
addressed.

I personally lean toward (5).  In which case I think the right thing to do is
(a) keep the "@failingTest" override, but change the comment to explain in a
sentence or two why we're ok with Fasta failing this test.  (b) add a test (or
tests) validating Fasta's behavior, along with a "@failingTest" override for
analyzer's scanner.  (c) after we've transitioned to Fasta and removed
analyzer's scanner, we can remove the failing test if we choose to.

I don't care which one of us does (a) and (b)--it should only take a few
minutes.

As a point of process: since it's probably going to take some time to resolve
all the TODOs in this CL, I'd like to land it as is first (assuming that you
don't come up with blocking issues in your review) and then address the TODOs
with follow-up CLs.  So I'll postpone doing (a) and (b) until after this CL
lands.

[ahe, 2017/02/06 11:01:20]

I agree. A corner case with the dart2js/fasta approach is that if you write
something like this:

main() {
  var operator = ["Hello, World!"];
  print(operator[]); // Forgot 0 here.
}

The fasta/dart2js parser won't recover well.

My gut feeling is that the analyzer's approach is superior here.
I agree, I don't even think the performance of fasta is necessarily better in
this case.
I think I see a slightly different option: create a subclass of
`fasta.StringScanner` that overrides tokenizeOpenSquareBracket to behave as the
analyzer expects. This should have no impact on fasta at all, so we can just do
this in general, and later look at deciding which approach is the best long term
(in this case, I think it's the dartanalyzer's).
I like this approach, and is what I'm used to. Normally, my comments are
comments, and if I feel something is important, I'll often say "l*tm, provided
you address so and so." Sometimes addressing my comments is explaining to me
that I misunderstood something :-)

[Paul Berry, 2017/02/06 13:46:54]

SGTM.

[Siggi Cherem (dart-lang), 2017/02/06 23:51:29]

A small question: could we annotate somehow these tests that highlights a
difference we expect to be visible in error recovery?

Once we have a way of running error recovery tests, I'd suggest just adding a
test there, but for now, maybe a small comment with a grep-able text? or filing
a short bug with a new label (e.g. fasta-recovery)?

[Paul Berry, 2017/02/07 13:56:11]

I've filed an issue: https://github.com/dart-lang/sdk/issues/28665
I'll follow up with a CL that links to the issue from this test.

+    super.test_index();
+  }
+
+  @override
+  @failingTest
+  void test_index_eq() {
+    // TODO(paulberry,ahe): "[]=" should be parsed as a single token.
+    super.test_index_eq();
+  }
+
+  @override
+  @failingTest
+  void test_scriptTag_withArgs() {
+    // TODO(paulberry,ahe): script tags are needed by analyzer.
+    super.test_scriptTag_withArgs();
+  }
+
+  @override
+  @failingTest
+  void test_scriptTag_withoutSpace() {
+    // TODO(paulberry,ahe): script tags are needed by analyzer.
+    super.test_scriptTag_withoutSpace();
+  }
+
+  @override
+  @failingTest
+  void test_scriptTag_withSpace() {
+    // TODO(paulberry,ahe): script tags are needed by analyzer.
+    super.test_scriptTag_withSpace();
+  }
+
+  @override
+  @failingTest
+  void test_string_multi_unterminated() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_multi_unterminated();
+  }
+
+  @override
+  @failingTest
+  void test_string_multi_unterminated_interpolation_block() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_multi_unterminated_interpolation_block();
+  }
+
+  @override
+  @failingTest
+  void test_string_multi_unterminated_interpolation_identifier() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_multi_unterminated_interpolation_identifier();
+  }
+
+  @override
+  @failingTest
+  void test_string_raw_multi_unterminated() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_raw_multi_unterminated();
+  }
+
+  @override
+  @failingTest
+  void test_string_raw_simple_unterminated_eof() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_raw_simple_unterminated_eof();
+  }
+
+  @override
+  @failingTest
+  void test_string_raw_simple_unterminated_eol() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_raw_simple_unterminated_eol();
+  }
+
+  @override
+  @failingTest
+  void test_string_simple_interpolation_missingIdentifier() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_simple_interpolation_missingIdentifier();
+  }
+
+  @override
+  @failingTest
+  void test_string_simple_interpolation_nonIdentifier() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_simple_interpolation_nonIdentifier();
+  }
+
+  @override
+  @failingTest
+  void test_string_simple_unterminated_eof() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_simple_unterminated_eof();
+  }
+
+  @override
+  @failingTest
+  void test_string_simple_unterminated_eol() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_simple_unterminated_eol();
+  }
+
+  @override
+  @failingTest
+  void test_string_simple_unterminated_interpolation_block() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_simple_unterminated_interpolation_block();
+  }
+
+  @override
+  @failingTest
+  void test_string_simple_unterminated_interpolation_identifier() {
+    // TODO(paulberry,ahe): bad error recovery.
+    super.test_string_simple_unterminated_interpolation_identifier();
+  }
+
+  @override
+  Token _scanWithListener(String source, _ErrorListener listener,
+      {bool genericMethodComments: false,
+      bool lazyAssignmentOperators: false}) {
+    if (genericMethodComments) {
+      // Fasta doesn't support generic method comments.
+      // TODO(paulberry): once the analyzer toolchain no longer needs generic
+      // method comments, remove tests that exercise them.
+      fail('No generic method comment support in Fasta');
+    }
+    // Note: Fasta always supports lazy assignment operators (`&&=` and `||=`),
+    // so we can ignore the `lazyAssignmentOperators` flag.
+    // TODO(paulberry): once lazyAssignmentOperators are fully supported by
+    // Dart, remove this flag.
+    var scanner = new fasta.StringScanner(source, includeComments: true);
+    var token = scanner.tokenize();
+    var analyzerTokenHead = new Token(null, 0);
+    analyzerTokenHead.previous = analyzerTokenHead;
+    var analyzerTokenTail = analyzerTokenHead;
+    // TODO(paulberry,ahe): Fasta includes comments directly in the token
+    // stream, rather than pointing to them via a "precedingComment" pointer, as
+    // analyzer does.  This seems like it will complicate parsing and other
+    // operations.
+    CommentToken currentCommentHead;
+    CommentToken currentCommentTail;
+    while (true) {
+      if (token is fasta.ErrorToken) {
+        var error = _translateErrorToken(token, source.length);
+        if (error != null) {
+          listener.errors.add(error);
+        }
+      } else if (token is fasta.StringToken &&
+          token.info.kind == fasta.COMMENT_TOKEN) {
+        var translatedToken = _translateToken(token, null) as CommentToken;
+        if (currentCommentHead == null) {
+          currentCommentHead = currentCommentTail = translatedToken;
+        } else {
+          currentCommentTail.setNext(translatedToken);
+          currentCommentTail = translatedToken;
+        }
+      } else {
+        var translatedToken = _translateToken(token, currentCommentHead);
+        translatedToken.setNext(translatedToken);
+        currentCommentHead = currentCommentTail = null;
+        analyzerTokenTail.setNext(translatedToken);
+        translatedToken.previous = analyzerTokenTail;
+        analyzerTokenTail = translatedToken;
+      }
+      if (token.isEof) {
+        return analyzerTokenHead.next;
+      }
+      token = token.next;
+    }
+  }
+
+  _TestError _translateErrorToken(fasta.ErrorToken token, int inputLength) {
+    int charOffset = token.charOffset;
+    // TODO(paulberry,ahe): why is endOffset sometimes null?
+    int endOffset = token.endOffset ?? charOffset;
+    _TestError _makeError(ScannerErrorCode errorCode, List<Object> arguments) {
+      int errorLength = endOffset - charOffset;
+      if (charOffset == inputLength) {
+        // Analyzer never generates an error message past the end of the input,
+        // since such an error would not be visible in an editor.
+        // TODO(paulberry,ahe): would it make sense to replicate this behavior
+        // in fasta, or move it elsewhere in analyzer?
+        charOffset--;
+      }
+      if (errorLength == 0) {
+        // Analyzer never generates an error message of length zero,
+        // since such an error would not be visible in an editor.
+        // TODO(paulberry,ahe): would it make sense to replicate this behavior
+        // in fasta, or move it elsewhere in analyzer?
+        errorLength = 1;
+      }
+      return new _TestError(charOffset, errorLength, errorCode, arguments);
+    }
+
+    if (token is fasta.UnterminatedToken) {
+      // TODO(paulberry,ahe): How to tell what kind of error to
+      // report?  It could be ScannerErrorCode.UNTERMINATED_STRING_LITERAL,
+      // ScannerErrorCode.MISSING_HEX_DIGIT, or
+      // ScannerErrorCode.UNTERMINATED_MULTI_LINE_COMMENT

[ahe, 2017/02/04 09:48:19]

Absolutely. Just this week, I was thinking about cleaning this up.

It's already a little bit better as I added ErrorToken.errorCode, and the API
should probably be good enough for testing, but I do want to simplify
UnterminatedToken.errorCode.

See:
https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan...

Another thought: perhaps I should move errorCode to Token. This way we don't
have to rely on type tests of tokens.

[Paul Berry, 2017/02/04 14:57:55]

Aha, thank you.  I failed to notice the presence of ErrorToken.errorCode.  I'll
rework this code to use it.
Interesting.  We would still have to do a single megamorphic dispatch for
token.info, but thereafter we would avoid "is" checks and implicit downcasts,
and we could probably make everything else monomorphic.

I'm a little wary of making too many changes to Fasta right now, since it's not
hooked up to golem or to analyzer tests yet, but I suppose moving errorCode to
Token is pretty safe as changes go.

[ahe, 2017/02/06 11:01:20]

We should be careful about polymorphism, but in this case, errorCode is only
used *after* we know that the tokens contains an error. This is because I set a
bit on the scanner object whenever I create an ErrorToken.

Another point is that one of the experiments I've considered for a long time is
to have only one token class. This would be the ultimate way to reduce
polymorphism of tokens. I think could potentially make the parser a little
faster, but we'll see. Right now, I feel it's more important to get the compiler
good enough wrt feature completeness to replace dartk.
I think it should be safe. As for benchmarking, the only thing I've been
tracking is compile-time of dart2js. How to compile dart2js is described in
pkg/front_end/lib/src/fasta/README.md, so if you run it a few times with and
without your changes, you should be able to convince yourself that there's no
impact.

[Paul Berry, 2017/02/06 13:46:54]

Aha, nice.  When I do my rework of this code (which I think I'll save for a
follow-up CL) I'll make use of that bit.
Yeah, I had a similar thought.
Agreed :)

+      return _makeError(
+          throw new UnimplementedError("distinguish unterminated errors"),
+          null);
+    } else if (token is fasta.UnmatchedToken) {
+      return null;
+    } else if (token is fasta.NonAsciiIdentifierToken) {
+      return _makeError(ScannerErrorCode.ILLEGAL_CHARACTER, [token.character]);
+    } else if (token is fasta.NonAsciiWhitespaceToken) {
+      return _makeError(ScannerErrorCode.ILLEGAL_CHARACTER, [token.character]);
+    }
+    throw new UnimplementedError('${token.runtimeType}');
+  }
+
+  Keyword _translateKeyword(String syntax) =>
+      _keywordMap[syntax] ?? (throw new UnimplementedError('$syntax'));
+
+  Token _translateToken(fasta.Token token, CommentToken comment) {
+    var type = _translateTokenInfoKind(token.info.kind);
+    int offset = token.charOffset;
+    Token makeStringToken(String value) {
+      if (comment == null) {
+        return new StringToken(type, value, offset);
+      } else {
+        return new StringTokenWithComment(type, value, offset, comment);
+      }
+    }
+
+    Token makeKeywordToken(Keyword keyword) {
+      if (comment == null) {
+        return new KeywordToken(keyword, offset);
+      } else {
+        return new KeywordTokenWithComment(keyword, offset, comment);
+      }
+    }
+
+    Token makeBeginToken() {
+      if (comment == null) {
+        return new BeginToken(type, offset);
+      } else {
+        return new BeginTokenWithComment(type, offset, comment);
+      }
+    }
+
+    Token makeCommentToken() {
+      return new CommentToken(type, token.value, offset);
+    }
+
+    if (token is fasta.StringToken) {
+      if (token.info.kind == fasta.COMMENT_TOKEN) {
+        return makeCommentToken();
+      } else {
+        return makeStringToken(token.value);
+      }
+    } else if (token is fasta.KeywordToken) {
+      return makeKeywordToken(_translateKeyword(token.keyword.syntax));
+    } else if (token is fasta.SymbolToken) {
+      if (token is fasta.BeginGroupToken) {
+        if (type == TokenType.LT) {
+          return makeStringToken(token.value);
+        } else {
+          return makeBeginToken();
+        }
+      } else {
+        return makeStringToken(token.value);
+      }
+    }
+    throw new UnimplementedError('${token.runtimeType}');
+  }
+
+  TokenType _translateTokenInfoKind(int kind) {
+    switch (kind) {
+      case fasta.EOF_TOKEN:
+        return TokenType.EOF;
+      case fasta.KEYWORD_TOKEN:
+        return TokenType.KEYWORD;
+      case fasta.IDENTIFIER_TOKEN:
+        return TokenType.IDENTIFIER;
+      case fasta.BAD_INPUT_TOKEN:
+        return TokenType.STRING;
+      case fasta.DOUBLE_TOKEN:
+        return TokenType.DOUBLE;
+      case fasta.INT_TOKEN:
+        return TokenType.INT;
+      case fasta.HEXADECIMAL_TOKEN:
+        return TokenType.HEXADECIMAL;
+      case fasta.STRING_TOKEN:
+        return TokenType.STRING;
+      case fasta.AMPERSAND_TOKEN:
+        return TokenType.AMPERSAND;
+      case fasta.BACKPING_TOKEN:
+        return TokenType.BACKPING;
+      case fasta.BACKSLASH_TOKEN:
+        return TokenType.BACKSLASH;
+      case fasta.BANG_TOKEN:
+        return TokenType.BANG;
+      case fasta.BAR_TOKEN:
+        return TokenType.BAR;
+      case fasta.COLON_TOKEN:
+        return TokenType.COLON;
+      case fasta.COMMA_TOKEN:
+        return TokenType.COMMA;
+      case fasta.EQ_TOKEN:
+        return TokenType.EQ;
+      case fasta.GT_TOKEN:
+        return TokenType.GT;
+      case fasta.HASH_TOKEN:
+        return TokenType.HASH;
+      case fasta.OPEN_CURLY_BRACKET_TOKEN:
+        return TokenType.OPEN_CURLY_BRACKET;
+      case fasta.OPEN_SQUARE_BRACKET_TOKEN:
+        return TokenType.OPEN_SQUARE_BRACKET;
+      case fasta.OPEN_PAREN_TOKEN:
+        return TokenType.OPEN_PAREN;
+      case fasta.LT_TOKEN:
+        return TokenType.LT;
+      case fasta.MINUS_TOKEN:
+        return TokenType.MINUS;
+      case fasta.PERIOD_TOKEN:
+        return TokenType.PERIOD;
+      case fasta.PLUS_TOKEN:
+        return TokenType.PLUS;
+      case fasta.QUESTION_TOKEN:
+        return TokenType.QUESTION;
+      case fasta.AT_TOKEN:
+        return TokenType.AT;
+      case fasta.CLOSE_CURLY_BRACKET_TOKEN:
+        return TokenType.CLOSE_CURLY_BRACKET;
+      case fasta.CLOSE_SQUARE_BRACKET_TOKEN:
+        return TokenType.CLOSE_SQUARE_BRACKET;
+      case fasta.CLOSE_PAREN_TOKEN:
+        return TokenType.CLOSE_PAREN;
+      case fasta.SEMICOLON_TOKEN:
+        return TokenType.SEMICOLON;
+      case fasta.SLASH_TOKEN:
+        return TokenType.SLASH;
+      case fasta.TILDE_TOKEN:
+        return TokenType.TILDE;
+      case fasta.STAR_TOKEN:
+        return TokenType.STAR;
+      case fasta.PERCENT_TOKEN:
+        return TokenType.PERCENT;
+      case fasta.CARET_TOKEN:
+        return TokenType.CARET;
+      case fasta.STRING_INTERPOLATION_TOKEN:
+        return TokenType.STRING_INTERPOLATION_EXPRESSION;
+      case fasta.LT_EQ_TOKEN:
+        return TokenType.LT_EQ;
+      case fasta.FUNCTION_TOKEN:
+        return TokenType.FUNCTION;
+      case fasta.SLASH_EQ_TOKEN:
+        return TokenType.SLASH_EQ;
+      case fasta.PERIOD_PERIOD_PERIOD_TOKEN:
+        return TokenType.PERIOD_PERIOD_PERIOD;
+      case fasta.PERIOD_PERIOD_TOKEN:
+        return TokenType.PERIOD_PERIOD;
+      case fasta.EQ_EQ_EQ_TOKEN:
+        // TODO(paulberry,ahe): what is this?
+        throw new UnimplementedError();
+      case fasta.EQ_EQ_TOKEN:
+        return TokenType.EQ_EQ;
+      case fasta.LT_LT_EQ_TOKEN:
+        return TokenType.LT_LT_EQ;
+      case fasta.LT_LT_TOKEN:
+        return TokenType.LT_LT;
+      case fasta.GT_EQ_TOKEN:
+        return TokenType.GT_EQ;
+      case fasta.GT_GT_EQ_TOKEN:
+        return TokenType.GT_GT_EQ;
+      case fasta.INDEX_EQ_TOKEN:
+        return TokenType.INDEX_EQ;
+      case fasta.INDEX_TOKEN:
+        return TokenType.INDEX;
+      case fasta.BANG_EQ_EQ_TOKEN:
+        // TODO(paulberry,ahe): what is this?
+        throw new UnimplementedError();
+      case fasta.BANG_EQ_TOKEN:
+        return TokenType.BANG_EQ;
+      case fasta.AMPERSAND_AMPERSAND_TOKEN:
+        return TokenType.AMPERSAND_AMPERSAND;
+      case fasta.AMPERSAND_EQ_TOKEN:
+        return TokenType.AMPERSAND_EQ;
+      case fasta.BAR_BAR_TOKEN:
+        return TokenType.BAR_BAR;
+      case fasta.BAR_EQ_TOKEN:
+        return TokenType.BAR_EQ;
+      case fasta.STAR_EQ_TOKEN:
+        return TokenType.STAR_EQ;
+      case fasta.PLUS_PLUS_TOKEN:
+        return TokenType.PLUS_PLUS;
+      case fasta.PLUS_EQ_TOKEN:
+        return TokenType.PLUS_EQ;
+      case fasta.MINUS_MINUS_TOKEN:
+        return TokenType.MINUS_MINUS;
+      case fasta.MINUS_EQ_TOKEN:
+        return TokenType.MINUS_EQ;
+      case fasta.TILDE_SLASH_EQ_TOKEN:
+        return TokenType.TILDE_SLASH_EQ;
+      case fasta.TILDE_SLASH_TOKEN:
+        return TokenType.TILDE_SLASH;
+      case fasta.PERCENT_EQ_TOKEN:
+        return TokenType.PERCENT_EQ;
+      case fasta.GT_GT_TOKEN:
+        return TokenType.GT_GT;
+      case fasta.CARET_EQ_TOKEN:
+        return TokenType.CARET_EQ;
+      case fasta.COMMENT_TOKEN:
+        // TODO(paulberry,ahe): how to distinguish multi-line from
+        // single-line comments?  Causes a failure in test_comment_single().

[ahe, 2017/02/06 11:01:20]

I'd just look at the first two characters of the comment:

  bool get isMultiline => value.startsWith("/*");

But we could just as well distinguish these kinds in the scanner. Since
tokenizing comments is optional and not normally used by fasta, it can't affect
performance in any way.

[Paul Berry, 2017/02/06 13:46:54]

Ok.  I'll make a follow-up CL that adjusts this code to use
value.startsWith("/*") as a short-term solution.

+        return TokenType.MULTI_LINE_COMMENT;
+      case fasta.STRING_INTERPOLATION_IDENTIFIER_TOKEN:
+        return TokenType.STRING_INTERPOLATION_IDENTIFIER;
+      case fasta.QUESTION_PERIOD_TOKEN:
+        return TokenType.QUESTION_PERIOD;
+      case fasta.QUESTION_QUESTION_TOKEN:
+        return TokenType.QUESTION_QUESTION;
+      case fasta.QUESTION_QUESTION_EQ_TOKEN:
+        return TokenType.QUESTION_QUESTION_EQ;
+      default:
+        throw new UnimplementedError('$kind');
+    }
+  }
+}
+
+abstract class ScannerTestBase {
   void fail_incomplete_string_interpolation() {
     // https://code.google.com/p/dart/issues/detail?id=18073
     _assertErrorAndTokens(
@@ -230,6 +785,36 @@ class ScannerTest {
     _assertComment(TokenType.MULTI_LINE_COMMENT, "/* comment */");
   }
 
+  void test_comment_multi_consecutive_2() {

[Paul Berry, 2017/02/03 23:10:36]

Note: during this process I discovered that consecutive comments weren't
adequately tested, so I added these tests.

+    Token token = _scan("/* x */ /* y */ z");
+    expect(token.type, TokenType.IDENTIFIER);
+    expect(token.precedingComments, isNotNull);
+    expect(token.precedingComments.value(), "/* x */");
+    expect(token.precedingComments.previous, isNull);
+    expect(token.precedingComments.next, isNotNull);
+    expect(token.precedingComments.next.value(), "/* y */");
+    expect(
+        token.precedingComments.next.previous, same(token.precedingComments));
+    expect(token.precedingComments.next.next, isNull);
+  }
+
+  void test_comment_multi_consecutive_3() {
+    Token token = _scan("/* x */ /* y */ /* z */ a");
+    expect(token.type, TokenType.IDENTIFIER);
+    expect(token.precedingComments, isNotNull);
+    expect(token.precedingComments.value(), "/* x */");
+    expect(token.precedingComments.previous, isNull);
+    expect(token.precedingComments.next, isNotNull);
+    expect(token.precedingComments.next.value(), "/* y */");
+    expect(
+        token.precedingComments.next.previous, same(token.precedingComments));
+    expect(token.precedingComments.next.next, isNotNull);
+    expect(token.precedingComments.next.next.value(), "/* z */");
+    expect(token.precedingComments.next.next.previous,
+        same(token.precedingComments.next));
+    expect(token.precedingComments.next.next.next, isNull);
+  }
+
   void test_comment_multi_lineEnds() {
     String code = r'''
 /**
@@ -1149,14 +1734,7 @@ class ScannerTest {
   }
 
   Token _scanWithListener(String source, _ErrorListener listener,
-      {bool genericMethodComments: false,
-      bool lazyAssignmentOperators: false}) {
-    Scanner scanner =
-        new _TestScanner(new CharSequenceReader(source), listener);
-    scanner.scanGenericMethodComments = genericMethodComments;
-    scanner.scanLazyAssignmentOperators = lazyAssignmentOperators;
-    return scanner.tokenize();
-  }
+      {bool genericMethodComments: false, bool lazyAssignmentOperators: false});
 }
 
 @reflectiveTest