[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.

Author: jvoung

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
 -----------------------------------
 

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);
   }
 

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);
 }