Enable clang-tidy for pre-push (opt-out), exclude `performance-unnece…

…ssary-value-param` (#44936)

Closes flutter/flutter#132687.

Zach, this is a pretty naive attempt, so feel free to suggest alternatives. I considered for example just using `--checks=-{{LINT}}` as well, but figured that might be less discoverable (especially for non-core folks) than just a file called `.clang-tidy-for-githooks`.

/cc @jonahwilliams

.clang-tidy

@@ -1,27 +1,30 @@
-# Prefix check with "-" to ignore.
-# Note: Some of the checks here are used as errors selectively, see
-# //ci/lint.sh
-Checks: "bugprone-use-after-move,\
-bugprone-unchecked-optional-access,\
-clang-analyzer-*,\
-clang-diagnostic-*,\
-darwin-*,\
-google-*,\
-modernize-use-default-member-init,\
-objc-*,\
--objc-nsinvocation-argument-lifetime,\
-readability-identifier-naming,\
--google-build-using-namespace,\
--google-default-arguments,\
--google-objc-global-variable-declaration,\
--google-objc-avoid-throwing-exception,\
--google-readability-casting,\
--clang-analyzer-nullability.NullPassedToNonnull,\
--clang-analyzer-nullability.NullablePassedToNonnull,\
--clang-analyzer-nullability.NullReturnedFromNonnull,\
--clang-analyzer-nullability.NullableReturnedFromNonnull,\
-performance-move-const-arg,\
-performance-unnecessary-value-param"
+# These checks are run by the CI presubmit script, but are not run by the
+# githooks presubmit script. The githooks presubmit script runs a subset of
+# these checks.
+#
+# See "./tools/clang_tidy/lib/src/hooks_exclude.dart".
+Checks: >-
+  bugprone-use-after-move,
+  bugprone-unchecked-optional-access,
+  clang-analyzer-*,
+  clang-diagnostic-*,
+  darwin-*,
+  google-*,
+  modernize-use-default-member-init,
+  objc-*,
+  -objc-nsinvocation-argument-lifetime,
+  readability-identifier-naming,
+  -google-build-using-namespace,
+  -google-default-arguments,
+  -google-objc-global-variable-declaration,
+  -google-objc-avoid-throwing-exception,
+  -google-readability-casting,
+  -clang-analyzer-nullability.NullPassedToNonnull,
+  -clang-analyzer-nullability.NullablePassedToNonnull,
+  -clang-analyzer-nullability.NullReturnedFromNonnull,
+  -clang-analyzer-nullability.NullableReturnedFromNonnull,
+  performance-move-const-arg,
+  performance-unnecessary-value-param
 
 CheckOptions:
   - key:    modernize-use-default-member-init.UseAssignment

tools/clang_tidy/lib/clang_tidy.dart

@@ -3,14 +3,15 @@
 // found in the LICENSE file.
 
 import 'dart:convert' show LineSplitter, jsonDecode;
-import 'dart:io' as io show File, stderr, stdout;
+import 'dart:io' as io show Directory, File, stderr, stdout;
 
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 import 'package:process_runner/process_runner.dart';
 
 import 'src/command.dart';
 import 'src/git_repo.dart';
+import 'src/hooks_exclude.dart';
 import 'src/options.dart';
 
 const String _linterOutputHeader = '''
@@ -47,6 +48,8 @@ class ClangTidy {
   /// an instance of [ClangTidy].
   ///
   /// `buildCommandsPath` is the path to the build_commands.json file.
+  /// `configPath` is a path to a `.clang-tidy` config file.
+  /// `excludeSlowChecks` when true indicates that slow checks should be skipped.
   /// `repoPath` is the path to the Engine repo.
   /// `checksArg` are specific checks for clang-tidy to do.
   /// `lintAll` when true indicates that all files should be linted.
@@ -56,6 +59,8 @@ class ClangTidy {
   /// will otherwise go to stderr.
   ClangTidy({
     required io.File buildCommandsPath,
+    io.File? configPath,
+    bool excludeSlowChecks = false,
     String checksArg = '',
     bool lintAll = false,
     bool lintHead = false,
@@ -65,6 +70,8 @@ class ClangTidy {
   }) :
     options = Options(
       buildCommandsPath: buildCommandsPath,
+      configPath: configPath,
+      excludeSlowChecks: excludeSlowChecks,
       checksArg: checksArg,
       lintAll: lintAll,
       lintHead: lintHead,
@@ -154,10 +161,20 @@ class ClangTidy {
       );
     }
 
+    io.File? configPath = options.configPath;
+    io.Directory? rewriteDir;
+    if (configPath != null && options.excludeSlowChecks) {
+      configPath = _createClangTidyConfigExcludingSlowLints(configPath);
+      rewriteDir = io.Directory(path.dirname(configPath.path));
+    }
     final _ComputeJobsResult computeJobsResult = await _computeJobs(
       changedFileBuildCommands,
       options,
+      configPath,
     );
+    if (rewriteDir != null) {
+      rewriteDir.deleteSync(recursive: true);
+    }
     final int computeResult = computeJobsResult.sawMalformed ? 1 : 0;
     final List<WorkerJob> jobs = computeJobsResult.jobs;
 
@@ -291,6 +308,7 @@ class ClangTidy {
   Future<_ComputeJobsResult> _computeJobs(
     List<Command> commands,
     Options options,
+    io.File? configPath,
   ) async {
     bool sawMalformed = false;
     final List<WorkerJob> jobs = <WorkerJob>[];
@@ -311,7 +329,7 @@ class ClangTidy {
           sawMalformed = true;
         case LintAction.lint:
           _outSink.writeln('🔶 linting $relativePath');
-          jobs.add(command.createLintJob(options));
+          jobs.add(command.createLintJob(options, withPath: configPath));
         case LintAction.skipThirdParty:
           _outSink.writeln('🔷 ignoring $relativePath (third_party)');
         case LintAction.skipMissing:
@@ -321,6 +339,10 @@ class ClangTidy {
     return _ComputeJobsResult(jobs, sawMalformed);
   }
 
+  static io.File _createClangTidyConfigExcludingSlowLints(io.File configPath) {
+    return rewriteClangTidyConfig(configPath);
+  }
+
   static Iterable<String> _trimGenerator(String output) sync* {
     const LineSplitter splitter = LineSplitter();
     final List<String> lines = splitter.convert(output);

tools/clang_tidy/lib/src/command.dart

@@ -131,10 +131,12 @@ class Command {
   }
 
   /// The job for the process runner for the lint needed for this command.
-  WorkerJob createLintJob(Options options) {
+  WorkerJob createLintJob(Options options, {io.File? withPath}) {
     final List<String> args = <String>[
       filePath,
       '--warnings-as-errors=${options.warningsAsErrors ?? '*'}',
+      if (options.configPath != null)
+        '--config-file=${withPath != null ? withPath.path : options.configPath}',
       if (options.checks != null)
         options.checks!,
       if (options.fix) ...<String>[

tools/clang_tidy/lib/src/hooks_exclude.dart

@@ -0,0 +1,40 @@
+// Copyright 2013 The Flutter Authors. 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' as io show Directory, File;
+
+import 'package:path/path.dart' as path;
+
+// TODO(matanl): Replace this file by embedding in //.clang-tidy directly.
+//
+// By bringing in package:yaml, we can instead embed this in a key in the
+// .clang-tidy file, read the checks in, and create a new .clang-tidy file (i.e.
+// in /tmp/.../.clang-tidy) with the checks we want to run.
+//
+// However that requires a bit more work, so for now we just have this file.
+const List<String> _kExcludeChecks = <String>[
+  'performance-unnecessary-value-param',
+];
+
+/// Given a `.clang-tidy` file, rewrites it to exclude non-performant checks.
+///
+/// Returns a path to the rewritten file.
+io.File rewriteClangTidyConfig(io.File input) {
+  // Because the file is YAML, and we aren't using a YAML package to parse it,
+  // instead we'll carefully remove the name of the check, optionally followed
+  // by a comma and a newline.
+  String contents = input.readAsStringSync();
+
+  for (final String check in _kExcludeChecks) {
+    // \s+{{CHECK}},?\n, with {{CHECK}} escaped for regex.
+    final RegExp checkRegex = RegExp(r'\s+' + check + r',?\n');
+    contents = contents.replaceAll(checkRegex, '');
+  }
+
+  final io.Directory tmpDir = io.Directory.systemTemp.createTempSync('clang_tidy');
+  final io.File output = io.File(path.join(tmpDir.path, '.clang-tidy-for-githooks'));
+  output.writeAsStringSync(contents);
+
+  return output;
+}

tools/clang_tidy/lib/src/options.dart

@@ -28,6 +28,8 @@ class Options {
     required this.buildCommandsPath,
     this.help = false,
     this.verbose = false,
+    this.configPath,
+    this.excludeSlowChecks = false,
     this.checksArg = '',
     this.lintAll = false,
     this.lintHead = false,
@@ -74,6 +76,8 @@ class Options {
       help: options['help'] as bool,
       verbose: options['verbose'] as bool,
       buildCommandsPath: buildCommandsPath,
+      configPath: options.wasParsed('config-file') ? io.File(options['config-file'] as String) : null,
+      excludeSlowChecks: options['exclude-slow-checks'] as bool,
       checksArg: options.wasParsed('checks') ? options['checks'] as String : '',
       lintAll: io.Platform.environment['FLUTTER_LINT_ALL'] != null ||
                options['lint-all'] as bool,
@@ -197,6 +201,16 @@ class Options {
             'string, indicating all checks should be performed.',
       defaultsTo: '',
     )
+    ..addOption(
+      'config-file',
+      help: 'Path to a .clang-tidy configuration file.',
+      valueHelp: 'path/to/.clang-tidy',
+    )
+    ..addFlag(
+      'exclude-slow-checks',
+      help: 'Exclude checks that are too slow for git hooks.',
+      negatable: false,
+    )
     ..addFlag(
       'enable-check-profile',
       help: 'Enable per-check timing profiles and print a report to stderr.',
@@ -212,6 +226,12 @@ class Options {
   /// The location of the compile_commands.json file.
   final io.File buildCommandsPath;
 
+  /// A location of a `.clang-tidy` configuration file.
+  final io.File? configPath;
+
+  /// Whether to exclude lints that are too slow for git hooks.
+  final bool excludeSlowChecks;
+
   /// The location of shard compile_commands.json files.
   final List<io.File> shardCommandsPaths;
 
@@ -271,6 +291,13 @@ class Options {
       return 'ERROR: --compile-commands option cannot be used with --target-variant.';
     }
 
+    if (argResults.wasParsed('config-file')) {
+      final io.File configPath = io.File(argResults['config-file'] as String);
+      if (!configPath.existsSync()) {
+        return 'ERROR: Config file ${configPath.path} does not exist.';
+      }
+    }
+
     if (compileCommandsParsed && argResults.wasParsed('src-dir')) {
       return 'ERROR: --compile-commands option cannot be used with --src-dir.';
     }

tools/clang_tidy/test/clang_tidy_test.dart

@@ -6,6 +6,7 @@ import 'dart:io' as io show Directory, File, Platform, stderr;
 
 import 'package:clang_tidy/clang_tidy.dart';
 import 'package:clang_tidy/src/command.dart';
+import 'package:clang_tidy/src/hooks_exclude.dart';
 import 'package:clang_tidy/src/options.dart';
 import 'package:litetest/litetest.dart';
 import 'package:path/path.dart' as path;
@@ -148,6 +149,44 @@ Future<int> main(List<String> args) async {
     print(outBuffer);
   });
 
+  test('Accepts --config-file', () async {
+    // If buildCommands is in "$ENGINE/src/out/host_debug", then the config
+    // file should be in "$ENGINE/src/flutter/.clang-tidy-for-githooks".
+    late final String flutterRoot;
+
+    // Find the 'src' directory and append 'flutter/.clang-tidy-for-githooks'.
+    {
+      final List<String> buildCommandParts = path.split(path.absolute(buildCommands));
+      for (int i = 0; i < buildCommandParts.length; ++i) {
+        if (buildCommandParts[i] == 'src') {
+          flutterRoot = path.joinAll(<String>[
+            ...buildCommandParts.sublist(0, i + 1),
+            'flutter',
+          ]);
+          break;
+        }
+      }
+    }
+
+    final StringBuffer outBuffer = StringBuffer();
+    final StringBuffer errBuffer = StringBuffer();
+    final ClangTidy clangTidy = ClangTidy.fromCommandLine(
+      <String>[
+        '--compile-commands',
+        buildCommands,
+        '--config-file=${path.join(flutterRoot, '.clang-tidy')}',
+      ],
+      outSink: outBuffer,
+      errSink: errBuffer,
+    );
+
+    final int result = await clangTidy.run();
+
+    expect(result, equals(0));
+    expect(clangTidy.options.configPath?.path, endsWith('.clang-tidy'));
+    print(outBuffer);
+  });
+
   test('shard-id valid', () async {
     _withTempFile('shard-id-valid', (String path) {
       final Options options = Options.fromCommandLine( <String>[
@@ -478,5 +517,38 @@ Future<int> main(List<String> args) async {
     expect(lintAction, equals(LintAction.lint));
   });
 
+  test('rewriteClangTidyConfig should remove "performance-unnecessary-value-param"', () {
+    final io.Directory tmpDir = io.Directory.systemTemp.createTempSync('clang_tidy_test');
+
+    final io.File input = io.File(path.join(tmpDir.path, '.clang-tidy'));
+    input.writeAsStringSync('''
+Checks: >-
+  bugprone-use-after-move,
+  bugprone-unchecked-optional-access,
+  clang-analyzer-*,
+  clang-diagnostic-*,
+  darwin-*,
+  google-*,
+  modernize-use-default-member-init,
+  objc-*,
+  -objc-nsinvocation-argument-lifetime,
+  readability-identifier-naming,
+  -google-build-using-namespace,
+  -google-default-arguments,
+  -google-objc-global-variable-declaration,
+  -google-objc-avoid-throwing-exception,
+  -google-readability-casting,
+  -clang-analyzer-nullability.NullPassedToNonnull,
+  -clang-analyzer-nullability.NullablePassedToNonnull,
+  -clang-analyzer-nullability.NullReturnedFromNonnull,
+  -clang-analyzer-nullability.NullableReturnedFromNonnull,
+  performance-move-const-arg,
+  performance-unnecessary-value-param
+    ''');
+
+    final io.File output = rewriteClangTidyConfig(input);
+    expect(output.readAsStringSync().contains('performance-unnecessary-value-param'), isFalse);
+  });
+
   return 0;
 }