From 9707b7e6428032e23c3e27c33b09620cab41e7f6 Mon Sep 17 00:00:00 2001
From: Chris Bracken <chris@bracken.jp>
Date: Fri, 16 Oct 2020 12:59:37 -0700
Subject: [PATCH] Require that FLUTTER_NOLINT include issue link

This adds enforcement to the linter that all FLUTTER_NOLINT comments be
of the form:

    // FLUTTER_NOLINT: https://github.com/flutter/flutter/issue/ID

Every linter opt-out should have an associated bug describing what issue
it works around so that others can work on eliminating it, or at least
understanding the rationale and whether it's still relevant.

This also reduces the likelihood of inadvertent copy-pasting into new
files either because the author fails to spot it when copying the
copyright block from another file, or assumes that it's necessary for
some subcomponent of the engine.

Bug: https://github.com/flutter/flutter/issues/68273
---
 ci/bin/lint.dart | 102 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 29 deletions(-)

diff --git a/ci/bin/lint.dart b/ci/bin/lint.dart
index 321bd3d9989b3..695473265701a 100644
--- a/ci/bin/lint.dart
+++ b/ci/bin/lint.dart
@@ -17,7 +17,7 @@ import 'package:args/args.dart';
 import 'package:path/path.dart' as path;
 import 'package:process_runner/process_runner.dart';
 
-String _linterOutputHeader = '''
+const String _linterOutputHeader = '''
 ┌──────────────────────────┐
 │ Engine Clang Tidy Linter │
 └──────────────────────────┘
@@ -26,6 +26,8 @@ more information on addressing these issues please see:
 https://github.com/flutter/flutter/wiki/Engine-Clang-Tidy-Linter
 ''';
 
+const String issueUrlPrefix = 'https://github.com/flutter/flutter/issues';
+
 class Command {
   Directory directory = Directory('');
   String command = '';
@@ -109,23 +111,59 @@ File buildFileAsRepoFile(String buildFile, Directory repoPath) {
   return result;
 }
 
-Future<String> shouldIgnoreFile(File file) async {
-  if (path.split(file.path).contains('third_party')) {
-    return 'third_party';
-  } else {
-    final RegExp exp = RegExp(r'//\s*FLUTTER_NOLINT');
-    await for (String line
-        in file.openRead().transform(utf8.decoder).transform(const LineSplitter())) {
-      if (exp.hasMatch(line)) {
-        return 'FLUTTER_NOLINT';
-      } else if (line.isNotEmpty && line[0] != '\n' && line[0] != '/') {
-        // Quick out once we find a line that isn't empty or a comment.  The
-        // FLUTTER_NOLINT must show up before the first real code.
-        return '';
-      }
+/// Lint actions to apply to a file.
+enum LintAction {
+  /// Run the linter over the file.
+  lint,
+
+  /// Ignore files under third_party/.
+  skipThirdParty,
+
+  /// Ignore due to a well-formed FLUTTER_NOLINT comment.
+  skipNoLint,
+
+  /// Fail due to a malformed FLUTTER_NOLINT comment.
+  failMalformedNoLint,
+}
+
+bool isThirdPartyFile(File file) {
+  return path.split(file.path).contains('third_party');
+}
+
+Future<LintAction> getLintAction(File file) async {
+  if (isThirdPartyFile(file)) {
+    return LintAction.skipThirdParty;
+  }
+
+  // Check for FlUTTER_NOLINT at top of file.
+  final RegExp exp = RegExp('\/\/\\s*FLUTTER_NOLINT(: $issueUrlPrefix/\\d+)?');
+  final Stream<String> lines = file.openRead()
+    .transform(utf8.decoder)
+    .transform(const LineSplitter());
+  await for (String line in lines) {
+    final RegExpMatch match = exp.firstMatch(line);
+    if (match != null) {
+      return match.group(1) != null
+        ? LintAction.skipNoLint
+        : LintAction.failMalformedNoLint;
+    } else if (line.isNotEmpty && line[0] != '\n' && line[0] != '/') {
+      // Quick out once we find a line that isn't empty or a comment.  The
+      // FLUTTER_NOLINT must show up before the first real code.
+      return LintAction.lint;
     }
-    return '';
   }
+  return LintAction.lint;
+}
+
+WorkerJob createLintJob(Command command, String checks, String tidyPath) {
+  final String tidyArgs = calcTidyArgs(command);
+  final List<String> args = <String>[command.file.path, checks, '--'];
+  args.addAll(tidyArgs?.split(' ') ?? <String>[]);
+  return WorkerJob(
+    <String>[tidyPath, ...args],
+    workingDirectory: command.directory,
+    name: 'clang-tidy on ${command.file.path}',
+  );
 }
 
 void _usage(ArgParser parser, {int exitCode = 1}) {
@@ -226,19 +264,23 @@ void main(List<String> arguments) async {
   final List<WorkerJob> jobs = <WorkerJob>[];
   for (Command command in changedFileBuildCommands) {
     final String relativePath = path.relative(command.file.path, from: repoPath.parent.path);
-    final String ignoreReason = await shouldIgnoreFile(command.file);
-    if (ignoreReason.isEmpty) {
-      final String tidyArgs = calcTidyArgs(command);
-      final List<String> args = <String>[command.file.path, checks, '--'];
-      args.addAll(tidyArgs?.split(' ') ?? <String>[]);
-      print('🔶 linting $relativePath');
-      jobs.add(WorkerJob(
-        <String>[tidyPath, ...args],
-        workingDirectory: command.directory,
-        name: 'clang-tidy on ${command.file.path}',
-      ));
-    } else {
-      print('🔷 ignoring $relativePath ($ignoreReason)');
+    final LintAction action = await getLintAction(command.file);
+    switch (action) {
+      case LintAction.skipNoLint:
+        print('🔷 ignoring $relativePath (FLUTTER_NOLINT)');
+        break;
+      case LintAction.failMalformedNoLint:
+        print('❌ malformed opt-out $relativePath');
+        print('   Required format: // FLUTTER_NOLINT: $issueUrlPrefix/ISSUE_ID');
+        exitCode = 1;
+        break;
+      case LintAction.lint:
+        print('🔶 linting $relativePath');
+        jobs.add(createLintJob(command, checks, tidyPath));
+        break;
+      case LintAction.skipThirdParty:
+        print('🔷 ignoring $relativePath (third_party)');
+        break;
     }
   }
   final ProcessPool pool = ProcessPool();
@@ -260,5 +302,7 @@ void main(List<String> arguments) async {
   print('\n');
   if (exitCode == 0) {
     print('No lint problems found.');
+  } else {
+    print('Lint problems found.');
   }
 }