fix: Remaining clang-tidy unchecked optionals - #6979
Conversation
godexsoft
added
the
DraftRunCI
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6979 +/- ##
=========================================
- Coverage 82.5% 81.9% -0.7%
=========================================
Files 1010 1010
Lines 79246 76395 -2851
Branches 7529 7529
=========================================
- Hits 65389 62532 -2857
- Misses 13857 13863 +6
... and 413 files with indirect coverage changes 🚀 New features to boost your workflow:
|
9185926 to
b8fe16f
Compare
Fix level More clang-tidy fixes Fix more from ci Autofixes on linux Fix format Bring back original shell Fix remaining clang-tidy More
16005c0 to
3303daf
Compare
godexsoft
removed
the
DraftRunCI
There was a problem hiding this comment.
Pull request overview
This PR enables the bugprone-unchecked-optional-access clang-tidy check and updates the codebase to address the remaining findings, primarily by adding targeted NOLINT annotations and a few small runtime guards where needed.
Changes:
- Enable
bugprone-unchecked-optional-accessin.clang-tidy. - Add
NOLINTsuppressions around optional dereferences that are currently treated as invariants/assert-protected. - Add a small number of functional fixes to avoid unchecked dereferences (e.g., extra null/optional checks).
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/rpc/detail/RPCLedgerHelpers.cpp | Adds NOLINTBEGIN/END around refHash dereferences in ledger acquisition flow. |
| src/xrpld/rpc/detail/PathRequest.cpp | Adds NOLINT annotations around raSrcAccount/raDstAccount dereferences after isValid(). |
| src/xrpld/perflog/detail/PerfLogImp.cpp | Removes an unused include. |
| src/xrpld/overlay/detail/PeerImp.cpp | Adds NOLINT around assert-guarded optional dereferences. |
| src/xrpld/overlay/detail/OverlayImpl.cpp | Adds NOLINTBEGIN/END around manifest optional access guarded by assert. |
| src/xrpld/consensus/LedgerTrie.h | Adds NOLINT for optional access guarded by asserts/invariants. |
| src/xrpld/consensus/Consensus.h | Adds NOLINT for result_ dereferences and minor formatting/operator-token changes. |
| src/xrpld/app/misc/detail/ValidatorSite.cpp | Adds NOLINT for optional port dereference (documented as defaulted earlier). |
| src/xrpld/app/misc/detail/ValidatorList.cpp | Adds NOLINT around optional publisher key/max sequence accesses. |
| src/xrpld/app/misc/detail/TxQ.cpp | Avoids dereferencing an empty optional fee level (changes fallback behavior). |
| src/xrpld/app/misc/TxQ.h | Adds NOLINT comments for pfResult invariants. |
| src/xrpld/app/main/Application.cpp | Adds NOLINT comments for invariant/assert-protected member dereferences. |
| src/xrpld/app/ledger/LedgerHistory.cpp | Adds NOLINT around consensus.value() access (documented invariant). |
| src/test/protocol/Hooks_test.cpp | Wraps optional deref with NOLINTBEGIN/END. |
| src/test/jtx/mpt.h | Adds NOLINT for optional issuance ID dereference after test assertion. |
| src/test/jtx/impl/TestHelpers.cpp | Adds missing null check (po.object) and adds NOLINT on optional path element deref. |
| src/test/jtx/TrustedPublisherServer.h | Adds NOLINT around optional dereferences in test server helper. |
| src/test/core/Config_test.cpp | Adds protocol include (and keeps it via IWYU pragma). |
| src/test/app/GRPCServerTLS_test.cpp | Adds NOLINT around optional port dereferences after has_value() checks. |
| src/test/app/AccountTxPaging_test.cpp | Removes unused include. |
| src/test/app/AMMMPT_test.cpp | Adds NOLINTBEGIN/END around optional AMM dereferences in tests. |
| src/libxrpl/tx/transactors/payment/DepositPreauth.cpp | Adds NOLINT on conditional optional dereference. |
| src/libxrpl/tx/transactors/dex/AMMVote.cpp | Adds NOLINTBEGIN/END around optional dereference guarded by loop/invariant. |
| src/libxrpl/tx/transactors/dex/AMMBid.cpp | Adds NOLINTBEGIN/END around assert-guarded optional dereference. |
| src/libxrpl/tx/paths/OfferStream.cpp | Adds a guard to avoid using ownerFunds_ when not set. |
| src/libxrpl/tx/paths/MPTEndpointStep.cpp | Adds NOLINT around cache optional dereferences under documented invariants/asserts. |
| src/libxrpl/tx/paths/Flow.cpp | Adds NOLINT for sandbox optional deref on success path. |
| src/libxrpl/tx/paths/DirectStep.cpp | Adds NOLINT around cache optional dereferences under documented invariants/asserts. |
| src/libxrpl/tx/paths/BookStep.cpp | Adds NOLINT around quality optional dereference and cache invariants. |
| src/libxrpl/tx/invariants/AMMInvariant.cpp | Adds NOLINTBEGIN/END around invariant-protected optionals. |
| src/libxrpl/tx/ApplyContext.cpp | Adds NOLINT around view optional dereferences. |
| src/libxrpl/shamap/SHAMapInnerNode.cpp | Adds NOLINT around optional child-index accesses guarded by asserts/flags. |
| src/libxrpl/server/Manifest.cpp | Adds NOLINT around signingKey optional dereferences under documented invariants. |
| src/libxrpl/net/HTTPClient.cpp | Adds NOLINT around global optional SSL context access. |
| include/xrpl/tx/paths/detail/StrandFlow.h | Adds NOLINTBEGIN/END around cachedIn/cachedOut optional accesses. |
| include/xrpl/tx/paths/OfferStream.h | Adds NOLINT for ownerFunds_ dereference (documented invariant). |
| include/xrpl/tx/ApplyContext.h | Adds NOLINT for view optional dereferences. |
| include/xrpl/protocol/PublicKey.h | Fixes unchecked optional use by validating strUnHex result before deref. |
| .github/scripts/levelization/results/ordering.txt | Updates test layering to include xrpl.protocol. |
| .clang-tidy | Enables bugprone-unchecked-optional-access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto const refIndex = getCandidateLedger(ledgerIndex); | ||
| auto refHash = hashOfSeq(*ledger, refIndex, j); | ||
| XRPL_ASSERT(refHash, "xrpl::RPC::getOrAcquireLedger : nonzero ledger hash"); | ||
|
|
||
| // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above | ||
| ledger = ledgerMaster.getLedgerByHash(*refHash); | ||
| if (!ledger) | ||
| { | ||
| // We don't have the ledger we need to figure out which | ||
| // ledger they want. Try to get it. | ||
|
|
||
| if (auto il = context.app.getInboundLedgers().acquire( | ||
| *refHash, refIndex, InboundLedger::Reason::GENERIC)) | ||
| { | ||
| Json::Value jvResult = RPC::make_error( | ||
| rpcLGR_NOT_FOUND, "acquiring ledger containing requested index"); | ||
| jvResult[jss::acquiring] = getJson(LedgerFill(*il, &context)); | ||
| return Unexpected(jvResult); | ||
| } | ||
|
|
||
| if (auto il = context.app.getInboundLedgers().find(*refHash)) | ||
| // NOLINTEND(bugprone-unchecked-optional-access) |
There was a problem hiding this comment.
refHash is an std::optional from hashOfSeq(). The only guard here is XRPL_ASSERT(refHash, ...), which compiles out under NDEBUG, so *refHash can be dereferenced when empty in release builds (UB/crash). Please replace the assert+NOLINT with a real runtime check/early-return error path (similar to the neededHash handling above) before using *refHash in getLedgerByHash() / acquire() / find().
There was a problem hiding this comment.
We have an XRPL_ASSERT that suggests it should never be nullptr. however clang-tidy does not understand it. so for now we nolint this
| if (!sumNlast.first) | ||
| return {sumNlast.first, FeeLevel64{sumNlast.second}}; | ||
| auto const totalFeeLevel = | ||
| mulDiv(multiplier, sumNlast.second - sumNcurrent.second, target * target); | ||
|
|
||
| return {totalFeeLevel.has_value(), *totalFeeLevel}; | ||
| return {totalFeeLevel.has_value(), totalFeeLevel.value_or(FeeLevel64{})}; | ||
| } |
There was a problem hiding this comment.
When mulDiv(...) overflows it returns std::nullopt. Returning FeeLevel64{} (0) in that case is likely incorrect/misleading (and differs from nearby patterns that saturate to muldiv_max / std::numeric_limits<...>::max() on overflow). Consider returning a saturated maximum fee level for the second element when totalFeeLevel is empty, while keeping the boolean flag to indicate overflow.
There was a problem hiding this comment.
I think nolint is actually correct here. value_or changed behaviour so let's not do that.
| std::optional<STPathElement> const& srcElement, | ||
| std::optional<AccountID> const& srcIssuer, | ||
| std::optional<uint256> const& domain) | ||
| { | ||
| return find_paths( | ||
| env, src, dst, saDstAmount, saSendMax, srcElement->getPathAsset(), srcIssuer, domain); | ||
| env, | ||
| src, | ||
| dst, | ||
| saDstAmount, | ||
| saSendMax, | ||
| srcElement->getPathAsset(), // NOLINT(bugprone-unchecked-optional-access) callers always | ||
| // pass non-null srcElement | ||
| srcIssuer, | ||
| domain); |
There was a problem hiding this comment.
find_paths_by_element takes srcElement as std::optional<STPathElement> with a default of std::nullopt, but unconditionally dereferences it (srcElement->...). This makes the API foot-gun-y and can crash if callers ever use the default. Please either (1) make srcElement a required parameter (non-optional, no default) or (2) add an explicit if (!srcElement) ... handling/assert before dereferencing.
There was a problem hiding this comment.
I don't think we can change it because optional is used to provide default values for surrounding params. It was intentional and i think that it is expected that it wont be nullopt. I added an XRPL_ASSERT to make it obvious
There was a problem hiding this comment.
Tbf it is only called from one test as far as i can see. Let's come back to this when we handle nolints across the codebase.
godexsoft
added
the
Ready to merge
|
|
||
| // NOLINTBEGIN(bugprone-unchecked-optional-access) cachedIn/Out set after strand is stepped | ||
| // above | ||
| auto const strandIn = *strand.front()->cachedIn(); |
There was a problem hiding this comment.
Maybe add assert for NOLINT places?
There was a problem hiding this comment.
That's probably a good idea but i was thinking we will deal with nolints in a separate PR entirely. I'd like to get as many checks working first so we prevent more bad code being added, then work on the things we did not figure out while enabling checks
| // NOLINTBEGIN(bugprone-unchecked-optional-access) acctTxCount > 0 implies txIter is set | ||
| if (acctTxCount == 1 && txIter->first->second.consequences().isBlocker() && |
There was a problem hiding this comment.
Should the if-statement here be changed from acctTxCount == 1 to acctTxCount > 0`, or should the comment be changed to reflect the if-condition?
There was a problem hiding this comment.
I guess the comment should change as the code stayed the same as it was
|
|
||
| if ((ahead == 0u) || (laggards == 0u) || (totalValidators == 0u) || !adaptor_.validator() || | ||
| !adaptor_.haveValidated() || result_->roundTime.read() > parms.ledgerMAX_CONSENSUS) | ||
| if ((ahead == 0u) or (laggards == 0u) or (totalValidators == 0u) or not adaptor_.validator() or |
There was a problem hiding this comment.
Why is or and not used here instead of || and !?
There was a problem hiding this comment.
It's because it reads a bit more like English this way - my personal preference. I can roll it back but we do the same in Clio when it makes sense
There was a problem hiding this comment.
I'd like consistency. I don't mind || or or for instance, but as long as the chosen one is used everywhere. Right now this just looks like Python mixed in with C++, so let's roll it back.
There was a problem hiding this comment.
I disagree, imo it's ok to have this particular inconsistency when it makes it read better. I guess this case is not really a good candidate so i have no issue rolling it back here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
High Level Overview of Change
This PR fixes remaining issues from
bugprone-unchecked-optional-access.API Impact
No impact.