build: support for running clang-tidy (#26150)

DEPS

@@ -78,6 +78,8 @@ vars = {
     False,
   'checkout_google_benchmark':
     False,
+  'checkout_clang_tidy':
+    True,
 }
 
 deps = {

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)

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.

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",

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<SpawnAsyncResult> {
+  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<boolean> {
+  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<string[]> => {
+    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<string[]> {
+  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<boolean> {
+  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);
+    });
+}

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"