build: Don't apply clang-tidy fixes twice via header symlinks (#8636)

.clang-tidy

@@ -140,5 +140,9 @@ CheckOptions:
   readability-identifier-naming.GlobalFunctionIgnoredRegexp: "^(to_string|hash_append|tuple_hash)$"
 
 HeaderFilterRegex: '^.*/(tests?|xrpl|xrpld)/.*\.(h|hpp|ipp)$'
-ExcludeHeaderFilterRegex: '^.*/protocol_autogen/.*\.(h|hpp)$|^.*\.pb\.h$'
+# Skip headers reached via add_module's symlinks (<build-dir>/modules/xrpl.lib*/...).
+# verify_headers already checks each one under its real path,
+# so reporting it twice would also apply its fixes twice.
+# .ipp files are kept: they are only ever reached via the symlink.
+ExcludeHeaderFilterRegex: '^.*/protocol_autogen/.*\.(h|hpp)$|^.*\.pb\.h$|^.*/modules/xrpl\.lib[^/]+/.*\.(h|hpp)$'
 WarningsAsErrors: "*"

CONTRIBUTING.md

@@ -365,14 +365,15 @@ With this set, the hook will run as part of `git commit` alongside the other pre
 Then run clang-tidy on your local changes:
 
 ```
-run-clang-tidy -p build -allow-no-checks src tests
+run-clang-tidy -p build -allow-no-checks include src tests
 ```
 
-This will check all source files in the `src`, `include` and `tests` directories using the compile commands from your `build` directory.
+This will check all source files in the `include`, `src` and `tests` directories using the compile commands from your `build` directory.
+Headers are checked on their own, which requires the `verify_headers` CMake option (ON by default).
 If you wish to automatically fix whatever clang-tidy finds _and_ is capable of fixing, add `-fix -format` to the above command:
 
 ```
-run-clang-tidy -p build -quiet -fix -format -allow-no-checks src tests
+run-clang-tidy -p build -quiet -fix -format -allow-no-checks include src tests
 ```
 
 `-format` reformats the fixed code with [`.clang-format`](./.clang-format); without it the fixes are inserted in LLVM style and the `clang-format` hook rewrites them afterwards.

bin/pre-commit/clang_tidy_check.py

@@ -5,15 +5,6 @@ The script determines the staged files itself (see `pass_filenames: false` in
 .pre-commit-config.yaml) so run-clang-tidy is run once and handles parallelism
 internally: pre-commit would otherwise split the files across parallel hook
 invocations that race when fixes edit a shared header.
-
-Fixes are collected with `-export-fixes` and applied by clang-apply-replacements
-in a separate step rather than with run-clang-tidy's `-fix`. The `add_module`
-build isolates each module's headers behind a per-module symlink directory
-(build/modules/<module>/...), so a header reachable from several translation
-units is referenced through different paths that all resolve to the same source
-file. clang-apply-replacements deduplicates identical replacements by their
-literal path, so those paths must be canonicalised to the real source path
-first; otherwise the same fix is applied once per path and corrupts the header.
 """
 
 from __future__ import annotations
@@ -23,7 +14,6 @@ import re
 import shutil
 import subprocess
 import sys
-import tempfile
 from pathlib import Path
 
 # Extensions run-clang-tidy can analyse: `.cpp` translation units and, thanks to
@@ -31,11 +21,6 @@ from pathlib import Path
 # compile_commands.json entry). `.ipp` fragments have no entry and are skipped.
 TIDY_EXTENSIONS = {".cpp", ".h", ".hpp"}
 
-# A single-quoted `FilePath:` entry in an -export-fixes YAML file, allowing the
-# `- ` marker that precedes it inside a `Replacements:` sequence. clang-tidy
-# emits paths single-quoted and doubles any embedded quote per YAML rules.
-FILEPATH_RE = re.compile(r"^(\s*(?:-\s+)?FilePath:\s*)'((?:[^']|'')*)'\s*$")
-
 
 def clang_tidy_major() -> str | None:
     """Major version of the `clang-tidy` on PATH, which run-clang-tidy invokes."""
@@ -79,25 +64,6 @@ def staged_files(repo_root: Path) -> list[Path]:
     return [repo_root / rel for rel in output.splitlines() if rel]
 
 
-def canonicalize_fix_paths(fixes_dir: Path) -> None:
-    """Rewrite every `FilePath` in the exported fixes to its real source path.
-
-    A header included through a module's isolation symlink is recorded under that
-    symlink's path; collapsing all paths to the same real file lets
-    clang-apply-replacements recognise the per-translation-unit duplicates and
-    apply each fix once.
-    """
-    for yaml in fixes_dir.glob("*.yaml"):
-        lines = []
-        for line in yaml.read_text().splitlines():
-            if m := FILEPATH_RE.match(line):
-                path = m.group(2).replace("''", "'")
-                real = os.path.realpath(path).replace("'", "''")
-                line = f"{m.group(1)}'{real}'"
-            lines.append(line)
-        yaml.write_text("\n".join(lines) + "\n")
-
-
 def main():
     if not os.environ.get("TIDY"):
         return 0
@@ -116,19 +82,10 @@ def main():
 
     version = clang_tidy_major()
     run_clang_tidy = find_tool("run-clang-tidy", version)
-    clang_apply_replacements = find_tool("clang-apply-replacements", version)
-    missing = [
-        name
-        for name, path in (
-            ("run-clang-tidy", run_clang_tidy),
-            ("clang-apply-replacements", clang_apply_replacements),
-        )
-        if not path
-    ]
-    if missing:
+    if not run_clang_tidy:
         tried = f" (tried the '-{version}' suffix too)" if version else ""
         print(
-            f"clang-tidy check failed: TIDY is enabled but {' and '.join(missing)} "
+            f"clang-tidy check failed: TIDY is enabled but run-clang-tidy "
             f"was not found in PATH{tried}.",
             file=sys.stderr,
         )
@@ -143,27 +100,19 @@ def main():
         )
         return 1
 
-    with tempfile.TemporaryDirectory() as fixes_dir:
-        result = subprocess.run(
-            [
-                run_clang_tidy,
-                "-quiet",
-                "-p",
-                build_dir,
-                "-export-fixes",
-                fixes_dir,
-                "-allow-no-checks",
-            ]
-            + files
-        )
-        canonicalize_fix_paths(Path(fixes_dir))
-        # `FormatStyle` in .clang-tidy does not reach this path,
-        # so ask for the repository style here.
-        applied = subprocess.run(
-            [clang_apply_replacements, "--format", "--style=file", fixes_dir]
-        )
-
-    return result.returncode or applied.returncode
+    result = subprocess.run(
+        [
+            run_clang_tidy,
+            "-quiet",
+            "-p",
+            build_dir,
+            "-fix",
+            "-format",
+            "-allow-no-checks",
+        ]
+        + files
+    )
+    return result.returncode
 
 
 if __name__ == "__main__":

cmake/isolate_headers.cmake

@@ -34,11 +34,12 @@ include(create_symbolic_link)
 # sometimes `PUBLIC` and sometimes `PRIVATE`.
 # The Cs are all guaranteed to be unique.
 # We can guarantee a unique X per target by using
-# `${CMAKE_CURRENT_BINARY_DIR}/include/${target}`.
+# `${CMAKE_CURRENT_BINARY_DIR}/modules/${target}`.
 #
 # isolate_headers(target A B scope)
 function(isolate_headers target A B scope)
     file(RELATIVE_PATH C "${A}" "${B}")
+    # Keep in sync with ExcludeHeaderFilterRegex in .clang-tidy.
     set(X "${CMAKE_CURRENT_BINARY_DIR}/modules/${target}")
     set(Y "${X}/${C}")
     cmake_path(GET Y PARENT_PATH parent)

include/xrpl/tx/ApplyContext.h

@@ -54,10 +54,6 @@ public:
         XRPL_ASSERT((flags & TapBatch) == 0, "Batch apply flag should not be set");
     }
 
-    // viewRef_ points into view_, so a copied or moved context would alias the original.
-    ApplyContext(ApplyContext const&) = delete;
-    ApplyContext(ApplyContext&&) = delete;
-
     std::reference_wrapper<ServiceRegistry> registry;
     STTx const& tx;
     TER const preclaimResult;
@@ -67,20 +63,20 @@ public:
     ApplyView&
     view()
     {
-        return viewRef_.get();
+        return *view_;  // NOLINT(bugprone-unchecked-optional-access) view_ emplaced in constructor
     }
 
     [[nodiscard]] ApplyView const&
     view() const
     {
-        return viewRef_.get();
+        return *view_;  // NOLINT(bugprone-unchecked-optional-access) view_ emplaced in constructor
     }
 
     // VFALCO Unfortunately this is necessary
     RawView&
     rawView()
     {
-        return viewRef_.get();
+        return *view_;  // NOLINT(bugprone-unchecked-optional-access) view_ emplaced in constructor
     }
 
     [[nodiscard]] ApplyFlags const&
@@ -95,7 +91,8 @@ public:
     void
     deliver(STAmount const& amount)
     {
-        viewRef_.get().deliver(amount);
+        // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor
+        view_->deliver(amount);
     }
 
     /**
@@ -129,7 +126,8 @@ public:
     void
     destroyXRP(XRPAmount const& fee)
     {
-        viewRef_.get().rawDestroyXRP(fee);
+        // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor
+        view_->rawDestroyXRP(fee);
     }
 
     /**
@@ -143,16 +141,16 @@ public:
     ApplyViewContext
     getApplyViewContext()
     {
-        return {.view = viewRef_.get(), .tx = tx};
+        XRPL_ASSERT(
+            view_.has_value(),
+            "xrpl::ApplyContext::getApplyViewContext : view_ emplaced in constructor");
+        return {.view = *view_, .tx = tx};
     }
 
 private:
     OpenView& base_;
     ApplyFlags flags_;
     std::optional<ApplyViewImpl> view_;
-    // Always refers to the engaged view_. Reseated whenever view_ is re-emplaced, so the
-    // accessors need no optional check.
-    std::reference_wrapper<ApplyViewImpl> viewRef_;
 
     // The ID of the batch transaction we are executing under, if set.
     std::optional<UInt256 const> parentBatchId_;

nix/packages.nix

@@ -62,8 +62,8 @@ let
   # version-suffixed name first and fall back to a system binary on the PATH
   # when Nix doesn't provide it:
   #   - Conan's Boost recipe looks up `g++-<major>` before plain `g++`.
-  #   - bin/pre-commit/clang_tidy_check.py looks up `run-clang-tidy-<v>` and
-  #     `clang-apply-replacements-<v>` before the unsuffixed names.
+  #   - bin/pre-commit/clang_tidy_check.py looks up `run-clang-tidy-<v>`
+  #     before the unsuffixed name.
   # On a host that also has the matching system binary
   # (e.g. Ubuntu's `/usr/bin/g++-<v>` or `clang-tidy-<v>`),
   # the probe escapes Nix and mixes a system tool into the Nix environment.

src/libxrpl/tx/ApplyContext.cpp

@@ -35,25 +35,25 @@ ApplyContext::ApplyContext(
     , journal(journal)
     , base_(base)
     , flags_(flags)
-    , viewRef_(view_.emplace(&base_, flags_))
     , parentBatchId_(parentBatchId)
 {
     XRPL_ASSERT(
         parentBatchId.has_value() == ((flags_ & TapBatch) == TapBatch),
         "Parent Batch ID should be set if batch apply flag is set");
+    view_.emplace(&base_, flags_);
 }
 
 void
 ApplyContext::discard()
 {
-    viewRef_ = view_.emplace(&base_, flags_);
+    view_.emplace(&base_, flags_);
 }
 
 std::optional<TxMeta>
 ApplyContext::apply(TER ter)
 {
-    return viewRef_.get().apply(
-        base_, tx, ter, parentBatchId_, (flags_ & TapDryRun) != 0u, journal);
+    // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor
+    return view_->apply(base_, tx, ter, parentBatchId_, (flags_ & TapDryRun) != 0u, journal);
 }
 
 void
@@ -66,14 +66,14 @@ ApplyContext::addOrderBook(Book const& book)
 std::size_t
 ApplyContext::size()
 {
-    return viewRef_.get().size();
+    return view_->size();  // NOLINT(bugprone-unchecked-optional-access)
 }
 
 void
 ApplyContext::visit(
     std::function<void(UInt256 const&, bool, SLE::ConstRef, SLE::ConstRef)> const& func)
 {
-    viewRef_.get().visit(base_, func);
+    view_->visit(base_, func);  // NOLINT(bugprone-unchecked-optional-access)
 }
 
 }  // namespace xrpl

src/tests/libxrpl/helpers/TxTest.cpp

@@ -7,6 +7,7 @@
 #include <xrpl/ledger/CanonicalTXSet.h>
 #include <xrpl/ledger/Ledger.h>
 #include <xrpl/ledger/OpenView.h>
+#include <xrpl/ledger/ReadView.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Fees.h>
@@ -173,7 +174,7 @@ TxTest::getOpenLedger() const
     return *openLedger_;
 }
 
-Ledger const&
+ReadView const&
 TxTest::getClosedLedger() const
 {
     return *closedLedger_;

src/tests/libxrpl/helpers/TxTest.h

@@ -7,6 +7,7 @@
 #include <xrpl/core/ServiceRegistry.h>
 #include <xrpl/ledger/Ledger.h>
 #include <xrpl/ledger/OpenView.h>
+#include <xrpl/ledger/ReadView.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/LedgerFormats.h>
@@ -298,7 +299,7 @@ public:
      * @brief Get the closed (base) ledger view.
      * @return A const reference to the closed ledger.
      */
-    [[nodiscard]] Ledger const&
+    [[nodiscard]] ReadView const&
     getClosedLedger() const;
 
     /**

src/tests/libxrpl/tx/ApplyContext.cpp

@@ -1,115 +0,0 @@
-#include <xrpl/tx/ApplyContext.h>
-
-#include <xrpl/ledger/ApplyView.h>
-#include <xrpl/ledger/Ledger.h>
-#include <xrpl/ledger/OpenView.h>
-#include <xrpl/protocol/Indexes.h>
-#include <xrpl/protocol/SField.h>
-#include <xrpl/protocol/STAmount.h>
-#include <xrpl/protocol/STLedgerEntry.h>
-#include <xrpl/protocol/STTx.h>
-#include <xrpl/protocol/SeqProxy.h>
-#include <xrpl/protocol/TER.h>
-#include <xrpl/protocol/TxMeta.h>
-#include <xrpl/protocol/XRPAmount.h>
-#include <xrpl/protocol_autogen/transactions/AccountSet.h>
-
-#include <gtest/gtest.h>
-#include <helpers/Account.h>
-#include <helpers/TxTest.h>
-
-#include <cstdint>
-#include <memory>
-
-namespace xrpl::test {
-
-/**
- * A funded alice over an open ledger, plus a signed transaction to build an
- * ApplyContext around.
- */
-class ApplyContextTests : public ::testing::Test
-{
-protected:
-    TxTest env_;
-    Account const alice_{"alice"};
-    std::shared_ptr<STTx const> tx_;
-
-    ApplyContextTests()
-    {
-        env_.createAccount(alice_, XRP(10'000));
-        transactions::AccountSetBuilder builder{alice_.id()};
-        builder.setSequence(env_.getAccountRoot(alice_.id()).getSequence());
-        builder.setFee(XRPAmount(10));
-        tx_ = builder.build(alice_.pk(), alice_.sk()).getSTTx();
-    }
-
-    /**
-     * Build a ticket entry that is not in the ledger.
-     *
-     * @param seq The raw ticket sequence.
-     * @return The new entry.
-     */
-    SLE::pointer
-    makeTicket(std::uint32_t seq) const
-    {
-        auto sle = std::make_shared<SLE>(keylet::ticket(alice_.id(), SeqProxy::rawTicket(seq)));
-        sle->setAccountID(sfAccount, alice_.id());
-        sle->setFieldU32(sfTicketSequence, seq);
-        sle->setFieldU64(sfOwnerNode, 0);
-        return sle;
-    }
-};
-
-TEST_F(ApplyContextTests, discard_drops_pending_state_and_keeps_the_context_usable)
-{
-    ASSERT_NE(tx_, nullptr);
-    // A closed base makes apply() produce metadata, which an open ledger
-    // does not.
-    OpenView open(&env_.getClosedLedger());
-    ApplyContext ctx(env_.getServiceRegistry(), open, *tx_, tesSUCCESS, XRPAmount{10}, TapRetry);
-
-    // Buffer an inserted entry, a modified entry, destroyed drops and a
-    // delivered amount through the context's own surface.
-    auto const stale = makeTicket(1);
-    ctx.view().insert(stale);
-    auto const account = ctx.view().peek(keylet::account(alice_.id()));
-    ASSERT_NE(account, nullptr);
-    auto const baseSeq = account->getFieldU32(sfSequence);
-    account->setFieldU32(sfSequence, baseSeq + 1);
-    ctx.view().update(account);
-    ctx.destroyXRP(XRPAmount{10});
-    ctx.deliver(STAmount{XRPAmount{5}});
-    ASSERT_EQ(ctx.size(), 2);
-
-    ctx.discard();
-
-    // Reads fall through to the open ledger again and the flags survive.
-    EXPECT_EQ(ctx.size(), 0);
-    EXPECT_FALSE(ctx.view().exists(keylet::unchecked(stale->key())));
-    auto const reread = ctx.view().read(keylet::account(alice_.id()));
-    ASSERT_NE(reread, nullptr);
-    EXPECT_EQ(reread->getFieldU32(sfSequence), baseSeq);
-    EXPECT_EQ(ctx.view().flags(), TapRetry);
-
-    // Only changes made after the discard reach the target view.
-    auto const fresh = makeTicket(2);
-    ctx.view().insert(fresh);
-    ctx.destroyXRP(XRPAmount{3});
-    auto const meta = ctx.apply(tesSUCCESS);
-    ASSERT_TRUE(meta.has_value());
-    EXPECT_FALSE(meta->getDeliveredAmount().has_value());
-    EXPECT_FALSE(open.exists(keylet::unchecked(stale->key())));
-    EXPECT_TRUE(open.exists(keylet::unchecked(fresh->key())));
-    auto const applied = open.read(keylet::account(alice_.id()));
-    ASSERT_NE(applied, nullptr);
-    EXPECT_EQ(applied->getFieldU32(sfSequence), baseSeq);
-
-    // OpenView buffers the burn without touching its header, so flush into a
-    // real Ledger to see it: 3 drops are gone, not 13.
-    Ledger const& prev = env_.getClosedLedger();
-    Ledger next(prev, prev.header().closeTime + prev.header().closeTimeResolution);
-    open.apply(next);
-    EXPECT_EQ(next.header().drops, prev.header().drops - XRPAmount{3});
-}
-
-}  // namespace xrpl::test