From 1a90908443607fa792137ab9854f1a45b8ec517d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 11:35:25 +0200 Subject: [PATCH 1/2] lint: make four NOLINTNEXTLINE directives apply, and gate the ones that cannot (fixes #627) A NOLINTNEXTLINE annotates the next *physical* line. Four directives in the tree had their reason wrapped onto a second comment line, so each annotated that comment instead of the code, and clang-tidy reported nothing about it -- the directive parses, the file looks annotated, and the findings leak. Measured on 0e3b8823, clang-tidy 22.1.8, with the CI clang-tidy job's own option set. Before, the four leaked six findings: forms.hpp:489:39 forwarding reference parameter 'action' is never forwarded forms.hpp:489:57 forwarding reference parameter 'visitor' is never forwarded forms.hpp:496:41 possibly unsafe 'operator[]' forms.hpp:496:66 possibly unsafe 'operator[]' oom_injector.cpp:110:21 do not manage memory manually oom_injector.cpp:110:9 initializing non-owner with a newly created owner After, all six are gone. The fourth directive, test_bridge_lifetime.cpp:519, was inert *and* unnecessary -- cppcoreguidelines-owning-memory does not fire on placement new, confirmed by measurement -- so it is now merely effective, and kept so a later edit to that line cannot reintroduce the finding silently. Each site moves its reason above the directive rather than adding a clang-format guard, so the fix survives reformatting. forms.hpp:487's directive also dropped an unchecked-container-access it never needed; only missing-std-forward fires on that line, and the reason now says why neither parameter may be forwarded rather than restating the check's name. The guard is the part that matters. scripts/check_nolint_directives.sh fails when a NOLINTNEXTLINE is followed by a comment, a blank line, or nothing at all. Run against unmodified master it reports exactly the four sites above, at exactly those line numbers, and it does not flag fixed_string.hpp:48 -- the prose that documents this hazard and whose existence is why the scan anchors the directive at the start of the comment. That anchoring is a stated residual, not an oversight. scripts/test_check_nolint_directives.sh drives the checker against tests/lint/nolint_directives/: the two effective shapes must be accepted, each inert shape rejected on its own while naming its own file, and a directory with no directives at all rejected rather than called clean. A gate for suppressions that suppress nothing would be the same defect one level up if it were not itself tested. The job belongs in drift-guard.yml and is in its own workflow only because that file and ci.yml are both held by open PRs (#614, #623); the workflow's header says so and folding it in changes nothing about its behaviour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- .github/workflows/suppression-guard.yml | 61 ++++++ include/morph/forms/forms.hpp | 19 +- scripts/check_nolint_directives.sh | 183 ++++++++++++++++++ scripts/test_check_nolint_directives.sh | 100 ++++++++++ .../invalid/blank_line/blank.cpp | 14 ++ .../invalid/last_line/trailing.cpp | 11 ++ .../invalid/wrapped_reason/wrapped.cpp | 13 ++ .../valid/effective_directives.hpp | 43 ++++ tests/oom_injector.cpp | 7 +- tests/test_bridge_lifetime.cpp | 8 +- 10 files changed, 449 insertions(+), 10 deletions(-) create mode 100644 .github/workflows/suppression-guard.yml create mode 100755 scripts/check_nolint_directives.sh create mode 100755 scripts/test_check_nolint_directives.sh create mode 100644 tests/lint/nolint_directives/invalid/blank_line/blank.cpp create mode 100644 tests/lint/nolint_directives/invalid/last_line/trailing.cpp create mode 100644 tests/lint/nolint_directives/invalid/wrapped_reason/wrapped.cpp create mode 100644 tests/lint/nolint_directives/valid/effective_directives.hpp diff --git a/.github/workflows/suppression-guard.yml b/.github/workflows/suppression-guard.yml new file mode 100644 index 000000000..5817b9abf --- /dev/null +++ b/.github/workflows/suppression-guard.yml @@ -0,0 +1,61 @@ +name: Suppression guard + +# Gates for the "a suppression that suppresses nothing" class of bug. +# +# NOLINTNEXTLINE applies to the next *physical* line. When the reason is wrapped +# onto a second comment line, the directive annotates that comment, the findings +# it names are still reported, and clang-tidy says nothing about it -- the +# directive parses and the file looks annotated. Four such directives existed on +# master and six findings leaked past three of them (#627). +# +# That is the failure mode AGENTS.md names first -- a control that reports +# success while measuring nothing -- one level below the gate. It also matters +# for the clang-tidy campaign in #580, whose agreed remedy is partly "write an +# individually reasoned NOLINT": that campaign cannot proceed honestly on a tree +# where NOLINTs silently fail to apply, so this gate is its precondition. +# +# WHY THIS IS ITS OWN WORKFLOW, AND WHERE IT BELONGS +# +# This job is a fast, dependency-free text scan that compiles nothing, which is +# exactly .github/workflows/drift-guard.yml's remit, and it should be a job in +# that file. It is here instead only because drift-guard.yml was held by an open +# PR (#614) when this landed, and ci.yml by another (#623). Folding it into +# drift-guard.yml once that has landed is tracked as its own issue; nothing +# about the gate's behaviour changes when it moves. +# +# The self-test runs before the gate, as every other lint in this repository +# does: a gate that detects nothing reports the same green as a clean tree, and +# for this gate that would be the very defect it exists to catch, one level up. + +on: + push: + branches: [main, master] + pull_request: + +# Supersede a run a newer commit on the same ref has made obsolete -- see the +# note in ci.yml. Cheap here, but the queue it shares is account-wide. +concurrency: + group: suppression-guard-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + nolint-lint: + name: NOLINT directives that cannot take effect + runs-on: ubuntu-24.04 + steps: + - name: Checkout + uses: actions/checkout@v4 + + # Drives the checker against tests/lint/nolint_directives/ first: the two + # effective shapes must be accepted, each inert shape must be rejected on + # its own and name its own file, and a directory with no directives at all + # must be rejected rather than called clean. Without this the step below + # would report green whether or not it still detects anything. + - name: Self-test the NOLINT-directive checker + run: bash scripts/test_check_nolint_directives.sh + + - name: Check every NOLINTNEXTLINE annotates a line of code + run: bash scripts/check_nolint_directives.sh diff --git a/include/morph/forms/forms.hpp b/include/morph/forms/forms.hpp index f5b9c8f47..85a726bc7 100644 --- a/include/morph/forms/forms.hpp +++ b/include/morph/forms/forms.hpp @@ -484,15 +484,26 @@ template /// @brief Invokes `visitor.operator()(name, member)` for every reflected /// member of @p action (glaze pure reflection). template -// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward, cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) -// — member-tie iteration +// Neither forwarding reference is forwarded, and neither may be. `action` is +// bound by `glz::to_tie` into a tuple of references that outlives this line and +// is read member-by-member below; moving from it would leave the tie pointing +// at a moved-from object. `visitor` is invoked once per reflected member by the +// fold expression, so forwarding it would move from it on the first member and +// call a moved-from callable for every one after. Both are `&&` to preserve the +// argument's cv-qualification through the tie — a `const A&` must tie to const +// members — not to enable a move. The directive stays on one physical line +// deliberately; see the note at detail/fixed_string.hpp:48. +// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward) constexpr void forEachNamedMember(A&& action, Visitor&& visitor) { using Plain = std::remove_cvref_t; constexpr auto memberCount = glz::reflect::size; auto memberTie = glz::to_tie(action); [&](std::index_sequence) { - // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) — index bounded by - // reflect::size + // `I` is a pack of `std::index_sequence`, i.e. every value in + // [0, glz::reflect::size), and `keys` is an array of exactly that + // size — the index cannot be out of range by construction. The directive + // stays on one physical line; see the note at detail/fixed_string.hpp:48. + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) (visitor.template operator()(glz::reflect::keys[I], glz::get_member(action, get(memberTie))), ...); }(std::make_index_sequence{}); diff --git a/scripts/check_nolint_directives.sh b/scripts/check_nolint_directives.sh new file mode 100755 index 000000000..e3e5fb794 --- /dev/null +++ b/scripts/check_nolint_directives.sh @@ -0,0 +1,183 @@ +#!/usr/bin/env bash +# Usage: bash scripts/check_nolint_directives.sh [DIR...] +# +# Fails if any NOLINTNEXTLINE directive cannot take effect -- see issue #627. +# +# NOLINTNEXTLINE suppresses diagnostics on the line *immediately* following it, +# counted in physical lines. If that next line is another comment, or is blank, +# or does not exist, the suppression lands on nothing: +# +# // NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward) -- the reason +# // wrapped onto a second line +# constexpr void forEachNamedMember(A&& action, Visitor&& visitor) { +# +# The directive above annotates the *comment* on the next line. The function is +# unannotated and its findings are still reported. clang-tidy says nothing about +# this: the directive parses, the file looks annotated, and the finding leaks. +# +# That is the failure mode AGENTS.md names first -- a control that reports +# success while measuring nothing -- one level below the gate: a suppression +# that reports success while suppressing nothing. It is also silent by +# construction, because the only symptom is a finding nobody expected to be +# suppressed in the first place, sitting in a file that looks like it has an +# opinion about it. +# +# include/morph/detail/fixed_string.hpp documents the hazard in prose and shows +# the fix, including the `// clang-format off` guard that stops the formatter +# re-wrapping a long directive. Nothing enforced it, so four other sites drifted +# into doing exactly what that comment warns about (#627). This gate is the +# enforcement. +# +# Remedies, either of which this gate accepts: +# +# 1. Put the reason *above* the directive, so the directive is the last +# comment line before the code. This is what the four #627 sites now do. +# 2. Keep the reason on the directive's own line and add `// clang-format off` +# / `// clang-format on` around it so the formatter cannot wrap it, as +# fixed_string.hpp does for its four-check directive. +# +# WHAT THIS GATE DELIBERATELY DOES NOT MATCH +# +# A directive is recognised only when `NOLINTNEXTLINE` is the first token after +# the comment marker (`// NOLINTNEXTLINE...`, `/* NOLINTNEXTLINE...`). clang-tidy +# itself is looser -- it scans for the literal anywhere in a comment -- so prose +# that merely *names* the directive, as fixed_string.hpp's warning does: +# +# // A NOLINTNEXTLINE directive must sit on ONE physical line to apply to the +# +# is a directive as far as clang-tidy is concerned (a vacuous one, annotating +# the next comment line, which is harmless) but is not scanned here. Anchoring +# is what lets the in-tree documentation of this hazard exist at all. The +# residual is a directive hidden mid-sentence in a comment; no such site exists +# in the tree, and writing one would be a stranger thing to do than the defect +# this gate catches. +# +# Exits 0 when every anchored NOLINTNEXTLINE is followed by a line of code. +# Exits 1 when one is not, printing a "file:line:" diagnostic and the two lines +# for each offender -- and also when no directive was found at all, because a +# gate that scanned nothing must not report success. +set -euo pipefail + +if [ "$#" -eq 0 ]; then + dirs=(include src tests examples) +else + dirs=("$@") +fi + +for dir in "${dirs[@]}"; do + if [ ! -d "$dir" ]; then + echo "error: not a directory: ${dir}" >&2 + exit 1 + fi +done + +# find writes into a file rather than a process substitution so that its exit +# status is actually observed -- inside `< <(...)` it is discarded, and an +# unreadable tree would then reach the "no directives found" branch below and be +# reported as a tree that has none, which is a wrong diagnosis of a real +# failure. (Same reasoning as scripts/check_automoc_includes.sh.) +src_list="$(mktemp)" +trap 'rm -f "$src_list"' EXIT +# tests/lint holds this gate's own fixtures, three of which are deliberately +# inert directives; scanning them in the default whole-tree run would make the +# gate permanently red against its own test data. They are reached explicitly by +# scripts/test_check_nolint_directives.sh, which passes their directory as an +# argument -- so naming a path under tests/lint still scans it. +if ! find "${dirs[@]}" -type d \( -name build -o -name _deps -o -name .git \) -prune -o \ + -type d -path '*tests/lint' -prune -o \ + -type f \( -name '*.hpp' -o -name '*.h' -o -name '*.cpp' -o -name '*.cc' \ + -o -name '*.cxx' -o -name '*.ipp' -o -name '*.c' \) -print0 \ + > "$src_list"; then + echo "error: find failed while scanning for sources under: ${dirs[*]}" >&2 + exit 1 +fi +mapfile -d '' -t src_files < "$src_list" + +if [ "${#src_files[@]}" -eq 0 ]; then + echo "error: no C/C++ sources found under: ${dirs[*]}" >&2 + exit 1 +fi + +total=0 +offenders="" + +for file in "${src_files[@]}"; do + # awk holds the previous line so a directive can be judged against the one + # that follows it. `found`/`bad` are printed on a trailing marker line so the + # shell can accumulate both counts and the diagnostics in one pass. + result="$(awk -v path="$file" ' + function trim(s) { sub(/^[[:space:]]+/, "", s); return s } + { + if (pending) { + t = trim($0) + # A directive is inert when the next physical line is another + # comment, is blank, or is a continuation of a block comment. + if (t == "" || t ~ /^\/\// || t ~ /^\/\*/ || t ~ /^\*/) { + printf "%s:%d: NOLINTNEXTLINE is followed by a comment or blank line, so it suppresses nothing\n", path, pendingline + printf " %d | %s\n", pendingline, pendingtext + printf " %d | %s\n", NR, $0 + bad++ + } + pending = 0 + } + if (trim($0) ~ /^(\/\/|\/\*)[[:space:]]*NOLINTNEXTLINE([[:space:]]|\(|$)/) { + found++ + pending = 1 + pendingline = NR + pendingtext = $0 + } + } + END { + # A directive on the final line of a file annotates nothing at all. + if (pending) { + printf "%s:%d: NOLINTNEXTLINE is the last line of the file, so it suppresses nothing\n", path, pendingline + printf " %d | %s\n", pendingline, pendingtext + bad++ + } + printf "@@ %d %d\n", found + 0, bad + 0 + } + ' "$file")" + + marker="${result##*@@ }" + body="${result%@@ *}" + file_found="${marker%% *}" + total=$((total + file_found)) + if [ -n "${body//[$'\n\t ']/}" ]; then + offenders+="${body}" + fi +done + +if [ "$total" -eq 0 ]; then + cat >&2 <&2 <&2; failures=$((failures + 1)); } + +# ── valid/ must pass ───────────────────────────────────────────────────────── +if output="$(bash "$checker" "${fixtures}/valid" 2>&1)"; then + note "ok: valid fixtures accepted" +else + fail "valid fixtures were rejected by the checker:" + printf '%s\n' "$output" >&2 +fi + +# ── every invalid/ subdirectory must be rejected on its own ───────────────── +shopt -s nullglob +invalid_dirs=("${fixtures}"/invalid/*/) +shopt -u nullglob + +if [ "${#invalid_dirs[@]}" -eq 0 ]; then + fail "no fixtures found in ${fixtures}/invalid -- the self-test would pass vacuously" +fi + +# A nonzero exit is not enough on its own: the checker's other failure path is +# "no directives found", and a fixture whose files stopped matching the find +# patterns would take it -- so the fixture would still look "rejected" while the +# checker no longer scanned that kind of file at all. Assert the rejection is +# the inert-directive diagnostic and that it names a file under this fixture. +# +# Here-strings rather than `printf ... | grep -q`: `grep -q` exits on first +# match, and under `set -o pipefail` the SIGPIPE that kills the writer becomes +# the pipeline's status -- so a diagnostic long enough to fill the pipe buffer +# would read as *not* matching the pattern it contains. +for dir in "${invalid_dirs[@]}"; do + name="$(basename "$dir")" + if output="$(bash "$checker" "$dir" 2>&1)"; then + fail "invalid fixture ${name} was accepted; the checker no longer detects it" + elif ! grep -q 'NOLINT directive lint failed' <<<"$output"; then + fail "invalid fixture ${name} was rejected, but not as an inert directive \ +-- the checker failed for some other reason (a vacuous scan, most likely):" + printf '%s\n' "$output" >&2 + elif ! grep -qF "$dir" <<<"$output"; then + fail "invalid fixture ${name} was rejected without naming any file under \ +${dir}; the diagnostic does not point at the offender:" + printf '%s\n' "$output" >&2 + else + note "ok: invalid fixture ${name} rejected" + fi +done + +# ── a directory with no directives at all must be rejected ────────────────── +# "Found nothing" is the one outcome this gate must never call clean: it is what +# a broken directive pattern, a narrowed file-extension list, or a wrong path +# argument all look like from the outside. +empty_dir="$(mktemp -d)" +trap 'rm -rf "$empty_dir"' EXIT +printf 'int main() { return 0; }\n' > "${empty_dir}/plain.cpp" +if bash "$checker" "$empty_dir" >/dev/null 2>&1; then + fail "a directory containing no NOLINTNEXTLINE directives was accepted; the gate can pass vacuously" +else + note "ok: directory with no directives rejected" +fi + +if [ "$failures" -ne 0 ]; then + printf '\n%s self-test check(s) failed\n' "$failures" >&2 + exit 1 +fi + +note "all NOLINT-directive checker self-tests passed" diff --git a/tests/lint/nolint_directives/invalid/blank_line/blank.cpp b/tests/lint/nolint_directives/invalid/blank_line/blank.cpp new file mode 100644 index 000000000..231e414dc --- /dev/null +++ b/tests/lint/nolint_directives/invalid/blank_line/blank.cpp @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Fixture for scripts/check_nolint_directives.sh: a directive separated from the +// code it means to annotate by a blank line. Same defect as the wrapped-reason +// case, different cause -- an edit inserted the blank rather than the formatter +// wrapping the line. Not compiled -- scanned as text. + +#include + +void* allocate(unsigned long size) { + // NOLINTNEXTLINE(cppcoreguidelines-no-malloc) + + return std::malloc(size); +} diff --git a/tests/lint/nolint_directives/invalid/last_line/trailing.cpp b/tests/lint/nolint_directives/invalid/last_line/trailing.cpp new file mode 100644 index 000000000..0ab33a9d7 --- /dev/null +++ b/tests/lint/nolint_directives/invalid/last_line/trailing.cpp @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Fixture for scripts/check_nolint_directives.sh: a directive as the final line +// of a file, annotating nothing at all -- what is left when the code it guarded +// is deleted and the directive is not. Not compiled -- scanned as text. + +#include + +void* allocate(unsigned long size) { return std::malloc(size); } + +// NOLINTNEXTLINE(cppcoreguidelines-no-malloc) diff --git a/tests/lint/nolint_directives/invalid/wrapped_reason/wrapped.cpp b/tests/lint/nolint_directives/invalid/wrapped_reason/wrapped.cpp new file mode 100644 index 000000000..af4b69baf --- /dev/null +++ b/tests/lint/nolint_directives/invalid/wrapped_reason/wrapped.cpp @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Fixture for scripts/check_nolint_directives.sh: the #627 shape. The reason is +// wrapped onto a second comment line, so the directive annotates that comment +// and the finding on the statement below leaks. Not compiled -- scanned as text. + +#include + +void* allocate(unsigned long size) { + // NOLINTNEXTLINE(cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) -- + // this *is* the process-wide operator new/delete pair + return std::malloc(size); +} diff --git a/tests/lint/nolint_directives/valid/effective_directives.hpp b/tests/lint/nolint_directives/valid/effective_directives.hpp new file mode 100644 index 000000000..592b5c3a6 --- /dev/null +++ b/tests/lint/nolint_directives/valid/effective_directives.hpp @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Fixture for scripts/check_nolint_directives.sh: every shape the gate must +// ACCEPT. Not compiled -- it is scanned as text. + +#pragma once + +#include + +namespace morph::lint_fixture { + +// The reason sits above the directive, so the directive is the last comment +// line before the code. This is the preferred remedy and the shape the four +// #627 sites were converted to. +// NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) +inline int firstOf(const int* values) { return values[0]; } + +// The reason shares the directive's physical line, guarded so clang-format +// cannot wrap it. This is fixed_string.hpp's shape. +// clang-format off +// NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) -- index bounded by the caller's contract +inline int at(const int* values, std::size_t index) { return values[index]; } +// clang-format on + +// Prose that NAMES the directive without being one. The gate anchors on +// `// NOLINTNEXTLINE` at the start of the comment, so a sentence like this -- +// which is what include/morph/detail/fixed_string.hpp:48 is, and which is the +// in-tree documentation of the whole hazard -- must not be flagged even though +// the line that follows it is another comment. +// +// A NOLINTNEXTLINE directive must sit on ONE physical line to apply to the +// next one; wrapped, it silently annotates the comment instead. +inline int identity(int value) { return value; } + +// NOLINTBEGIN and NOLINTEND are not line-scoped and are out of this gate's +// scope; a wrapped reason after either of them is harmless. +// NOLINTBEGIN(readability-identifier-length) +// The block form applies until NOLINTEND regardless of what follows it on the +// next line, so this comment costs nothing. +inline int id(int v) { return v; } +// NOLINTEND(readability-identifier-length) + +} // namespace morph::lint_fixture diff --git a/tests/oom_injector.cpp b/tests/oom_injector.cpp index 92e6ac3b3..6966085f4 100644 --- a/tests/oom_injector.cpp +++ b/tests/oom_injector.cpp @@ -104,9 +104,10 @@ void* allocateOrInject(std::size_t size) { // this thread afterward) allocates normally. throw std::bad_alloc{}; } - // NOLINTNEXTLINE(cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) -- - // this *is* the process-wide operator new/delete pair; std::malloc/free - // is what it has to be built from. + // This *is* the process-wide operator new/delete pair; std::malloc/free is + // what it has to be built from. The directive stays on one physical line; + // see the note at include/morph/detail/fixed_string.hpp:48. + // NOLINTNEXTLINE(cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) if (void* ptr = std::malloc(size == 0 ? 1 : size)) { return ptr; } diff --git a/tests/test_bridge_lifetime.cpp b/tests/test_bridge_lifetime.cpp index ccc9ac024..da5264ad3 100644 --- a/tests/test_bridge_lifetime.cpp +++ b/tests/test_bridge_lifetime.cpp @@ -516,9 +516,11 @@ TEST_CASE("Bridge: hasSubscribers is not read once the bridge is destroyed (guar objAddr -= objAddr % alignof(morph::bridge::Bridge); void* const bridgeMem = reinterpret_cast(objAddr); // NOLINT(performance-no-int-to-ptr) - // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) -- placement new into - // the mmap'd region above; destroyed via an explicit dtor call below, not - // `delete` (the memory is not heap-owned). + // Placement new into the mmap'd region above; destroyed via an explicit dtor + // call below, not `delete` (the memory is not heap-owned). The directive + // stays on one physical line; see the note at + // include/morph/detail/fixed_string.hpp:48. + // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) auto* const bridge = new (bridgeMem) morph::bridge::Bridge(std::move(backendOwner)); auto binding = std::make_shared(); From 4e464be4fb9d98d6625a712f91d2d5f888e600ff Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 11:41:42 +0200 Subject: [PATCH 2/2] lint: clear clang-tidy batch A, and set the campaign's suppression precedents (fixes #600) #580's census of 596 findings splits into six batches by file tree. Batch A is the tail -- version/attributes/journal/render/qt-forms/detail -- and it is first not because it is smallest but because it is the only batch that forces every policy precedent the other five need, on a corpus where getting one wrong is cheap. `include/morph/render/locale_format.hpp` is excluded: PR #630 rewrites it and changes its answer, so its one finding must be re-measured, not inherited. Measured on 0e3b8823, clang-tidy 22.1.8, .clang-tidy unmodified, with the CI clang-tidy job's own configure. Nine TUs -- the VIHS stub for each batch-A header plus quantity.hpp's stub, examples/forms/gui_qml/FormsController.cpp and tests/test_quantity.cpp for the findings a standalone header stub cannot reach because it instantiates nothing. Before, deduplicated by path+line+column, that reproduces the census row for row: 7 include/morph/version.hpp 4 include/morph/journal/action_log.hpp 4 include/morph/qt/forms/forms_controller_core.hpp 2 include/morph/journal/file_action_log.hpp 1 include/morph/attributes.hpp 1 include/morph/detail/quantity_equation.hpp 1 include/morph/render/i18n.hpp -- 20 After: 0. The same run still reports 261 findings elsewhere under include/morph/, so it analysed the tree rather than failing to. Fixed rather than suppressed, four checks: readability-use-concise-preprocessor-directives (1) -- attributes.hpp's `#if defined(__has_cpp_attribute)` is now `#ifdef`. This is #600's finding. readability-redundant-member-init (4) -- action_log.hpp's `std::string x{}` members drop the initializer. std::string's default constructor is non-trivial, so aggregate and default initialization are unchanged. readability-identifier-length (1) -- file_action_log.hpp's `std::ifstream in` becomes `input`. performance-unnecessary-value-param (4) -- forms_controller_core.hpp's submitIfValid/fetchOptions took `std::string` by value and passed it to executeJson, whose parameters are `std::string_view`. Neither was ever moved, so the copies bought nothing; both are now `const std::string&`, which is source-compatible. examples/bookmarks' mirror moves with it -- its own doc comment asserts it has the same body as this one, and that claim has to stay true. cppcoreguidelines-pro-bounds-avoid-unchecked-container-access (1) -- file_action_log.hpp's `lines[i]` becomes `lines.at(i)`, hoisted out of the try/catch that surrounds it. The loop condition already bounds `i`, so the check cannot fire; if it ever could, std::out_of_range inside that try would be caught and mis-reported as a malformed journal line. Suppressed with a reason, three checks -- the precedents the remaining batches inherit, written to the standard set by render/locale_format.hpp:181: macro-to-enum + macro-usage (7) -- version.hpp's macros are the `#if`-testable half of the version API. An enumerator is invisible to the preprocessor and a constexpr function cannot be called from a `#if`, so the checks do not propose a different spelling, they propose removing the capability. Unfixable by construction; the constants the checks ask for already exist beside them, defined from the macros so the two cannot drift. bugprone-easily-swappable-parameters (1) -- render/i18n.hpp's resolveText. The check is right that swapping derivedKey and schemaLiteral would be silent. They stay because the order is the documented resolution chain, mirrored parameter-for-parameter by DynamicForm.qml's resolveText. misc-header-include-cycle (1) -- quantity_equation.hpp's include back into quantity.hpp. The cycle is real as a graph statement and deliberate as a design: it is closed by `#pragma once` and it is what makes the header analysable standalone. The remedy the check proposes is the state this file was moved away from. No bare NOLINT anywhere: each suppression says why the check is wrong at that site, not what the check is called. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- .../gui_lib/bookmark_forms_controller.hpp | 2 +- include/morph/attributes.hpp | 2 +- include/morph/detail/quantity_equation.hpp | 11 ++++++++ include/morph/journal/action_log.hpp | 8 +++--- include/morph/journal/file_action_log.hpp | 13 +++++++--- .../morph/qt/forms/forms_controller_core.hpp | 5 ++-- include/morph/render/i18n.hpp | 25 ++++++++++++++++++ include/morph/version.hpp | 26 +++++++++++++++++++ 8 files changed, 80 insertions(+), 12 deletions(-) diff --git a/examples/bookmarks/gui_lib/bookmark_forms_controller.hpp b/examples/bookmarks/gui_lib/bookmark_forms_controller.hpp index 45df47506..c0db944b0 100644 --- a/examples/bookmarks/gui_lib/bookmark_forms_controller.hpp +++ b/examples/bookmarks/gui_lib/bookmark_forms_controller.hpp @@ -114,7 +114,7 @@ class BookmarkFormsController { /// @param onReply Success callback. /// @param onError Failure callback. template - void submitIfValid(std::string actionType, std::string bodyJson, OnReply onReply, OnError onError) { + void submitIfValid(const std::string& actionType, const std::string& bodyJson, OnReply onReply, OnError onError) { try { dispatch(actionType, bodyJson) .then( diff --git a/include/morph/attributes.hpp b/include/morph/attributes.hpp index 845eeedc8..3e6a7f267 100644 --- a/include/morph/attributes.hpp +++ b/include/morph/attributes.hpp @@ -24,7 +24,7 @@ /// diagnostic — a build break under the project's `-Werror`. So every use in /// morph goes through this macro, which expands to nothing on a compiler that /// does not know the attribute. -#if defined(__has_cpp_attribute) +#ifdef __has_cpp_attribute #if __has_cpp_attribute(clang::lifetimebound) #define MORPH_LIFETIMEBOUND [[clang::lifetimebound]] #elif __has_cpp_attribute(msvc::lifetimebound) diff --git a/include/morph/detail/quantity_equation.hpp b/include/morph/detail/quantity_equation.hpp index 7806a0727..11b57d7ae 100644 --- a/include/morph/detail/quantity_equation.hpp +++ b/include/morph/detail/quantity_equation.hpp @@ -27,6 +27,17 @@ // opens a header on its own -- clang-tidy analysing a changed header, an IDE, // include-what-you-use -- otherwise sees `unknown type name 'ASTNode'` on // every line and reports a cascade of findings about code that compiles fine. +// +// misc-header-include-cycle sees the back-edge and is, as a graph statement, +// correct: quantity.hpp -> quantity_equation.hpp -> quantity.hpp is a cycle. +// What it cannot see is that the cycle is closed by `#pragma once` on the +// second visit, which is the mechanism the paragraph above relies on rather +// than an accident it survives. The check's remedy -- break the edge -- is the +// state this file was deliberately moved away from, and it would restore the +// standalone-analysis failure the paragraph describes. Suppressed here rather +// than at the definition because this one include is the only cycle in the +// tree; if a second appears, it should be argued for on its own. +// NOLINTNEXTLINE(misc-header-include-cycle) #include "../util/quantity.hpp" #include "../util/rational.hpp" diff --git a/include/morph/journal/action_log.hpp b/include/morph/journal/action_log.hpp index 442df8c95..6ec36b8c5 100644 --- a/include/morph/journal/action_log.hpp +++ b/include/morph/journal/action_log.hpp @@ -74,7 +74,7 @@ struct LogEntry { /// entry — one written before this field existed, or appended directly by /// application code — whose payload shape is unknown and therefore /// unverifiable. See `UnstampedPayloadPolicy` for what replay does with one. - std::string schema{}; + std::string schema; /// @brief JSON-encoded result (`ActionTraits::resultToJson`), captured after /// successful execution. Empty when `outcome == Outcome::Failed`. @@ -88,7 +88,7 @@ struct LogEntry { /// @brief `std::exception::what()` from the exception that rejected the /// action. Empty unless `outcome == Outcome::Failed`. - std::string error{}; + std::string error; /// @brief Auth principal from `morph::session::current()`, if any. Empty if unset. std::string principal; @@ -102,7 +102,7 @@ struct LogEntry { /// `morph::offline::QueueItem::idempotencyKey`'s exact contract: opaque, /// stored verbatim, stable across restarts for one logical outbox row. /// See `journal::OutboxRelay` (`outbox.hpp`) for how it's used. - std::string idempotencyKey{}; + std::string idempotencyKey; /// @brief Line-format version this entry was written at. /// @@ -135,7 +135,7 @@ struct LogEntry { /// entry at the point it is created, independent of whatever `seq` any /// sink later assigns it, and reuse that same identity as every cascaded /// entry's `causalParentId`. - std::string causalParentId{}; + std::string causalParentId; }; } // namespace morph::journal diff --git a/include/morph/journal/file_action_log.hpp b/include/morph/journal/file_action_log.hpp index 33f4ba231..801e9a20d 100644 --- a/include/morph/journal/file_action_log.hpp +++ b/include/morph/journal/file_action_log.hpp @@ -257,8 +257,8 @@ class FileActionLog : public IActionLog { /// @return Matching entries, in on-disk (append) order. [[nodiscard]] std::vector entries(std::string_view entityKey = {}) const override { std::scoped_lock const lock{_mtx}; - std::ifstream in{_path}; - if (!in && std::filesystem::exists(_path)) { + std::ifstream input{_path}; + if (!input && std::filesystem::exists(_path)) { // Distinguish "no journal yet" (absent: legitimately empty, and the // constructor's dedup rebuild depends on that) from "journal present // but unreadable". Returning {} for the second silently empties the @@ -268,16 +268,21 @@ class FileActionLog : public IActionLog { } std::vector lines; std::string line; - while (std::getline(in, line)) { + while (std::getline(input, line)) { if (!line.empty()) { lines.push_back(line); } } std::vector out; for (std::size_t i = 0; i < lines.size(); ++i) { + // .at() outside the try, not lines[i] inside it: the loop condition + // already bounds i, so the bounds check cannot fire, but if it ever + // could its std::out_of_range would be swallowed by the catch below + // and mis-reported as a malformed journal line. + std::string const& rawLine = lines.at(i); LogEntry entry; try { - entry = fromJson(lines[i]); + entry = fromJson(rawLine); } catch (const std::exception& exc) { // A crash between `append`'s `fwrite` and the next flush can leave // a truncated final line. Tolerate exactly that — skip a malformed diff --git a/include/morph/qt/forms/forms_controller_core.hpp b/include/morph/qt/forms/forms_controller_core.hpp index c3c1ec7dc..8ff468e18 100644 --- a/include/morph/qt/forms/forms_controller_core.hpp +++ b/include/morph/qt/forms/forms_controller_core.hpp @@ -93,7 +93,7 @@ class FormsControllerCore { /// @param onReply Success callback. /// @param onError Failure callback. template - void submitIfValid(std::string actionType, std::string bodyJson, OnReply onReply, OnError onError) { + void submitIfValid(const std::string& actionType, const std::string& bodyJson, OnReply onReply, OnError onError) { _handler.executeJson(actionType, bodyJson) .then([onReply = std::move(onReply)](std::string resultJson) mutable { onReply(std::move(resultJson)); }) .onError([onError = std::move(onError)](const std::exception_ptr& err) mutable { onError(err); }); @@ -114,7 +114,8 @@ class FormsControllerCore { /// @param onReply Success callback. /// @param onError Failure callback. template - void fetchOptions(std::string optionsAction, std::string bodyJson, OnReply onReply, OnError onError) { + void fetchOptions(const std::string& optionsAction, const std::string& bodyJson, OnReply onReply, + OnError onError) { _handler.executeJson(optionsAction, bodyJson) .then([onReply = std::move(onReply)](std::string resultJson) mutable { onReply(std::move(resultJson)); }) .onError([onError = std::move(onError)](const std::exception_ptr& err) mutable { onError(err); }); diff --git a/include/morph/render/i18n.hpp b/include/morph/render/i18n.hpp index 83b4ed371..3bd723937 100644 --- a/include/morph/render/i18n.hpp +++ b/include/morph/render/i18n.hpp @@ -51,9 +51,34 @@ using TranslationProvider = /// @param derivedKey The mechanically-derived key for this slot. /// @param schemaLiteral The schema's authored fallback text for this slot. /// @return The resolved display text. +// NOLINTBEGIN(bugprone-easily-swappable-parameters) +// `derivedKey` and `schemaLiteral` are adjacent `std::string_view`s, and the +// check is right that swapping them would be silent and wrong: the derived key +// would be rendered to the user as display text, and the schema's authored +// title would be looked up in the catalog, miss, and fall back to the key. No +// type or arity error would catch it. +// +// They stay in this order because the order *is* the contract. The three +// parameters are the resolution chain in precedence order -- explicit key, then +// derived key, then the schema literal as the fallback -- which is how the +// @brief above states it, how the body below tries them, and how +// docs/spec/forms/forms.md specifies it. The QML renderer carries a mirror of +// this function with the same parameters in the same order +// (src/qt/forms/qml/DynamicForm.qml, `resolveText(explicitKey, derivedKey, +// literal)`), and the two are meant to be read against each other. Reordering +// to break the adjacency here would desynchronise that pair and leave the +// signature the only place in the stack that does not read as the chain -- +// trading a mistake that no caller in the tree is positioned to make for one a +// reader of both renderers would. +// +// Strong types would remove the hazard outright, but a `TranslationKey` wrapper +// on this seam would have to be threaded through every caller in morph::forms, +// which is a design change to the renderer boundary and not a lint fix. If that +// is ever done, delete this suppression rather than widening it. [[nodiscard]] inline std::string resolveText(const TranslationProvider& provider, std::string_view bcp47Locale, const std::optional& explicitKey, std::string_view derivedKey, std::string_view schemaLiteral) { + // NOLINTEND(bugprone-easily-swappable-parameters) if (provider) { if (explicitKey.has_value()) { if (auto hit = provider(*explicitKey, bcp47Locale)) { diff --git a/include/morph/version.hpp b/include/morph/version.hpp index 2efb4d48c..89164df66 100644 --- a/include/morph/version.hpp +++ b/include/morph/version.hpp @@ -3,6 +3,30 @@ #pragma once #include +// The five macros below are the `#if`-testable half of morph's version API, and +// being macros is the whole of what they are for. A downstream compiling against +// two morph releases writes +// +// #if MORPH_VERSION >= MORPH_MAKE_VERSION(1, 2, 0) +// +// which the preprocessor has to answer before any C++ declaration exists. An +// enumerator or a `constexpr int` is invisible in a `#if`, and a `constexpr` +// function cannot be called from one, so `MORPH_MAKE_VERSION` has to be +// function-like for the same reason. The three checks below do not propose a +// different spelling of this header; they propose removing the capability it +// exists to provide. That is the meaning of "unfixable by construction", and it +// is why this is a suppression rather than a deferral -- there is no later +// version of this file in which the finding goes away. +// +// The C++-visible constants the checks ask for do exist: `morph::version::kMajor` +// and friends, below, defined *from* these macros so the two cannot drift. Code +// that does not need a `#if` should use those, and this header offering both is +// the resolution, not a duplication. +// +// docs/spec/VERSIONING.md is the contract; tests/test_version.cpp pins these +// against the top-level `project(morph VERSION ...)`. +// NOLINTBEGIN(cppcoreguidelines-macro-to-enum, modernize-macro-to-enum, cppcoreguidelines-macro-usage) + /// @brief morph's major version component (semantic-versioning MAJOR: /// incremented for a breaking source change to the stable public surface — /// see docs/spec/VERSIONING.md). @@ -31,6 +55,8 @@ /// `CMakeLists.txt` — the two are cross-checked by `tests/test_version.cpp`. #define MORPH_VERSION MORPH_MAKE_VERSION(MORPH_VERSION_MAJOR, MORPH_VERSION_MINOR, MORPH_VERSION_PATCH) +// NOLINTEND(cppcoreguidelines-macro-to-enum, modernize-macro-to-enum, cppcoreguidelines-macro-usage) + namespace morph::version { /// @brief morph's major version component, as a compile-time constant. See `MORPH_VERSION_MAJOR`.