[clang][dataflow] Add test for crash repro and clean up const accessor handling (#129930)

Add test for https://github.com/llvm/llvm-project/issues/125589

The crash is actually incidentally fixed by
https://github.com/llvm/llvm-project/pull/128437 since it added a branch
for the reference case and would no longer fall through when the return
type is a reference to a pointer.

Clean up a bit as well:
- make the fallback for early returns more consistent (check if
  returning optional and call transfer function for that case)
- check RecordLoc == nullptr in one place
- clean up extra spaces in test
- clean up parameterization in test of `std::` vs `$ns::$`

1 files changed, 62 insertions, 53 deletions

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 9381c5c..c28424f 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -551,83 +551,92 @@ void transferCallReturningOptional(const CallExpr *E,
   setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
 }
 
-void handleConstMemberCall(const CallExpr *CE,
+// Returns true if the const accessor is handled by caching.
+// Returns false if we could not cache. We should perform default handling
+// in that case.
+bool 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())) {
-    const FunctionDecl *DirectCallee = CE->getDirectCallee();
-    if (DirectCallee == nullptr)
-      return;
-    StorageLocation &Loc =
-        State.Lattice.getOrCreateConstMethodReturnStorageLocation(
-            *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
-              setHasValue(cast<RecordStorageLocation>(Loc),
-                          State.Env.makeAtomicBoolValue(), State.Env);
-            });
-    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;
-  }
+  if (RecordLoc == nullptr)
+    return false;
 
-  // Cache if the const method returns a reference
-  if (RecordLoc != nullptr && CE->isGLValue()) {
+  // Cache if the const method returns a reference.
+  if (CE->isGLValue()) {
     const FunctionDecl *DirectCallee = CE->getDirectCallee();
     if (DirectCallee == nullptr)
-      return;
+      return false;
 
+    // Initialize the optional's "has_value" property to true if the type is
+    // optional, otherwise no-op. If we want to support const ref to pointers or
+    // bools we should initialize their values here too.
+    auto Init = [&](StorageLocation &Loc) {
+      if (isSupportedOptionalType(CE->getType()))
+        setHasValue(cast<RecordStorageLocation>(Loc),
+                    State.Env.makeAtomicBoolValue(), State.Env);
+    };
     StorageLocation &Loc =
         State.Lattice.getOrCreateConstMethodReturnStorageLocation(
-            *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
-              // no-op
-            });
+            *RecordLoc, DirectCallee, State.Env, Init);
 
     State.Env.setStorageLocation(*CE, Loc);
-    return;
+    return true;
   }
-
-  // Cache if the const method returns a boolean or pointer type.
-  // We may decide to cache other return types in the future.
-  if (RecordLoc != nullptr &&
-      (CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
+  // PRValue cases:
+  if (CE->getType()->isBooleanType() || CE->getType()->isPointerType()) {
+    // If the const method returns a boolean or pointer type.
     Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, CE,
                                                                  State.Env);
     if (Val == nullptr)
-      return;
+      return false;
     State.Env.setValue(*CE, *Val);
-    return;
+    return true;
   }
-
-  // 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);
+    // If the const method returns an optional by value.
+    const FunctionDecl *DirectCallee = CE->getDirectCallee();
+    if (DirectCallee == nullptr)
+      return false;
+    StorageLocation &Loc =
+        State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+            *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+              setHasValue(cast<RecordStorageLocation>(Loc),
+                          State.Env.makeAtomicBoolValue(), State.Env);
+            });
+    // 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 true;
   }
+
+  return false;
 }
 
-void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
-                                   const MatchFinder::MatchResult &Result,
-                                   LatticeTransferState &State) {
-  handleConstMemberCall(
+void handleConstMemberCallWithFallbacks(
+    const CallExpr *CE, dataflow::RecordStorageLocation *RecordLoc,
+    const MatchFinder::MatchResult &Result, LatticeTransferState &State) {
+  if (handleConstMemberCall(CE, RecordLoc, Result, State))
+    return;
+  // Perform default handling if the call returns an optional, but wasn't
+  // handled by caching.
+  if (isSupportedOptionalType(CE->getType()))
+    transferCallReturningOptional(CE, Result, State);
+}
+
+void transferConstMemberCall(const CXXMemberCallExpr *MCE,
+                             const MatchFinder::MatchResult &Result,
+                             LatticeTransferState &State) {
+  handleConstMemberCallWithFallbacks(
       MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
 }
 
-void transferValue_ConstMemberOperatorCall(
-    const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
-    LatticeTransferState &State) {
+void transferConstMemberOperatorCall(const CXXOperatorCallExpr *OCE,
+                                     const MatchFinder::MatchResult &Result,
+                                     LatticeTransferState &State) {
   auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
       State.Env.getStorageLocation(*OCE->getArg(0)));
-  handleConstMemberCall(OCE, RecordLoc, Result, State);
+  handleConstMemberCallWithFallbacks(OCE, RecordLoc, Result, State);
 }
 
 void handleNonConstMemberCall(const CallExpr *CE,
@@ -1094,9 +1103,9 @@ auto buildTransferMatchSwitch() {
 
       // const accessor calls
       .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
-                                        transferValue_ConstMemberCall)
+                                        transferConstMemberCall)
       .CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
-                                          transferValue_ConstMemberOperatorCall)
+                                          transferConstMemberOperatorCall)
       // non-const member calls that may modify the state of an object.
       .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
                                         transferValue_NonConstMemberCall)