[FlowSensitive] [StatusOr] [11/N] Assume const accessor calls are stable

[fmayer]

This is not necessarily correct, but prevents us from flagging lots of
false positives because code usually abides by this.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Florian Mayer (fmayer)

Changes

This is not necessarily correct, but prevents us from flagging lots of
false positives because code usually abides by this.

---

Full diff: https://github.com/llvm/llvm-project/pull/170935.diff

3 Files Affected:

• (modified) clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h (+3-1)
• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp (+161)
• (modified) clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp (+173)

diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
index 24f8b0b99870a..2d54bd3c6f7ad 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
@@ -13,6 +13,7 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/MatchSwitch.h"
@@ -69,7 +70,8 @@ struct UncheckedStatusOrAccessModelOptions {};
 
 // Dataflow analysis that discovers unsafe uses of StatusOr values.
 class UncheckedStatusOrAccessModel
-    : public DataflowAnalysis<UncheckedStatusOrAccessModel, NoopLattice> {
+    : public DataflowAnalysis<UncheckedStatusOrAccessModel,
+                              CachedConstAccessorsLattice<NoopLattice>> {
 public:
   explicit UncheckedStatusOrAccessModel(ASTContext &Ctx, Environment &Env);
 
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 5869aa9a3e227..038f2b0338c8d 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -237,6 +237,49 @@ static auto isAsStatusCallWithStatusOr() {
       hasArgument(0, hasType(statusOrType())));
 }
 
+static auto possiblyReferencedStatusOrType() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return anyOf(statusOrType(), referenceType(pointee(statusOrType())));
+}
+
+static auto isConstStatusOrAccessorMemberCall() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return cxxMemberCallExpr(callee(
+      cxxMethodDecl(parameterCountIs(0), isConst(),
+                    returns(qualType(possiblyReferencedStatusOrType())))));
+}
+
+static auto isConstStatusOrAccessorMemberOperatorCall() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return cxxOperatorCallExpr(
+      callee(cxxMethodDecl(parameterCountIs(0), isConst(),
+                           returns(possiblyReferencedStatusOrType()))));
+}
+
+static auto isConstStatusOrPointerAccessorMemberCall() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return cxxMemberCallExpr(callee(cxxMethodDecl(
+      parameterCountIs(0), isConst(),
+      returns(pointerType(pointee(possiblyReferencedStatusOrType()))))));
+}
+
+static auto isConstStatusOrPointerAccessorMemberOperatorCall() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return cxxOperatorCallExpr(callee(cxxMethodDecl(
+      parameterCountIs(0), isConst(),
+      returns(pointerType(pointee(possiblyReferencedStatusOrType()))))));
+}
+
+static auto isNonConstMemberCall() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
+static auto isNonConstMemberOperatorCall() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
 static auto
 buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
   return CFGMatchSwitchBuilder<const Environment,
@@ -697,6 +740,107 @@ static void transferPointerToBoolean(const ImplicitCastExpr *Expr,
           dyn_cast_or_null<BoolValue>(State.Env.getValue(*Expr->getSubExpr())))
     State.Env.setValue(*Expr, *SubExprVal);
 }
+static void handleConstStatusOrAccessorMemberCall(
+    const CallExpr *Expr, RecordStorageLocation *RecordLoc,
+    const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
+  assert(isStatusOrType(Expr->getType()));
+  if (RecordLoc == nullptr)
+    return;
+  const FunctionDecl *DirectCallee = Expr->getDirectCallee();
+  if (DirectCallee == nullptr)
+    return;
+  StorageLocation &Loc =
+      State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+          *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+            initializeStatusOr(cast<RecordStorageLocation>(Loc), State.Env);
+          });
+  if (Expr->isPRValue()) {
+    auto &ResultLoc = State.Env.getResultObjectLocation(*Expr);
+    copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
+  } else {
+    State.Env.setStorageLocation(*Expr, Loc);
+  }
+}
+
+static void handleConstStatusOrPointerAccessorMemberCall(
+    const CallExpr *Expr, RecordStorageLocation *RecordLoc,
+    const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
+  if (RecordLoc == nullptr)
+    return;
+  auto *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, Expr,
+                                                              State.Env);
+  State.Env.setValue(*Expr, *Val);
+}
+
+static void
+transferConstStatusOrAccessorMemberCall(const CXXMemberCallExpr *Expr,
+                                        const MatchFinder::MatchResult &Result,
+                                        LatticeTransferState &State) {
+  handleConstStatusOrAccessorMemberCall(
+      Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State);
+}
+
+static void transferConstStatusOrAccessorMemberOperatorCall(
+    const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result,
+    LatticeTransferState &State) {
+  auto *RecordLoc = cast_or_null<RecordStorageLocation>(
+      State.Env.getStorageLocation(*Expr->getArg(0)));
+  handleConstStatusOrAccessorMemberCall(Expr, RecordLoc, Result, State);
+}
+
+static void transferConstStatusOrPointerAccessorMemberCall(
+    const CXXMemberCallExpr *Expr, const MatchFinder::MatchResult &Result,
+    LatticeTransferState &State) {
+  handleConstStatusOrPointerAccessorMemberCall(
+      Expr, getImplicitObjectLocation(*Expr, State.Env), Result, State);
+}
+
+static void transferConstStatusOrPointerAccessorMemberOperatorCall(
+    const CXXOperatorCallExpr *Expr, const MatchFinder::MatchResult &Result,
+    LatticeTransferState &State) {
+  auto *RecordLoc = cast_or_null<RecordStorageLocation>(
+      State.Env.getStorageLocation(*Expr->getArg(0)));
+  handleConstStatusOrPointerAccessorMemberCall(Expr, RecordLoc, Result, State);
+}
+
+static void transferStatusOrReturningCall(const CallExpr *Expr,
+                                          LatticeTransferState &State) {
+  RecordStorageLocation *StatusOrLoc =
+      Expr->isPRValue() ? &State.Env.getResultObjectLocation(*Expr)
+                        : State.Env.get<RecordStorageLocation>(*Expr);
+  if (StatusOrLoc != nullptr &&
+      State.Env.getValue(locForOk(locForStatus(*StatusOrLoc))) == nullptr)
+    initializeStatusOr(*StatusOrLoc, State.Env);
+}
+
+static void handleNonConstMemberCall(const CallExpr *Expr,
+                                     RecordStorageLocation *RecordLoc,
+                                     const MatchFinder::MatchResult &Result,
+                                     LatticeTransferState &State) {
+  if (RecordLoc == nullptr)
+    return;
+  State.Lattice.clearConstMethodReturnValues(*RecordLoc);
+  State.Lattice.clearConstMethodReturnStorageLocations(*RecordLoc);
+
+  if (isStatusOrType(Expr->getType()))
+    transferStatusOrReturningCall(Expr, State);
+}
+
+static void transferNonConstMemberCall(const CXXMemberCallExpr *Expr,
+                                       const MatchFinder::MatchResult &Result,
+                                       LatticeTransferState &State) {
+  handleNonConstMemberCall(Expr, getImplicitObjectLocation(*Expr, State.Env),
+                           Result, State);
+}
+
+static void
+transferNonConstMemberOperatorCall(const CXXOperatorCallExpr *Expr,
+                                   const MatchFinder::MatchResult &Result,
+                                   LatticeTransferState &State) {
+  auto *RecordLoc = cast_or_null<RecordStorageLocation>(
+      State.Env.getStorageLocation(*Expr->getArg(0)));
+  handleNonConstMemberCall(Expr, RecordLoc, Result, State);
+}
 
 CFGMatchSwitch<LatticeTransferState>
 buildTransferMatchSwitch(ASTContext &Ctx,
@@ -755,6 +899,23 @@ buildTransferMatchSwitch(ASTContext &Ctx,
                                transferLoggingGetReferenceableValueCall)
       .CaseOfCFGStmt<CallExpr>(isLoggingCheckEqImpl(),
                                transferLoggingCheckEqImpl)
+      // const accessor calls
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isConstStatusOrAccessorMemberCall(),
+                                        transferConstStatusOrAccessorMemberCall)
+      .CaseOfCFGStmt<CXXOperatorCallExpr>(
+          isConstStatusOrAccessorMemberOperatorCall(),
+          transferConstStatusOrAccessorMemberOperatorCall)
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isConstStatusOrPointerAccessorMemberCall(),
+          transferConstStatusOrPointerAccessorMemberCall)
+      .CaseOfCFGStmt<CXXOperatorCallExpr>(
+          isConstStatusOrPointerAccessorMemberOperatorCall(),
+          transferConstStatusOrPointerAccessorMemberOperatorCall)
+      // non-const member calls that may modify the state of an object.
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
+                                        transferNonConstMemberCall)
+      .CaseOfCFGStmt<CXXOperatorCallExpr>(isNonConstMemberOperatorCall(),
+                                          transferNonConstMemberOperatorCall)
       // N.B. These need to come after all other CXXConstructExpr.
       // These are there to make sure that every Status and StatusOr object
       // have their ok boolean initialized when constructed. If we were to
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 13cde05df0a3f..e075818f8a2c1 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -3270,6 +3270,179 @@ TEST_P(UncheckedStatusOrAccessModelTest, ConstructStatusFromReference) {
   )cc");
 }
 
+TEST_P(UncheckedStatusOrAccessModelTest, AccessorCall) {
+  // Accessor returns reference.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          const STATUSOR_INT& sor() const { return sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo.sor().ok()) foo.sor().value();
+        }
+      )cc");
+
+  // Uses an operator
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          const STATUSOR_INT& operator()() const { return sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo().ok()) foo().value();
+        }
+      )cc");
+
+  // Calls nonconst method inbetween.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          void invalidate() {}
+
+          const STATUSOR_INT& sor() const { return sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo.sor().ok()) {
+            foo.invalidate();
+            foo.sor().value();  // [[unsafe]]
+          }
+        }
+      )cc");
+
+  // Calls nonconst operator inbetween.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          void operator()() {}
+
+          const STATUSOR_INT& sor() const { return sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo.sor().ok()) {
+            foo();
+            foo.sor().value();  // [[unsafe]]
+          }
+        }
+      )cc");
+
+  // Accessor returns copy.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          STATUSOR_INT sor() const { return sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo.sor().ok()) foo.sor().value();
+        }
+      )cc");
+
+  // Non-const accessor.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          const STATUSOR_INT& sor() { return sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo.sor().ok()) foo.sor().value();  // [[unsafe]]
+        }
+      )cc");
+
+  // Non-const rvalue accessor.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          STATUSOR_INT&& sor() { return std::move(sor_); }
+        };
+
+        void target(Foo foo) {
+          if (foo.sor().ok()) foo.sor().value();  // [[unsafe]]
+        }
+      )cc");
+
+  // const pointer accessor.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          const STATUSOR_INT* sor() const { return &sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo.sor()->ok()) foo.sor()->value();
+        }
+      )cc");
+
+  // const pointer operator.
+  ExpectDiagnosticsFor(
+      R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+        struct Foo {
+          STATUSOR_INT sor_;
+
+          const STATUSOR_INT* operator->() const { return &sor_; }
+        };
+
+        void target(Foo foo) {
+          if (foo->ok()) foo->value();
+        }
+      )cc");
+
+  // We copy the result of the accessor.
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    struct Foo {
+      STATUSOR_INT sor_;
+
+      const STATUSOR_INT& sor() const { return sor_; }
+    };
+    void target() {
+      Foo foo;
+      if (!foo.sor().ok()) return;
+      const auto sor = foo.sor();
+      sor.value();
+    }
+  )cc");
+}
+
 } // namespace
 
 std::string

[jvoung]

Would it make sense to do the default handling of

"if (isStatusOrType(Expr->getType())) transferStatusOrReturningCall(Expr, State);"

when RecordLoc is null?

[fmayer]

done

[jvoung]

Would it make sense to do the default handling of

"if (isStatusOrType(Expr->getType())) transferStatusOrReturningCall(Expr, State);"

when DirectCallee is null?

[fmayer]

done

[jvoung]

nit: "in between"?

[fmayer]

done