From 93b05467adced1d473ca34c716977c480e58899b Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Wed, 23 Aug 2023 13:17:13 -0700 Subject: [PATCH] Revert "Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param`" (#45020) Reverts flutter/engine#44936 --- .clang-tidy | 51 +++++------ tools/clang_tidy/lib/clang_tidy.dart | 26 +----- tools/clang_tidy/lib/src/command.dart | 4 +- tools/clang_tidy/lib/src/hooks_exclude.dart | 40 --------- tools/clang_tidy/lib/src/options.dart | 27 ------ tools/clang_tidy/test/clang_tidy_test.dart | 72 --------------- tools/githooks/lib/src/pre_push_command.dart | 94 +++++--------------- tools/githooks/pre-push | 2 +- tools/githooks/pubspec.yaml | 2 +- tools/githooks/test/githooks_test.dart | 84 ----------------- 10 files changed, 53 insertions(+), 349 deletions(-) delete mode 100644 tools/clang_tidy/lib/src/hooks_exclude.dart diff --git a/.clang-tidy b/.clang-tidy index 0bc769e37ee76..716f63fbb6f1e 100644 --- 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 index 5dac58af7629f..c0bffa0afc126 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -3,7 +3,7 @@ // 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; @@ -11,7 +11,6 @@ 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 jobs = computeJobsResult.jobs; @@ -308,7 +291,6 @@ class ClangTidy { Future<_ComputeJobsResult> _computeJobs( List commands, Options options, - io.File? configPath, ) async { bool sawMalformed = false; final List jobs = []; @@ -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 _trimGenerator(String output) sync* { const LineSplitter splitter = LineSplitter(); final List lines = splitter.convert(output); diff --git a/tools/clang_tidy/lib/src/command.dart b/tools/clang_tidy/lib/src/command.dart index b19c19375b3b4..41dbea5914592 100644 --- 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 args = [ 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) ...[ diff --git a/tools/clang_tidy/lib/src/hooks_exclude.dart b/tools/clang_tidy/lib/src/hooks_exclude.dart deleted file mode 100644 index b0c9f8d28c89a..0000000000000 --- a/tools/clang_tidy/lib/src/hooks_exclude.dart +++ /dev/null @@ -1,40 +0,0 @@ -// 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 _kExcludeChecks = [ - '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; -} diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart index 02f3c2e6888e8..401d34b8e81bc 100644 --- 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 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 index 9a24cccd93dc5..9b2d6d0e160e7 100644 --- 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 main(List 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 buildCommandParts = path.split(path.absolute(buildCommands)); - for (int i = 0; i < buildCommandParts.length; ++i) { - if (buildCommandParts[i] == 'src') { - flutterRoot = path.joinAll([ - ...buildCommandParts.sublist(0, i + 1), - 'flutter', - ]); - break; - } - } - } - - final StringBuffer outBuffer = StringBuffer(); - final StringBuffer errBuffer = StringBuffer(); - final ClangTidy clangTidy = ClangTidy.fromCommandLine( - [ - '--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( [ @@ -517,38 +478,5 @@ Future main(List 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; } diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index 166c91bb77b8b..6b456cbe53ebe 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -6,7 +6,6 @@ import 'dart:io' as io; import 'package:args/command_runner.dart'; import 'package:clang_tidy/clang_tidy.dart'; -import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; /// The command that implements the pre-push githook @@ -25,9 +24,8 @@ class PrePushCommand extends Command { final String flutterRoot = globalResults!['flutter']! as String; if (!enableClangTidy) { - print( - 'The clang-tidy check was explicitly disabled. To enable clear ' - 'the environment variable PRE_PUSH_CLANG_TIDY or set it to true.'); + print('The clang-tidy check is disabled. To enable set the environment ' + 'variable PRE_PUSH_CLANG_TIDY to any value.'); } final List checkResults = [ @@ -40,40 +38,36 @@ class PrePushCommand extends Command { return !checkResults.contains(false); } - /// Different `host_xxx` targets are built depending on the host platform. - @visibleForTesting - static const List supportedHostTargets = [ - 'host_debug_unopt_arm64', - 'host_debug_arm64', - 'host_debug_unopt', - 'host_debug', - ]; - Future _runClangTidy(String flutterRoot, bool verbose) async { io.stdout.writeln('Starting clang-tidy checks.'); final Stopwatch sw = Stopwatch()..start(); - - // First ensure that out/host_{{flags}}/compile_commands.json exists by running - // //flutter/tools/gn. See _checkForHostTargets above for supported targets. - final io.File? compileCommands = findMostRelevantCompileCommands(flutterRoot, verbose: verbose); - if (compileCommands == null) { - io.stderr.writeln( - 'clang-tidy requires a fully built host directory, such as: ' - '${supportedHostTargets.join(', ')}.' - ); - return false; + // First ensure that out/host_debug/compile_commands.json exists by running + // //flutter/tools/gn. + io.File compileCommands = io.File(path.join( + flutterRoot, + '..', + 'out', + 'host_debug', + 'compile_commands.json', + )); + if (!compileCommands.existsSync()) { + compileCommands = io.File(path.join( + flutterRoot, + '..', + 'out', + 'host_debug_unopt', + 'compile_commands.json', + )); + if (!compileCommands.existsSync()) { + io.stderr.writeln('clang-tidy requires a fully built host_debug or ' + 'host_debug_unopt build directory'); + return false; + } } - - // Because we are using a heuristic to pick a host build directory, we - // should print some debug information explaining which directory we picked. - io.stdout.writeln('Using compile_commands.json from ${compileCommands.parent.path}'); - final StringBuffer outBuffer = StringBuffer(); final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( buildCommandsPath: compileCommands, - configPath: io.File(path.join(flutterRoot, '.clang-tidy-for-githooks')), - excludeSlowChecks: true, outSink: outBuffer, errSink: errBuffer, ); @@ -87,46 +81,6 @@ class PrePushCommand extends Command { return true; } - /// Returns the most recent `compile_commands.json` for the given root. - /// - /// For example, if the following builds exist with the following timestamps: - /// - /// ```txt - /// - /// out/host_debug_unopt_arm64/compile_commands.json 1/1/2023 - /// out/host_debug_arm64/compile_commands.json 1/2/2023 - /// out/host_debug_unopt/compile_commands.json 1/3/2023 - /// out/host_debug/compile_commands.json 1/4/2023 - /// ``` - /// - /// ... then the returned file will be `out/host_debug/compile_commands.json`. - @visibleForTesting - static io.File? findMostRelevantCompileCommands(String flutterRoot, {required bool verbose}) { - final String engineRoot = path.normalize(path.join(flutterRoot, '../')); - - // Create a list of all the compile_commands.json files that exist, - // including their last modified time. - final List<(io.File, DateTime)> compileCommandsFiles = supportedHostTargets - .map((String target) => io.File(path.join(engineRoot, 'out', target, 'compile_commands.json'))) - .where((io.File file) => file.existsSync()) - .map((io.File file) => (file, file.lastModifiedSync())) - .toList(); - - // Sort the list by last modified time, most recent first. - compileCommandsFiles.sort(((io.File, DateTime) a, (io.File, DateTime) b) => b.$2.compareTo(a.$2)); - - // If there are more than one entry, and we're in verbose mode, explain. - if (verbose && compileCommandsFiles.length > 1) { - io.stdout.writeln('Found multiple compile_commands.json files. Using the most recent one.'); - for (final (io.File file, DateTime lastModified) in compileCommandsFiles) { - io.stdout.writeln(' ${file.path} (last modified: $lastModified)'); - } - } - - // Return the first file in the list, or null if the list is empty. - return compileCommandsFiles.firstOrNull?.$1; - } - Future _runFormatter(String flutterRoot, bool verbose) async { io.stdout.writeln('Starting formatting checks.'); final Stopwatch sw = Stopwatch()..start(); diff --git a/tools/githooks/pre-push b/tools/githooks/pre-push index 2505421b5fc25..0f629a08c8704 100755 --- a/tools/githooks/pre-push +++ b/tools/githooks/pre-push @@ -23,7 +23,7 @@ def Main(argv): FLUTTER_DIR, ] - if ENABLE_CLANG_TIDY is not False: + if ENABLE_CLANG_TIDY is not None: githook_args += [ '--enable-clang-tidy', ] diff --git a/tools/githooks/pubspec.yaml b/tools/githooks/pubspec.yaml index 2ea2b17c7520b..6b5bb5c2340a7 100644 --- a/tools/githooks/pubspec.yaml +++ b/tools/githooks/pubspec.yaml @@ -5,7 +5,7 @@ name: githooks publish_to: none environment: - sdk: '>=3.0.0 <4.0.0' + sdk: '>=3.0.0-0 <4.0.0' # Do not add any dependencies that require more than what is provided in # //third_party.pkg, //third_party/dart/pkg, or diff --git a/tools/githooks/test/githooks_test.dart b/tools/githooks/test/githooks_test.dart index 87d75219720d1..2b347d31dd20f 100644 --- a/tools/githooks/test/githooks_test.dart +++ b/tools/githooks/test/githooks_test.dart @@ -5,9 +5,7 @@ import 'dart:io' as io; import 'package:githooks/githooks.dart'; -import 'package:githooks/src/pre_push_command.dart'; import 'package:litetest/litetest.dart'; -import 'package:path/path.dart' as path; void main() { test('Fails gracefully without a command', () async { @@ -68,86 +66,4 @@ void main() { } expect(result, equals(1)); }); - - group('findMostRelevantCompileCommands', () { - late io.Directory fakeEngineRoot; - late io.Directory fakeFlutterRoot; - - // We can't use standard setUp because this package uses 'litetest'. - void setUp() { - fakeEngineRoot = io.Directory.systemTemp.createTempSync('flutter_tools_githooks_test'); - fakeFlutterRoot = io.Directory(path.join(fakeEngineRoot.path, 'flutter')); - fakeFlutterRoot.createSync(recursive: true); - } - - void createHostFor(String target, {DateTime? lastModified}) { - final io.Directory host = io.Directory(path.join(fakeEngineRoot.path, 'out', target)); - host.createSync(recursive: true); - - final io.File compileCommands = io.File(path.join(host.path, 'compile_commands.json')); - compileCommands.createSync(); - if (lastModified != null) { - compileCommands.setLastModifiedSync(lastModified); - } - } - - test('returns null if there are no built outputs', () { - setUp(); - - expect( - PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false), - isNull, - ); - }); - - test('returns the most recently modified compile_commands.json', () { - setUp(); - - // Assume host_debug_unopt was created on 8/5, and then *_arm64 on 8/6. - createHostFor('host_debug_unopt', lastModified: DateTime(2023, 8, 5)); - createHostFor('host_debug_unopt_arm64', lastModified: DateTime(2023, 8, 6)); - - expect( - PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false)!.path, - equals(path.join(fakeEngineRoot.path, 'out', 'host_debug_unopt_arm64', 'compile_commands.json')), - ); - }); - - test('in verbose mode, if there are multiple outputs, prints all of them', () { - final StringBuffer outBuffer = StringBuffer(); - io.IOOverrides.runZoned(() { - setUp(); - - createHostFor('host_debug_unopt', lastModified: DateTime(2023, 8, 5)); - createHostFor('host_debug_unopt_arm64', lastModified: DateTime(2023, 8, 6)); - PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: true); - }, stdout: () => _BufferedStdOut(outBuffer)); - - final String outString = outBuffer.toString(); - expect(outString, contains('out/host_debug_unopt/compile_commands.json')); - expect(outString, contains('out/host_debug_unopt_arm64/compile_commands.json')); - }); - }); -} - -final class _BufferedStdOut implements io.Stdout { - _BufferedStdOut(this.buffer); - - final StringBuffer buffer; - - // We don't need to implement any other methods. - @override - dynamic noSuchMethod(Invocation invocation) { - return super.noSuchMethod(invocation); - } - - @override - void write(Object? obj) { - buffer.write(obj); - } - - @override - void writeln([Object? obj = '']) { - buffer.writeln(obj); - } }