https://github.com/BaLiKfromUA updated https://github.com/llvm/llvm-project/pull/186363
>From 66386dd0ee217fdf0e477cc9599aa7a562f77acd Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Mon, 2 Mar 2026 12:23:50 +0000 Subject: [PATCH 01/15] add failing test --- .../UncheckedOptionalAccessModelTest.cpp | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index ba509e875ef9d..0cd122a9c3061 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -106,6 +106,7 @@ class UncheckedOptionalAccessTest #include "std_optional.h" #include "std_string.h" #include "std_utility.h" + #include "testing_defs.h" template <typename T> T Make(); @@ -2896,6 +2897,27 @@ TEST_P(UncheckedOptionalAccessTest, DiagnosticsHaveRanges) { )cc"); } +TEST_P(UncheckedOptionalAccessTest, AssertTrueGtestMacro) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional<int> opt; + ASSERT_TRUE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); + + $ns::$optional<int> opt1; + ASSERT_TRUE(opt1); + EXPECT_EQ(*opt1, 42); + + $ns::$optional<int> opt2; + EXPECT_TRUE(opt2.has_value()); + EXPECT_EQ(opt2.value(), 42); // [[unsafe]] + } + )cc"); +} + + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) >From f065c47bf1644d8c6ea40359334f5b6ff31bbcd4 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Thu, 12 Mar 2026 23:59:11 +0000 Subject: [PATCH 02/15] test is passing, initial implementation --- .../Models/UncheckedOptionalAccessModel.cpp | 97 ++++++++++++++++++- .../UncheckedOptionalAccessModelTest.cpp | 43 ++++++-- 2 files changed, 132 insertions(+), 8 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 04a2a557debb2..0c3e5f25c59fd 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -115,6 +115,18 @@ static bool isSupportedOptionalType(QualType Ty) { return Optional != nullptr; } +static bool isAssertionResultType(QualType Type) { + if (Type.isNull()) + return false; + + if (auto *RD = Type->getAsRecordDecl()) + if (RD->getName() == "AssertionResult") + if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD->getDeclContext())) + return isFullyQualifiedNamespaceEqualTo(*N, "testing"); + + return false; +} + namespace { using namespace ::clang::ast_matchers; @@ -909,6 +921,75 @@ valueOperatorCall(const std::optional<StatementMatcher> &IgnorableOptional) { isOptionalOperatorCallWithName("->", IgnorableOptional))); } +auto isAssertionResultOperatorBoolCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxMemberCallExpr( + on(expr(unless(cxxThisExpr()))), + callee(cxxMethodDecl(hasName("operator bool"), + ofClass(hasName("testing::AssertionResult"))))); +} + +static auto isAssertionResultConstructFromBoolCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxConstructExpr( + hasType(recordDecl(hasName("testing::AssertionResult"))), + hasArgument(0, hasType(booleanType()))); +} + +static auto isAssertionResultConstructFromOptionalCall() { + using namespace ::clang::ast_matchers; // NOLINT: Too many names + return cxxConstructExpr( + hasType(recordDecl(hasName("testing::AssertionResult"))), + hasArgument(0, hasOptionalOrDerivedType())); +} + +void transferAssertionResultOperatorBoolCall( + const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + auto *OptionalLoc = getImplicitObjectLocation(*Expr, State.Env); + if (OptionalLoc == nullptr) + return; + + BoolValue *HasVal = getHasValue(State.Env, OptionalLoc); + if (HasVal == nullptr) + return; + + State.Env.setValue(*Expr, *HasVal); +} + +void transferAssertionResultConstructFromBoolCall( + const CXXConstructExpr *ConstructExpr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + assert(ConstructExpr->getNumArgs() > 0); + const Expr *Arg = ConstructExpr->getArg(0)->IgnoreImplicit(); + + BoolValue *HasVal = State.Env.get<BoolValue>(*Arg); + if (HasVal == nullptr) + return; + + auto &ResultLoc = State.Env.getResultObjectLocation(*ConstructExpr); + State.Env.setValue(locForHasValue(ResultLoc), *HasVal); +} + +void transferAssertionResultConstructFromOptionalCall( + const CXXConstructExpr *ConstructExpr, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + assert(ConstructExpr->getNumArgs() > 0); + + const Expr *Arg = ConstructExpr->getArg(0)->IgnoreImplicit(); + auto *OptionalLoc = cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(*Arg)); + if (OptionalLoc == nullptr) + return; + + BoolValue *HasVal = getHasValue(State.Env, OptionalLoc); + if (HasVal == nullptr) + return; + + auto &ResultLoc = State.Env.getResultObjectLocation(*ConstructExpr); + State.Env.setValue(locForHasValue(ResultLoc), *HasVal); +} + auto buildTransferMatchSwitch() { // FIXME: Evaluate the efficiency of matchers. If using matchers results in a // lot of duplicated work (e.g. string comparisons), consider providing APIs @@ -1117,6 +1198,16 @@ auto buildTransferMatchSwitch() { [](StorageLocation &Loc) {}); }) + // gtest + .CaseOfCFGStmt<CXXMemberCallExpr>( + isAssertionResultOperatorBoolCall(), + transferAssertionResultOperatorBoolCall) + .CaseOfCFGStmt<CXXConstructExpr>( + isAssertionResultConstructFromBoolCall(), + transferAssertionResultConstructFromBoolCall) + .CaseOfCFGStmt<CXXConstructExpr>( + isAssertionResultConstructFromOptionalCall(), + transferAssertionResultConstructFromOptionalCall) // const accessor calls .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(), transferConstMemberCall) @@ -1128,11 +1219,10 @@ auto buildTransferMatchSwitch() { .CaseOfCFGStmt<CXXOperatorCallExpr>( isNonConstMemberOperatorCall(), transferValue_NonConstMemberOperatorCall) - // other cases of returning optional .CaseOfCFGStmt<CallExpr>(isCallReturningOptional(), transferCallReturningOptional) - + .Build(); } @@ -1202,6 +1292,9 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx, TransferMatchSwitch(buildTransferMatchSwitch()) { Env.getDataflowAnalysisContext().setSyntheticFieldCallback( [&Ctx](QualType Ty) -> llvm::StringMap<QualType> { + if (isAssertionResultType(Ty)) + return {{"has_value", Ctx.BoolTy}}; + const CXXRecordDecl *Optional = getOptionalBaseClass(Ty->getAsCXXRecordDecl()); if (Optional == nullptr) diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 0cd122a9c3061..2a38afe7f7b49 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -2906,18 +2906,49 @@ TEST_P(UncheckedOptionalAccessTest, AssertTrueGtestMacro) { ASSERT_TRUE(opt.has_value()); EXPECT_EQ(opt.value(), 42); - $ns::$optional<int> opt1; - ASSERT_TRUE(opt1); - EXPECT_EQ(*opt1, 42); + opt.reset(); + EXPECT_EQ(opt.value(), 42); // [[unsafe]] + ASSERT_TRUE(opt.value()); // [[unsafe]] + } + )cc"); + + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional<int> opt; + ASSERT_TRUE(opt); + EXPECT_EQ(*opt, 42); + } + )cc"); + + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" - $ns::$optional<int> opt2; - EXPECT_TRUE(opt2.has_value()); - EXPECT_EQ(opt2.value(), 42); // [[unsafe]] + void target() { + $ns::$optional<int> opt; + ASSERT_FALSE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); // [[unsafe]] } )cc"); + + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional<int> opt; + EXPECT_TRUE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); // [[unsafe]] + + EXPECT_TRUE(opt); + EXPECT_EQ(*opt, 42); // [[unsafe]] + } + )cc"); + } + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) >From 471b35b02ae56284208ab739daaaa23b0135e135 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 00:10:02 +0000 Subject: [PATCH 03/15] format --- .../Models/UncheckedOptionalAccessModel.cpp | 29 +++++++++---------- .../UncheckedOptionalAccessModelTest.cpp | 3 -- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 0c3e5f25c59fd..531a1d19d29b5 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -943,10 +943,9 @@ static auto isAssertionResultConstructFromOptionalCall() { hasArgument(0, hasOptionalOrDerivedType())); } -void transferAssertionResultOperatorBoolCall( - const CXXMemberCallExpr *Expr, - const MatchFinder::MatchResult &, - LatticeTransferState &State) { +void transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { auto *OptionalLoc = getImplicitObjectLocation(*Expr, State.Env); if (OptionalLoc == nullptr) return; @@ -967,7 +966,7 @@ void transferAssertionResultConstructFromBoolCall( BoolValue *HasVal = State.Env.get<BoolValue>(*Arg); if (HasVal == nullptr) return; - + auto &ResultLoc = State.Env.getResultObjectLocation(*ConstructExpr); State.Env.setValue(locForHasValue(ResultLoc), *HasVal); } @@ -978,7 +977,8 @@ void transferAssertionResultConstructFromOptionalCall( assert(ConstructExpr->getNumArgs() > 0); const Expr *Arg = ConstructExpr->getArg(0)->IgnoreImplicit(); - auto *OptionalLoc = cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(*Arg)); + auto *OptionalLoc = + cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(*Arg)); if (OptionalLoc == nullptr) return; @@ -1198,16 +1198,15 @@ auto buildTransferMatchSwitch() { [](StorageLocation &Loc) {}); }) - // gtest - .CaseOfCFGStmt<CXXMemberCallExpr>( - isAssertionResultOperatorBoolCall(), - transferAssertionResultOperatorBoolCall) + // gtest + .CaseOfCFGStmt<CXXMemberCallExpr>(isAssertionResultOperatorBoolCall(), + transferAssertionResultOperatorBoolCall) .CaseOfCFGStmt<CXXConstructExpr>( isAssertionResultConstructFromBoolCall(), transferAssertionResultConstructFromBoolCall) .CaseOfCFGStmt<CXXConstructExpr>( - isAssertionResultConstructFromOptionalCall(), - transferAssertionResultConstructFromOptionalCall) + isAssertionResultConstructFromOptionalCall(), + transferAssertionResultConstructFromOptionalCall) // const accessor calls .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(), transferConstMemberCall) @@ -1222,7 +1221,7 @@ auto buildTransferMatchSwitch() { // other cases of returning optional .CaseOfCFGStmt<CallExpr>(isCallReturningOptional(), transferCallReturningOptional) - + .Build(); } @@ -1292,8 +1291,8 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx, TransferMatchSwitch(buildTransferMatchSwitch()) { Env.getDataflowAnalysisContext().setSyntheticFieldCallback( [&Ctx](QualType Ty) -> llvm::StringMap<QualType> { - if (isAssertionResultType(Ty)) - return {{"has_value", Ctx.BoolTy}}; + if (isAssertionResultType(Ty)) + return {{"has_value", Ctx.BoolTy}}; const CXXRecordDecl *Optional = getOptionalBaseClass(Ty->getAsCXXRecordDecl()); diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 2a38afe7f7b49..30cd5ef8ca302 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -2944,11 +2944,8 @@ TEST_P(UncheckedOptionalAccessTest, AssertTrueGtestMacro) { EXPECT_EQ(*opt, 42); // [[unsafe]] } )cc"); - } - - // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) >From 60fef9a201d823226bf92d13a0120d41e9250d62 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 09:37:03 +0000 Subject: [PATCH 04/15] add test for `isNull` --- .../UncheckedOptionalAccessModelTest.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 30cd5ef8ca302..8afd8766ea1d7 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -2944,6 +2944,25 @@ TEST_P(UncheckedOptionalAccessTest, AssertTrueGtestMacro) { EXPECT_EQ(*opt, 42); // [[unsafe]] } )cc"); + + + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + namespace BloombergLP::bdlb { + template <typename T> + struct NullableValue : $ns::$optional<T> { + bool isNull() const; + }; + } + + void target() { + BloombergLP::bdlb::NullableValue<int> opt; + + ASSERT_FALSE(opt.isNull()); + EXPECT_EQ(*opt, 42); + } + )cc"); } // FIXME: Add support for: >From 9cc21e7077656fc77818c05f5827dfec10b53887 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 09:53:55 +0000 Subject: [PATCH 05/15] format --- .../Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 8afd8766ea1d7..72956327a5346 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -2945,7 +2945,6 @@ TEST_P(UncheckedOptionalAccessTest, AssertTrueGtestMacro) { } )cc"); - ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" >From 0d520a5e3602d02d096116fd65bdb71c30da0482 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 10:14:30 +0000 Subject: [PATCH 06/15] add docs --- .../bugprone/unchecked-optional-access.rst | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst index 3423eaaf63eb2..804acbd623d4e 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst @@ -310,6 +310,25 @@ advantages: scope. This gives the user the best of both worlds -- the safety of a dynamic check, but without incurring redundant costs. +GTEST awareness +------- + +The check recognizes common macros like `ASSERT_TRUE` and `ASSERT_FALSE`: +.. code-block:: c++ + + TEST(OptionalTest, CheckValue) { + std::optional<int> opt; + EXPECT_TRUE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); // unsafe: EXPECT_TRUE doesn't terminate test. + + ASSERT_TRUE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); // safe: ASSERT_TRUE terminates if no value. + } + + +Less common macros such as `ASSERT_NE(..., nullopt)` and `ASSERT_THAT` are +not currently supported and are ignored, which may result in false positives. + Options ------- >From 24e0730feddf5b23b217a205acc9aa1e373dafdc Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 10:30:45 +0000 Subject: [PATCH 07/15] small tweaks and release note --- clang-tools-extra/docs/ReleaseNotes.rst | 5 +++++ .../checks/bugprone/unchecked-optional-access.rst | 2 +- .../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 6 +++--- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 77be492d4093e..c55ef5506e562 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -99,6 +99,11 @@ Improvements to clang-tidy manages the creation of temporary header files and ensures that diagnostics and fixes are verified for the specified headers. +- Improved :doc:`bugprone-unchecked-optional-access + <clang-tidy/checks/bugprone/unchecked-optional-access>` to recognize common + GoogleTest macros such as `ASSERT_TRUE` and `ASSERT_FALSE`, reducing the + number of false positives in test code. + New checks ^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst index 804acbd623d4e..e4cbc3e2885b3 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst @@ -310,7 +310,7 @@ advantages: scope. This gives the user the best of both worlds -- the safety of a dynamic check, but without incurring redundant costs. -GTEST awareness +GoogleTest awareness ------- The check recognizes common macros like `ASSERT_TRUE` and `ASSERT_FALSE`: diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 531a1d19d29b5..9e8adba13c06a 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -946,11 +946,11 @@ static auto isAssertionResultConstructFromOptionalCall() { void transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &, LatticeTransferState &State) { - auto *OptionalLoc = getImplicitObjectLocation(*Expr, State.Env); - if (OptionalLoc == nullptr) + auto *RecordLoc = getImplicitObjectLocation(*Expr, State.Env); + if (RecordLoc == nullptr) return; - BoolValue *HasVal = getHasValue(State.Env, OptionalLoc); + BoolValue *HasVal = getHasValue(State.Env, RecordLoc); if (HasVal == nullptr) return; >From 51ee36ca649041ded93661b952a48385fe138689 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 10:50:20 +0000 Subject: [PATCH 08/15] fix docs formatting --- .../clang-tidy/checks/bugprone/unchecked-optional-access.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst index e4cbc3e2885b3..a70a9b15d157b 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst @@ -311,7 +311,7 @@ advantages: dynamic check, but without incurring redundant costs. GoogleTest awareness -------- +-------------------- The check recognizes common macros like `ASSERT_TRUE` and `ASSERT_FALSE`: .. code-block:: c++ @@ -325,8 +325,7 @@ The check recognizes common macros like `ASSERT_TRUE` and `ASSERT_FALSE`: EXPECT_EQ(opt.value(), 42); // safe: ASSERT_TRUE terminates if no value. } - -Less common macros such as `ASSERT_NE(..., nullopt)` and `ASSERT_THAT` are +Less common macros such as `ASSERT_NE(..., nullopt)` and `ASSERT_THAT` are not currently supported and are ignored, which may result in false positives. Options >From ef02a6fe5e8ac9b66c5d5338a02202493c0932e6 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 10:55:58 +0000 Subject: [PATCH 09/15] fix formatting --- .../clang-tidy/checks/bugprone/unchecked-optional-access.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst index a70a9b15d157b..40484b2e24cb7 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst @@ -314,6 +314,7 @@ GoogleTest awareness -------------------- The check recognizes common macros like `ASSERT_TRUE` and `ASSERT_FALSE`: + .. code-block:: c++ TEST(OptionalTest, CheckValue) { >From 7d8828fdbee366fb0b86796e1c98d32a47e6a9d7 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 10:57:08 +0000 Subject: [PATCH 10/15] formatting of `ASSERT`` --- .../clang-tidy/checks/bugprone/unchecked-optional-access.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst index 40484b2e24cb7..69834270a80f6 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst @@ -313,7 +313,7 @@ advantages: GoogleTest awareness -------------------- -The check recognizes common macros like `ASSERT_TRUE` and `ASSERT_FALSE`: +The check recognizes common macros like ``ASSERT_TRUE`` and ``ASSERT_FALSE``: .. code-block:: c++ @@ -326,7 +326,7 @@ The check recognizes common macros like `ASSERT_TRUE` and `ASSERT_FALSE`: EXPECT_EQ(opt.value(), 42); // safe: ASSERT_TRUE terminates if no value. } -Less common macros such as `ASSERT_NE(..., nullopt)` and `ASSERT_THAT` are +Less common macros such as ``ASSERT_NE(..., nullopt)`` and ``ASSERT_THAT`` are not currently supported and are ignored, which may result in false positives. Options >From d5d905b316e6c0f6a61b12d4e5985da773255cd5 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 17:43:35 +0000 Subject: [PATCH 11/15] Update clang-tools-extra/docs/ReleaseNotes.rst Co-authored-by: EugeneZelenko <[email protected]> --- clang-tools-extra/docs/ReleaseNotes.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index c55ef5506e562..d657c4d947758 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -101,7 +101,7 @@ Improvements to clang-tidy - Improved :doc:`bugprone-unchecked-optional-access <clang-tidy/checks/bugprone/unchecked-optional-access>` to recognize common - GoogleTest macros such as `ASSERT_TRUE` and `ASSERT_FALSE`, reducing the + GoogleTest macros such as ``ASSERT_TRUE`` and ``ASSERT_FALSE``, reducing the number of false positives in test code. New checks >From ee79ca302717f92324816fb121e7dabbf6e6a4a3 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 17:54:14 +0000 Subject: [PATCH 12/15] fix release note section --- clang-tools-extra/docs/ReleaseNotes.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index d657c4d947758..12d00caf4ad3a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -99,11 +99,6 @@ Improvements to clang-tidy manages the creation of temporary header files and ensures that diagnostics and fixes are verified for the specified headers. -- Improved :doc:`bugprone-unchecked-optional-access - <clang-tidy/checks/bugprone/unchecked-optional-access>` to recognize common - GoogleTest macros such as ``ASSERT_TRUE`` and ``ASSERT_FALSE``, reducing the - number of false positives in test code. - New checks ^^^^^^^^^^ @@ -174,6 +169,11 @@ Changes in existing checks <clang-tidy/checks/bugprone/macro-parentheses>` check by printing the macro definition in the warning message if the macro is defined on command line. +- Improved :doc:`bugprone-unchecked-optional-access + <clang-tidy/checks/bugprone/unchecked-optional-access>` to recognize common + GoogleTest macros such as ``ASSERT_TRUE`` and ``ASSERT_FALSE``, reducing the + number of false positives in test code. + - Improved :doc:`bugprone-string-constructor <clang-tidy/checks/bugprone/string-constructor>` check to detect suspicious string constructor calls when the string class constructor has a default >From 8e4835d75e7f4e9164a03d76b519d845a7e49b89 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Fri, 13 Mar 2026 18:50:16 +0000 Subject: [PATCH 13/15] sort release notes --- clang-tools-extra/docs/ReleaseNotes.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 3fef6e03860cf..6a47b1da7f822 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -210,11 +210,6 @@ Changes in existing checks <clang-tidy/checks/bugprone/macro-parentheses>` check by printing the macro definition in the warning message if the macro is defined on command line. -- Improved :doc:`bugprone-unchecked-optional-access - <clang-tidy/checks/bugprone/unchecked-optional-access>` to recognize common - GoogleTest macros such as ``ASSERT_TRUE`` and ``ASSERT_FALSE``, reducing the - number of false positives in test code. - - Improved :doc:`bugprone-std-namespace-modification <clang-tidy/checks/bugprone/std-namespace-modification>` check by fixing false positives when extending the standard library with a specialization of @@ -225,6 +220,11 @@ Changes in existing checks string constructor calls when the string class constructor has a default allocator argument. +- Improved :doc:`bugprone-unchecked-optional-access + <clang-tidy/checks/bugprone/unchecked-optional-access>` to recognize common + GoogleTest macros such as ``ASSERT_TRUE`` and ``ASSERT_FALSE``, reducing the + number of false positives in test code. + - Improved :doc:`bugprone-unsafe-functions <clang-tidy/checks/bugprone/unsafe-functions>` check by adding the function ``std::get_temporary_buffer`` to the default list of unsafe functions. (This >From 08c95ce98281bf2cd3c4464ad20175bdcc090e50 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Wed, 18 Mar 2026 02:00:46 +0000 Subject: [PATCH 14/15] small code review remarks --- .../Models/UncheckedOptionalAccessModel.cpp | 90 +++++++++---------- .../UncheckedOptionalAccessModelTest.cpp | 46 +++++----- 2 files changed, 64 insertions(+), 72 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 9e8adba13c06a..5134f3585b62d 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -312,6 +312,25 @@ auto isStdForwardCall() { hasArgument(0, hasOptionalOrDerivedType())); } +auto isAssertionResultOperatorBoolCall() { + return cxxMemberCallExpr( + on(expr(unless(cxxThisExpr()))), + callee(cxxMethodDecl(hasName("operator bool"), + ofClass(hasName("testing::AssertionResult"))))); +} + +auto isAssertionResultConstructFromBoolCall() { + return cxxConstructExpr( + hasType(recordDecl(hasName("testing::AssertionResult"))), + hasArgument(0, hasType(booleanType()))); +} + +auto isAssertionResultConstructFromOptionalCall() { + return cxxConstructExpr( + hasType(recordDecl(hasName("testing::AssertionResult"))), + hasArgument(0, hasOptionalOrDerivedType())); +} + constexpr llvm::StringLiteral ValueOrCallID = "ValueOrCall"; auto isValueOrStringEmptyCall() { @@ -897,52 +916,6 @@ void transferOptionalAndNulloptCmp(const clang::CXXOperatorCallExpr *CmpExpr, } } -std::optional<StatementMatcher> -ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) { - if (Options.IgnoreSmartPointerDereference) { - auto SmartPtrUse = expr(ignoringParenImpCasts(cxxOperatorCallExpr( - anyOf(hasOverloadedOperatorName("->"), hasOverloadedOperatorName("*")), - unless(hasArgument(0, expr(hasOptionalType())))))); - return expr( - anyOf(SmartPtrUse, memberExpr(hasObjectExpression(SmartPtrUse)))); - } - return std::nullopt; -} - -StatementMatcher -valueCall(const std::optional<StatementMatcher> &IgnorableOptional) { - return isOptionalMemberCallWithNameMatcher(hasName("value"), - IgnorableOptional); -} - -StatementMatcher -valueOperatorCall(const std::optional<StatementMatcher> &IgnorableOptional) { - return expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional), - isOptionalOperatorCallWithName("->", IgnorableOptional))); -} - -auto isAssertionResultOperatorBoolCall() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return cxxMemberCallExpr( - on(expr(unless(cxxThisExpr()))), - callee(cxxMethodDecl(hasName("operator bool"), - ofClass(hasName("testing::AssertionResult"))))); -} - -static auto isAssertionResultConstructFromBoolCall() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return cxxConstructExpr( - hasType(recordDecl(hasName("testing::AssertionResult"))), - hasArgument(0, hasType(booleanType()))); -} - -static auto isAssertionResultConstructFromOptionalCall() { - using namespace ::clang::ast_matchers; // NOLINT: Too many names - return cxxConstructExpr( - hasType(recordDecl(hasName("testing::AssertionResult"))), - hasArgument(0, hasOptionalOrDerivedType())); -} - void transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &, LatticeTransferState &State) { @@ -990,6 +963,30 @@ void transferAssertionResultConstructFromOptionalCall( State.Env.setValue(locForHasValue(ResultLoc), *HasVal); } +std::optional<StatementMatcher> +ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) { + if (Options.IgnoreSmartPointerDereference) { + auto SmartPtrUse = expr(ignoringParenImpCasts(cxxOperatorCallExpr( + anyOf(hasOverloadedOperatorName("->"), hasOverloadedOperatorName("*")), + unless(hasArgument(0, expr(hasOptionalType())))))); + return expr( + anyOf(SmartPtrUse, memberExpr(hasObjectExpression(SmartPtrUse)))); + } + return std::nullopt; +} + +StatementMatcher +valueCall(const std::optional<StatementMatcher> &IgnorableOptional) { + return isOptionalMemberCallWithNameMatcher(hasName("value"), + IgnorableOptional); +} + +StatementMatcher +valueOperatorCall(const std::optional<StatementMatcher> &IgnorableOptional) { + return expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional), + isOptionalOperatorCallWithName("->", IgnorableOptional))); +} + auto buildTransferMatchSwitch() { // FIXME: Evaluate the efficiency of matchers. If using matchers results in a // lot of duplicated work (e.g. string comparisons), consider providing APIs @@ -1218,6 +1215,7 @@ auto buildTransferMatchSwitch() { .CaseOfCFGStmt<CXXOperatorCallExpr>( isNonConstMemberOperatorCall(), transferValue_NonConstMemberOperatorCall) + // other cases of returning optional .CaseOfCFGStmt<CallExpr>(isCallReturningOptional(), transferCallReturningOptional) diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 72956327a5346..295ab238e3642 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -2901,47 +2901,43 @@ TEST_P(UncheckedOptionalAccessTest, AssertTrueGtestMacro) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" - void target() { - $ns::$optional<int> opt; - ASSERT_TRUE(opt.has_value()); - EXPECT_EQ(opt.value(), 42); + void target($ns::$optional<int> opt) { + ASSERT_TRUE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); - opt.reset(); - EXPECT_EQ(opt.value(), 42); // [[unsafe]] - ASSERT_TRUE(opt.value()); // [[unsafe]] + opt.reset(); + EXPECT_EQ(opt.value(), 42); // [[unsafe]] + ASSERT_TRUE(opt.value()); // [[unsafe]] } )cc"); ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" - void target() { - $ns::$optional<int> opt; - ASSERT_TRUE(opt); - EXPECT_EQ(*opt, 42); + void target($ns::$optional<int> opt) { + ASSERT_TRUE(opt); + EXPECT_EQ(*opt, 42); } )cc"); ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" - void target() { - $ns::$optional<int> opt; - ASSERT_FALSE(opt.has_value()); - EXPECT_EQ(opt.value(), 42); // [[unsafe]] + void target($ns::$optional<int> opt) { + ASSERT_FALSE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); // [[unsafe]] } )cc"); ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" - void target() { - $ns::$optional<int> opt; - EXPECT_TRUE(opt.has_value()); - EXPECT_EQ(opt.value(), 42); // [[unsafe]] + void target($ns::$optional<int> opt) { + EXPECT_TRUE(opt.has_value()); + EXPECT_EQ(opt.value(), 42); // [[unsafe]] - EXPECT_TRUE(opt); - EXPECT_EQ(*opt, 42); // [[unsafe]] + EXPECT_TRUE(opt); + EXPECT_EQ(*opt, 42); // [[unsafe]] } )cc"); @@ -2955,11 +2951,9 @@ TEST_P(UncheckedOptionalAccessTest, AssertTrueGtestMacro) { }; } - void target() { - BloombergLP::bdlb::NullableValue<int> opt; - - ASSERT_FALSE(opt.isNull()); - EXPECT_EQ(*opt, 42); + void target(BloombergLP::bdlb::NullableValue<int> opt) { + ASSERT_FALSE(opt.isNull()); + EXPECT_EQ(*opt, 42); } )cc"); } >From 2036537e984e2cd4c8879d54d49a320657e23123 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Wed, 18 Mar 2026 02:13:25 +0000 Subject: [PATCH 15/15] rename assert synt field to not confuse with optional synt field --- .../Models/UncheckedOptionalAccessModel.cpp | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 5134f3585b62d..797afcd434651 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -419,6 +419,10 @@ StorageLocation &locForValue(const RecordStorageLocation &OptionalLoc) { return OptionalLoc.getSyntheticField("value"); } +StorageLocation &locForAssertResultSuccess(const RecordStorageLocation &AssertResultLoc) { + return AssertResultLoc.getSyntheticField("success"); +} + /// Sets `HasValueVal` as the symbolic value that represents the "has_value" /// property of the optional at `OptionalLoc`. void setHasValue(RecordStorageLocation &OptionalLoc, BoolValue &HasValueVal, @@ -919,15 +923,15 @@ void transferOptionalAndNulloptCmp(const clang::CXXOperatorCallExpr *CmpExpr, void transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &, LatticeTransferState &State) { - auto *RecordLoc = getImplicitObjectLocation(*Expr, State.Env); - if (RecordLoc == nullptr) + auto *AssertResultLoc = getImplicitObjectLocation(*Expr, State.Env); + if (AssertResultLoc == nullptr) return; - BoolValue *HasVal = getHasValue(State.Env, RecordLoc); - if (HasVal == nullptr) + BoolValue *SuccessVal = State.Env.get<BoolValue>(locForAssertResultSuccess(*AssertResultLoc)); + if (SuccessVal == nullptr) return; - State.Env.setValue(*Expr, *HasVal); + State.Env.setValue(*Expr, *SuccessVal); } void transferAssertionResultConstructFromBoolCall( @@ -936,12 +940,12 @@ void transferAssertionResultConstructFromBoolCall( assert(ConstructExpr->getNumArgs() > 0); const Expr *Arg = ConstructExpr->getArg(0)->IgnoreImplicit(); - BoolValue *HasVal = State.Env.get<BoolValue>(*Arg); - if (HasVal == nullptr) + BoolValue *SuccessVal = State.Env.get<BoolValue>(*Arg); + if (SuccessVal == nullptr) return; auto &ResultLoc = State.Env.getResultObjectLocation(*ConstructExpr); - State.Env.setValue(locForHasValue(ResultLoc), *HasVal); + State.Env.setValue(locForAssertResultSuccess(ResultLoc), *SuccessVal); } void transferAssertionResultConstructFromOptionalCall( @@ -960,7 +964,7 @@ void transferAssertionResultConstructFromOptionalCall( return; auto &ResultLoc = State.Env.getResultObjectLocation(*ConstructExpr); - State.Env.setValue(locForHasValue(ResultLoc), *HasVal); + State.Env.setValue(locForAssertResultSuccess(ResultLoc), *HasVal); } std::optional<StatementMatcher> @@ -1290,7 +1294,7 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx, Env.getDataflowAnalysisContext().setSyntheticFieldCallback( [&Ctx](QualType Ty) -> llvm::StringMap<QualType> { if (isAssertionResultType(Ty)) - return {{"has_value", Ctx.BoolTy}}; + return {{"success", Ctx.BoolTy}}; const CXXRecordDecl *Optional = getOptionalBaseClass(Ty->getAsCXXRecordDecl()); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
