diff --git a/.clang-tidy b/.clang-tidy index 716f63fbb6f1e..0bc769e37ee76 100644 --- a/.clang-tidy +++ b/.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 diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index c0bffa0afc126..5dac58af7629f 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 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; @@ -11,6 +11,7 @@ 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 jobs = computeJobsResult.jobs; @@ -291,6 +308,7 @@ class ClangTidy { Future<_ComputeJobsResult> _computeJobs( List commands, Options options, + io.File? configPath, ) async { bool sawMalformed = false; final List jobs = []; @@ -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 _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 41dbea5914592..b19c19375b3b4 100644 --- a/tools/clang_tidy/lib/src/command.dart +++ b/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 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 new file mode 100644 index 0000000000000..b0c9f8d28c89a --- /dev/null +++ b/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 _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 401d34b8e81bc..02f3c2e6888e8 100644 --- a/tools/clang_tidy/lib/src/options.dart +++ b/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 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.'; } diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 9b2d6d0e160e7..9a24cccd93dc5 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/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 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( [ @@ -478,5 +517,38 @@ 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 6b456cbe53ebe..166c91bb77b8b 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -6,6 +6,7 @@ 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 @@ -24,8 +25,9 @@ class PrePushCommand extends Command { final String flutterRoot = globalResults!['flutter']! as String; if (!enableClangTidy) { - print('The clang-tidy check is disabled. To enable set the environment ' - 'variable PRE_PUSH_CLANG_TIDY to any value.'); + print( + 'The clang-tidy check was explicitly disabled. To enable clear ' + 'the environment variable PRE_PUSH_CLANG_TIDY or set it to true.'); } final List checkResults = [ @@ -38,36 +40,40 @@ 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_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; - } + + // 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; } + + // 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, ); @@ -81,6 +87,46 @@ 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 0f629a08c8704..2505421b5fc25 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 None: + if ENABLE_CLANG_TIDY is not False: githook_args += [ '--enable-clang-tidy', ] diff --git a/tools/githooks/pubspec.yaml b/tools/githooks/pubspec.yaml index 6b5bb5c2340a7..2ea2b17c7520b 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-0 <4.0.0' + sdk: '>=3.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 2b347d31dd20f..87d75219720d1 100644 --- a/tools/githooks/test/githooks_test.dart +++ b/tools/githooks/test/githooks_test.dart @@ -5,7 +5,9 @@ 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 { @@ -66,4 +68,86 @@ 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); + } }