[FlowSensitive] [StatusOr] [13/N] Add support for gtest ASSERTs #170947
+444
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.7 [skip ci]
Created using spr 1.3.7
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) ChangesFull diff: https://github.com/llvm/llvm-project/pull/170947.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 3e56094fcbc32..1b68d704239e8 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -281,6 +281,76 @@ static auto isNonConstMemberOperatorCall() {
return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst()))));
}
+static auto isMakePredicateFormatterFromIsOkMatcherCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return callExpr(
+ callee(functionDecl(
+ hasName("::testing::internal::MakePredicateFormatterFromMatcher"))),
+ hasArgument(
+ 0, hasType(cxxRecordDecl(hasAnyName(
+ "::testing::status::internal_status::IsOkMatcher",
+ "::absl_testing::status_internal::IsOkMatcher",
+ "::testing::status::internal_status::IsOkAndHoldsMatcher",
+ "::absl_testing::status_internal::IsOkAndHoldsMatcher")))));
+}
+
+static auto isStatusIsOkMatcherCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return callExpr(callee(functionDecl(hasAnyName(
+ "::testing::status::StatusIs", "absl_testing::StatusIs",
+ "::testing::status::CanonicalStatusIs",
+ "::absl_testing::CanonicalStatusIs"))),
+ hasArgument(0, declRefExpr(to(enumConstantDecl(hasAnyName(
+ "::absl::StatusCode::kOk", "OK"))))));
+}
+
+static auto isMakePredicateFormatterFromStatusIsMatcherCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return callExpr(
+ callee(functionDecl(
+ hasName("::testing::internal::MakePredicateFormatterFromMatcher"))),
+ hasArgument(0, hasType(cxxRecordDecl(hasAnyName(
+ "::testing::status::internal_status::StatusIsMatcher",
+ "::testing::status::internal_status::"
+ "CanonicalStatusIsMatcher",
+ "::absl_testing::status_internal::StatusIsMatcher",
+ "::absl_testing::status_internal::"
+ "CanonicalStatusIsMatcher")))));
+}
+
+static auto isPredicateFormatterFromStatusMatcherCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("()"),
+ callee(cxxMethodDecl(ofClass(
+ hasName("testing::internal::PredicateFormatterFromMatcher")))),
+ hasArgument(2, hasType(cxxRecordDecl(hasName("absl::Status")))));
+}
+
+static auto isPredicateFormatterFromStatusOrMatcherCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("()"),
+ callee(cxxMethodDecl(ofClass(
+ hasName("testing::internal::PredicateFormatterFromMatcher")))),
+ hasArgument(2, hasType(statusOrType())));
+}
+
+static 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
buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
return CFGMatchSwitchBuilder<const Environment,
@@ -365,6 +435,26 @@ bool isStatusType(QualType Type) {
return isTypeNamed(Type, {"absl"}, "Status");
}
+static bool isPredicateFormatterFromMatcherType(QualType Type) {
+ return isTypeNamed(Type, {"testing", "internal"},
+ "PredicateFormatterFromMatcher");
+}
+
+static bool isAssertionResultType(QualType Type) {
+ return isTypeNamed(Type, {"testing"}, "AssertionResult");
+}
+
+static bool isStatusIsMatcherType(QualType Type) {
+ return isTypeNamed(Type, {"testing", "status", "internal_status"},
+ "StatusIsMatcher") ||
+ isTypeNamed(Type, {"testing", "status", "internal_status"},
+ "CanonicalStatusIsMatcher") ||
+ isTypeNamed(Type, {"absl_testing", "status_internal"},
+ "StatusIsMatcher") ||
+ isTypeNamed(Type, {"absl_testing", "status_internal"},
+ "CanonicalStatusIsMatcher");
+}
+
llvm::StringMap<QualType> getSyntheticFields(QualType Ty, QualType StatusType,
const CXXRecordDecl &RD) {
if (auto *TRD = getStatusOrBaseClass(Ty))
@@ -372,6 +462,12 @@ llvm::StringMap<QualType> getSyntheticFields(QualType Ty, QualType StatusType,
if (isStatusType(Ty) || (RD.hasDefinition() &&
RD.isDerivedFrom(StatusType->getAsCXXRecordDecl())))
return {{"ok", RD.getASTContext().BoolTy}};
+ if (isAssertionResultType(Ty))
+ return {{"ok", RD.getASTContext().BoolTy}};
+ if (isPredicateFormatterFromMatcherType(Ty))
+ return {{"ok_predicate", RD.getASTContext().BoolTy}};
+ if (isStatusIsMatcherType(Ty))
+ return {{"ok_matcher", RD.getASTContext().BoolTy}};
return {};
}
@@ -388,6 +484,13 @@ BoolValue &valForOk(RecordStorageLocation &StatusLoc, Environment &Env) {
return *Val;
return initializeStatus(StatusLoc, Env);
}
+static StorageLocation &locForOkPredicate(RecordStorageLocation &StatusLoc) {
+ return StatusLoc.getSyntheticField("ok_predicate");
+}
+
+static StorageLocation &locForOkMatcher(RecordStorageLocation &StatusLoc) {
+ return StatusLoc.getSyntheticField("ok_matcher");
+}
static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr,
const MatchFinder::MatchResult &,
@@ -843,6 +946,97 @@ transferNonConstMemberOperatorCall(const CXXOperatorCallExpr *Expr,
handleNonConstMemberCall(Expr, RecordLoc, Result, State);
}
+static void transferMakePredicateFormatterFromIsOkMatcherCall(
+ const CallExpr *Expr, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ State.Env.setValue(
+ locForOkPredicate(State.Env.getResultObjectLocation(*Expr)),
+ State.Env.getBoolLiteralValue(true));
+}
+
+static void transferStatusIsOkMatcherCall(const CallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ BoolValue &OkMatcherVal = State.Env.getBoolLiteralValue(true);
+ State.Env.setValue(locForOkMatcher(State.Env.getResultObjectLocation(*Expr)),
+ OkMatcherVal);
+}
+
+static void transferMakePredicateFormatterFromStatusIsMatcherCall(
+ const CallExpr *Expr, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ assert(Expr->isPRValue());
+ auto &Loc = State.Env.getResultObjectLocation(*Expr->getArg(0));
+ auto &OkMatcherLoc = locForOkMatcher(Loc);
+ BoolValue *OkMatcherVal = State.Env.get<BoolValue>(OkMatcherLoc);
+ if (OkMatcherVal == nullptr)
+ return;
+ State.Env.setValue(
+ locForOkPredicate(State.Env.getResultObjectLocation(*Expr)),
+ *OkMatcherVal);
+}
+
+static void
+transferPredicateFormatterMatcherCall(const CXXOperatorCallExpr *Expr,
+ LatticeTransferState &State,
+ bool IsStatusOr) {
+ auto *Loc = State.Env.get<RecordStorageLocation>(*Expr->getArg(0));
+ if (Loc == nullptr)
+ return;
+
+ auto *ObjectLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(2));
+ if (ObjectLoc == nullptr)
+ return;
+
+ auto &OkPredicateLoc = locForOkPredicate(*Loc);
+ BoolValue *OkPredicateVal = State.Env.get<BoolValue>(OkPredicateLoc);
+ if (OkPredicateVal == nullptr)
+ return;
+
+ if (IsStatusOr)
+ ObjectLoc = &locForStatus(*ObjectLoc);
+ auto &StatusOk = valForOk(*ObjectLoc, State.Env);
+
+ auto &A = State.Env.arena();
+ auto &Res = State.Env.makeAtomicBoolValue();
+ State.Env.assume(
+ A.makeImplies(OkPredicateVal->formula(),
+ A.makeEquals(StatusOk.formula(), Res.formula())));
+ State.Env.setValue(locForOk(State.Env.getResultObjectLocation(*Expr)), Res);
+}
+
+static void
+transferAssertionResultConstructFromBoolCall(const CXXConstructExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ assert(Expr->getNumArgs() > 0);
+
+ auto *StatusAdaptorLoc = State.Env.get<StorageLocation>(*Expr->getArg(0));
+ if (StatusAdaptorLoc == nullptr)
+ return;
+ BoolValue *OkVal = State.Env.get<BoolValue>(*StatusAdaptorLoc);
+ if (OkVal == nullptr)
+ return;
+ State.Env.setValue(locForOk(State.Env.getResultObjectLocation(*Expr)),
+ *OkVal);
+}
+
+static void
+transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ auto *RecordLoc = getImplicitObjectLocation(*Expr, State.Env);
+ if (RecordLoc == nullptr)
+ return;
+ BoolValue *OkVal = State.Env.get<BoolValue>(locForOk(*RecordLoc));
+ if (OkVal == nullptr)
+ return;
+ auto &A = State.Env.arena();
+ auto &Res = State.Env.makeAtomicBoolValue();
+ State.Env.assume(A.makeEquals(OkVal->formula(), Res.formula()));
+ State.Env.setValue(*Expr, Res);
+}
+
static RecordStorageLocation *
getSmartPtrLikeStorageLocation(const Expr &E, const Environment &Env) {
if (!E.isPRValue())
@@ -858,6 +1052,33 @@ buildTransferMatchSwitch(ASTContext &Ctx,
CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
using namespace ::clang::ast_matchers; // NOLINT: Too many names
return std::move(Builder)
+ .CaseOfCFGStmt<CallExpr>(
+ isMakePredicateFormatterFromIsOkMatcherCall(),
+ transferMakePredicateFormatterFromIsOkMatcherCall)
+ .CaseOfCFGStmt<CallExpr>(isStatusIsOkMatcherCall(),
+ transferStatusIsOkMatcherCall)
+ .CaseOfCFGStmt<CallExpr>(
+ isMakePredicateFormatterFromStatusIsMatcherCall(),
+ transferMakePredicateFormatterFromStatusIsMatcherCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isPredicateFormatterFromStatusOrMatcherCall(),
+ [](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ transferPredicateFormatterMatcherCall(Expr, State,
+ /*IsStatusOr=*/true);
+ })
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isPredicateFormatterFromStatusMatcherCall(),
+ [](const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ transferPredicateFormatterMatcherCall(Expr, State,
+ /*IsStatusOr=*/false);
+ })
+ .CaseOfCFGStmt<CXXConstructExpr>(
+ isAssertionResultConstructFromBoolCall(),
+ transferAssertionResultConstructFromBoolCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isAssertionResultOperatorBoolCall(),
+ transferAssertionResultOperatorBoolCall)
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("ok"),
transferStatusOrOkCall)
.CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("status"),
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index dcb1cc13146bd..48e61abf09f19 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -2173,6 +2173,181 @@ TEST_P(UncheckedStatusOrAccessModelTest, ExpectOkMacro) {
)cc");
}
+TEST_P(UncheckedStatusOrAccessModelTest, AssertThatMacro) {
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, testing::status::IsOk());
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, absl_testing::IsOk());
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor.status(), testing::status::IsOk());
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+ void target() {
+ STATUSOR_INT sor = Make<STATUSOR_INT>();
+ ASSERT_THAT(sor, testing::status::IsOk());
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ using absl::StatusCode::kOk;
+ ASSERT_THAT(sor, testing::status::StatusIs(kOk));
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, testing::status::StatusIs(absl::StatusCode::kOk));
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, testing::status::CanonicalStatusIs(absl::StatusCode::kOk));
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, absl_testing::CanonicalStatusIs(absl::StatusCode::kOk));
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ auto matcher = testing::status::StatusIs(absl::StatusCode::kOk);
+ ASSERT_THAT(sor, matcher);
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ auto matcher = absl_testing::StatusIs(absl::StatusCode::kOk);
+ ASSERT_THAT(sor, matcher);
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(
+ sor, testing::status::StatusIs(absl::StatusCode::kInvalidArgument));
+
+ sor.value(); // [[unsafe]]
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, absl_testing::StatusIs(absl::StatusCode::kInvalidArgument));
+
+ sor.value(); // [[unsafe]]
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, testing::status::IsOkAndHolds(testing::IsTrue()));
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_THAT(sor, absl_testing::IsOkAndHolds(testing::IsTrue()));
+
+ sor.value();
+ }
+ )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, AssertOkMacro) {
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_OK(sor);
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(STATUSOR_INT sor) {
+ ASSERT_OK(sor.status());
+
+ sor.value();
+ }
+ )cc");
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+ void target() {
+ STATUSOR_INT sor = Make<STATUSOR_INT>();
+ ASSERT_OK(sor);
+
+ sor.value();
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ void target(std::optional<STATUSOR_INT> sor_opt) {
+ STATUSOR_INT sor = *sor_opt;
+ ASSERT_OK(sor);
+
+ sor.value();
+ }
+ )cc");
+}
+
TEST_P(UncheckedStatusOrAccessModelTest, BreadthFirstBlockTraversalLoop) {
// Evaluating the CFG blocks of the code below in breadth-first order results
// in an infinite loop. Each iteration of the while loop below results in a
|
Created using spr 1.3.7 [skip ci]
Created using spr 1.3.7 [skip ci]
jvoung
reviewed
Dec 10, 2025
| hasOverloadedOperatorName("()"), | ||
| callee(cxxMethodDecl(ofClass( | ||
| hasName("testing::internal::PredicateFormatterFromMatcher")))), | ||
| hasArgument(2, hasType(cxxRecordDecl(hasName("absl::Status"))))); |
Contributor
There was a problem hiding this comment.
nit: hasType(statusType()) ?
like how the isPredicateFormatterFromStatusOrMatcherCall uses hasType(statusOrType())
| } | ||
| )cc"); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Worth adding some cases like
ASSERT_TRUE(sor.ok());
sor.value();
I assume that the isAssertionResultConstructFromBoolCall etc would handle that?
Contributor
Author
There was a problem hiding this comment.
These already worked before and are tested
TEST_P(UncheckedStatusOrAccessModelTest, AssertOkMacro)
Contributor
There was a problem hiding this comment.
Which line is that on? AssertOkMacro only has ASSERT_OK?
Contributor
Author
There was a problem hiding this comment.
sorry, my fault. added
Created using spr 1.3.7 [skip ci]
fmayer
changed the base branch from
users/fmayer/spr/main.flowsensitive-statusor-13n-add-support-for-gtest-asserts
to
main
fmayer
deleted the
users/fmayer/spr/flowsensitive-statusor-13n-add-support-for-gtest-asserts
branch
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Dec 10, 2025
…SERTs Reviewers: jvoung Reviewed By: jvoung Pull Request: llvm/llvm-project#170947
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.