[analyzer][NFC] Change return value of StoreManager::attemptDownCast function from SVal to Optional<SVal>

[ASDenysPetrov]

Summary

Refactor return value of StoreManager::attemptDownCast function by removing the last parameter bool &Failed and replace the return value SVal with Optional<SVal>. Make the function consistent with the family of evalDerivedToBase by renaming it to evalBaseToDerived. Align the code on the call side with these changes.

[xazax.hun]

LGTM, thanks!

[ASDenysPetrov]

@xazax.hun
Many thanks for the review.

[ASDenysPetrov]

Closed with rGda8bd972a33ad642d6237b5b95b31a2a20f46a35

clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h

public:
   ///        dynamic_cast.
   /// The callback may result in the following 3 scenarios:
   ///  - Successful cast (ex: derived is subclass of base).
   ///  - Failed cast (ex: derived is definitely not a subclass of base).
   ///    The distinction of this case from the next one is necessary to model
   ///    dynamic_cast.
   ///  - We don't know (base is a symbolic region and we don't have
   ///    enough info to determine if the cast will succeed at run time).
-  /// The function returns an SVal representing the derived class; it's
-  /// valid only if Failed flag is set to false.
-  SVal attemptDownCast(SVal Base, QualType DerivedPtrType, bool &Failed);
+  /// The function returns an optional with SVal representing the derived class
+  /// in case of a successful cast and `None` otherwise.
+  Optional<SVal> evalBaseToDerived(SVal Base, QualType DerivedPtrType);
 
   const ElementRegion *GetElementZeroRegion(const SubRegion *R, QualType T);
 
   /// castRegion - Used by ExprEngine::VisitCast to handle casts from
   ///  a MemRegion* to a specific location type.  'R' is the region being
   ///  casted and 'CastToTy' the result type of the cast.
   Optional<const MemRegion *> castRegion(const MemRegion *region,
                                          QualType CastToTy);

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

if (!ThisVal.isUnknown()) {
     // If we devirtualized to a different member function, we need to make sure
     // we have the proper layering of CXXBaseObjectRegions.
     if (MD->getCanonicalDecl() != getDecl()->getCanonicalDecl()) {
       ASTContext &Ctx = SVB.getContext();
       const CXXRecordDecl *Class = MD->getParent();
       QualType Ty = Ctx.getPointerType(Ctx.getRecordType(Class));
 
       // FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
-      bool Failed;
-      ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);
-      if (Failed) {
+      Optional<SVal> V =
+          StateMgr.getStoreManager().evalBaseToDerived(ThisVal, Ty);
+      if (!V.hasValue()) {
         // We might have suffered some sort of placement new earlier, so
         // we're constructing in a completely unexpected storage.
         // Fall back to a generic pointer cast for this-value.
         const CXXMethodDecl *StaticMD = cast<CXXMethodDecl>(getDecl());
         const CXXRecordDecl *StaticClass = StaticMD->getParent();
         QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
         ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
-      }
+      } else
+        ThisVal = *V;
     }
 
     if (!ThisVal.isUnknown())
       Bindings.push_back(std::make_pair(ThisLoc, ThisVal));
   }
 }
 
 const Expr *CXXMemberCall::getCXXThisExpr() const {

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

switch (CastE->getCastKind()) {
       case CK_Dynamic: {
         SVal val = state->getSVal(Ex, LCtx);
 
         // Compute the type of the result.
         QualType resultType = CastE->getType();
         if (CastE->isGLValue())
           resultType = getContext().getPointerType(resultType);
 
-        bool Failed = false;
+        bool Failed = true;
 
-        // Check if the value being cast evaluates to 0.
-        if (val.isZeroConstant())
-          Failed = true;
-        // Else, evaluate the cast.
-        else
-          val = getStoreManager().attemptDownCast(val, T, Failed);
+        // Check if the value being cast does not evaluates to 0.
+        if (!val.isZeroConstant())
+          if (Optional<SVal> V =
+                  StateMgr.getStoreManager().evalBaseToDerived(val, T)) {
+            val = *V;
+            Failed = false;
+          }
 
         if (Failed) {
           if (T->isReferenceType()) {
             // A bad_cast exception is thrown if input value is a reference.
             // Currently, we model this, by generating a sink.
             Bldr.generateSink(CastE, Pred, state);
             continue;
           } else {
switch (CastE->getCastKind()) {
         continue;
       }
       case CK_BaseToDerived: {
         SVal val = state->getSVal(Ex, LCtx);
         QualType resultType = CastE->getType();
         if (CastE->isGLValue())
           resultType = getContext().getPointerType(resultType);
 
-        bool Failed = false;
-
         if (!val.isConstant()) {
-          val = getStoreManager().attemptDownCast(val, T, Failed);
+          Optional<SVal> V = getStoreManager().evalBaseToDerived(val, T);
+          val = V ? *V : UnknownVal();
         }
 
         // Failed to cast or the result is unknown, fall back to conservative.
-        if (Failed || val.isUnknown()) {
+        if (val.isUnknown()) {
           val =
             svalBuilder.conjureSymbolVal(nullptr, CastE, LCtx, resultType,
                                          currBldrCtx->blockCount());
         }
         state = state->BindExpr(CastE, LCtx, val);
         Bldr.generateNode(CastE, Pred, state);
         continue;
       }

clang/lib/StaticAnalyzer/Core/Store.cpp

 static const CXXRecordDecl *getCXXRecordType(const MemRegion *MR) {
   if (const auto *TVR = dyn_cast<TypedValueRegion>(MR))
     return TVR->getValueType()->getAsCXXRecordDecl();
   if (const auto *SR = dyn_cast<SymbolicRegion>(MR))
     return SR->getSymbol()->getType()->getPointeeCXXRecordDecl();
   return nullptr;
 }
 
-SVal StoreManager::attemptDownCast(SVal Base, QualType TargetType,
-                                   bool &Failed) {
-  Failed = false;
-
+Optional<SVal> StoreManager::evalBaseToDerived(SVal Base, QualType TargetType) {
   const MemRegion *MR = Base.getAsRegion();
   if (!MR)
     return UnknownVal();
 
   // Assume the derived class is a pointer or a reference to a CXX record.
   TargetType = TargetType->getPointeeType();
   assert(!TargetType.isNull());
   const CXXRecordDecl *TargetClass = TargetType->getAsCXXRecordDecl();
if (const auto *SR = dyn_cast<SymbolicRegion>(MR)) {
     const CXXRecordDecl *SourceClass = T->getPointeeCXXRecordDecl();
     if (TargetClass && SourceClass && TargetClass->isDerivedFrom(SourceClass))
       return loc::MemRegionVal(
           MRMgr.getCXXDerivedObjectRegion(TargetClass, SR));
     return loc::MemRegionVal(GetElementZeroRegion(SR, TargetType));
   }
 
   // We failed if the region we ended up with has perfect type info.
-  Failed = isa<TypedValueRegion>(MR);
+  if (isa<TypedValueRegion>(MR))
+    return None;
+
   return UnknownVal();
 }
 
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
     return Base;
 
   Loc BaseL = Base.castAs<Loc>();