Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable clang-tidy for pre-push (opt-out), exclude performance-unnecessary-value-param #44936

Merged
merged 15 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 27 additions & 24 deletions .clang-tidy
Original file line number Diff line number Diff line change
@@ -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
Expand Down
26 changes: 24 additions & 2 deletions tools/clang_tidy/lib/clang_tidy.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '''
Expand Down Expand Up @@ -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.
Expand All @@ -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,
Expand All @@ -65,6 +70,8 @@ class ClangTidy {
}) :
options = Options(
buildCommandsPath: buildCommandsPath,
configPath: configPath,
excludeSlowChecks: excludeSlowChecks,
checksArg: checksArg,
lintAll: lintAll,
lintHead: lintHead,
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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>[];
Expand All @@ -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:
Expand All @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion tools/clang_tidy/lib/src/command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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>[
Expand Down
40 changes: 40 additions & 0 deletions tools/clang_tidy/lib/src/hooks_exclude.dart
Original file line number Diff line number Diff line change
@@ -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;
}
27 changes: 27 additions & 0 deletions tools/clang_tidy/lib/src/options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.',
Expand All @@ -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;

Expand Down Expand Up @@ -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.';
}
Expand Down
72 changes: 72 additions & 0 deletions tools/clang_tidy/test/clang_tidy_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>[
Expand Down Expand Up @@ -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;
}
Loading