Linting for status files.

This both works as a tester for correct canonicalization of status files and can
be used later to check if changes to status files follow the guide lines.

It checks that:
- Comments only exists before a section or after a test path entry.
- That there are no disjunctions in headers.
- That variables and clauses are correctly ordered in section headers.
- That paths are alphabetically ordered in sections.
- That sections are lexicographically ordered with respect to negation.

Bug:
Change-Id: I0f5e2cc16f20bb48ee53a9a55a22aaab710e32ff
Reviewed-on: https://dart-review.googlesource.com/17786
Reviewed-by: Bob Nystrom <rnystrom@google.com>
Commit-Queue: Morten Krogh-jespersen <mkroghj@google.com>

pkg/status_file/.packages

@@ -1,4 +1,5 @@
 # Generated by pub on 2017-07-24 16:32:37.651832.
 expect:../expect/lib/
 path:../../third_party/pkg/path/lib/
+args:../../third_party/pkg/args/lib/
 status_file:lib/

pkg/status_file/bin/lint.dart

@@ -0,0 +1,61 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:io';
+
+import 'package:args/args.dart';
+import 'package:status_file/canonical_status_file.dart';
+import 'package:status_file/status_file.dart' as status_file;
+import 'package:status_file/status_file_linter.dart';
+
+void main(List<String> arguments) {
+  var parser = new ArgParser();
+  parser.addFlag("check-for-disjunctions",
+      negatable: false,
+      defaultsTo: false,
+      help: "Warn if a status header expression contains '||'.");
+  var results = parser.parse(arguments);
+  if (results.rest.length != 1) {
+    print("Usage: dart status_file/bin/lint.dart <path>");
+    exit(1);
+  }
+  print("");
+  var path = results.rest.first;
+  bool result = true;
+  if (new File(path).existsSync()) {
+    result =
+        lintFile(path, checkForDisjunctions: results['check-for-disjunctions']);
+  } else if (new Directory(path).existsSync()) {
+    var allResults = new Directory(path).listSync(recursive: true).map((entry) {
+      if (!entry.path.endsWith(".status")) {
+        return true;
+      }
+      return lintFile(entry.path,
+          checkForDisjunctions: results['check-for-disjunctions']);
+    }).toList();
+    return allResults.every((result) => result);
+  }
+  if (!result) {
+    exit(1);
+  }
+}
+
+bool lintFile(String path, {bool checkForDisjunctions = false}) {
+  try {
+    var statusFile = new StatusFile.read(path);
+    var lintingErrors =
+        lint(statusFile, checkForDisjunctions: checkForDisjunctions);
+    if (lintingErrors.isEmpty) {
+      return true;
+    }
+    print("${path}:");
+    var errors = lintingErrors.toList();
+    errors.sort((a, b) => a.lineNumber.compareTo((b.lineNumber)));
+    errors.forEach(print);
+    print("");
+  } on status_file.SyntaxError catch (error) {
+    stderr.writeln("Could not parse $path:\n$error");
+  }
+  return false;
+}

pkg/status_file/lib/status_file_linter.dart

@@ -0,0 +1,187 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:math' as math;
+
+import 'package:status_file/canonical_status_file.dart';
+
+class LintingError {
+  final int lineNumber;
+  final String message;
+  LintingError(this.lineNumber, this.message);
+
+  String toString() {
+    return "Error at line $lineNumber: $message";
+  }
+}
+
+/// Main function to check a status file for linting errors.
+List<LintingError> lint(StatusFile file, {checkForDisjunctions = false}) {
+  var errors = <LintingError>[];
+  for (var section in file.sections) {
+    errors
+      ..addAll(lintCommentLinesInSection(section))
+      ..addAll(lintAlphabeticalOrderingOfPaths(section))
+      ..addAll(lintNormalizedSection(section));
+    if (checkForDisjunctions) {
+      errors.addAll(lintDisjunctionsInHeader(section));
+    }
+  }
+  errors.addAll(checkSectionHeaderOrdering(file.sections));
+  return errors;
+}
+
+/// Checks for invalid comment lines in a section.
+///
+/// We do not allow the following:
+///
+/// [ ... ]
+///
+/// vm/test: Skip # description
+/// # Some comment <-- invalid
+/// ...
+///
+/// This function checks for such invalid comments.
+Iterable<LintingError> lintCommentLinesInSection(StatusSection section) {
+  if (section.lineNumber == -1) {
+    // This is the default section, which also has the dart copyright notice.
+    // Allow comment entries in the beginning of the file, until the first
+    // status entry.
+    var seenStatusEntry = false;
+    var lintingErrors = <LintingError>[];
+    for (var entry in section.entries) {
+      seenStatusEntry = seenStatusEntry || entry is StatusEntry;
+      if (seenStatusEntry && entry is CommentEntry) {
+        lintingErrors.add(new LintingError(
+            entry.lineNumber, "Comment is on a line by itself."));
+      }
+    }
+    return lintingErrors;
+  }
+  return section.entries.where((entry) => entry is CommentEntry).map((entry) =>
+      new LintingError(entry.lineNumber, "Comment is on a line by itself."));
+}
+
+/// Checks for disjunctions in headers. Disjunctions should be separated out.
+///
+/// Example:
+/// [ $mode == debug || $mode == release ]
+///
+/// should not be allowed. The clauses should be refactored into own sections:
+/// [ $mode == debug ]
+/// ...
+///
+///
+/// [ $mode == release ]
+/// ...
+///
+/// Removing disjunctions will turn some sections into two or more sections with
+/// the same status entries, but these will be much easier to process with our
+/// tools.
+Iterable<LintingError> lintDisjunctionsInHeader(StatusSection section) {
+  if (section.condition.toString().contains("||")) {
+    return [
+      new LintingError(
+          section.lineNumber,
+          "Expression contains '||'. Please split the expression into multiple "
+          "separate sections.")
+    ];
+  }
+  return [];
+}
+
+/// Checks for correct ordering of test entries in sections. They should be
+/// ordered alphabetically.
+Iterable<LintingError> lintAlphabeticalOrderingOfPaths(StatusSection section) {
+  var entries = section.entries.where((entry) => entry is StatusEntry).toList();
+  var sortedList = entries.toList()..sort((a, b) => a.path.compareTo(b.path));
+  var witness = _findNotEqualWitness<StatusEntry>(sortedList, entries);
+  if (witness != null) {
+    return [
+      new LintingError(
+          section.lineNumber,
+          "Test paths are not alphabetically ordered in section. "
+          "${witness.first.path} should come before ${witness.second.path}.")
+    ];
+  }
+  return [];
+}
+
+/// Checks that each section expression have been normalized.
+Iterable<LintingError> lintNormalizedSection(StatusSection section) {
+  if (section.condition == null) return const [];
+  var normalized = section.condition.normalize().toString();
+  if (section.condition.toString() != normalized) {
+    return [
+      new LintingError(
+          section.lineNumber, "Condition expression should be '$normalized'.")
+    ];
+  }
+  return const [];
+}
+
+/// Checks for incorrect ordering of section headers. Section headers should be
+/// alphabetically ordered, except, when negation is used, it should be
+/// lexicographically close to the none-negated one, but still come after.
+///
+/// [ $compiler == dart2js ] < [ $strong ]
+/// [ $mode == debug ]       < [ $mode != debug ]
+/// [ $strong ]              < [ ! $strong ]
+///
+/// A larger example could be the following:
+///
+/// [ $mode != debug ]
+/// [ !strong ]
+/// [ $mode == debug ]
+/// [ strong ]
+/// [ $compiler == dart2js ]
+///
+/// which should should become:
+///
+/// [ $compiler == dart2js ]
+/// [ $mode == debug ]
+/// [ $mode != debug ]
+/// [ strong ]
+/// [ !strong ]
+///
+Iterable<LintingError> checkSectionHeaderOrdering(
+    List<StatusSection> sections) {
+  var unsorted = sections.where((section) => section.lineNumber != -1).toList();
+  var sorted = unsorted.toList()
+    ..sort((a, b) => a.condition.compareTo(b.condition));
+  var witness = _findNotEqualWitness<StatusSection>(sorted, unsorted);
+  if (witness != null) {
+    return [
+      new LintingError(
+          witness.second.lineNumber,
+          "Section expressions are not correctly ordered in file. "
+          "${witness.first.condition} on line ${witness.first.lineNumber} "
+          "should come before ${witness.second.condition} at line "
+          "${witness.second.lineNumber}.")
+    ];
+  }
+  return [];
+}
+
+ListNotEqualWitness<T> _findNotEqualWitness<T>(List<T> first, List<T> second) {
+  if (first.isEmpty && second.isEmpty) {
+    return null;
+  }
+  for (var i = 0; i < math.max(first.length, second.length); i++) {
+    if (i >= second.length) {
+      return new ListNotEqualWitness(first[i], null);
+    } else if (i >= first.length) {
+      return new ListNotEqualWitness(null, second[i]);
+    } else if (first[i] != second[i]) {
+      return new ListNotEqualWitness(first[i], second[i]);
+    }
+  }
+  return null;
+}
+
+class ListNotEqualWitness<T> {
+  final T first;
+  final T second;
+  ListNotEqualWitness(this.first, this.second);
+}

pkg/status_file/pubspec.yaml

@@ -6,6 +6,7 @@ homepage: http://www.dartlang.org
 #  sdk: '>=1.0.0 <2.0.0'
 dependencies:
   path: "^1.4.0"
+  args: "^0.13.7"
 dev_dependencies:
   expect:
     path: ../expect

pkg/status_file/test/linter_test.dart

@@ -0,0 +1,244 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+import 'package:status_file/canonical_status_file.dart';
+import 'package:status_file/status_file_linter.dart';
+
+void main() {
+  testCommentLinesInSection_invalidCommentInSection();
+  testCommentLinesInSection_invalidCommentInSectionAfterEntries();
+  testCommentLinesInSection_okSectionEntryComment();
+  testCommentLinesInSection_okSectionComment();
+
+  testCheckForDisjunctions_notAllowedDisjunction();
+  testCheckForDisjunctions_shouldBeAllowedInComments();
+
+  testCheckForAlphabeticalOrderingOfPaths_invalidOrdering();
+  testCheckForAlphabeticalOrderingOfPaths_okOrdering();
+
+  testCheckForCorrectOrderingInSections_invalidRuntimeBeforeCompiler();
+  testCheckForCorrectOrderingInSections_invalidRuntimeBeforeMode();
+  testCheckForCorrectOrderingInSections_invalidSystemBeforeMode();
+  testCheckForCorrectOrderingInSections_invalidStrongBeforeKernel();
+  testCheckForCorrectOrderingInSections_invalidOrdering();
+  testCheckForCorrectOrderingInSections_okOrdering();
+
+  checkLintNormalizedSection_invalidAlphabeticalOrderingVariables();
+  checkLintNormalizedSection_invalidAlphabeticalOrderingVariableArguments();
+  checkLintNormalizedSection_invalidOrderingWithNotEqual();
+  checkLintNormalizedSection_invalidOrderingWithNegation();
+}
+
+StatusFile createFromString(String text) {
+  return new StatusFile.parse("test", text.split('\n'));
+}
+
+expectError(String text, String expectedError, {bool disjunctions = false}) {
+  var statusFile = createFromString(text);
+  var errors = lint(statusFile, checkForDisjunctions: disjunctions).toList();
+  Expect.equals(expectedError, errors.first.toString());
+}
+
+expectNoError(String text, {bool disjunctions = true}) {
+  var errors =
+      lint(createFromString(text), checkForDisjunctions: disjunctions).toList();
+  Expect.listEquals([], errors);
+}
+
+void testCommentLinesInSection_invalidCommentInSection() {
+  expectError(r"""[ $mode == debug ]
+# this comment is invalid
+""", "Error at line 2: Comment is on a line by itself.");
+}
+
+void testCommentLinesInSection_invalidCommentInSectionAfterEntries() {
+  expectError(r"""[ $mode == debug ]
+vm/tests: Skip
+# this comment is invalid
+""", "Error at line 3: Comment is on a line by itself.");
+}
+
+void testCommentLinesInSection_okSectionEntryComment() {
+  expectNoError(r"""[ $mode == debug ]
+vm/tests: Skip # this comment is valid
+vm/tests2: Timeout # this comment is also valid
+""");
+}
+
+void testCommentLinesInSection_okSectionComment() {
+  expectNoError(r"""
+recursive_mixin_test: Crash
+
+# These comment lines belong to the section header. These are alright to have.
+# Even having multiple lines of these should not be a problem.
+[ $mode == debug ]
+vm/tests: Skip 
+vm/tests2: Timeout # this comment is also valid
+""");
+}
+
+void testCheckForDisjunctions_notAllowedDisjunction() {
+  expectError(
+      r"""[ $mode == debug || $mode == release ]
+vm/tests: Skip # this comment is valid
+""",
+      "Error at line 1: Expression contains '||'. Please split the expression "
+      "into multiple separate sections.",
+      disjunctions: true);
+}
+
+void testCheckForDisjunctions_shouldBeAllowedInComments() {
+  expectNoError(r"""# This should allow || in comments
+[ $mode == debug ]
+vm/tests: Skip # this comment is valid
+""", disjunctions: true);
+}
+
+void testCheckForAlphabeticalOrderingOfPaths_invalidOrdering() {
+  expectError(
+      r"""[ $mode == debug ]
+vm/tests: Skip # this should come after a_test
+a_test: Pass
+""",
+      "Error at line 1: Test paths are not alphabetically ordered in "
+      "section. a_test should come before vm/tests.");
+}
+
+void testCheckForAlphabeticalOrderingOfPaths_okOrdering() {
+  expectNoError(r"""[ $mode == debug ]
+a_test: Pass
+b_test: Pass
+bc_test: Pass
+xyz_test: Skip
+""");
+}
+
+void testCheckForCorrectOrderingInSections_invalidRuntimeBeforeCompiler() {
+  expectError(
+      r"""[ $runtime == ff && $compiler == dart2js]
+a_test: Pass
+""",
+      r"Error at line 1: Condition expression should be '$compiler == dart2js "
+      r"&& $runtime == ff'.");
+}
+
+void testCheckForCorrectOrderingInSections_invalidRuntimeBeforeMode() {
+  expectError(
+      r"""[ $runtime == ff && $mode == debug ]
+a_test: Pass
+""",
+      r"Error at line 1: Condition expression should be '$mode == debug && "
+      r"$runtime == ff'.");
+}
+
+void testCheckForCorrectOrderingInSections_invalidSystemBeforeMode() {
+  expectError(
+      r"""[ $system == win && $mode == debug ]
+a_test: Pass
+""",
+      r"Error at line 1: Condition expression should be '$mode == debug && "
+      r"$system == win'.");
+}
+
+void testCheckForCorrectOrderingInSections_invalidStrongBeforeKernel() {
+  expectError(r"""[ !$strong && !$kernel ]
+a_test: Pass
+""", r"Error at line 1: Condition expression should be '!$kernel && !$strong'.");
+}
+
+void testCheckForCorrectOrderingInSections_invalidOrdering() {
+  expectError(
+      r"""[ $compiler == dart2js && $builder_tag == strong && !$browser ]
+a_test: Pass
+""",
+      r"Error at line 1: Condition expression should be '$builder_tag == "
+      r"strong && $compiler == dart2js && !$browser'.");
+}
+
+void testCheckForCorrectOrderingInSections_okOrdering() {
+  expectNoError(r"""[ $compiler == dart2js && $runtime != ff && !$browser ]
+a_test: Pass
+""");
+}
+
+void checkLintNormalizedSection_invalidAlphabeticalOrderingVariables() {
+  expectError(
+      r"""[ $runtime == ff ]
+a_test: Pass
+
+[ $compiler == dart2js ]
+a_test: Pass
+""",
+      r"Error at line 1: Section expressions are not correctly ordered in file."
+      r" $compiler == dart2js on line 4 should come before $runtime == ff at "
+      r"line 1.");
+}
+
+void checkLintNormalizedSection_invalidAlphabeticalOrderingVariableArguments() {
+  expectError(
+      r"""[ $runtime == ff ]
+a_test: Pass
+
+[ $runtime == chrome ]
+a_test: Pass
+""",
+      r"Error at line 1: Section expressions are not correctly ordered in file."
+      r" $runtime == chrome on line 4 should come before $runtime == ff at "
+      r"line 1.");
+}
+
+void checkLintNormalizedSection_invalidOrderingWithNotEqual() {
+  expectError(
+      r"""
+[ $ runtime == chrome ]
+a_test: Pass
+
+[ $runtime != ff ]
+a_test: Pass
+
+[ $runtime == ff ]
+a_test: Pass
+""",
+      r"Error at line 4: Section expressions are not correctly ordered in file."
+      r" $runtime == ff on line 7 should come before $runtime != ff at line 4.");
+}
+
+void checkLintNormalizedSection_invalidOrderingWithNegation() {
+  expectError(
+      r"""
+[ ! $browser ]
+a_test: Pass
+
+[ ! $checked ]
+a_test: Pass
+
+[ $checked ]
+a_test: Pass
+
+""",
+      r"Error at line 4: Section expressions are not correctly ordered in file."
+      r" $checked on line 7 should come before !$checked at line 4.");
+}
+
+void checkLintNormalizedSection_correctOrdering() {
+  expectNoError(r"""
+[ ! $browser ]
+a_test: Pass
+
+[ $compiler == dart2js ]
+
+[ $compiler == dartk ]
+
+[ $checked ]
+a_test: Pass
+
+[ !$checked ]
+a_test: Pass
+
+[ $runtime == chrome ]
+a_test: Pass
+
+""");
+}