Enable clang-tidy in CI (#12213)

Summary:
At long last, we will have clang-tidy enabled in CI. For a while I thought I could clean up the project enough to enable clang-tidy with all checks enabled, but I figure it's smarter to set up the minimal checks and at least have those in CI. We can fix more going forward.

ezyang apaszke
Pull Request resolved: https://github.com/pytorch/pytorch/pull/12213

Differential Revision: D10183069

Pulled By: goldsborough

fbshipit-source-id: 7ecd2d368258f46efe23a2449c0a206d10f3a769

.clang-tidy

@@ -1,51 +1,11 @@
 ---
 # NOTE: there must be no spaces before the '-', so put the comma first.
 Checks: '
-  *
-  ,clang-analyzer-*
-  ,modernize-*
-  ,-cert-dcl21-cpp
-  ,-cert-err58-cpp
-  ,-cert-err60-cpp
-  ,-clang-diagnostic-*
-  ,-cppcoreguidelines-owning-memory
-  ,-cppcoreguidelines-pro-bounds-array-to-pointer-decay
-  ,-cppcoreguidelines-pro-bounds-constant-array-index
-  ,-cppcoreguidelines-pro-type-member-init
-  ,-cppcoreguidelines-pro-type-static-cast-downcast
-  ,-cppcoreguidelines-pro-type-union-access
-  ,-cppcoreguidelines-pro-type-vararg
-  ,-cppcoreguidelines-special-member-functions
-  ,-fuchsia-*
-  ,-google-build-using-namespace
-  ,-google-default-arguments
-  ,-google-explicit-constructor
-  ,-google-readability-braces-around-statements
-  ,-google-readability-namespace-comments
-  ,-google-readability-todo
-  ,-google-runtime-references
-  ,-google-runtime-references
-  ,-hicpp-braces-around-statements
-  ,-hicpp-explicit-conversions
-  ,-hicpp-member-init
-  ,-hicpp-no-array-decay
-  ,-hicpp-signed-bitwise
-  ,-hicpp-special-member-functions
-  ,-hicpp-vararg
-  ,-llvm-header-guard
-  ,-llvm-include-order
-  ,-llvm-namespace-comment
-  ,-misc-unused-parameters
-  ,-modernize-make-unique
-  ,-modernize-use-default-member-init
-  ,-performance-unnecessary-value-param
-  ,-readability-braces-around-statements
-  ,-readability-else-after-return
-  ,-readability-implicit-bool-conversion
-  ,-readability-named-parameter
+  -*
+  ,modernize-deprecated-headers
   '
-WarningsAsErrors: ''
-HeaderFilterRegex: 'torch/csrc/'
+WarningsAsErrors: '*'
+HeaderFilterRegex: 'torch/csrc/.*'
 AnalyzeTemporaryDtors: false
 CheckOptions:
 ...

.travis.yml

@@ -29,3 +29,12 @@ matrix:
       - env: CPP_DOC_CHECK
         install: sudo apt-get install -y doxygen
         script: cd docs/cpp/source && ./check-doxygen.sh
+      - env: CLANG_TIDY
+        python: "3.6"
+        addons:
+          apt:
+            sources:
+              - ubuntu-toolchain-r-test
+              - llvm-toolchain-trusty-6.0
+            packages: clang-tidy-6.0
+        script: tools/run-clang-tidy-in-ci.sh -e "$(which clang-tidy-6.0)"

CMakeLists.txt

@@ -10,6 +10,8 @@ if (NOT MSVC)
   set(CMAKE_C_STANDARD 11)
 endif()
 
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+
 # One variable that determines whether the current cmake process is being run
 # with the main Caffe2 library. This is useful for building modules - if
 # modules are built with the main Caffe2 library then one does not need to do

CONTRIBUTING.md

@@ -354,6 +354,36 @@ static_assert(std::is_same(A*, decltype(A::singelton()))::value, "hmm");
   are too large.  Splitting such files into separate files helps.
   (Example: `THTensorMath`, `THTensorMoreMath`, `THTensorEvenMoreMath`.)
 
+### Running Clang-Tidy
+
+[Clang-Tidy](https://clang.llvm.org/extra/clang-tidy/index.html) is a C++
+linter and static analysis tool based on the clang compiler. We run clang-tidy
+in our CI to make sure that new C++ code is safe, sane and efficient. See our
+[.travis.yml](https://github.com/pytorch/pytorch/blob/master/.travis.yml) file
+for the simple commands we use for this.
+
+To run clang-tidy locally, follow these steps:
+
+1. Install clang-tidy. First, check if you already have clang-tidy by simply
+writing `clang-tidy` in your terminal. If you don't yet have clang-tidy, you
+should be able to install it easily with your package manager, e.g. by writing
+`apt-get install clang-tidy` on Ubuntu. See https://apt.llvm.org for details on
+how to install the latest version. Note that newer versions of clang-tidy will
+have more checks than older versions. In our CI, we run clang-tidy-6.0.
+
+2. Use our driver script to run clang-tidy over any changes relative to some
+   git revision (you may want to replace `HEAD~1` with `HEAD` to pick up
+   uncommitted changes). Changes are picked up based on a `git diff` with the
+   given revision:
+  ```sh
+  $ python tools/clang_tidy.py -d build -p torch/csrc -r HEAD~1
+  ```
+
+Above, it is assumed you are in the PyTorch root folder. `path/to/build` should
+be the path to where you built PyTorch from source, e.g. `build` in the PyTorch
+root folder if you used `setup.py build`. You can use `-c <clang-tidy-binary>`
+to change the clang-tidy this script uses.
+
 ## Caffe2 notes
 
 In 2018, we merged Caffe2 into the PyTorch source repository.  While the

tools/autograd/templates/Functions.cpp

@@ -1,3 +1,10 @@
+// NB: Must be at the top of file to avoid including the deprecated "math.h".
+// https://stackoverflow.com/questions/6563810/m-pi-works-with-math-h-but-not-with-cmath-in-visual-studio
+#ifdef _MSC_VER
+#define _USE_MATH_DEFINES
+#include <cmath>
+#endif
+
 #include "Functions.h"
 #include <ATen/Utils.h>
 #include <ATen/core/TensorOptions.h>
@@ -6,12 +13,7 @@
 #include <ATen/ExpandUtils.h>
 #include <ATen/core/Reduction.h>
 
-// define constants like M_PI and C keywords for MSVC
-#ifdef _MSC_VER
-#define _USE_MATH_DEFINES
 #include <ciso646>
-#endif
-#include <math.h>
 #include <algorithm>
 #include <numeric>
 

tools/build_pytorch_libs.bat

@@ -201,7 +201,6 @@ goto:eof
                   -DMKLDNN_LIBRARY="%MKLDNN_LIBRARY%" ^
                   -DATEN_NO_CONTRIB=1 ^
                   -DCMAKE_INSTALL_PREFIX="%INSTALL_DIR%" ^
-                  -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ^
                   -DCMAKE_C_FLAGS="%USER_CFLAGS%" ^
                   -DCMAKE_CXX_FLAGS="/EHa %USER_CFLAGS%" ^
                   -DCMAKE_EXE_LINKER_FLAGS="%USER_LDFLAGS%" ^

tools/build_pytorch_libs.sh

@@ -197,7 +197,7 @@ function build() {
 		       -DCMAKE_DEBUG_POSTFIX="" \
 		       -DCMAKE_BUILD_TYPE=$BUILD_TYPE \
 		       ${@:2} \
-		       -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ${CMAKE_ARGS[@]}
+		       ${CMAKE_ARGS[@]}
   fi
   ${CMAKE_INSTALL} -j"$MAX_JOBS"
   popd
@@ -301,7 +301,6 @@ function build_caffe2() {
 		       -DMKLDNN_LIB_DIR=$MKLDNN_LIB_DIR \
 		       -DMKLDNN_LIBRARY=$MKLDNN_LIBRARY \
 		       -DCMAKE_INSTALL_PREFIX="$INSTALL_DIR" \
-		       -DCMAKE_EXPORT_COMPILE_COMMANDS=1 \
 		       -DCMAKE_C_FLAGS="$USER_CFLAGS" \
 		       -DCMAKE_CXX_FLAGS="$USER_CFLAGS" \
 		       -DCMAKE_EXE_LINKER_FLAGS="$LDFLAGS $USER_LDFLAGS" \

tools/clang_tidy.py

@@ -1,111 +1,91 @@
 #!/usr/bin/env python
+"""
+A driver script to run clang-tidy on changes detected via git.
+
+By default, clang-tidy runs on all files you point it at. This means that even
+if you changed only parts of that file, you will get warnings for the whole
+file. This script has the ability to ask git for the exact lines that have
+changed since a particular git revision, and makes clang-tidy only lint those.
+This makes it much less overhead to integrate in CI and much more relevant to
+developers. This git-enabled mode is optional, and full scans of a directory
+tree are also possible. In both cases, the script allows filtering files via
+glob or regular expressions.
+"""
 
 import argparse
+import fnmatch
 import json
 import os.path
 import re
+import shlex
 import subprocess
 import sys
 
 
-DEFAULT_FILE_PATTERN = r".*\.[ch](pp)?"
+# NOTE: Clang-tidy cannot lint headers directly, because headers are not
+# compiled -- translation units are, of which there is one per implementation
+# (c/cc/cpp) file.
+DEFAULT_FILE_PATTERN = re.compile(r".*\.c(c|pp)?")
 
 # @@ -start,count +start,count @@
 CHUNK_PATTERN = r"^@@\s+-\d+,\d+\s+\+(\d+)(?:,(\d+))?\s+@@"
 
+# Set from command line arguments in main().
+VERBOSE = False
 
-def run_shell_command(arguments, process_name=None):
+
+def run_shell_command(arguments):
     """Executes a shell command."""
-    assert len(arguments) > 0
-    try:
-        output = subprocess.check_output(arguments, stderr=subprocess.STDOUT)
-    except OSError:
-        _, e, _ = sys.exc_info()
-        process_name = process_name or arguments[0]
-        raise RuntimeError("Error executing {}: {}".format(process_name, e))
-    else:
-        return output.decode()
+    if VERBOSE:
+        print(" ".join(arguments))
+    result = subprocess.run(arguments, stdout=subprocess.PIPE)
+    output = result.stdout.decode().strip()
+    if result.returncode != 0:
+        raise RuntimeError("Error executing {}: {}".format(" ".join(arguments), output))
 
-
-def normalize_directory_path(path):
-    """Normalizes a directory path."""
-    return path.rstrip('/')
-
-
-def transform_globs_into_regexes(globs):
-    """Turns glob patterns into regular expressions."""
-    return [glob.replace("*", ".*").replace("?", ".") for glob in globs]
+    return output
 
 
 def get_file_patterns(globs, regexes):
     """Returns a list of compiled regex objects from globs and regex pattern strings."""
-    regexes += transform_globs_into_regexes(globs)
-    if not regexes:
-        regexes = [DEFAULT_FILE_PATTERN]
-    return [re.compile(regex + "$") for regex in regexes]
+    # fnmatch.translate converts a glob into a regular expression.
+    # https://docs.python.org/2/library/fnmatch.html#fnmatch.translate
+    regexes += [fnmatch.translate(glob) for glob in globs]
+    return [re.compile(regex) for regex in regexes] or [DEFAULT_FILE_PATTERN]
 
 
-def git_diff(args, verbose):
-    """Executes a git diff command in the shell and returns its output."""
-    # --no-pager gets us the plain output, without pagination.
-    # --no-color removes color codes.
-    command = ["git", "--no-pager", "diff", "--no-color"] + args
-    if verbose:
-        print(" ".join(command))
-    return run_shell_command(command, process_name="git diff")
-
-
-def filter_files(files, file_patterns, verbose):
+def filter_files(files, file_patterns):
     """Returns all files that match any of the patterns."""
-    filtered = []
     for file in files:
         has_match = False
         for pattern in file_patterns:
-            if pattern.search(file):
-                filtered.append(file)
+            if pattern.match(file):
+                yield file
                 has_match = True
-        if not has_match and verbose:
-            message = "{} does not match any ".format(file)
-            message += "file pattern in {{{}}}".format(', '.join(map(str, file_patterns)))
-            print(message)
-    return filtered
+        if not has_match and VERBOSE:
+            message = "{} does not match any file pattern in {{{}}}"
+            print(message.format(file, ", ".join(map(str, file_patterns))))
 
 
-def remove_recursive_files(files, paths, verbose):
-    """
-    Removes all files that are not immediately under one of the given paths.
-    """
-    for file in files:
-        if os.path.dirname(file) in paths:
-            yield file
-        else:
-            if verbose:
-
-                message = "{} ({}) does not match any ".format(file, os.path.dirname(file))
-                message += "non-recursive path in {{{}}}".format(", ".join(paths))
-                print(message)
-
-
-def get_changed_files(revision, paths, verbose):
+def get_changed_files(revision, paths):
     """Runs git diff to get the paths of all changed files."""
     # --diff-filter AMU gets us files that are (A)dded, (M)odified or (U)nmerged (in the working copy).
     # --name-only makes git diff return only the file paths, without any of the source changes.
-    args = ["--diff-filter", "AMU", "--ignore-all-space", "--name-only", revision]
-    output = git_diff(args + paths, verbose)
+    command = "git diff-index --diff-filter=AMU --ignore-all-space --name-only"
+    output = run_shell_command(shlex.split(command) + [revision] + paths)
     return output.split("\n")
 
 
 def get_all_files(paths):
-    """Yields all files in any of the given paths"""
-    for path in paths:
-        for root, _, files in os.walk(path):
-            for file in files:
-                yield os.path.join(root, file)
+    """Returns all files that are tracked by git in the given paths."""
+    output = run_shell_command(["git", "ls-files"] + paths)
+    return output.split("\n")
 
 
-def get_changed_lines(revision, filename, verbose):
+def get_changed_lines(revision, filename):
     """Runs git diff to get the line ranges of all file changes."""
-    output = git_diff(["--unified=0", revision, filename], verbose)
+    command = shlex.split("git diff-index --unified=0") + [revision, filename]
+    output = run_shell_command(command)
     changed_lines = []
     for chunk in re.finditer(CHUNK_PATTERN, output, re.MULTILINE):
         start = int(chunk.group(1))
@@ -126,58 +106,56 @@ def run_clang_tidy(options, line_filters, files):
         with open(options.config_file) as config:
             # Here we convert the YAML config file to a JSON blob.
             command += ["-config", json.dumps(yaml.load(config))]
-    if options.checks:
-        command += ["-checks", options.checks]
     if line_filters:
         command += ["-line-filter", json.dumps(line_filters)]
-    command += ["-{}".format(arg) for arg in options.extra_args]
+    command += options.extra_args
     command += files
 
-    if options.verbose:
-        print(" ".join(command))
-    if options.show_command_only:
+    if options.dry_run:
         command = [re.sub(r"^([{[].*[]}])$", r"'\1'", arg) for arg in command]
         return " ".join(command)
 
-    return run_shell_command(command)
+    output = run_shell_command(command)
+    if not options.keep_going and "[clang-diagnostic-error]" in output:
+        message = "Found clang-diagnostic-errors in clang-tidy output: {}"
+        raise RuntimeError(message.format(output))
+
+    return output
 
 
 def parse_options():
+    """Parses the command line options."""
     parser = argparse.ArgumentParser(description="Run Clang-Tidy (on your Git changes)")
     parser.add_argument(
-        "-c",
+        "-e",
         "--clang-tidy-exe",
         default="clang-tidy",
         help="Path to clang-tidy executable",
     )
-    parser.add_argument(
-        "-e",
-        "--extra-args",
-        nargs="+",
-        default=[],
-        help="Extra arguments to forward to clang-tidy, without the hypen (e.g. -e 'header-filter=\"path\"')",
-    )
     parser.add_argument(
         "-g",
         "--glob",
         nargs="+",
         default=[],
-        help="File patterns as UNIX globs (support * and ?, not recursive **)",
+        help="Only lint files that match these glob patterns "
+        "(see documentation for `fnmatch` for supported syntax)",
     )
     parser.add_argument(
         "-x",
         "--regex",
         nargs="+",
         default=[],
-        help="File patterns as regular expressions",
+        help="Only lint files that match these regular expressions (from the start of the filename)",
     )
     parser.add_argument(
-        "-d",
+        "-c",
         "--compile-commands-dir",
-        default=".",
+        default="build",
         help="Path to the folder containing compile_commands.json",
     )
-    parser.add_argument("-r", "--revision", help="Git revision to get changes from")
+    parser.add_argument(
+        "-d", "--diff", help="Git revision to diff against to get changes"
+    )
     parser.add_argument(
         "-p",
         "--paths",
@@ -187,13 +165,7 @@ def parse_options():
     )
     parser.add_argument(
         "-n",
-        "--no-recursive",
-        action="store_true",
-        help="If paths are supplied with -p/--paths, do not recurse into paths",
-    )
-    parser.add_argument(
-        "-s",
-        "--show-command-only",
+        "--dry-run",
         action="store_true",
         help="Only show the command to be executed, without running it",
     )
@@ -203,22 +175,33 @@ def parse_options():
         help="Path to a clang-tidy config file. Defaults to '.clang-tidy'.",
     )
     parser.add_argument(
-        "--checks", help="Appends checks to those from the config file (if any)"
+        "-k",
+        "--keep-going",
+        action="store_true",
+        help="Don't error on compiler errors (clang-diagnostic-error)",
+    )
+    parser.add_argument(
+        "extra_args", nargs="*", help="Extra arguments to forward to clang-tidy"
     )
     return parser.parse_args()
 
 
 def main():
     options = parse_options()
-    paths = list(map(normalize_directory_path, options.paths))
-    if options.revision:
-        files = get_changed_files(options.revision, paths, options.verbose)
+
+    # This flag is pervasive enough to set it globally. It makes the code
+    # cleaner compared to threading it through every single function.
+    global VERBOSE
+    VERBOSE = options.verbose
+
+    # Normalize the paths first.
+    paths = [path.rstrip("/") for path in options.paths]
+    if options.diff:
+        files = get_changed_files(options.diff, paths)
     else:
         files = get_all_files(paths)
-    if options.no_recursive:
-        files = remove_recursive_files(files, paths, options.verbose)
     file_patterns = get_file_patterns(options.glob, options.regex)
-    files = filter_files(files, file_patterns, options.verbose)
+    files = list(filter_files(files, file_patterns))
 
     # clang-tidy error's when it does not get input files.
     if not files:
@@ -226,12 +209,8 @@ def main():
         sys.exit()
 
     line_filters = []
-    if options.revision:
-        for filename in files:
-            changed_lines = get_changed_lines(
-                options.revision, filename, options.verbose
-            )
-            line_filters.append(changed_lines)
+    if options.diff:
+        line_filters = [get_changed_lines(options.diff, f) for f in files]
 
     print(run_clang_tidy(options, line_filters, files))
 

tools/run-clang-tidy-in-ci.sh

@@ -0,0 +1,31 @@
+#!/bin/bash
+
+set -ex
+
+if [[ ! -d build ]]; then
+  git submodule update --init --recursive
+
+  mkdir build
+  pushd build
+  # We really only need compile_commands.json, so no need to build!
+  time cmake -DBUILD_TORCH=ON ..
+  popd
+
+  # Generate ATen files.
+  time python aten/src/ATen/gen.py \
+    -s aten/src/ATen \
+    -d build/aten/src/ATen \
+    aten/src/ATen/Declarations.cwrap \
+    aten/src/THNN/generic/THNN.h \
+    aten/src/THCUNN/generic/THCUNN.h \
+    aten/src/ATen/nn.yaml \
+    aten/src/ATen/native/native_functions.yaml
+
+  # Generate PyTorch files.
+  time python tools/setup_helpers/generate_code.py            \
+    --declarations-path build/aten/src/ATen/Declarations.yaml \
+    --nn-path aten/src
+fi
+
+# Run Clang-Tidy
+time python tools/clang_tidy.py -vp torch/csrc -d HEAD~1 "$@"

torch/CMakeLists.txt

@@ -7,6 +7,7 @@ else()
   project(torch CXX C)
   find_package(Caffe2 REQUIRED)
   option(USE_CUDA "Use CUDA" ON)
+  set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 endif()
 
 option(BUILD_TEST "Build torch test binaries" ON)

torch/csrc/autograd/profiler.h

@@ -22,7 +22,7 @@
 #include <cuda_runtime.h>
 #endif
 #ifndef _WIN32
-#include <time.h>
+#include <ctime>
 #endif
 
 namespace torch { namespace autograd {

torch/csrc/jit/interned_strings.cpp

@@ -8,7 +8,7 @@
 #include <vector>
 #include "ATen/core/Error.h"
 #include "ATen/core/optional.h"
-#include "string.h"
+#include <cstring>
 #include "torch/csrc/jit/interned_strings_class.h"
 
 namespace torch { namespace jit {

torch/csrc/jit/interned_strings_class.h

@@ -7,7 +7,7 @@
 #include <vector>
 #include "ATen/core/Error.h"
 #include "ATen/core/optional.h"
-#include "string.h"
+#include <cstring>
 #include "torch/csrc/jit/interned_strings.h"
 
 namespace torch {