DEV/flutter_flutter
1
0
Fork 0
mirror of https://github.com/flutter/flutter.git synced 2026-02-20 02:29:02 +08:00
Code Issues Packages Projects Releases Wiki Activity

Revert "Enable clang-tidy for pre-push (opt-out), exclude performance-unnecessary-value-param" (flutter/engine#45020)

Browse Source 
Reverts flutter/engine#44936
This commit is contained in:
Brandon DeRosier 2023-08-23 13:17:13 -07:00 committed by GitHub
parent ce517f3b2a
commit cf500170ce
10 changed files with 53 additions and 349 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
engine/src/flutter/.clang-tidy
View File

@ -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

26
engine/src/flutter/tools/clang_tidy/lib/clang_tidy.dart
View File

@ -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<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);

4
engine/src/flutter/tools/clang_tidy/lib/src/command.dart
View File

@ -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>[

40
engine/src/flutter/tools/clang_tidy/lib/src/hooks_exclude.dart
View File

@ -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<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;
}

27
engine/src/flutter/tools/clang_tidy/lib/src/options.dart
View File

@ -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.';
}

72
engine/src/flutter/tools/clang_tidy/test/clang_tidy_test.dart
View File

@ -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;
}

94
engine/src/flutter/tools/githooks/lib/src/pre_push_command.dart
View File

@ -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<bool> {
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<bool> checkResults = <bool>[
@ -40,40 +38,36 @@ class PrePushCommand extends Command<bool> {
return !checkResults.contains(false);
}
/// Different `host_xxx` targets are built depending on the host platform.
@visibleForTesting
static const List<String> supportedHostTargets = <String>[
'host_debug_unopt_arm64',
'host_debug_arm64',
'host_debug_unopt',
'host_debug',
];
Future<bool> _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<bool> {
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
/// <filename> <last modified>
/// 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<bool> _runFormatter(String flutterRoot, bool verbose) async {
io.stdout.writeln('Starting formatting checks.');
final Stopwatch sw = Stopwatch()..start();

2
engine/src/flutter/tools/githooks/pre-push
View File

@ -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',
]

2
engine/src/flutter/tools/githooks/pubspec.yaml
View File

@ -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

84
engine/src/flutter/tools/githooks/test/githooks_test.dart
View File

@ -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);
}
}