Refactor clang-tidy script to use actual compiler flags and includes (#2133)

Co-authored-by: Otto winter <otto@otto-winter.com>

.clang-tidy

@@ -14,7 +14,12 @@ Checks: >-
   -cert-str34-c,
   -clang-analyzer-optin.cplusplus.UninitializedObject,
   -clang-analyzer-osx.*,
+  -clang-diagnostic-delete-abstract-non-virtual-dtor,
+  -clang-diagnostic-delete-non-abstract-non-virtual-dtor,
   -clang-diagnostic-shadow-field,
+  -clang-diagnostic-sign-compare,
+  -clang-diagnostic-unused-variable,
+  -clang-diagnostic-unused-const-variable,
   -cppcoreguidelines-avoid-c-arrays,
   -cppcoreguidelines-avoid-goto,
   -cppcoreguidelines-avoid-magic-numbers,
@@ -80,7 +85,6 @@ Checks: >-
   -readability-use-anyofallof,
   -warnings-as-errors
 WarningsAsErrors: '*'
-HeaderFilterRegex: '^.*/src/esphome/.*'
 AnalyzeTemporaryDtors: false
 FormatStyle:     google
 CheckOptions:

.github/workflows/ci.yml

@@ -36,10 +36,6 @@ jobs:
     container: ghcr.io/esphome/esphome-lint:1.1
     steps:
       - uses: actions/checkout@v2
-      # Set up the pio project so that the cpp checks know how files are compiled
-      # (build flags, libraries etc)
-      - name: Set up platformio environment
-        run: pio init --ide atom
 
       - name: Register problem matchers
         run: |

.gitignore

@@ -125,4 +125,5 @@ config/
 tests/build/
 tests/.esphome/
 /.temp-clang-tidy.cpp
+/.temp/
 .pio/

.vscode/tasks.json

@@ -10,7 +10,7 @@
     {
       "label": "clang-tidy",
       "type": "shell",
-      "command": "test -f .gcc-flags.json || pio init --silent --ide atom; ./script/clang-tidy",
+      "command": "./script/clang-tidy",
       "problemMatcher": [
         {
           "owner": "clang-tidy",

esphome/components/sgp30/sgp30.cpp

@@ -47,7 +47,7 @@ void SGP30Component::setup() {
   }
   this->serial_number_ = (uint64_t(raw_serial_number[0]) << 24) | (uint64_t(raw_serial_number[1]) << 16) |
                          (uint64_t(raw_serial_number[2]));
-  ESP_LOGD(TAG, "Serial Number: %llu", this->serial_number_);
+  ESP_LOGD(TAG, "Serial Number: %" PRIu64, this->serial_number_);
 
   // Featureset identification for future use
   if (!this->write_command_(SGP30_CMD_GET_FEATURESET)) {
@@ -245,7 +245,7 @@ void SGP30Component::dump_config() {
         break;
     }
   } else {
-    ESP_LOGCONFIG(TAG, "  Serial number: %llu", this->serial_number_);
+    ESP_LOGCONFIG(TAG, "  Serial number: %" PRIu64, this->serial_number_);
     if (this->eco2_baseline_ != 0x0000 && this->tvoc_baseline_ != 0x0000) {
       ESP_LOGCONFIG(TAG, "  Baseline:");
       ESP_LOGCONFIG(TAG, "    eCO2 Baseline: 0x%04X", this->eco2_baseline_);

esphome/components/sgp40/sgp40.cpp

@@ -23,7 +23,7 @@ void SGP40Component::setup() {
   }
   this->serial_number_ = (uint64_t(raw_serial_number[0]) << 24) | (uint64_t(raw_serial_number[1]) << 16) |
                          (uint64_t(raw_serial_number[2]));
-  ESP_LOGD(TAG, "Serial Number: %llu", this->serial_number_);
+  ESP_LOGD(TAG, "Serial Number: %" PRIu64, this->serial_number_);
 
   // Featureset identification for future use
   if (!this->write_command_(SGP40_CMD_GET_FEATURESET)) {
@@ -248,7 +248,7 @@ void SGP40Component::dump_config() {
         break;
     }
   } else {
-    ESP_LOGCONFIG(TAG, "  Serial number: %llu", this->serial_number_);
+    ESP_LOGCONFIG(TAG, "  Serial number: %" PRIu64, this->serial_number_);
     ESP_LOGCONFIG(TAG, "  Minimum Samples: %f", VOC_ALGORITHM_INITIAL_BLACKOUT);
   }
   LOG_UPDATE_INTERVAL(this);

esphome/components/sntp/sntp_component.cpp

@@ -8,6 +8,11 @@
 #include "sntp.h"
 #endif
 
+// Yes, the server names are leaked, but that's fine.
+#ifdef CLANG_TIDY
+#define strdup(x) (const_cast<char *>(x))
+#endif
+
 namespace esphome {
 namespace sntp {
 

platformio.ini

@@ -1,12 +1,28 @@
-; This file is so that the C++ files in this repo
-; can be edited with IDEs like VSCode or CLion
-; with the platformio system
+; This PlatformIO project is for development purposes *only*: clang-tidy derives its compilation
+; database from here, and IDEs like CLion and VSCode also use it. This does not actually create a
+; usable binary.
 ; It's *not* used during runtime.
 
 [platformio]
-default_envs = livingroom8266
+default_envs = esp8266
 src_dir = .
-include_dir = include
+include_dir =
+
+[runtime]
+; This are the flags as set by the runtime.
+build_flags =
+    -Wno-unused-variable
+    -Wno-unused-but-set-variable
+    -Wno-sign-compare
+
+[clangtidy]
+; This are the flags for clang-tidy.
+build_flags =
+    -Wall
+    -Wunreachable-code
+    -Wfor-loop-analysis
+    -Wshadow-field
+    -Wshadow-field-in-constructor
 
 [common]
 lib_deps =
@@ -19,17 +35,13 @@ lib_deps =
     6865@1.0.0  ; TM1651 Battery Display
     6306@1.0.3  ; HM3301
 build_flags =
-    -fno-exceptions
-    -Wno-sign-compare
-    -Wno-unused-but-set-variable
-    -Wno-unused-variable
-    -DCLANG_TIDY
     -DESPHOME_LOG_LEVEL=ESPHOME_LOG_LEVEL_VERY_VERBOSE
 src_filter =
     +<esphome>
     +<tests/dummy_main.cpp>
+    +<.temp/all-include.cpp>
 
-[env:livingroom8266]
+[common:esp8266]
 ; use Arduino framework v2.4.2 for clang-tidy (latest 2.5.2 breaks static code analysis, see #760)
 platform = platformio/espressif8266@1.8.0
 framework = arduino
@@ -42,7 +54,7 @@ lib_deps =
 build_flags = ${common.build_flags}
 src_filter = ${common.src_filter}
 
-[env:livingroom32]
+[common:esp32]
 platform = platformio/espressif32@3.2.0
 framework = arduino
 board = nodemcu-32s
@@ -56,3 +68,19 @@ build_flags =
 src_filter =
     ${common.src_filter}
     -<esphome/components/esp8266_pwm>
+
+[env:esp8266]
+extends = common:esp8266
+build_flags = ${common:esp8266.build_flags} ${runtime.build_flags}
+
+[env:esp8266-tidy]
+extends = common:esp8266
+build_flags = ${common:esp8266.build_flags} ${clangtidy.build_flags}
+
+[env:esp32]
+extends = common:esp32
+build_flags = ${common:esp32.build_flags} ${runtime.build_flags}
+
+[env:esp32-tidy]
+extends = common:esp32
+build_flags = ${common:esp32.build_flags} ${clangtidy.build_flags}

script/build_compile_commands.py

@@ -1,16 +0,0 @@
-#!/usr/bin/env python3
-import sys
-import os.path
-
-sys.path.append(os.path.dirname(__file__))
-from helpers import build_all_include, build_compile_commands
-
-
-def main():
-    build_all_include()
-    build_compile_commands()
-    print("Done.")
-
-
-if __name__ == "__main__":
-    main()

script/clang-tidy

@@ -1,6 +1,7 @@
 #!/usr/bin/env python3
 
 import argparse
+import json
 import multiprocessing
 import os
 import queue
@@ -15,27 +16,57 @@ import click
 import pexpect
 
 sys.path.append(os.path.dirname(__file__))
-from helpers import basepath, shlex_quote, get_output, build_compile_commands, \
-    build_all_include, temp_header_file, git_ls_files, filter_changed
+from helpers import shlex_quote, get_output, \
+    build_all_include, temp_header_file, git_ls_files, filter_changed, load_idedata
 
 
-def run_tidy(args, tmpdir, queue, lock, failed_files):
+def clang_options(idedata):
+    cmd = [
+        # target 32-bit arch (this prevents size mismatch errors on a 64-bit host)
+        '-m32',
+        # disable built-in include directories from the host
+        '-nostdinc',
+        '-nostdinc++',
+        # allow to condition code on the presence of clang-tidy
+        '-DCLANG_TIDY'
+    ]
+
+    # copy compiler flags, except those clang doesn't understand.
+    cmd.extend(flag for flag in idedata['cxx_flags'].split(' ')
+               if flag not in ('-free', '-fipa-pta', '-mlongcalls', '-mtext-section-literals'))
+
+    # defines
+    cmd.extend(f'-D{define}' for define in idedata['defines'])
+
+    # add include directories, using -isystem for dependencies to suppress their errors
+    for directory in idedata['includes']['toolchain']:
+        cmd.extend(['-isystem', directory])
+    for directory in sorted(set(idedata['includes']['build'])):
+        dependency = "framework-arduino" in directory or "/libdeps/" in directory
+        cmd.extend(['-isystem' if dependency else '-I', directory])
+
+    return cmd
+
+
+def run_tidy(args, options, tmpdir, queue, lock, failed_files):
     while True:
         path = queue.get()
-        invocation = ['clang-tidy-11', '-header-filter=^{}/.*'.format(re.escape(basepath))]
+        invocation = ['clang-tidy-11']
+
         if tmpdir is not None:
-            invocation.append('-export-fixes')
+            invocation.append('--export-fixes')
             # Get a temporary file. We immediately close the handle so clang-tidy can
             # overwrite it.
             (handle, name) = tempfile.mkstemp(suffix='.yaml', dir=tmpdir)
             os.close(handle)
             invocation.append(name)
-        invocation.append('-p=.')
+
         if args.quiet:
             invocation.append('-quiet')
-        for arg in ['-Wfor-loop-analysis', '-Wshadow-field', '-Wshadow-field-in-constructor']:
-            invocation.append('-extra-arg={}'.format(arg))
+
         invocation.append(os.path.abspath(path))
+        invocation.append('--')
+        invocation.extend(options)
         invocation_s = ' '.join(shlex_quote(x) for x in invocation)
 
         # Use pexpect for a pseudy-TTY with colored output
@@ -95,8 +126,8 @@ def main():
         """)
         return 1
 
-    build_all_include()
-    build_compile_commands()
+    idedata = load_idedata("esp8266-tidy")
+    options = clang_options(idedata)
 
     files = []
     for path in git_ls_files(['*.cpp']):
@@ -116,6 +147,7 @@ def main():
         files = split_list(files, args.split_num)[args.split_at - 1]
 
     if args.all_headers and args.split_at in (None, 1):
+        build_all_include()
         files.insert(0, temp_header_file)
 
     tmpdir = None
@@ -128,7 +160,7 @@ def main():
         lock = threading.Lock()
         for _ in range(args.jobs):
             t = threading.Thread(target=run_tidy,
-                                 args=(args, tmpdir, task_queue, lock, failed_files))
+                                 args=(args, options, tmpdir, task_queue, lock, failed_files))
             t.daemon = True
             t.start()
 

script/helpers.py

@@ -1,13 +1,14 @@
 import codecs
-import json
 import os.path
 import re
 import subprocess
-import sys
+import json
+from pathlib import Path
 
 root_path = os.path.abspath(os.path.normpath(os.path.join(__file__, "..", "..")))
 basepath = os.path.join(root_path, "esphome")
-temp_header_file = os.path.join(root_path, ".temp-clang-tidy.cpp")
+temp_folder = os.path.join(root_path, ".temp")
+temp_header_file = os.path.join(temp_folder, "all-include.cpp")
 
 
 def shlex_quote(s):
@@ -33,63 +34,9 @@ def build_all_include():
     headers.sort()
     headers.append("")
     content = "\n".join(headers)
-    with codecs.open(temp_header_file, "w", encoding="utf-8") as f:
-        f.write(content)
-
-
-def build_compile_commands():
-    gcc_flags_json = os.path.join(root_path, ".gcc-flags.json")
-    if not os.path.isfile(gcc_flags_json):
-        print("Could not find {} file which is required for clang-tidy.".format(gcc_flags_json))
-        print(
-            'Please run "pio init --ide atom" in the root esphome folder to generate that file.'
-        )
-        sys.exit(1)
-    with codecs.open(gcc_flags_json, "r", encoding="utf-8") as f:
-        gcc_flags = json.load(f)
-    exec_path = gcc_flags["execPath"]
-    include_paths = gcc_flags["gccIncludePaths"].split(",")
-    includes = [f"-I{p}" for p in include_paths]
-    cpp_flags = gcc_flags["gccDefaultCppFlags"].split(" ")
-    defines = [flag for flag in cpp_flags if flag.startswith("-D")]
-    command = [exec_path]
-    command.extend(includes)
-    command.extend(defines)
-    command.append("-std=gnu++11")
-    command.append("-Wall")
-    command.append("-Wno-delete-non-virtual-dtor")
-    command.append("-Wno-unused-variable")
-    command.append("-Wunreachable-code")
-
-    source_files = []
-    for path in walk_files(basepath):
-        filetypes = (".cpp",)
-        ext = os.path.splitext(path)[1]
-        if ext in filetypes:
-            source_files.append(os.path.abspath(path))
-    source_files.append(temp_header_file)
-    source_files.sort()
-    compile_commands = [
-        {
-            "directory": root_path,
-            "command": " ".join(
-                shlex_quote(x) for x in (command + ["-o", p + ".o", "-c", p])
-            ),
-            "file": p,
-        }
-        for p in source_files
-    ]
-    compile_commands_json = os.path.join(root_path, "compile_commands.json")
-    if os.path.isfile(compile_commands_json):
-        with codecs.open(compile_commands_json, "r", encoding="utf-8") as f:
-            try:
-                if json.load(f) == compile_commands:
-                    return
-            # pylint: disable=bare-except
-            except:
-                pass
-    with codecs.open(compile_commands_json, "w", encoding="utf-8") as f:
-        json.dump(compile_commands, f, indent=2)
+    p = Path(temp_header_file)
+    p.parent.mkdir(exist_ok=True)
+    p.write_text(content)
 
 
 def walk_files(path):
@@ -153,3 +100,28 @@ def git_ls_files(patterns=None):
     output, err = proc.communicate()
     lines = [x.split() for x in output.decode("utf-8").splitlines()]
     return {s[3].strip(): int(s[0]) for s in lines}
+
+
+def load_idedata(environment):
+    platformio_ini = Path(root_path) / "platformio.ini"
+    temp_idedata = Path(temp_folder) / f"idedata-{environment}.json"
+    if not platformio_ini.is_file() or not temp_idedata.is_file():
+        changed = True
+    elif platformio_ini.stat().st_mtime >= temp_idedata.stat().st_mtime:
+        changed = True
+    else:
+        changed = False
+
+    if not changed:
+        text = temp_idedata.read_text()
+    else:
+        stdout = subprocess.check_output(
+            ["pio", "run", "-t", "idedata", "-e", environment]
+        )
+        match = re.search(r'{\s*".*}', stdout.decode("utf-8"))
+        text = match.group()
+
+        temp_idedata.parent.mkdir(exist_ok=True)
+        temp_idedata.write_text(text)
+
+    return json.loads(text)