Merge pull request #208 from ausimian/claude/nif-static-analysis-tools-t3rmhg

Add clang-tidy static analysis for the NIF

Author: ausimian

.clang-tidy

@@ -0,0 +1,33 @@
+# clang-tidy configuration for the C++ NIF under c_src/.
+#
+# Run it with `mix clang.tidy` (which supplies the MLX/Fine/ERTS build
+# env) — see lib/mix/tasks/clang.tidy.ex and the `clang-tidy` target in
+# the Makefile.
+#
+# A focused, high-signal set: bug patterns, the clang static analyzer
+# (clang-analyzer-*), and performance checks. Deliberately not the
+# modernize/readability/cppcoreguidelines firehose yet — broaden once this
+# baseline stays green.
+#
+# Disabled checks and why:
+#   * bugprone-easily-swappable-parameters — fires on nearly every NIF
+#     entry point (many same-typed args) without pointing at a real defect.
+#   * performance-unnecessary-value-param — every NIF takes its
+#     fine::ResourcePtr args by value because Fine's FINE_NIF macro decodes
+#     each BEAM term into a value and passes it in; the signature is the
+#     binding convention, not a stray copy (the cppcheck build suppresses
+#     the same thing as passedByValueCallback).
+#   * bugprone-throwing-static-initialization — FINE_NIF / FINE_RESOURCE
+#     register callbacks at static-init time via throwing constructors;
+#     this is inherent to Fine's registration model, across ~25 macro
+#     expansions we don't own.
+#   * performance-enum-size — Opcode and ref::Kind are int64_t on purpose
+#     (they pack into the int64 refs the Elixir lowerer emits); shrinking
+#     the base type would break that ABI.
+#
+# HeaderFilterRegex scopes diagnostics to our own headers; MLX and Fine
+# arrive via -isystem and are skipped, exactly as the compiler skips them.
+Checks: '-*,bugprone-*,clang-analyzer-*,performance-*,-bugprone-easily-swappable-parameters,-performance-unnecessary-value-param,-bugprone-throwing-static-initialization,-performance-enum-size'
+WarningsAsErrors: '*'
+HeaderFilterRegex: 'c_src/'
+FormatStyle: none

.github/workflows/ci.yml

@@ -220,6 +220,57 @@ jobs:
       - name: Run cppcheck
         run: make cppcheck
 
+  # C++ static analysis of the NIF sources with clang-tidy (which also runs
+  # the clang static analyzer via its clang-analyzer-* checks). Unlike
+  # cppcheck, clang-tidy compiles each TU, so it needs the MLX/Fine/ERTS
+  # headers — hence the Beam setup and the MLX cache. It reuses the MLX
+  # install the precommit lane builds: the restore-keys pick up main's
+  # cache even when this branch's Makefile hash differs, since only the
+  # headers are needed and the MLX version is unchanged. clang-tidy ships
+  # with LLVM (brew), not stock Xcode. Runs on macOS to match the platform
+  # the NIF ships on, via the same `mix clang.tidy` a developer runs.
+  clang-tidy:
+    name: clang-tidy (NIF static analysis)
+    if: github.event_name != 'push' || github.ref_type == 'branch'
+    runs-on: macos-14
+    env:
+      MIX_ENV: test
+      EMILY_MLX_VARIANT: aot
+      EMILY_CACHE: ~/Library/Caches/emily
+    steps:
+      - uses: actions/checkout@v6
+
+      - name: Setup Beam
+        uses: erlef/setup-beam@v1
+        with:
+          version-file: .tool-versions
+          version-type: strict
+
+      - name: Install LLVM (clang-tidy)
+        run: brew install llvm
+
+      - name: Cache deps
+        uses: actions/cache@v5
+        with:
+          path: deps
+          key: deps-${{ runner.os }}-${{ hashFiles('mix.lock') }}
+          restore-keys: deps-${{ runner.os }}-
+
+      - name: Cache MLX and NIF objects
+        uses: actions/cache@v5
+        with:
+          path: ~/Library/Caches/emily
+          key: mlx-${{ runner.os }}-aot-${{ hashFiles('c_src/**', 'Makefile', 'mix.exs', 'scripts/build-mlx.sh') }}
+          restore-keys: mlx-${{ runner.os }}-aot-
+
+      - run: mix deps.get
+
+      - name: Run clang-tidy
+        run: |
+          export CLANG_TIDY="$(brew --prefix llvm)/bin/clang-tidy"
+          export SDKROOT="$(xcrun --show-sdk-path)"
+          mix clang.tidy
+
   # ASan CI deferred: requires OTP built with --enable-sanitizers=address
   # (macOS SIP blocks DYLD_INSERT_LIBRARIES, and late-loaded libasan
   # fails). See Makefile and RELEASE.md for details.

MAINTAINING.md

@@ -205,6 +205,23 @@ the `cppcheck` job). See the `cppcheck` target in the `Makefile` for the
 enabled checks and suppressions; use inline `// cppcheck-suppress <id>`
 for one-off false positives.
 
+### Static analysis of the NIF (clang-tidy)
+
+```sh
+brew install llvm   # one-time; clang-tidy isn't in stock Xcode
+mix clang.tidy
+```
+
+Runs clang-tidy (including the clang static analyzer, via its
+`clang-analyzer-*` checks) over `c_src/` and exits non-zero on any finding
+— the same tool the `clang-tidy` CI job runs. Unlike cppcheck it compiles
+each translation unit, so it needs the MLX/Fine/ERTS headers and build
+flags; `mix clang.tidy` supplies that env (reusing the cached MLX) and
+drives the `clang-tidy` Makefile target, so `make clang-tidy` on its own
+will refuse to run. Enabled checks and the header filter live in the
+repo-root `.clang-tidy`; use inline `// NOLINT(<check>)` for one-off false
+positives. Point at a specific binary with `CLANG_TIDY=/path/to/clang-tidy`.
+
 ### Build MLX in isolation
 
 ```sh

Makefile

@@ -47,7 +47,7 @@ ifeq ($(EMILY_ASAN),1)
     LDFLAGS  += -fsanitize=address
 endif
 
-.PHONY: all clean bench-native cppcheck
+.PHONY: all clean bench-native cppcheck clang-tidy
 
 all: $(NIF_SO) $(METALLIB)
 
@@ -89,6 +89,33 @@ CPPCHECK_FLAGS := --enable=warning,performance,portability \
 cppcheck:
 	$(CPPCHECK) $(CPPCHECK_FLAGS) $(SOURCES)
 
+# ------------------------------------------------------------------
+# clang-tidy: static analysis (incl. the clang static analyzer, via the
+# clang-analyzer-* checks) of the first-party NIF sources.
+#
+# Unlike cppcheck, clang-tidy actually *compiles* each translation unit,
+# so it needs the MLX / Fine / ERTS headers and the exact build flags —
+# it reuses this Makefile's `$(CXXFLAGS)` verbatim via the trailing `--`.
+# That means it needs the same env the NIF build gets (MLX_INCLUDE_DIR,
+# FINE_INCLUDE_DIR, ERTS_INCLUDE_DIR), which `make` alone does not set.
+# Run it through `mix clang.tidy`, which supplies that env (reusing the
+# already-built/cached MLX) exactly like `mix bench.native` does; the
+# recipe below refuses to run without it rather than emit a confusing
+# clang error about an empty `-isystem`.
+#
+# Enabled checks and the header filter (diagnostics scoped to c_src/,
+# never MLX/Fine which arrive via -isystem) live in the repo-root
+# `.clang-tidy`. Install the tool with `brew install llvm`; override the
+# binary with CLANG_TIDY=/path/to/clang-tidy.
+# ------------------------------------------------------------------
+CLANG_TIDY ?= clang-tidy
+
+clang-tidy:
+	@test -n "$(MLX_INCLUDE_DIR)" || { \
+	  echo "clang-tidy needs the NIF build env — run 'mix clang.tidy', not 'make clang-tidy'." >&2; \
+	  exit 1; }
+	$(CLANG_TIDY) --quiet $(SOURCES) -- $(CXXFLAGS)
+
 # ------------------------------------------------------------------
 # bench-native: standalone C++ microbenchmarks under bench/native/.
 #

c_src/emily/async.hpp

@@ -44,13 +44,16 @@ namespace emily {
 
 namespace mx = mlx::core;
 
-namespace __async {
+namespace async_detail {
 
 // Build a binary term in msg_env from a null-terminated C string.
 inline ERL_NIF_TERM make_binary_from_cstr(ErlNifEnv *msg_env, const char *s) {
   size_t len = std::strlen(s);
   ERL_NIF_TERM term;
   unsigned char *data = enif_make_new_binary(msg_env, len, &term);
+  // The destination is a length-counted BEAM binary of exactly `len`
+  // bytes, not a C string, so a trailing NUL is neither needed nor wanted.
+  // NOLINTNEXTLINE(bugprone-not-null-terminated-result)
   std::memcpy(data, s, len);
   return term;
 }
@@ -80,7 +83,7 @@ error_reason_from_current_exception(ErlNifEnv *msg_env) {
   }
 }
 
-}  // namespace __async
+}  // namespace async_detail
 
 // Run `build_payload` on the worker thread of `w` and post the
 // result back to the caller PID as a message. Returns a fresh ref
@@ -132,7 +135,7 @@ fine::Term async_reply(ErlNifEnv *env,
               msg_env, ref_in_msg,
               enif_make_tuple2(
                   msg_env, fine::encode(msg_env, emily::atoms::error),
-                  __async::error_reason_from_current_exception(msg_env)));
+                  async_detail::error_reason_from_current_exception(msg_env)));
         }
       }
 

c_src/emily/op_cores.hpp

@@ -154,8 +154,8 @@ inline mx::array sliding_windows_view(
   out_dims.assign(rank, 0);
   mx::Shape new_shape;
   mx::Strides new_strides;
-  new_shape.reserve(2 * rank);
-  new_strides.reserve(2 * rank);
+  new_shape.reserve(2 * static_cast<std::size_t>(rank));
+  new_strides.reserve(2 * static_cast<std::size_t>(rank));
 
   for (int i = 0; i < rank; ++i) {
     int64_t eff = (window_shape[i] - 1) * dilations[i] + 1;
@@ -251,11 +251,11 @@ inline mx::array window_scatter_core(
     bool is_max,
     mx::Stream &s) {
   int rank = static_cast<int>(window_shape.size());
-  auto original_shape = tensor.shape();
+  const auto &original_shape = tensor.shape();
 
   // 1. Pad input with init_value.
   auto padded = do_pad(tensor, pad_lo, pad_hi, init_value, s);
-  auto padded_shape = padded.shape();
+  const auto &padded_shape = padded.shape();
 
   // 2. Sliding-window view (dilation is implicitly 1 per axis for scatter).
   std::vector<int64_t> dilations(rank, 1);
@@ -329,7 +329,7 @@ inline mx::array window_scatter_core(
 
   // 7. Reshape source so each index tuple is a single-point write.
   mx::Shape source_reshape;
-  source_reshape.reserve(2 * rank);
+  source_reshape.reserve(2 * static_cast<std::size_t>(rank));
   for (int i = 0; i < rank; ++i)
     source_reshape.push_back(static_cast<mx::ShapeElem>(out_dims[i]));
   for (int i = 0; i < rank; ++i)

c_src/emily/opcodes.hpp

@@ -280,7 +280,7 @@ inline double f64_from_bits(int64_t bits) {
   return d;
 }
 
-namespace __op {
+namespace op_detail {
 
 inline const mx::array &arg1(const std::vector<mx::array> &in,
                              const char *name) {
@@ -338,13 +338,13 @@ inline bool keepdims_attr(const std::vector<std::vector<int64_t>> &a,
   return v[0] != 0;
 }
 
-} // namespace __op
+} // namespace op_detail
 
 // Replay one instruction: apply `op` to its resolved operands + attrs.
 inline mx::array dispatch_op(Opcode op, const std::vector<mx::array> &in,
                              const std::vector<std::vector<int64_t>> &iattrs,
                              mx::Stream &s) {
-  using namespace emily::__op;
+  using namespace emily::op_detail;
   switch (op) {
   // --- Binary arithmetic / bitwise ---
   case Opcode::Add:

c_src/emily/program.hpp

@@ -122,6 +122,11 @@ class Program {
   //   * the recycled-`fun_id` collision the on-worker erase guards against
   //     can't fire on a stopping worker — it runs no further compiles.
   // So on the declined path we simply let `drop` destruct below (issue #172).
+  //
+  // post_to_worker constructs a std::function (may throw bad_alloc); a throw
+  // from this best-effort cleanup destructor is unrecoverable and would
+  // std::terminate regardless, so the escape is accepted here.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   ~Program() {
     for (auto &kv : compiled) {
       CompiledEntry &entry = kv.second;

c_src/emily/worker.hpp

@@ -148,6 +148,10 @@ class Reaper {
     }
   }
 
+  // shutdown() joins the reaper thread; std::thread::join can in theory
+  // throw std::system_error, but a throw from this best-effort teardown
+  // destructor is unrecoverable and would std::terminate regardless.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   ~Reaper() { shutdown(); }
 
 private:
@@ -198,6 +202,10 @@ class WorkerThread {
 
   // Non-blocking: signal stop and hand the thread to the Reaper to join
   // off-scheduler. Pending tasks are cancelled with {:error, :stopped}.
+  //
+  // retire() takes a lock and moves the thread to the reaper; a throw here
+  // is unrecoverable from a destructor and would std::terminate regardless.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   ~WorkerThread() { Reaper::instance().retire(state_.get()); }
 
   // Enqueue a task. Throws if the worker has been stopped or the queue is

lib/mix/tasks/clang.tidy.ex

@@ -0,0 +1,73 @@
+defmodule Mix.Tasks.Clang.Tidy do
+  @moduledoc """
+  Run clang-tidy over the C++ NIF sources in `c_src/`.
+
+  Unlike `make cppcheck`, clang-tidy compiles each translation unit, so it
+  needs the MLX / Fine / ERTS headers and the exact build flags. This task
+  supplies the same env `elixir_make` uses when building the NIF — reusing
+  the already-built (cached) MLX rather than fetching a second copy — and
+  then invokes the `clang-tidy` Makefile target, which analyses the NIF
+  sources with the build's `$(CXXFLAGS)`.
+
+  ## Usage
+
+      mix clang.tidy
+
+  Requires clang-tidy on `PATH` (`brew install llvm`); point at a specific
+  binary with `CLANG_TIDY=/path/to/clang-tidy`. The enabled checks and the
+  header filter live in the repo-root `.clang-tidy`.
+  """
+
+  use Mix.Task
+
+  @shortdoc "Run clang-tidy over the C++ NIF sources"
+
+  @impl Mix.Task
+  def run(_args) do
+    # Ensure MLX is available (its headers are what clang-tidy parses). The
+    # :emily_mlx compiler alias reuses an existing install and only builds
+    # from source on a cold cache — same as `mix bench.native`.
+    Mix.Task.run("compile.emily_mlx", [])
+
+    # Ask the mix project for its make_env — the same map `elixir_make`
+    # passes to `make` — and add ERTS_INCLUDE_DIR, which elixir_make sets
+    # itself at build time (so make_env/0 omits it) but a standalone `make`
+    # invocation does not get.
+    env =
+      Mix.Project.config()
+      |> Keyword.fetch!(:make_env)
+      |> case do
+        f when is_function(f, 0) -> f.()
+        m when is_map(m) -> m
+      end
+      |> Map.put("ERTS_INCLUDE_DIR", erts_include_dir())
+      |> Enum.to_list()
+
+    make = System.get_env("MAKE") || "make"
+
+    Mix.shell().info("Running: #{make} clang-tidy")
+
+    {_out, status} =
+      System.cmd(make, ["clang-tidy"],
+        env: env,
+        into: IO.stream(:stdio, :line),
+        stderr_to_stdout: true
+      )
+
+    if status != 0 do
+      Mix.raise("clang.tidy reported findings (exit #{status})")
+    end
+  end
+
+  # Mirror how elixir_make derives ERTS_INCLUDE_DIR: the Erlang headers
+  # (erl_nif.h, reached through fine.hpp) live under the OTP install. Honour
+  # an explicit override if one is already exported.
+  defp erts_include_dir do
+    System.get_env("ERTS_INCLUDE_DIR") ||
+      Path.join([
+        to_string(:code.root_dir()),
+        "erts-#{:erlang.system_info(:version)}",
+        "include"
+      ])
+  end
+end