[clang] [FlowSensitive] [StatusOr] [11/N] Assume const accessor calls are stable (PR #170935)
Florian Mayer via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 5 14:18:11 PST 2025
https://github.com/fmayer created https://github.com/llvm/llvm-project/pull/170935
This is not necessarily correct, but prevents us from flagging lots of
false positives because code usually abides by this.
>From 651e3bf72e1680a69ee138fcd07e50f1395d8307 Mon Sep 17 00:00:00 2001
From: Florian Mayer <fmayer at google.com>
Date: Fri, 5 Dec 2025 14:17:56 -0800
Subject: [PATCH] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20initia?=
=?UTF-8?q?l=20version?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Created using spr 1.3.7
---
.../Models/UncheckedStatusOrAccessModel.h | 4 +-
.../Models/UncheckedStatusOrAccessModel.cpp | 161 ++++++++++++++++
...ncheckedStatusOrAccessModelTestFixture.cpp | 173 ++++++++++++++++++
3 files changed, 337 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
index 24f8b0b99870a..2d54bd3c6f7ad 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
@@ -13,6 +13,7 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
@@ -69,7 +70,8 @@ struct UncheckedStatusOrAccessModelOptions {};
// Dataflow analysis that discovers unsafe uses of StatusOr values.
class UncheckedStatusOrAccessModel
- : public DataflowAnalysis<UncheckedStatusOrAccessModel, NoopLattice> {
+ : public DataflowAnalysis<UncheckedStatusOrAccessModel,
+ CachedConstAccessorsLattice<NoopLattice>> {
public:
explicit UncheckedStatusOrAccessModel(ASTContext &Ctx, Environment &Env);
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 5869aa9a3e227..038f2b0338c8d 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -237,6 +237,49 @@ static auto isAsStatusCallWithStatusOr() {
hasArgument(0, hasType(statusOrType())));
}
+static auto possiblyReferencedStatusOrType() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return anyOf(statusOrType(), referenceType(pointee(statusOrType())));
+}
+
+static auto isConstStatusOrAccessorMemberCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxMemberCallExpr(callee(
+ cxxMethodDecl(parameterCountIs(0), isConst(),
+ returns(qualType(possiblyReferencedStatusOrType())))));
+}
+
+static auto isConstStatusOrAccessorMemberOperatorCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxOperatorCallExpr(
+ callee(cxxMethodDecl(parameterCountIs(0), isConst(),
+ returns(possiblyReferencedStatusOrType()))));
+}
+
+static auto isConstStatusOrPointerAccessorMemberCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxMemberCallExpr(callee(cxxMethodDecl(
+ parameterCountIs(0), isConst(),
+ returns(pointerType(pointee(possiblyReferencedStatusOrType()))))));
+}
+
+static auto isConstStatusOrPointerAccessorMemberOperatorCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxOperatorCallExpr(callee(cxxMethodDecl(
+ parameterCountIs(0), isConst(),
+ returns(pointerType(pointee(possiblyReferencedStatusOrType()))))));
+}
+
+static auto isNonConstMemberCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
+static auto isNonConstMemberOperatorCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
static auto
buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
return CFGMatchSwitchBuilder<const Environment,
@@ -697,6 +740,107 @@ static void transferPointerToBoolean(const ImplicitCastExpr *Expr,
dyn_cast_or_null<BoolValue>(State.Env.getValue(*Expr->getSubExpr())))
State.Env.setValue(*Expr, *SubExprVal);
}
+static void handleConstStatusOrAccessorMemberCall(
+ const CallExpr *Expr, RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
+ assert(isStatusOrType(Expr->getType()));
+ if (RecordLoc == nullptr)
+ return;
+ const FunctionDecl *DirectCallee = Expr->getDirectCallee();
+ if (DirectCallee == nullptr)
+ return;
+ StorageLocation &Loc =
+ State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+ *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+ initializeStatusOr(cast<RecordStorageLocation>(Loc), State.Env);
+ });
+ if (Expr->isPRValue()) {
+ auto &ResultLoc = State.Env.getResultObjectLocation(*Expr);
+ copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
+ } else {
+ State.Env.setStorageLocation(*Expr, Loc);
+ }
+}
+
+static void handleConstStatusOrPointerAccessorMemberCall(
+ const CallExpr *Expr, RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
+ if (RecordLoc == nullptr)
+ return;
+ auto *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, Expr,
+ State.Env);
+ State.Env.setValue(*Expr, *Val);
+}
+
+static void
+transferConstStatusOrAccessorMemberCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ handleConstStatusOrAccessorMemberCall(
+ Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State);
+}
+
+static void transferConstStatusOrAccessorMemberOperatorCall(
+ const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null<RecordStorageLocation>(
+ State.Env.getStorageLocation(*Expr->getArg(0)));
+ handleConstStatusOrAccessorMemberCall(Expr, RecordLoc, Result, State);
+}
+
+static void transferConstStatusOrPointerAccessorMemberCall(
+ const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ handleConstStatusOrPointerAccessorMemberCall(
+ Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State);
+}
+
+static void transferConstStatusOrPointerAccessorMemberOperatorCall(
+ const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null<RecordStorageLocation>(
+ State.Env.getStorageLocation(*Expr->getArg(0)));
+ handleConstStatusOrPointerAccessorMemberCall(Expr, RecordLoc, Result, State);
+}
+
+static void transferStatusOrReturningCall(const CallExpr *Expr,
+ LatticeTransferState &State) {
+ RecordStorageLocation *StatusOrLoc =
+ Expr->isPRValue() ? &State.Env.getResultObjectLocation(*Expr)
+ : State.Env.get<RecordStorageLocation>(*Expr);
+ if (StatusOrLoc != nullptr &&
+ State.Env.getValue(locForOk(locForStatus(*StatusOrLoc))) == nullptr)
+ initializeStatusOr(*StatusOrLoc, State.Env);
+}
+
+static void handleNonConstMemberCall(const CallExpr *Expr,
+ RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ if (RecordLoc == nullptr)
+ return;
+ State.Lattice.clearConstMethodReturnValues(*RecordLoc);
+ State.Lattice.clearConstMethodReturnStorageLocations(*RecordLoc);
+
+ if (isStatusOrType(Expr->getType()))
+ transferStatusOrReturningCall(Expr, State);
+}
+
+static void transferNonConstMemberCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ handleNonConstMemberCall(Expr, getImplicitObjectLocation(*Expr, State.Env),
+ Result, State);
+}
+
+static void
+transferNonConstMemberOperatorCall(const CXXOperatorCallExpr *Expr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null<RecordStorageLocation>(
+ State.Env.getStorageLocation(*Expr->getArg(0)));
+ handleNonConstMemberCall(Expr, RecordLoc, Result, State);
+}
CFGMatchSwitch<LatticeTransferState>
buildTransferMatchSwitch(ASTContext &Ctx,
@@ -755,6 +899,23 @@ buildTransferMatchSwitch(ASTContext &Ctx,
transferLoggingGetReferenceableValueCall)
.CaseOfCFGStmt<CallExpr>(isLoggingCheckEqImpl(),
transferLoggingCheckEqImpl)
+ // const accessor calls
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isConstStatusOrAccessorMemberCall(),
+ transferConstStatusOrAccessorMemberCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isConstStatusOrAccessorMemberOperatorCall(),
+ transferConstStatusOrAccessorMemberOperatorCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ isConstStatusOrPointerAccessorMemberCall(),
+ transferConstStatusOrPointerAccessorMemberCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isConstStatusOrPointerAccessorMemberOperatorCall(),
+ transferConstStatusOrPointerAccessorMemberOperatorCall)
+ // non-const member calls that may modify the state of an object.
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
+ transferNonConstMemberCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(isNonConstMemberOperatorCall(),
+ transferNonConstMemberOperatorCall)
// N.B. These need to come after all other CXXConstructExpr.
// These are there to make sure that every Status and StatusOr object
// have their ok boolean initialized when constructed. If we were to
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 13cde05df0a3f..e075818f8a2c1 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -3270,6 +3270,179 @@ TEST_P(UncheckedStatusOrAccessModelTest, ConstructStatusFromReference) {
)cc");
}
+TEST_P(UncheckedStatusOrAccessModelTest, AccessorCall) {
+ // Accessor returns reference.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ const STATUSOR_INT& sor() const { return sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo.sor().ok()) foo.sor().value();
+ }
+ )cc");
+
+ // Uses an operator
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ const STATUSOR_INT& operator()() const { return sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo().ok()) foo().value();
+ }
+ )cc");
+
+ // Calls nonconst method inbetween.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ void invalidate() {}
+
+ const STATUSOR_INT& sor() const { return sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo.sor().ok()) {
+ foo.invalidate();
+ foo.sor().value(); // [[unsafe]]
+ }
+ }
+ )cc");
+
+ // Calls nonconst operator inbetween.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ void operator()() {}
+
+ const STATUSOR_INT& sor() const { return sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo.sor().ok()) {
+ foo();
+ foo.sor().value(); // [[unsafe]]
+ }
+ }
+ )cc");
+
+ // Accessor returns copy.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ STATUSOR_INT sor() const { return sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo.sor().ok()) foo.sor().value();
+ }
+ )cc");
+
+ // Non-const accessor.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ const STATUSOR_INT& sor() { return sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo.sor().ok()) foo.sor().value(); // [[unsafe]]
+ }
+ )cc");
+
+ // Non-const rvalue accessor.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ STATUSOR_INT&& sor() { return std::move(sor_); }
+ };
+
+ void target(Foo foo) {
+ if (foo.sor().ok()) foo.sor().value(); // [[unsafe]]
+ }
+ )cc");
+
+ // const pointer accessor.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ const STATUSOR_INT* sor() const { return &sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo.sor()->ok()) foo.sor()->value();
+ }
+ )cc");
+
+ // const pointer operator.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ const STATUSOR_INT* operator->() const { return &sor_; }
+ };
+
+ void target(Foo foo) {
+ if (foo->ok()) foo->value();
+ }
+ )cc");
+
+ // We copy the result of the accessor.
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ STATUSOR_INT sor_;
+
+ const STATUSOR_INT& sor() const { return sor_; }
+ };
+ void target() {
+ Foo foo;
+ if (!foo.sor().ok()) return;
+ const auto sor = foo.sor();
+ sor.value();
+ }
+ )cc");
+}
+
} // namespace
std::string
More information about the cfe-commits
mailing list