Revert "Enable clang-tidy for pre-push (opt-out), exclude `performanc…

…e-unnecessary-value-param`" (flutter#45020) Reverts flutter#44936
matanlurey · Aug 23, 2023 · 90d3893 · 90d3893
1 parent decc8f7
commit 90d3893
Show file tree

Hide file tree

Showing 10 changed files with 53 additions and 349 deletions.
diff --git a/.clang-tidy b/.clang-tidy
@@ -1,30 +1,27 @@
-# 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
+# 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"
 
 CheckOptions:
   - key:    modernize-use-default-member-init.UseAssignment

diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart
@@ -3,15 +3,14 @@
 // found in the LICENSE file.
 
 import 'dart:convert' show LineSplitter, jsonDecode;
-import 'dart:io' as io show Directory, File, stderr, stdout;
+import 'dart:io' as io show 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 = '''
@@ -48,8 +47,6 @@ 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.
@@ -59,8 +56,6 @@ 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,
@@ -70,8 +65,6 @@ class ClangTidy {
   }) :
     options = Options(
       buildCommandsPath: buildCommandsPath,
-      configPath: configPath,
-      excludeSlowChecks: excludeSlowChecks,
       checksArg: checksArg,
       lintAll: lintAll,
       lintHead: lintHead,
@@ -161,20 +154,10 @@ 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;
 
@@ -308,7 +291,6 @@ class ClangTidy {
   Future<_ComputeJobsResult> _computeJobs(
     List<Command> commands,
     Options options,
-    io.File? configPath,
   ) async {
     bool sawMalformed = false;
     final List<WorkerJob> jobs = <WorkerJob>[];
@@ -329,7 +311,7 @@ class ClangTidy {
           sawMalformed = true;
         case LintAction.lint:
           _outSink.writeln('🔶 linting $relativePath');
-          jobs.add(command.createLintJob(options, withPath: configPath));
+          jobs.add(command.createLintJob(options));
         case LintAction.skipThirdParty:
           _outSink.writeln('🔷 ignoring $relativePath (third_party)');
         case LintAction.skipMissing:
@@ -339,10 +321,6 @@ 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);

diff --git a/tools/clang_tidy/lib/src/command.dart b/tools/clang_tidy/lib/src/command.dart
@@ -131,12 +131,10 @@ class Command {
   }
 
   /// The job for the process runner for the lint needed for this command.
-  WorkerJob createLintJob(Options options, {io.File? withPath}) {
+  WorkerJob createLintJob(Options options) {
     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>[

diff --git a/tools/clang_tidy/lib/src/hooks_exclude.dart b/tools/clang_tidy/lib/src/hooks_exclude.dart
diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart
@@ -28,8 +28,6 @@ class Options {
     required this.buildCommandsPath,
     this.help = false,
     this.verbose = false,
-    this.configPath,
-    this.excludeSlowChecks = false,
     this.checksArg = '',
     this.lintAll = false,
     this.lintHead = false,
@@ -76,8 +74,6 @@ 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,
@@ -201,16 +197,6 @@ 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.',
@@ -226,12 +212,6 @@ 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;
 
@@ -291,13 +271,6 @@ 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.';
     }

diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart
@@ -6,7 +6,6 @@ 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;
@@ -149,44 +148,6 @@ 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>[
@@ -517,38 +478,5 @@ 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;
 }