Skip to content

Run clang-tidy 21 on pull-request changed lines#4982

Open
matthargett wants to merge 2 commits into
bytecodealliance:mainfrom
rebeckerspecialties:ci/clang-tidy-on-prs
Open

Run clang-tidy 21 on pull-request changed lines#4982
matthargett wants to merge 2 commits into
bytecodealliance:mainfrom
rebeckerspecialties:ci/clang-tidy-on-prs

Conversation

@matthargett

@matthargett matthargett commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What

Run the repository's existing .clang-tidy on pull requests — but only on the lines the PR changes.

The root .clang-tidy is already a maintained, strict config (bugprone-*, cert-*, clang-analyzer-*, ... with WarningsAsErrors), but no workflow runs it today, so the null-deref / narrowing-conversion / use-after-move class of bug it catches is currently caught only by a reviewer reading the diff.

How

Mirrors the diff-scoping already used by the git-clang-format gate in coding_guidelines.yml:

  • permissions: contents: read, fetch-depth: 0, diff computed from ${{ github.event.pull_request.base.sha }}..HEAD.
  • clang-tidy-diff.py turns the changed-line ranges into -line-filter, so a PR is only flagged for issues it introduces on the lines it touches — pre-existing issues on untouched lines stay silent.
  • compile_commands.json comes from configuring the default linux iwasm build (interp + AOT runtime, no LLVM required); the database is emitted at configure time, so no build step is needed.
  • Only changed sources that are in compile_commands.json are analyzed, each with its real compile flags. A changed file not in the default build — or a header, which has no translation unit of its own — is skipped rather than analyzed without context (which would otherwise produce false failures from missing includes or the wrong #ifdef branch).
  • A PR that changes no C/C++ source skips install / configure / lint entirely, so docs-, test- and CI-only PRs cost almost no Action minutes while the job still reports success.

clang-tidy version

Runs clang-tidy 21 on ubuntu-24.04, installed from apt.llvm.org (a small ~50 MB package, not a full LLVM toolchain download). clang-tidy 21 matches the LLVM shipped in Xcode 26/27 and the clang-format-21 gate proposed in #4992, so lint results are consistent whether a contributor builds/auto-formats on macOS/Xcode or on another LLVM-built platform.

Curated .clang-tidy

Moving from clang-tidy 14 to 21 surfaces newer checks. Five are noise/style for this C codebase rather than bug-catchers, so they are disabled (on a full-tree scan they otherwise dominate the output with no correctness signal):

check why disabled
misc-include-cleaner IWYU-style; ~1750 findings, unusable noise
misc-header-include-cycle structural noise
modernize-macro-to-enum opinionated for a C codebase
readability-inconsistent-declaration-parameter-name cosmetic
bugprone-assignment-in-if-condition flags WAMR's deliberate assignment-in-condition idiom

The bug-catchers stay on, including bugprone-narrowing-conversions, bugprone-sizeof-expression, bugprone-implicit-widening-of-multiplication-result, bugprone-multi-level-implicit-pointer-conversion, performance-*, clang-analyzer-* and cert-* — the classes that have historically caught real portability / correctness bugs on this project.

Residual on existing code (why the gate can land now)

A full-tree scan with this config reports roughly 600 diagnostics on the default-config translation units — dominated by bugprone-narrowing-conversions — concentrated in core/iwasm/common and core/iwasm/interpreter. None of these block contributors: the gate is diff-scoped, so they only surface when a PR actually edits those lines. The residual is cleaned up incrementally in per-subsystem follow-up PRs (ordered stack in #4983).

Fork / upstream safety

Same properties as coding_guidelines.yml: only contents: read, diff from the base SHA — so it behaves identically for in-repo and forked pull requests.

Notes

  • Coverage is scoped to the default linux config's translation units; it can be broadened with additional configs.
  • Trigger is pull_request only: the gate diffs against the PR base, so a manual (workflow_dispatch) run would have no meaningful base.

Verification

Verified end-to-end against real CI on a throwaway fork PR:

  • a deliberate bugprone-narrowing-conversions on a changed line of an in-database translation unit fails the job — confirming both that configure-only still parses the linux sources and that the diff-scoped fail path works;
  • a PR that changes no C/C++ source skips the install / configure / lint steps and reports success (this PR's own run);
  • a changed file not in the compilation database is skipped rather than mis-analyzed (diff intersected with compile_commands.json).

@lum1n0us lum1n0us left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before turning this workflow into an enforced check, should we run a full  clang-tidy  scan on the current codebase and clean up any existing issues first? Otherwise, contributors may end up having to deal with pre-existing diagnostics while making unrelated changes in their PRs.

Comment thread .github/workflows/clang_tidy.yml Outdated
Comment thread .github/workflows/clang_tidy.yml
Comment thread .github/workflows/clang_tidy.yml
Comment thread .github/workflows/clang_tidy.yml
Comment thread .github/workflows/clang_tidy.yml
@matthargett matthargett force-pushed the ci/clang-tidy-on-prs branch from c367072 to 8b0d482 Compare July 11, 2026 04:33
@matthargett matthargett changed the title Run clang-tidy on pull-request changed lines Run clang-tidy 21 on pull-request changed lines Jul 11, 2026
The repository already ships a strict .clang-tidy (bugprone-*, cert-*,
clang-analyzer-*, ... with WarningsAsErrors) but no workflow runs it, so
the bug class it catches is currently caught only by a reviewer reading
the diff.

Add a pull_request workflow that runs clang-tidy on the lines a PR
changes (clang-tidy-diff.py driven from the base SHA, the same
diff-scoping approach as the git-clang-format gate in
coding_guidelines.yml), so a PR is only flagged for issues it introduces
on the lines it touches. Like that gate it needs only contents:read and
computes the diff from the pull_request base SHA, so it behaves
identically for in-repo and forked pull requests.

The job runs on ubuntu-24.04 and installs clang-tidy-21 from apt.llvm.org
(a small package, not a full LLVM toolchain download). clang-tidy 21
matches the LLVM shipped in Xcode 26/27 and the clang-format-21 gate, so
lint results are consistent for contributors building on macOS/Xcode and
on other LLVM-built platforms.

To keep it correct and cheap:

- compile_commands.json comes from configuring the default linux iwasm
  build (interp + AOT runtime, no LLVM required); the database is emitted
  at configure time, so no build step is needed.
- Only changed sources that are in compile_commands.json are analyzed,
  each with its real compile flags. A changed file not in the default
  build (or a header, which has no translation unit of its own) is
  skipped rather than analyzed without context, which would otherwise
  produce false failures from missing includes or the wrong #ifdef
  branch.
- A PR that changes no C/C++ source skips install / configure / lint
  entirely, so docs-, test- and CI-only PRs cost almost no Action minutes
  while the job still reports success.
Running clang-tidy-21 instead of clang-tidy-14 surfaces checks that did
not exist in 14. Five of them are noise or style for this C codebase
rather than bug-catchers; on a full-tree scan they account for the
overwhelming majority of findings with no correctness signal:

  - misc-include-cleaner        IWYU-style; ~1750 findings, unusable noise
  - misc-header-include-cycle   structural noise
  - modernize-macro-to-enum     opinionated for a C codebase
  - readability-inconsistent-declaration-parameter-name   cosmetic
  - bugprone-assignment-in-if-condition   flags WAMR's deliberate
                                          assignment-in-condition idiom

Disable only those five. The bug-catching checks that have historically
caught real portability / correctness bugs stay on, including
bugprone-narrowing-conversions, bugprone-sizeof-expression,
bugprone-implicit-widening-of-multiplication-result,
bugprone-multi-level-implicit-pointer-conversion, performance-*,
clang-analyzer-* and cert-*.

The gate added in the previous commit is diff-scoped, so these disables
only affect the handful of new lines a PR touches; residual findings on
existing code are addressed separately in per-subsystem cleanup PRs.
@matthargett matthargett force-pushed the ci/clang-tidy-on-prs branch from 8b0d482 to 7ec71c1 Compare July 11, 2026 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants