[clang][dataflow] Cache accessors for bugprone-unchecked-optional-access #112605
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
Treat calls to zero-param const methods as having stable return values (with a cache) to address issue llvm#58510. The cache is invalidated when non-const methods are called. This uses the infrastructure from PR llvm#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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-analysis Author: Jan Voung (jvoung) ChangesTreat calls to zero-param const methods as having stable return values For now we cache methods returning:
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:
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)
|
|
@llvm/pr-subscribers-clang Author: Jan Voung (jvoung) ChangesTreat calls to zero-param const methods as having stable return values For now we cache methods returning:
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:
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)
|
|
@llvm/pr-subscribers-clang-tidy Author: Jan Voung (jvoung) ChangesTreat calls to zero-param const methods as having stable return values For now we cache methods returning:
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:
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)
|
EugeneZelenko
requested review from
PiotrZSL,
SimplyDanny,
5chmidti and
HerrCai0907
ymand
approved these changes
Oct 21, 2024
Comment on lines
605
to
614
| 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); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
IIUC, this functionality is new and actually independent of the current change? that is, we were not previously clearing object state on non-const method calls, but arguably should have been (for safety purposes)?
If so, is it worth adding a test specifically for this behavior? (that is, checking that an optional field is cleared when a non-const method is called, even without any const-method related behavior)
There was a problem hiding this comment.
Yes this is new and independent of the accessors feature (overlap is handling non-const method calls now).
Sounds good! Added a test.
Also fix a warning about copies in for-each loop.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ymand
approved these changes
Oct 21, 2024
jvoung
added a commit
to jvoung/llvm-project
that referenced
this pull request
Jan 9, 2025
… pointer caching With caching added in llvm#120249, inform in notes that the `IgnoreSmartPointerDereference` option shouldn't be needed anymore. Other caching also added earlier: llvm#112605
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Feb 27, 2025
…ccess smart pointer caching (#122290) With caching added in llvm/llvm-project#120249, the `IgnoreSmartPointerDereference` option shouldn't be needed anymore. Other caching also added earlier: llvm/llvm-project#112605
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Feb 28, 2025
…prone-unchecked-optional-access` (#128437) Fixes llvm/llvm-project#126283 Extending llvm/llvm-project#112605 to cache const getters which return references. Fixes false positives from const reference accessors to object containing optional member
cheezeburglar
pushed a commit
to cheezeburglar/llvm-project
that referenced
this pull request
Feb 28, 2025
…cked-optional-access` (llvm#128437) Fixes llvm#126283 Extending llvm#112605 to cache const getters which return references. Fixes false positives from const reference accessors to object containing optional member
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Mar 3, 2025
… pointer caching (llvm#122290) With caching added in llvm#120249, the `IgnoreSmartPointerDereference` option shouldn't be needed anymore. Other caching also added earlier: llvm#112605
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.
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:
We can extend that to pointers to optional in a next change.