[FlowSensitive] [StatusOr] [3/N] Support absl::Status ops

[fmayer]

absl::StatusOr::status allows extraction of the status associated with
a StatusOr value. That can also be used to check whether the StatusOr
has a value or not.

This makes sure code like this is checked properly:

int target(absl::StatusOr<int> SOR) {
  if (SOR.status().ok())
    return *SOR;
  return 0;
}

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Florian Mayer (fmayer)

Changes

absl::StatusOr::status allows extraction of the status associated with
a StatusOr value. That can also be used to check whether the StatusOr
has a value or not.

This makes sure code like this is checked properly:

int target(absl::StatusOr<int> SOR) {
  if (SOR.status().ok())
    return *SOR;
  return 0;
}

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp (+72)
• (modified) clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp (+148)

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 9ea962abc24ee..11c4ad90293d9 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -24,6 +24,7 @@
 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/MatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/RecordOps.h"
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/LLVM.h"
@@ -73,6 +74,18 @@ static QualType getStatusOrValueType(ClassTemplateSpecializationDecl *TRD) {
   return TRD->getTemplateArgs().get(0).getAsType();
 }
 
+static auto ofClassStatus() {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return ofClass(hasName("::absl::Status"));
+}
+
+static auto isStatusMemberCallWithName(llvm::StringRef member_name) {
+  using namespace ::clang::ast_matchers; // NOLINT: Too many names
+  return cxxMemberCallExpr(
+      on(expr(unless(cxxThisExpr()))),
+      callee(cxxMethodDecl(hasName(member_name), ofClassStatus())));
+}
+
 static auto isStatusOrMemberCallWithName(llvm::StringRef member_name) {
   using namespace ::clang::ast_matchers; // NOLINT: Too many names
   return cxxMemberCallExpr(
@@ -238,6 +251,59 @@ static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr,
   State.Env.setValue(*Expr, OkVal);
 }
 
+static void transferStatusCall(const CXXMemberCallExpr *Expr,
+                               const MatchFinder::MatchResult &,
+                               LatticeTransferState &State) {
+  RecordStorageLocation *StatusOrLoc =
+      getImplicitObjectLocation(*Expr, State.Env);
+  if (StatusOrLoc == nullptr)
+    return;
+
+  RecordStorageLocation &StatusLoc = locForStatus(*StatusOrLoc);
+
+  if (State.Env.getValue(locForOk(StatusLoc)) == nullptr)
+    initializeStatusOr(*StatusOrLoc, State.Env);
+
+  if (Expr->isPRValue())
+    copyRecord(StatusLoc, State.Env.getResultObjectLocation(*Expr), State.Env);
+  else
+    State.Env.setStorageLocation(*Expr, StatusLoc);
+}
+
+static void transferStatusOkCall(const CXXMemberCallExpr *Expr,
+                                 const MatchFinder::MatchResult &,
+                                 LatticeTransferState &State) {
+  RecordStorageLocation *StatusLoc =
+      getImplicitObjectLocation(*Expr, State.Env);
+  if (StatusLoc == nullptr)
+    return;
+
+  if (Value *Val = State.Env.getValue(locForOk(*StatusLoc)))
+    State.Env.setValue(*Expr, *Val);
+}
+
+static void transferStatusUpdateCall(const CXXMemberCallExpr *Expr,
+                                     const MatchFinder::MatchResult &,
+                                     LatticeTransferState &State) {
+  assert(Expr->getNumArgs() == 1);
+  auto *Arg = Expr->getArg(0);
+  RecordStorageLocation *ArgRecord =
+      Arg->isPRValue() ? &State.Env.getResultObjectLocation(*Arg)
+                       : State.Env.get<RecordStorageLocation>(*Arg);
+  RecordStorageLocation *ThisLoc = getImplicitObjectLocation(*Expr, State.Env);
+  if (ThisLoc == nullptr || ArgRecord == nullptr)
+    return;
+
+  auto &ThisOkVal = valForOk(*ThisLoc, State.Env);
+  auto &ArgOkVal = valForOk(*ArgRecord, State.Env);
+  auto &A = State.Env.arena();
+  auto &NewVal = State.Env.makeAtomicBoolValue();
+  State.Env.assume(A.makeImplies(A.makeNot(ThisOkVal.formula()),
+                                 A.makeNot(NewVal.formula())));
+  State.Env.assume(A.makeImplies(NewVal.formula(), ArgOkVal.formula()));
+  State.Env.setValue(locForOk(*ThisLoc), NewVal);
+}
+
 CFGMatchSwitch<LatticeTransferState>
 buildTransferMatchSwitch(ASTContext &Ctx,
                          CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
@@ -245,6 +311,12 @@ buildTransferMatchSwitch(ASTContext &Ctx,
   return std::move(Builder)
       .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("ok"),
                                         transferStatusOrOkCall)
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("status"),
+                                        transferStatusCall)
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusMemberCallWithName("ok"),
+                                        transferStatusOkCall)
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusMemberCallWithName("Update"),
+                                        transferStatusUpdateCall)
       .Build();
 }
 
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 4827cc1d0a7e9..d6e326cae11fd 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -2453,6 +2453,154 @@ TEST_P(UncheckedStatusOrAccessModelTest, SubclassOperator) {
       )cc");
 }
 
+TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusCheck) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      if (sor.status().ok())
+        sor.value();
+      else
+        sor.value();  // [[unsafe]]
+    }
+  )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusRefCheck) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      const STATUS& s = sor.status();
+      if (s.ok())
+        sor.value();
+      else
+        sor.value();  // [[unsafe]]
+    }
+  )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, UnwrapValueWithStatusPtrCheck) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      const STATUS* s = &sor.status();
+      if (s->ok())
+        sor.value();
+      else
+        sor.value();  // [[unsafe]]
+    }
+  )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, MembersUsedInsideStatus) {
+  ExpectDiagnosticsFor(R"cc(
+    namespace absl {
+
+    class Status {
+     public:
+      bool ok() const;
+
+      void target() const { ok(); }
+    };
+
+    }  // namespace absl
+  )cc");
+}
+
+TEST_P(UncheckedStatusOrAccessModelTest, StatusUpdate) {
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor) {
+      STATUS s;
+      s.Update(sor.status());
+      if (s.ok())
+        sor.value();
+      else
+        sor.value();  // [[unsafe]]
+    }
+  )cc");
+
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+      STATUS s;
+      s.Update(sor1.status());
+      s.Update(sor2.status());
+      if (s.ok()) {
+        sor1.value();
+        sor2.value();
+      }
+    }
+  )cc");
+
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+      STATUS s;
+      s.Update(sor1.status());
+      CHECK(s.ok());
+      s.Update(sor2.status());
+      sor1.value();
+      sor2.value();  // [[unsafe]]
+    }
+  )cc");
+
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+      STATUS s;
+      s.Update(sor1.status());
+      CHECK(s.ok());
+      sor1.value();
+      sor2.value();  // [[unsafe]]
+    }
+  )cc");
+
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+      STATUS s;
+      STATUS sor1_status = sor1.status();
+      s.Update(std::move(sor1_status));
+      CHECK(s.ok());
+      sor1.value();
+      sor2.value();  // [[unsafe]]
+    }
+  )cc");
+
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    void target(STATUSOR_INT sor1, STATUSOR_INT sor2) {
+      STATUS s;
+      STATUS sor1_status = sor1.status();
+      sor1_status.Update(sor2.status());
+      s.Update(std::move(sor1_status));
+      CHECK(s.ok());
+      sor1.value();
+      sor2.value();
+    }
+  )cc");
+  ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+    const STATUS& OptStatus();
+
+    void target(STATUSOR_INT sor) {
+      auto s = sor.status();
+      s.Update(OptStatus());
+      if (s.ok()) sor.value();
+    }
+  )cc");
+}
+
 } // namespace
 
 std::string

[fmayer]

@BaLiKfromUA CC

[jvoung]

Just a few small comments.

Also, is isRecordTypeWithName now unused and replaced by isTypeNamed ?

[fmayer]

Also, is isRecordTypeWithName now unused and replaced by isTypeNamed ?

Good catch. #164351