Author: Jan Voung Date: 2024-10-22T10:18:22-04:00 New Revision: 6761b24ae2f34b923df46412475a9ece50542b97
URL: https://github.com/llvm/llvm-project/commit/6761b24ae2f34b923df46412475a9ece50542b97 DIFF: https://github.com/llvm/llvm-project/commit/6761b24ae2f34b923df46412475a9ece50542b97.diff LOG: [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (#112605) 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. Added: Modified: clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp Removed: ################################################################################ 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..b0bd8274405d02 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..0209703395bc11 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)); @@ -2166,6 +2167,26 @@ TEST_P(UncheckedOptionalAccessTest, OptionalReturnedFromFuntionCall) { )"); } +TEST_P(UncheckedOptionalAccessTest, OptionalFieldModified) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional<std::string> opt; + void clear(); // assume this may modify the opt field's state + }; + + void target(Foo& foo) { + if (foo.opt) { + foo.opt.value(); + foo.clear(); + foo.opt.value(); // [[unsafe]] + } + } + )"); +} + TEST_P(UncheckedOptionalAccessTest, StdSwap) { ExpectDiagnosticsFor( R"( @@ -3549,6 +3570,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) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
