[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 16 13:05:53 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Jan Voung (jvoung)
<details>
<summary>Changes</summary>
Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #<!-- -->58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR #<!-- -->111006.
For now we cache methods returning:
- ref to optional
- optional by value
- booleans
We can extend that to pointers to optional in a next change.
---
Full diff: https://github.com/llvm/llvm-project/pull/112605.diff
4 Files Affected:
- (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst (+10)
- (modified) clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h (+12-5)
- (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+131-5)
- (modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+176-1)
``````````diff
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 97fe37b5353560..815b5cdeeebe24 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
@@ -76,6 +76,16 @@ For example:
}
}
+Exception: accessor methods
+```````````````````````````
+
+The check assumes *accessor* methods of a class are stable, with a heuristic to
+determine which methods are accessors. Specifically, parameter-free ``const``
+methods are treated as accessors. Note that this is not guaranteed to be safe
+-- but, it is widely used (safely) in practice, and so we have chosen to treat
+it as generally safe. Calls to non ``const`` methods are assumed to modify
+the state of the object and affect the stability of earlier accessor calls.
+
Rely on invariants of uncommon APIs
-----------------------------------
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 09eb8b93822612..9d81cacb507351 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -17,6 +17,7 @@
#include "clang/AST/ASTContext.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/NoopLattice.h"
@@ -39,23 +40,28 @@ struct UncheckedOptionalAccessModelOptions {
bool IgnoreSmartPointerDereference = false;
};
+using UncheckedOptionalAccessLattice = CachedConstAccessorsLattice<NoopLattice>;
+
/// Dataflow analysis that models whether optionals hold values or not.
///
/// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
class UncheckedOptionalAccessModel
- : public DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice> {
+ : public DataflowAnalysis<UncheckedOptionalAccessModel,
+ UncheckedOptionalAccessLattice> {
public:
UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
- static NoopLattice initialElement() { return {}; }
+ static UncheckedOptionalAccessLattice initialElement() { return {}; }
- void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
+ void transfer(const CFGElement &Elt, UncheckedOptionalAccessLattice &L,
+ Environment &Env);
private:
- CFGMatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch;
+ CFGMatchSwitch<TransferState<UncheckedOptionalAccessLattice>>
+ TransferMatchSwitch;
};
class UncheckedOptionalAccessDiagnoser {
@@ -65,7 +71,8 @@ class UncheckedOptionalAccessDiagnoser {
llvm::SmallVector<SourceLocation>
operator()(const CFGElement &Elt, ASTContext &Ctx,
- const TransferStateForDiagnostics<NoopLattice> &State) {
+ const TransferStateForDiagnostics<UncheckedOptionalAccessLattice>
+ &State) {
return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
}
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 70ffe92753e053..47af36eec1e244 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -17,13 +17,14 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchersMacros.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/Formula.h"
-#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "clang/Analysis/FlowSensitive/RecordOps.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
#include "clang/Analysis/FlowSensitive/Value.h"
#include "clang/Basic/SourceLocation.h"
@@ -104,10 +105,17 @@ static const CXXRecordDecl *getOptionalBaseClass(const CXXRecordDecl *RD) {
return nullptr;
}
+static bool isSupportedOptionalType(QualType Ty) {
+ const CXXRecordDecl *Optional =
+ getOptionalBaseClass(Ty->getAsCXXRecordDecl());
+ return Optional != nullptr;
+}
+
namespace {
using namespace ::clang::ast_matchers;
-using LatticeTransferState = TransferState<NoopLattice>;
+
+using LatticeTransferState = TransferState<UncheckedOptionalAccessLattice>;
AST_MATCHER(CXXRecordDecl, optionalClass) { return hasOptionalClassName(Node); }
@@ -325,6 +333,19 @@ auto isValueOrNotEqX() {
ComparesToSame(integerLiteral(equals(0)))));
}
+auto isZeroParamConstMemberCall() {
+ return cxxMemberCallExpr(
+ callee(cxxMethodDecl(parameterCountIs(0), isConst())));
+}
+
+auto isNonConstMemberCall() {
+ return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
+auto isNonConstMemberOperatorCall() {
+ return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
auto isCallReturningOptional() {
return callExpr(hasType(qualType(
anyOf(desugarsToOptionalOrDerivedType(),
@@ -523,6 +544,99 @@ void transferCallReturningOptional(const CallExpr *E,
setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
}
+void handleConstMemberCall(const CallExpr *CE,
+ dataflow::RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ // If the const method returns an optional or reference to an optional.
+ if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
+ StorageLocation *Loc =
+ State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+ *RecordLoc, CE, State.Env, [&](StorageLocation &Loc) {
+ setHasValue(cast<RecordStorageLocation>(Loc),
+ State.Env.makeAtomicBoolValue(), State.Env);
+ });
+ if (Loc == nullptr)
+ return;
+ if (CE->isGLValue()) {
+ // If the call to the const method returns a reference to an optional,
+ // link the call expression to the cached StorageLocation.
+ State.Env.setStorageLocation(*CE, *Loc);
+ } else {
+ // If the call to the const method returns an optional by value, we
+ // need to use CopyRecord to link the optional to the result object
+ // of the call expression.
+ auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
+ copyRecord(*cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
+ }
+ return;
+ }
+
+ // Cache if the const method returns a boolean type.
+ // We may decide to cache other return types in the future.
+ if (RecordLoc != nullptr && CE->getType()->isBooleanType()) {
+ Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, CE,
+ State.Env);
+ if (Val == nullptr)
+ return;
+ State.Env.setValue(*CE, *Val);
+ return;
+ }
+
+ // Perform default handling if the call returns an optional
+ // but wasn't handled above (if RecordLoc is nullptr).
+ if (isSupportedOptionalType(CE->getType())) {
+ transferCallReturningOptional(CE, Result, State);
+ }
+}
+
+void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ handleConstMemberCall(
+ MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
+}
+
+void handleNonConstMemberCall(const CallExpr *CE,
+ dataflow::RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ // When a non-const member function is called, reset some state.
+ if (RecordLoc != nullptr) {
+ for (const auto [Field, FieldLoc] : RecordLoc->children()) {
+ if (isSupportedOptionalType(Field->getType())) {
+ auto *FieldRecordLoc = cast_or_null<RecordStorageLocation>(FieldLoc);
+ if (FieldRecordLoc) {
+ setHasValue(*FieldRecordLoc, State.Env.makeAtomicBoolValue(),
+ State.Env);
+ }
+ }
+ }
+ State.Lattice.clearConstMethodReturnValues(*RecordLoc);
+ State.Lattice.clearConstMethodReturnStorageLocations(*RecordLoc);
+ }
+
+ // Perform default handling if the call returns an optional.
+ if (isSupportedOptionalType(CE->getType())) {
+ transferCallReturningOptional(CE, Result, State);
+ }
+}
+
+void transferValue_NonConstMemberCall(const CXXMemberCallExpr *MCE,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ handleNonConstMemberCall(
+ MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
+}
+
+void transferValue_NonConstMemberOperatorCall(
+ const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
+ State.Env.getStorageLocation(*OCE->getArg(0)));
+ handleNonConstMemberCall(OCE, RecordLoc, Result, State);
+}
+
void constructOptionalValue(const Expr &E, Environment &Env,
BoolValue &HasValueVal) {
RecordStorageLocation &Loc = Env.getResultObjectLocation(E);
@@ -899,7 +1013,17 @@ auto buildTransferMatchSwitch() {
transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
})
- // returns optional
+ // const accessor calls
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
+ transferValue_ConstMemberCall)
+ // non-const member calls that may modify the state of an object.
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
+ transferValue_NonConstMemberCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isNonConstMemberOperatorCall(),
+ transferValue_NonConstMemberOperatorCall)
+
+ // other cases of returning optional
.CaseOfCFGStmt<CallExpr>(isCallReturningOptional(),
transferCallReturningOptional)
@@ -958,7 +1082,8 @@ UncheckedOptionalAccessModel::optionalClassDecl() {
UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
Environment &Env)
- : DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx),
+ : DataflowAnalysis<UncheckedOptionalAccessModel,
+ UncheckedOptionalAccessLattice>(Ctx),
TransferMatchSwitch(buildTransferMatchSwitch()) {
Env.getDataflowAnalysisContext().setSyntheticFieldCallback(
[&Ctx](QualType Ty) -> llvm::StringMap<QualType> {
@@ -972,7 +1097,8 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
}
void UncheckedOptionalAccessModel::transfer(const CFGElement &Elt,
- NoopLattice &L, Environment &Env) {
+ UncheckedOptionalAccessLattice &L,
+ Environment &Env) {
LatticeTransferState State(L, Env);
TransferMatchSwitch(Elt, getASTContext(), State);
}
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 877bfce8b27092..234f8ed3a45487 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1342,7 +1342,8 @@ class UncheckedOptionalAccessTest
Diagnoser =
UncheckedOptionalAccessDiagnoser(Options)](
ASTContext &Ctx, const CFGElement &Elt,
- const TransferStateForDiagnostics<NoopLattice>
+ const TransferStateForDiagnostics<
+ UncheckedOptionalAccessLattice>
&State) mutable {
auto EltDiagnostics = Diagnoser(Elt, Ctx, State);
llvm::move(EltDiagnostics, std::back_inserter(Diagnostics));
@@ -3549,6 +3550,180 @@ TEST_P(UncheckedOptionalAccessTest, ClassDerivedFromOptionalValueConstructor) {
)");
}
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessor) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ const $ns::$optional<int>& get() const { return x; }
+ $ns::$optional<int> x;
+ };
+
+ void target(A& a) {
+ if (a.get().has_value()) {
+ a.get().value();
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorWithModInBetween) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ const $ns::$optional<int>& get() const { return x; }
+ void clear();
+ $ns::$optional<int> x;
+ };
+
+ void target(A& a) {
+ if (a.get().has_value()) {
+ a.clear();
+ a.get().value(); // [[unsafe]]
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorWithModReturningOptional) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ const $ns::$optional<int>& get() const { return x; }
+ $ns::$optional<int> take();
+ $ns::$optional<int> x;
+ };
+
+ void target(A& a) {
+ if (a.get().has_value()) {
+ $ns::$optional<int> other = a.take();
+ a.get().value(); // [[unsafe]]
+ if (other.has_value()) {
+ other.value();
+ }
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorDifferentObjects) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ const $ns::$optional<int>& get() const { return x; }
+ $ns::$optional<int> x;
+ };
+
+ void target(A& a1, A& a2) {
+ if (a1.get().has_value()) {
+ a2.get().value(); // [[unsafe]]
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorLoop) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ const $ns::$optional<int>& get() const { return x; }
+ $ns::$optional<int> x;
+ };
+
+ void target(A& a, int N) {
+ for (int i = 0; i < N; ++i) {
+ if (a.get().has_value()) {
+ a.get().value();
+ }
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstByValueAccessor) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ $ns::$optional<int> get() const { return x; }
+ $ns::$optional<int> x;
+ };
+
+ void target(A& a) {
+ if (a.get().has_value()) {
+ a.get().value();
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstByValueAccessorWithModInBetween) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ $ns::$optional<int> get() const { return x; }
+ void clear();
+ $ns::$optional<int> x;
+ };
+
+ void target(A& a) {
+ if (a.get().has_value()) {
+ a.clear();
+ a.get().value(); // [[unsafe]]
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ bool isFoo() const { return f; }
+ bool f;
+ };
+
+ void target(A& a) {
+ std::optional<int> opt;
+ if (a.isFoo()) {
+ opt = 1;
+ }
+ if (a.isFoo()) {
+ opt.value();
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct A {
+ bool isFoo() const { return f; }
+ void clear();
+ bool f;
+ };
+
+ void target(A& a) {
+ std::optional<int> opt;
+ if (a.isFoo()) {
+ opt = 1;
+ }
+ a.clear();
+ if (a.isFoo()) {
+ opt.value(); // [[unsafe]]
+ }
+ }
+ )cc");
+}
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
``````````
</details>
https://github.com/llvm/llvm-project/pull/112605
More information about the cfe-commits
mailing list