diff --git a/DEPS b/DEPS index 2ac49e7b31f9..d702ebbcb339 100644 --- a/DEPS +++ b/DEPS @@ -78,6 +78,8 @@ vars = { False, 'checkout_google_benchmark': False, + 'checkout_clang_tidy': + True, } deps = { diff --git a/docs/development/README.md b/docs/development/README.md index ca2ec645d343..f5c9d6073fa1 100644 --- a/docs/development/README.md +++ b/docs/development/README.md @@ -12,6 +12,7 @@ For guides on Electron app development, see * [Source Code Directory Structure](source-code-directory-structure.md) * [Coding Style](coding-style.md) * [Using clang-format on C++ Code](clang-format.md) +* [Using clang-tidy on C++ Code](clang-tidy.md) * [Build System Overview](build-system-overview.md) * [Build Instructions (macOS)](build-instructions-macos.md) * [Build Instructions (Windows)](build-instructions-windows.md) diff --git a/docs/development/clang-tidy.md b/docs/development/clang-tidy.md new file mode 100644 index 000000000000..21210b8f6c7b --- /dev/null +++ b/docs/development/clang-tidy.md @@ -0,0 +1,37 @@ +# Using clang-tidy on C++ Code + +[`clang-tidy`](https://clang.llvm.org/extra/clang-tidy/) is a tool to +automatically check C/C++/Objective-C code for style violations, programming +errors, and best practices. + +Electron's `clang-tidy` integration is provided as a linter script which can +be run with `npm run lint:clang-tidy`. While `clang-tidy` checks your on-disk +files, you need to have built Electron so that it knows which compiler flags +were used. There is one required option for the script `--output-dir`, which +tells the script which build directory to pull the compilation information +from. A typical usage would be: +`npm run lint:clang-tiy --out-dir ../out/Testing` + +With no filenames provided, all C/C++/Objective-C files will be checked. +You can provide a list of files to be checked by passing the filenames after +the options: +`npm run lint:clang-tiy --out-dir ../out/Testing shell/browser/api/electron_api_app.cc` + +While `clang-tidy` has a +[long list](https://clang.llvm.org/extra/clang-tidy/checks/list.html) +of possible checks, in Electron only a few are enabled by default. At the +moment Electron doesn't have a `.clang-tidy` config, so `clang-tidy` will +find the one from Chromium at `src/.clang-tidy` and use the checks which +Chromium has enabled. You can change which checks are run by using the +`--checks=` option. This is passed straight through to `clang-tidy`, so see +its documentation for full details. Wildcards can be used, and checks can +be disabled by prefixing a `-`. By default any checks listed are added to +those in `.clang-tidy`, so if you'd like to limit the checks to specific +ones you should first exclude all checks then add back what you want, like +`--checks=-*,performance*`. + +Running `clang-tidy` is rather slow - internally it compiles each file and +then runs the checks so it will always be some factor slower than compilation. +While you can use parallel runs to speed it up using the `--jobs|-j` option, +`clang-tidy` also uses a lot of memory during its checks, so it can easily +run into out-of-memory errors. As such the default number of jobs is one. diff --git a/package.json b/package.json index ab12d0250b40..82d55fdfd6cb 100644 --- a/package.json +++ b/package.json @@ -16,11 +16,14 @@ "@types/dirty-chai": "^2.0.2", "@types/express": "^4.17.7", "@types/fs-extra": "^9.0.1", + "@types/klaw": "^3.0.1", + "@types/minimist": "^1.2.0", "@types/mocha": "^7.0.2", "@types/node": "^14.6.2", "@types/semver": "^7.3.3", "@types/send": "^0.14.5", "@types/split": "^1.0.0", + "@types/stream-json": "^1.5.1", "@types/uuid": "^3.4.6", "@types/webpack": "^4.41.21", "@types/webpack-env": "^1.15.2", @@ -56,6 +59,7 @@ "semver": "^5.6.0", "shx": "^0.3.2", "standard-markdown": "^6.0.0", + "stream-json": "^1.7.1", "sumchecker": "^2.0.2", "tap-xunit": "^2.4.1", "temp": "^0.8.3", @@ -74,6 +78,7 @@ "lint": "node ./script/lint.js && npm run lint:clang-format && npm run lint:docs", "lint:js": "node ./script/lint.js --js", "lint:clang-format": "python script/run-clang-format.py -r -c chromium_src/ shell/ || (echo \"\\nCode not formatted correctly.\" && exit 1)", + "lint:clang-tidy": "ts-node ./script/run-clang-tidy.ts", "lint:cpp": "node ./script/lint.js --cc", "lint:objc": "node ./script/lint.js --objc", "lint:py": "node ./script/lint.js --py", diff --git a/script/run-clang-tidy.ts b/script/run-clang-tidy.ts new file mode 100644 index 000000000000..d3611e743f3c --- /dev/null +++ b/script/run-clang-tidy.ts @@ -0,0 +1,370 @@ +import chalk from 'chalk'; +import * as childProcess from 'child_process'; +import * as fs from 'fs'; +import * as klaw from 'klaw'; +import * as minimist from 'minimist'; +import * as os from 'os'; +import * as path from 'path'; +import * as streamChain from 'stream-chain'; +import * as streamJson from 'stream-json'; +import { ignore as streamJsonIgnore } from 'stream-json/filters/Ignore'; +import { streamArray as streamJsonStreamArray } from 'stream-json/streamers/StreamArray'; + +const SOURCE_ROOT = path.normalize(path.dirname(__dirname)); +const LLVM_BIN = path.resolve( + SOURCE_ROOT, + '..', + 'third_party', + 'llvm-build', + 'Release+Asserts', + 'bin' +); +const PLATFORM = os.platform(); + +type SpawnAsyncResult = { + stdout: string; + stderr: string; + status: number | null; +}; + +class ErrorWithExitCode extends Error { + exitCode: number; + + constructor (message: string, exitCode: number) { + super(message); + this.exitCode = exitCode; + } +} + +async function spawnAsync ( + command: string, + args: string[], + options?: childProcess.SpawnOptionsWithoutStdio | undefined +): Promise { + return new Promise((resolve, reject) => { + try { + const stdio = { stdout: '', stderr: '' }; + const spawned = childProcess.spawn(command, args, options || {}); + + spawned.stdout.on('data', (data) => { + stdio.stdout += data; + }); + + spawned.stderr.on('data', (data) => { + stdio.stderr += data; + }); + + spawned.on('exit', (code) => resolve({ ...stdio, status: code })); + spawned.on('error', (err) => reject(err)); + } catch (err) { + reject(err); + } + }); +} + +function getDepotToolsEnv (): NodeJS.ProcessEnv { + let depotToolsEnv; + + const findDepotToolsOnPath = () => { + const result = childProcess.spawnSync( + PLATFORM === 'win32' ? 'where' : 'which', + ['gclient'] + ); + + if (result.status === 0) { + return process.env; + } + }; + + const checkForBuildTools = () => { + const result = childProcess.spawnSync( + 'electron-build-tools', + ['show', 'env', '--json'], + { shell: true } + ); + + if (result.status === 0) { + return { + ...process.env, + ...JSON.parse(result.stdout.toString().trim()) + }; + } + }; + + try { + depotToolsEnv = findDepotToolsOnPath(); + if (!depotToolsEnv) depotToolsEnv = checkForBuildTools(); + } catch {} + + if (!depotToolsEnv) { + throw new Error("Couldn't find depot_tools, ensure it's on your PATH"); + } + + if (!('CHROMIUM_BUILDTOOLS_PATH' in depotToolsEnv)) { + throw new Error( + 'CHROMIUM_BUILDTOOLS_PATH environment variable must be set' + ); + } + + return depotToolsEnv; +} + +function chunkFilenames (filenames: string[], offset: number = 0): string[][] { + // Windows has a max command line length of 2047 characters, so we can't + // provide too many filenames without going over that. To work around that, + // chunk up a list of filenames such that it won't go over that limit when + // used as args. Use a much higher limit on other platforms which will + // effectively be a no-op. + const MAX_FILENAME_ARGS_LENGTH = + PLATFORM === 'win32' ? 2047 - offset : 100 * 1024; + + return filenames.reduce( + (chunkedFilenames: string[][], filename) => { + const currChunk = chunkedFilenames[chunkedFilenames.length - 1]; + const currChunkLength = currChunk.reduce( + (totalLength, _filename) => totalLength + _filename.length + 1, + 0 + ); + if (currChunkLength + filename.length + 1 > MAX_FILENAME_ARGS_LENGTH) { + chunkedFilenames.push([filename]); + } else { + currChunk.push(filename); + } + return chunkedFilenames; + }, + [[]] + ); +} + +async function runClangTidy ( + outDir: string, + filenames: string[], + checks: string = '', + jobs: number = 1 +): Promise { + const cmd = path.resolve(LLVM_BIN, 'clang-tidy'); + const args = [`-p=${outDir}`]; + + if (checks) args.push(`--checks=${checks}`); + + // Remove any files that aren't in the compilation database to prevent + // errors from cluttering up the output. Since the compilation DB is hundreds + // of megabytes, this is done with streaming to not hold it all in memory. + const filterCompilationDatabase = (): Promise => { + const compiledFilenames: string[] = []; + + return new Promise((resolve) => { + const pipeline = streamChain.chain([ + fs.createReadStream(path.resolve(outDir, 'compile_commands.json')), + streamJson.parser(), + streamJsonIgnore({ filter: /\bcommand\b/i }), + streamJsonStreamArray(), + ({ value: { file, directory } }) => { + const filename = path.resolve(directory, file); + return filenames.includes(filename) ? filename : null; + } + ]); + + pipeline.on('data', (data) => compiledFilenames.push(data)); + pipeline.on('end', () => resolve(compiledFilenames)); + }); + }; + + // clang-tidy can figure out the file from a short relative filename, so + // to get the most bang for the buck on the command line, let's trim the + // filenames to the minimum so that we can fit more per invocation + filenames = (await filterCompilationDatabase()).map((filename) => + path.relative(SOURCE_ROOT, filename) + ); + + if (filenames.length === 0) { + throw new Error('No filenames to run'); + } + + const commandLength = + cmd.length + args.reduce((length, arg) => length + arg.length, 0); + + const results: boolean[] = []; + const asyncWorkers = []; + const chunkedFilenames: string[][] = []; + + const filesPerWorker = Math.ceil(filenames.length / jobs); + + for (let i = 0; i < jobs; i++) { + chunkedFilenames.push( + ...chunkFilenames(filenames.splice(0, filesPerWorker), commandLength) + ); + } + + const worker = async () => { + let filenames = chunkedFilenames.shift(); + + while (filenames) { + results.push( + await spawnAsync(cmd, [...args, ...filenames], {}).then((result) => { + // We lost color, so recolorize because it's much more legible + // There's a --use-color flag for clang-tidy but it has no effect + // on Windows at the moment, so just recolor for everyone + let state = null; + + for (const line of result.stdout.split('\n')) { + if (line.includes(' warning: ')) { + console.log( + line + .split(' warning: ') + .map((part) => chalk.whiteBright(part)) + .join(chalk.magentaBright(' warning: ')) + ); + state = 'code-line'; + } else if (line.includes(' note: ')) { + const lineParts = line.split(' note: '); + lineParts[0] = chalk.whiteBright(lineParts[0]); + console.log(lineParts.join(chalk.grey(' note: '))); + state = 'code-line'; + } else if (line.startsWith('error:')) { + console.log( + chalk.redBright('error: ') + line.split(' ').slice(1).join(' ') + ); + } else if (state === 'code-line') { + console.log(line); + state = 'post-code-line'; + } else if (state === 'post-code-line') { + console.log(chalk.greenBright(line)); + } else { + console.log(line); + } + } + + if (result.status !== 0) { + console.error(result.stderr); + } + + // On a clean run there's nothing on stdout. A run with warnings-only + // will have a status code of zero, but there's output on stdout + return result.status === 0 && result.stdout.length === 0; + }) + ); + + filenames = chunkedFilenames.shift(); + } + }; + + for (let i = 0; i < jobs; i++) { + asyncWorkers.push(worker()); + } + + try { + await Promise.all(asyncWorkers); + return results.every((x) => x); + } catch { + return false; + } +} + +async function findMatchingFiles ( + top: string, + test: (filename: string) => boolean +): Promise { + return new Promise((resolve) => { + const matches = [] as string[]; + klaw(top, { + filter: (f) => path.basename(f) !== '.bin' + }) + .on('end', () => resolve(matches)) + .on('data', (item) => { + if (test(item.path)) { + matches.push(item.path); + } + }); + }); +} + +function parseCommandLine () { + const showUsage = (arg?: string) : boolean => { + if (!arg || arg.startsWith('-')) { + console.log( + 'Usage: script/run-clang-tidy.ts [-h|--help] [--jobs|-j] ' + + '[--checks] --out-dir OUTDIR [file1 file2]' + ); + process.exit(0); + } + + return true; + }; + + const opts = minimist(process.argv.slice(2), { + boolean: ['help'], + string: ['checks', 'out-dir'], + default: { jobs: 1 }, + alias: { help: 'h', jobs: 'j' }, + stopEarly: true, + unknown: showUsage + }); + + if (opts.help) showUsage(); + + if (!opts['out-dir']) { + console.log('--out-dir is a required argunment'); + process.exit(0); + } + + return opts; +} + +async function main (): Promise { + const opts = parseCommandLine(); + const outDir = path.resolve(opts['out-dir']); + + if (!fs.existsSync(outDir)) { + throw new Error("Output directory doesn't exist"); + } else { + // Make sure the compile_commands.json file is up-to-date + const env = getDepotToolsEnv(); + + const result = childProcess.spawnSync( + 'gn', + ['gen', '.', '--export-compile-commands'], + { cwd: outDir, env, shell: true } + ); + + if (result.status !== 0) { + if (result.error) { + console.error(result.error.message); + } else { + console.error(result.stderr.toString()); + } + + throw new ErrorWithExitCode( + 'Failed to automatically generate compile_commands.json for ' + + 'output directory', + 2 + ); + } + } + + const filenames = []; + + if (opts._.length > 0) { + filenames.push(...opts._.map((filename) => path.resolve(filename))); + } else { + filenames.push( + ...(await findMatchingFiles( + path.resolve(SOURCE_ROOT, 'shell'), + (filename: string) => /.*\.(?:cc|h|mm)$/.test(filename) + )) + ); + } + + return runClangTidy(outDir, filenames, opts.checks, opts.jobs); +} + +if (require.main === module) { + main() + .then((success) => { + process.exit(success ? 0 : 1); + }) + .catch((err: ErrorWithExitCode) => { + console.error(`ERROR: ${err.message}`); + process.exit(err.exitCode || 1); + }); +} diff --git a/yarn.lock b/yarn.lock index 317189a7dd13..e620d48d7956 100644 --- a/yarn.lock +++ b/yarn.lock @@ -321,6 +321,11 @@ dependencies: "@types/node" "*" +"@types/klaw@^3.0.1": + version "3.0.1" + resolved "https://registry.yarnpkg.com/@types/klaw/-/klaw-3.0.1.tgz#29f90021c0234976aa4eb97efced9cb6db9fa8b3" + integrity sha512-acnF3n9mYOr1aFJKFyvfNX0am9EtPUsYPq22QUCGdJE+MVt6UyAN1jwo+PmOPqXD4K7ZS9MtxDEp/un0lxFccA== + "@types/linkify-it@*": version "2.1.0" resolved "https://registry.yarnpkg.com/@types/linkify-it/-/linkify-it-2.1.0.tgz#ea3dd64c4805597311790b61e872cbd1ed2cd806" @@ -348,6 +353,11 @@ resolved "https://registry.yarnpkg.com/@types/minimatch/-/minimatch-3.0.3.tgz#3dca0e3f33b200fc7d1139c0cd96c1268cadfd9d" integrity sha512-tHq6qdbT9U1IRSGf14CL0pUlULksvY9OZ+5eEgl1N7t+OA3tGvNpxJCzuKQlsNgCVwbAs670L1vcVQi8j9HjnA== +"@types/minimist@^1.2.0": + version "1.2.0" + resolved "https://registry.yarnpkg.com/@types/minimist/-/minimist-1.2.0.tgz#69a23a3ad29caf0097f06eda59b361ee2f0639f6" + integrity sha1-aaI6OtKcrwCX8G7aWbNh7i8GOfY= + "@types/mocha@^7.0.2": version "7.0.2" resolved "https://registry.yarnpkg.com/@types/mocha/-/mocha-7.0.2.tgz#b17f16cf933597e10d6d78eae3251e692ce8b0ce" @@ -427,6 +437,21 @@ "@types/node" "*" "@types/through" "*" +"@types/stream-chain@*": + version "2.0.0" + resolved "https://registry.yarnpkg.com/@types/stream-chain/-/stream-chain-2.0.0.tgz#aed7fc21ac3686bc721aebbbd971f5a857e567e4" + integrity sha512-O3IRJcZi4YddlS8jgasH87l+rdNmad9uPAMmMZCfRVhumbWMX6lkBWnIqr9kokO5sx8LHp8peQ1ELhMZHbR0Gg== + dependencies: + "@types/node" "*" + +"@types/stream-json@^1.5.1": + version "1.5.1" + resolved "https://registry.yarnpkg.com/@types/stream-json/-/stream-json-1.5.1.tgz#ae8d1133f9f920e18c6e94b233cb57d014a47b8d" + integrity sha512-Blg6GJbKVEB1J/y/2Tv+WrYiMzPTIqyuZ+zWDJtAF8Mo8A2XQh/lkSX4EYiM+qtS+GY8ThdGi6gGA9h4sjvL+g== + dependencies: + "@types/node" "*" + "@types/stream-chain" "*" + "@types/tapable@*": version "1.0.4" resolved "https://registry.yarnpkg.com/@types/tapable/-/tapable-1.0.4.tgz#b4ffc7dc97b498c969b360a41eee247f82616370" @@ -7317,6 +7342,11 @@ stream-browserify@^2.0.1: inherits "~2.0.1" readable-stream "^2.0.2" +stream-chain@^2.2.3: + version "2.2.3" + resolved "https://registry.yarnpkg.com/stream-chain/-/stream-chain-2.2.3.tgz#44cfa21ab673e53a3f1691b3d1665c3aceb1983b" + integrity sha512-w+WgmCZ6BItPAD3/4HD1eDiDHRLhjSSyIV+F0kcmmRyz8Uv9hvQF22KyaiAUmOlmX3pJ6F95h+C191UbS8Oe/g== + stream-each@^1.1.0: version "1.2.3" resolved "https://registry.yarnpkg.com/stream-each/-/stream-each-1.2.3.tgz#ebe27a0c389b04fbcc233642952e10731afa9bae" @@ -7336,6 +7366,13 @@ stream-http@^2.7.2: to-arraybuffer "^1.0.0" xtend "^4.0.0" +stream-json@^1.7.1: + version "1.7.1" + resolved "https://registry.yarnpkg.com/stream-json/-/stream-json-1.7.1.tgz#ec7e414c2eba456c89a4b4e5223794eabc3860c4" + integrity sha512-I7g0IDqvdJXbJ279/D3ZoTx0VMhmKnEF7u38CffeWdF8bfpMPsLo+5fWnkNjO2GU/JjWaRjdH+zmH03q+XGXFw== + dependencies: + stream-chain "^2.2.3" + stream-shift@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/stream-shift/-/stream-shift-1.0.0.tgz#d5c752825e5367e786f78e18e445ea223a155952"