Refactor parseMetadata.

lib/src/runner/parse_metadata.dart

Index: lib/src/runner/parse_metadata.dart
diff --git a/lib/src/runner/parse_metadata.dart b/lib/src/runner/parse_metadata.dart
index 21ed6e2cbd954fc71812c324522a6a34007b10c8..e4e4079608b8ee72a583eb3d7d9ad909a2aaf381 100644
--- a/lib/src/runner/parse_metadata.dart
+++ b/lib/src/runner/parse_metadata.dart
@@ -15,310 +15,331 @@ import '../backend/metadata.dart';
 import '../frontend/timeout.dart';
 import '../util/dart.dart';
 
-/// The valid argument names for [new Duration].
-const _durationArgs = const [
-  "days",
-  "hours",
-  "minutes",
-  "seconds",
-  "milliseconds",
-  "microseconds"
-];
-
 /// Parse the test metadata for the test file at [path].
 ///
 /// Throws an [AnalysisError] if parsing fails or a [FormatException] if the
 /// test annotations are incorrect.
-Metadata parseMetadata(String path) {
-  var timeout;
-  var testOn;
-  var skip;
-
-  var contents = new File(path).readAsStringSync();
-  var directives = parseDirectives(contents, name: path).directives;
-  var annotations = directives.isEmpty ? [] : directives.first.metadata;
-
-  // We explicitly *don't* just look for "package:test" imports here,
-  // because it could be re-exported from another library.
-  var prefixes = directives.map((directive) {
-    if (directive is! ImportDirective) return null;
-    if (directive.prefix == null) return null;
-    return directive.prefix.name;
-  }).where((prefix) => prefix != null).toSet();
-
-  for (var annotation in annotations) {
-    // The annotation syntax is ambiguous between named constructors and
-    // prefixed annotations, so we need to resolve that ambiguity using the
-    // known prefixes. The analyzer parses "@x.y()" as prefix "x", annotation
-    // "y", and named constructor null. It parses "@x.y.z()" as prefix "x",
-    // annotation "y", and named constructor "z".
-    var name;
-    var constructorName;
-    var identifier = annotation.name;
-    if (identifier is PrefixedIdentifier &&
-        !prefixes.contains(identifier.prefix.name) &&
-        annotation.constructorName == null) {
-      name = identifier.prefix.name;
-      constructorName = identifier.identifier.name;
-    } else {
-      name = identifier is PrefixedIdentifier
-          ? identifier.identifier.name
-          : identifier.name;
-      if (annotation.constructorName != null) {
-        constructorName = annotation.constructorName.name;
-      }
-    }
+Metadata parseMetadata(String path) => new _Parser(path).parse();
+
+/// A parser for test suite metadata.
+class _Parser {
+  /// The path to the test suite.
+  final String _path;
+
+  /// All annotations at the top of the file.
+  List<Annotation> _annotations;
+
+  /// All prefixes defined by imports in this file.
+  Set<String> _prefixes;
+
+  _Parser(String path)
+      : _path = path {
+    var contents = new File(path).readAsStringSync();
+    var directives = parseDirectives(contents, name: path).directives;
+    _annotations = directives.isEmpty ? [] : directives.first.metadata;
+
+    // We explicitly *don't* just look for "package:test" imports here,
+    // because it could be re-exported from another library.
+    _prefixes = directives.map((directive) {
+      if (directive is! ImportDirective) return null;
+      if (directive.prefix == null) return null;
+      return directive.prefix.name;
+    }).where((prefix) => prefix != null).toSet();
+  }
 
-    if (name == 'TestOn') {
-      if (testOn != null) {
-        throw new SourceSpanFormatException(
-            "Only a single TestOn annotation may be used for a given test file.",
-            _spanFor(annotation, path));
+  /// Parses the metadata.
+  Metadata parse() {
+    var timeout;
+    var testOn;
+    var skip;
+
+    for (var annotation in _annotations) {
+      // The annotation syntax is ambiguous between named constructors and
+      // prefixed annotations, so we need to resolve that ambiguity using the
+      // known prefixes. The analyzer parses "@x.y()" as prefix "x", annotation
+      // "y", and named constructor null. It parses "@x.y.z()" as prefix "x",
+      // annotation "y", and named constructor "z".
+      var name;
+      var constructorName;
+      var identifier = annotation.name;
+      if (identifier is PrefixedIdentifier &&
+          !_prefixes.contains(identifier.prefix.name) &&
+          annotation.constructorName == null) {
+        name = identifier.prefix.name;
+        constructorName = identifier.identifier.name;
+      } else {
+        name = identifier is PrefixedIdentifier
+            ? identifier.identifier.name
+            : identifier.name;
+        if (annotation.constructorName != null) {
+          constructorName = annotation.constructorName.name;
+        }
       }
-      testOn = _parseTestOn(annotation, constructorName, path);
-    } else if (name == 'Timeout') {
-      if (timeout != null) {
-        throw new SourceSpanFormatException(
-            "Only a single Timeout annotation may be used for a given test file.",
-            _spanFor(annotation, path));
-      }
-      timeout = _parseTimeout(annotation, constructorName, path);
-    } else if (name == 'Skip') {
-      if (skip != null) {
-        throw new SourceSpanFormatException(
-            "Only a single Skip annotation may be used for a given test file.",
-            _spanFor(annotation, path));
+
+      if (name == 'TestOn') {
+        _assertSingleAnnotation(testOn, 'TestOn', annotation);
+        testOn = _parseTestOn(annotation, constructorName);
+      } else if (name == 'Timeout') {
+        _assertSingleAnnotation(timeout, 'Timeout', annotation);
+        timeout = _parseTimeout(annotation, constructorName);
+      } else if (name == 'Skip') {
+        _assertSingleAnnotation(skip, 'Skip', annotation);
+        skip = _parseSkip(annotation, constructorName);
       }
-      skip = _parseSkip(annotation, constructorName, path);
     }
-  }
 
-  try {
-    return new Metadata.parse(
-        testOn: testOn == null ? null : testOn.stringValue,
-        timeout: timeout,
-        skip: skip);
-  } on SourceSpanFormatException catch (error) {
-    var file = new SourceFile(new File(path).readAsStringSync(),
-        url: p.toUri(path));
-    var span = contextualizeSpan(error.span, testOn, file);
-    if (span == null) rethrow;
-    throw new SourceSpanFormatException(error.message, span);
+    try {
+      return new Metadata.parse(
+          testOn: testOn == null ? null : testOn.stringValue,
+          timeout: timeout,
+          skip: skip);
+    } on SourceSpanFormatException catch (error) {
+      var file = new SourceFile(new File(_path).readAsStringSync(),
+          url: p.toUri(_path));
+      var span = contextualizeSpan(error.span, testOn, file);
+      if (span == null) rethrow;
+      throw new SourceSpanFormatException(error.message, span);
+    }
   }
-}
 
-/// Parses a `@TestOn` annotation.
-///
-/// [annotation] is the annotation. [constructorName] is the name of the named
-/// constructor for the annotation, if any. [path] is the path to the file from
-/// which the annotation was parsed.
-StringLiteral _parseTestOn(Annotation annotation, String constructorName,
-    String path) {
-  if (constructorName != null) {
-    throw new SourceSpanFormatException(
-        'TestOn doesn\'t have a constructor named "$constructorName".',
-        _spanFor(annotation, path));
+  /// Parses a `@TestOn` annotation.
+  ///
+  /// [annotation] is the annotation. [constructorName] is the name of the named
+  /// constructor for the annotation, if any.
+  StringLiteral _parseTestOn(Annotation annotation, String constructorName) {
+    _assertConstructorName(constructorName, 'TestOn', annotation);
+    _assertArguments(annotation.arguments, 'TestOn', annotation, positional: 1);
+    return _parseString(annotation.arguments.arguments.first);
   }
 
-  if (annotation.arguments == null) {
-    throw new SourceSpanFormatException(
-        'TestOn takes one argument.', _spanFor(annotation, path));
-  }
+  /// Parses a `@Timeout` annotation.
+  ///
+  /// [annotation] is the annotation. [constructorName] is the name of the named
+  /// constructor for the annotation, if any.
+  Timeout _parseTimeout(Annotation annotation, String constructorName) {
+    _assertConstructorName(constructorName, 'Timeout', annotation,
+        validNames: [null, 'factor']);
 
-  var args = annotation.arguments.arguments;
-  if (args.isEmpty) {
-    throw new SourceSpanFormatException(
-        'TestOn takes one argument.', _spanFor(annotation.arguments, path));
-  }
+    var description = 'Timeout';
+    if (constructorName != null) description += '.$constructorName'; 
 
-  if (args.first is NamedExpression) {
-    throw new SourceSpanFormatException(
-        "TestOn doesn't take named parameters.", _spanFor(args.first, path));
-  }
+    _assertArguments(annotation.arguments, description, annotation,
+        positional: 1);
 
-  if (args.length > 1) {
-    throw new SourceSpanFormatException(
-        "TestOn takes only one argument.",
-        _spanFor(annotation.arguments, path));
+    var args = annotation.arguments.arguments;
+    if (constructorName == null) return new Timeout(_parseDuration(args.first));
+    return new Timeout.factor(_parseNum(args.first));
   }
 
-  if (args.first is! StringLiteral) {
-    throw new SourceSpanFormatException(
-        "TestOn takes a String.", _spanFor(args.first, path));
+  /// Parses a `@Skip` annotation.
+  ///
+  /// [annotation] is the annotation. [constructorName] is the name of the named
+  /// constructor for the annotation, if any.
+  ///
+  /// Returns either `true` or a reason string.
+  _parseSkip(Annotation annotation, String constructorName) {
+    _assertConstructorName(constructorName, 'Skip', annotation);
+    _assertArguments(annotation.arguments, 'Skip', annotation, optional: 1);
+
+    var args = annotation.arguments.arguments;
+    return args.isEmpty ? true : _parseString(args.first).stringValue;
   }
 
-  return args.first;
-}
+  /// Parses a `const Duration` expression.
+  Duration _parseDuration(Expression expression) {
+    _parseConstructor(expression, 'Duration');
 
-/// Parses a `@Timeout` annotation.
-///
-/// [annotation] is the annotation. [constructorName] is the name of the named
-/// constructor for the annotation, if any. [path] is the path to the file from
-/// which the annotation was parsed.
-Timeout _parseTimeout(Annotation annotation, String constructorName,
-    String path) {
-  if (constructorName != null && constructorName != 'factor') {
-    throw new SourceSpanFormatException(
-        'Timeout doesn\'t have a constructor named "$constructorName".',
-        _spanFor(annotation, path));
-  }
+    var constructor = expression as InstanceCreationExpression;
+    var values = _assertArguments(
+        constructor.argumentList, 'Duration', constructor, named: [
+      'days', 'hours', 'minutes', 'seconds', 'milliseconds', 'microseconds'
+    ]);
 
-  var description = 'Timeout';
-  if (constructorName != null) description += '.$constructorName'; 
+    for (var key in values.keys.toList()) {
+      if (values.containsKey(key)) values[key] = _parseInt(values[key]);
+    }
 
-  if (annotation.arguments == null) {
-    throw new SourceSpanFormatException(
-        '$description takes one argument.', _spanFor(annotation, path));
+    return new Duration(
+        days: values["days"] == null ? 0 : values["days"],
+        hours: values["hours"] == null ? 0 : values["hours"],
+        minutes: values["minutes"] == null ? 0 : values["minutes"],
+        seconds: values["seconds"] == null ? 0 : values["seconds"],
+        milliseconds: values["milliseconds"] == null ? 0 : values["milliseconds"],
+        microseconds:
+            values["microseconds"] == null ? 0 : values["microseconds"]);
   }
 
-  var args = annotation.arguments.arguments;
-  if (args.isEmpty) {
+  /// Asserts that [existing] is null.
+  ///
+  /// [name] is the name of the annotation and [node] is its location, used for
+  /// error reporting.
+  void _assertSingleAnnotation(Object existing, String name, AstNode node) {
+    if (existing == null) return;
     throw new SourceSpanFormatException(
-        '$description takes one argument.',
-        _spanFor(annotation.arguments, path));
+        "Only a single $name annotation may be used for a given test file.",
+        _spanFor(node));
   }
 
-  if (args.first is NamedExpression) {
-    throw new SourceSpanFormatException(
-        "$description doesn't take named parameters.",
-        _spanFor(args.first, path));
+  /// Asserts that [constructorName] is a valid constructor name for an AST
+  /// node.
+  ///
+  /// [nodeName] is the name of the class being constructed, and [node] is the
+  /// AST node for that class. [validNames], if passed, is the set of valid
+  /// constructor names; if an unnamed constructor is valid, it should include
+  /// `null`. By default, only an unnamed constructor is allowed.
+  void _assertConstructorName(String constructorName, String nodeName,
+      AstNode node, {Iterable<String> validNames}) {
+    if (validNames == null) validNames = [null];
+    if (validNames.contains(constructorName)) return;
+
+    if (constructorName == null) {
+      throw new SourceSpanFormatException(
+          "$nodeName doesn't have an unnamed constructor.",
+          _spanFor(node));
+    } else {
+      throw new SourceSpanFormatException(
+          '$nodeName doesn\'t have a constructor named "$constructorName".',
+          _spanFor(node));
+    }
   }
 
-  if (args.length > 1) {
-    throw new SourceSpanFormatException(
-        "$description takes only one argument.",
-        _spanFor(annotation.arguments, path));
-  }
+  /// Parses a constructor invocation for [className].
+  ///
+  /// [validNames], if passed, is the set of valid constructor names; if an
+  /// unnamed constructor is valid, it should include `null`. By default, only
+  /// an unnamed constructor is allowed.
+  ///
+  /// Returns the name of the named constructor, if any.
+  String _parseConstructor(Expression expression, String className,
+      {Iterable<String> validNames}) {
+    if (validNames == null) validNames = [null];
+
+    if (expression is! InstanceCreationExpression) {
+      throw new SourceSpanFormatException(
+          "Expected a $className.", _spanFor(expression));
+    }
 
-  if (constructorName == null) {
-    return new Timeout(_parseDuration(args.first, path));
-  } else {
-    return new Timeout.factor(_parseNum(args.first, path));
-  }
-}
+    var constructor = expression as InstanceCreationExpression;
+    if (constructor.constructorName.type.name.name != className) {
+      throw new SourceSpanFormatException(
+          "Expected a $className.", _spanFor(constructor));
+    }
 
-/// Parses a `@Skip` annotation.
-///
-/// [annotation] is the annotation. [constructorName] is the name of the named
-/// constructor for the annotation, if any. [path] is the path to the file from
-/// which the annotation was parsed.
-///
-/// Returns either `true` or a reason string.
-_parseSkip(Annotation annotation, String constructorName, String path) {
-  if (constructorName != null) {
-    throw new SourceSpanFormatException(
-        'Skip doesn\'t have a constructor named "$constructorName".',
-        _spanFor(annotation, path));
-  }
+    if (constructor.keyword.lexeme != "const") {
+      throw new SourceSpanFormatException(
+          "$className must use a const constructor.", _spanFor(constructor));
+    }
 
-  if (annotation.arguments == null) {
-    throw new SourceSpanFormatException(
-        'Skip must have parentheses.', _spanFor(annotation, path));
+    var name = constructor.constructorName == null
+        ? null
+        : constructor.constructorName.name;
+    _assertConstructorName(name, className, expression,
+        validNames: validNames);
+    return name;
   }
 
-  var args = annotation.arguments.arguments;
-  if (args.length > 1) {
-    throw new SourceSpanFormatException(
-        'Skip takes zero arguments or one argument.',
-        _spanFor(annotation.arguments, path));
-  }
+  /// Assert that [arguments] is a valid argument list.
+  ///
+  /// [name] describes the function and [node] is its AST node. [positional] is
+  /// the number of required positional arguments, [optional] the number of
+  /// optional positional arguments, and [named] the set of valid argument
+  /// names.
+  ///
+  /// The set of parsed named arguments is returned.
+  Map<String, Expression> _assertArguments(ArgumentList arguments, String name,
+      AstNode node, {int positional, int optional, Iterable<String> named}) {
+    if (positional == null) positional = 0;
+    if (optional == null) optional = 0;
+    if (named == null) named = new Set();
+
+    if (arguments == null) {
+      throw new SourceSpanFormatException(
+          '$name takes arguments.', _spanFor(node));
+    }
 
-  if (args.isEmpty) return true;
+    var actualNamed = arguments.arguments
+        .where((arg) => arg is NamedExpression).toList();
+    if (!actualNamed.isEmpty && named.isEmpty) {
+      throw new SourceSpanFormatException(
+          "$name doesn't take named arguments.", _spanFor(actualNamed.first));
+    }
 
-  if (args.first is NamedExpression) {
-    throw new SourceSpanFormatException(
-        "Skip doesn't take named parameters.", _spanFor(args.first, path));
-  }
+    var namedValues = {};
+    for (var argument in actualNamed) {
+      var argumentName = argument.name.label.name;
+      if (!named.contains(argumentName)) {
+        throw new SourceSpanFormatException(
+            '$name doesn\'t take an argument named "$argumentName".',
+            _spanFor(argument));
+      } else if (namedValues.containsKey(argumentName)) {
+        throw new SourceSpanFormatException(
+            'An argument named "$argumentName" was already passed.',
+            _spanFor(argument));
+      } else {
+        namedValues[argumentName] = argument.expression;
+      }
+    }
 
-  if (args.first is! StringLiteral) {
-    throw new SourceSpanFormatException(
-        "Skip takes a String.", _spanFor(args.first, path));
-  }
+    var actualPositional = arguments.arguments.length - actualNamed.length;
+    if (actualPositional < positional) {
+      var buffer = new StringBuffer("$name takes ");
+      if (optional != 0) buffer.write("at least ");
+      buffer.write("$positional argument");
+      if (positional > 1) buffer.write("s");
+      buffer.write(".");
+      throw new SourceSpanFormatException(
+          buffer.toString(), _spanFor(arguments));
+    }
 
-  return args.first.stringValue;
-}
+    if (actualPositional > positional + optional) {
+      if (optional + positional == 0) {
+        var buffer = new StringBuffer("$name doesn't take ");
+        if (!named.isEmpty) buffer.write("positional ");
+        buffer.write("arguments.");
+        throw new SourceSpanFormatException(
+            buffer.toString(), _spanFor(arguments));
+      }
 
-/// Parses a `const Duration` expression.
-Duration _parseDuration(Expression expression, String path) {
-  if (expression is! InstanceCreationExpression) {
-    throw new SourceSpanFormatException(
-        "Expected a Duration.",
-        _spanFor(expression, path));
-  }
+      var buffer = new StringBuffer("$name takes ");
+      if (optional != 0) buffer.write("at most ");
+      buffer.write("${positional + optional} argument");
+      if (positional > 1) buffer.write("s");
+      buffer.write(".");
+      throw new SourceSpanFormatException(
+          buffer.toString(), _spanFor(arguments));
+    }
 
-  var constructor = expression as InstanceCreationExpression;
-  if (constructor.constructorName.type.name.name != 'Duration') {
-    throw new SourceSpanFormatException(
-        "Expected a Duration.",
-        _spanFor(constructor, path));
+    return namedValues;
   }
 
-  if (constructor.keyword.lexeme != "const") {
+  /// Parses a constant number literal.
+  num _parseNum(Expression expression) {
+    if (expression is IntegerLiteral) return expression.value;
+    if (expression is DoubleLiteral) return expression.value;
     throw new SourceSpanFormatException(
-        "Duration must use a const constructor.",
-        _spanFor(constructor, path));
+        "Expected a number.", _spanFor(expression));
   }
 
-  if (constructor.constructorName.name != null) {
+  /// Parses a constant int literal.
+  int _parseInt(Expression expression) {
+    if (expression is IntegerLiteral) return expression.value;
     throw new SourceSpanFormatException(
-        "Duration doesn't have a constructor named "
-            '"${constructor.constructorName}".',
-        _spanFor(constructor.constructorName, path));
+        "Expected an integer.", _spanFor(expression));
   }
 
-  var values = {};
-  var args = constructor.argumentList.arguments;
-  for (var argument in args) {
-    if (argument is! NamedExpression) {
-      throw new SourceSpanFormatException(
-          "Duration doesn't take positional arguments.",
-          _spanFor(argument, path));
-    }
-
-    var name = argument.name.label.name;
-    if (!_durationArgs.contains(name)) {
-      throw new SourceSpanFormatException(
-          'Duration doesn\'t take an argument named "$name".',
-          _spanFor(argument, path));
-    }
-
-    if (values.containsKey(name)) {
-      throw new SourceSpanFormatException(
-          'An argument named "$name" was already passed.',
-          _spanFor(argument, path));
-    }
-
-    values[name] = _parseInt(argument.expression, path);
+  /// Parses a constant String literal.
+  StringLiteral _parseString(Expression expression) {
+    if (expression is StringLiteral) return expression;
+    throw new SourceSpanFormatException(
+        "Expected a String.", _spanFor(expression));
   }
 
-  return new Duration(
-      days: values["days"] == null ? 0 : values["days"],
-      hours: values["hours"] == null ? 0 : values["hours"],
-      minutes: values["minutes"] == null ? 0 : values["minutes"],
-      seconds: values["seconds"] == null ? 0 : values["seconds"],
-      milliseconds: values["milliseconds"] == null ? 0 : values["milliseconds"],
-      microseconds:
-          values["microseconds"] == null ? 0 : values["microseconds"]);
-}
-
-/// Parses a constant number literal.
-num _parseNum(Expression expression, String path) {
-  if (expression is IntegerLiteral) return expression.value;
-  if (expression is DoubleLiteral) return expression.value;
-  throw new SourceSpanFormatException(
-      "Expected a number.", _spanFor(expression, path));
-}
-
-/// Parses a constant int literal.
-int _parseInt(Expression expression, String path) {
-  if (expression is IntegerLiteral) return expression.value;
-  throw new SourceSpanFormatException(
-      "Expected an integer.", _spanFor(expression, path));
-}
-
-/// Creates a [SourceSpan] for [node].
-SourceSpan _spanFor(AstNode node, String path) =>
+  /// Creates a [SourceSpan] for [node].
+  SourceSpan _spanFor(AstNode node) {
     // Load a SourceFile from scratch here since we're only ever going to emit
     // one error per file anyway.
-    new SourceFile(new File(path).readAsStringSync(), url: p.toUri(path))
+    var contents = new File(_path).readAsStringSync();
+    return new SourceFile(contents, url: p.toUri(_path))
         .span(node.offset, node.end);
+  }
+}