[clang][dataflow] Various refactorings to UncheckedOptionalAccessModel.

[mboehme]

Summary

These are intended to ease an upcoming change that will eliminate the duplication between AggregateStorageLocation and StructValue (see https://discourse.llvm.org/t/70086 for details), but many of the changes also have value in their own right.

Depends On D154586

[mboehme]

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

258

Moved Env parameter to the end for consistency with the newly added overload (below); Env is just a "context" parameter, HasValueVal is the more important parameter and should therefore come first.

[ymandel]

Nice! I don't think you need a second review for this one, since it's just superficial (if important!) refactoring.

[mboehme]

libcxx and clangd failures look unrelated

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

 /// Sets `HasValueVal` as the symbolic value that represents the "has_value"
 /// property of the optional value `OptionalVal`.
 void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) {
   OptionalVal.setProperty("has_value", HasValueVal);
 }
 
 /// Creates a symbolic value for an `optional` value using `HasValueVal` as the
 /// symbolic value of its "has_value" property.
-StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) {
+StructValue &createOptionalValue(BoolValue &HasValueVal, Environment &Env) {
   auto &OptionalVal = Env.create<StructValue>();
   setHasValue(OptionalVal, HasValueVal);
   return OptionalVal;
 }
 
+/// Creates a symbolic value for an `optional` value at an existing storage
+/// location. Uses `HasValueVal` as the symbolic value of the "has_value"
+/// property.
+StructValue &createOptionalValue(AggregateStorageLocation &Loc,
+                                 BoolValue &HasValueVal, Environment &Env) {
+  StructValue &OptionalVal = createOptionalValue(HasValueVal, Env);
+  Env.setValue(Loc, OptionalVal);
+  return OptionalVal;
+}
+
 /// Returns the symbolic value that represents the "has_value" property of the
 /// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
 BoolValue *getHasValue(Environment &Env, Value *OptionalVal) {
   if (OptionalVal != nullptr) {
     auto *HasValueVal =
         cast_or_null<BoolValue>(OptionalVal->getProperty("has_value"));
     if (HasValueVal == nullptr) {
       HasValueVal = &Env.makeAtomicBoolValue();

 /// Returns true if and only if `OptionalVal` is initialized and known to be
 /// non-empty in `Env`.
 bool isNonEmptyOptional(const Value &OptionalVal, const Environment &Env) {
   auto *HasValueVal =
       cast_or_null<BoolValue>(OptionalVal.getProperty("has_value"));
   return HasValueVal != nullptr && Env.flowConditionImplies(*HasValueVal);
 }
 
-StorageLocation *maybeSkipPointer(StorageLocation *Loc,
-                                  const Environment &Env) {
-  if (Loc == nullptr)
-    return nullptr;
-  if (auto *Val = dyn_cast_or_null<PointerValue>(Env.getValue(*Loc)))
-    return &Val->getPointeeLoc();
-  return Loc;
-}
-
 Value *getValueBehindPossiblePointer(const Expr &E, const Environment &Env) {
   Value *Val = Env.getValue(E, SkipPast::Reference);
   if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Val))
     return Env.getValue(PointerVal->getPointeeLoc());
   return Val;
 }
 
 void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,

 }
 
 void transferMakeOptionalCall(const CallExpr *E,
                               const MatchFinder::MatchResult &,
                               LatticeTransferState &State) {
   auto &Loc = State.Env.createStorageLocation(*E);
   State.Env.setStorageLocation(*E, Loc);
   State.Env.setValue(
-      Loc, createOptionalValue(State.Env, State.Env.getBoolLiteralValue(true)));
+      Loc, createOptionalValue(State.Env.getBoolLiteralValue(true), State.Env));
 }
 
 void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
                                   const MatchFinder::MatchResult &,
                                   LatticeTransferState &State) {
   if (auto *HasValueVal = getHasValue(
           State.Env, getValueBehindPossiblePointer(
                          *CallExpr->getImplicitObjectArgument(), State.Env))) {
void transferCallReturningOptional(const CallExpr *E,
                                    const MatchFinder::MatchResult &Result,
                                    LatticeTransferState &State) {
   if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
     return;
 
   auto &Loc = State.Env.createStorageLocation(*E);
   State.Env.setStorageLocation(*E, Loc);
   State.Env.setValue(
-      Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()));
+      Loc, createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env));
 }
 
-void assignOptionalValue(const Expr &E, Environment &Env,
+void constructOptionalValue(const Expr &E, Environment &Env,
                             BoolValue &HasValueVal) {
-  if (auto *OptionalLoc = maybeSkipPointer(
-          Env.getStorageLocation(E, SkipPast::Reference), Env)) {
-    Env.setValue(*OptionalLoc, createOptionalValue(Env, HasValueVal));
-  }
+  Env.setValueStrict(E, createOptionalValue(HasValueVal, Env));
 }
 
 /// Returns a symbolic value for the "has_value" property of an `optional<T>`
 /// value that is constructed/assigned from a value of type `U` or `optional<U>`
 /// where `T` is constructible from `U`.
 BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
                                      const MatchFinder::MatchResult &MatchRes,
                                      LatticeTransferState &State) {
BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
   return State.Env.makeAtomicBoolValue();
 }
 
 void transferValueOrConversionConstructor(
     const CXXConstructExpr *E, const MatchFinder::MatchResult &MatchRes,
     LatticeTransferState &State) {
   assert(E->getNumArgs() > 0);
 
-  assignOptionalValue(*E, State.Env,
+  constructOptionalValue(*E, State.Env,
                          valueOrConversionHasValue(*E->getConstructor(),
                                                    *E->getArg(0), MatchRes,
                                                    State));
 }
 
 void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal,
                         LatticeTransferState &State) {
   assert(E->getNumArgs() > 0);
 
-  auto *OptionalLoc =
-      State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
-  if (OptionalLoc == nullptr)
-    return;
-
-  State.Env.setValue(*OptionalLoc, createOptionalValue(State.Env, HasValueVal));
+  if (auto *Loc = cast<AggregateStorageLocation>(
+          State.Env.getStorageLocationStrict(*E->getArg(0)))) {
+    createOptionalValue(*Loc, HasValueVal, State.Env);
 
     // Assign a storage location for the whole expression.
-  State.Env.setStorageLocation(*E, *OptionalLoc);
+    State.Env.setStorageLocationStrict(*E, *Loc);
+  }
 }
 
 void transferValueOrConversionAssignment(
     const CXXOperatorCallExpr *E, const MatchFinder::MatchResult &MatchRes,
     LatticeTransferState &State) {
   assert(E->getNumArgs() > 1);
   transferAssignment(E,
                      valueOrConversionHasValue(*E->getDirectCallee(),
                                                *E->getArg(1), MatchRes, State),
                      State);
 }
 
 void transferNulloptAssignment(const CXXOperatorCallExpr *E,
                                const MatchFinder::MatchResult &,
                                LatticeTransferState &State) {
   transferAssignment(E, State.Env.getBoolLiteralValue(false), State);
 }
 
-void transferSwap(StorageLocation *Loc1, StorageLocation *Loc2,
-                  Environment &Env) {
+void transferSwap(AggregateStorageLocation *Loc1,
+                  AggregateStorageLocation *Loc2, Environment &Env) {
   // We account for cases where one or both of the optionals are not modeled,
   // either lacking associated storage locations, or lacking values associated
   // to such storage locations.
 
   if (Loc1 == nullptr) {
     if (Loc2 != nullptr)
-      Env.setValue(*Loc2, createOptionalValue(Env, Env.makeAtomicBoolValue()));
+      createOptionalValue(*Loc2, Env.makeAtomicBoolValue(), Env);
     return;
   }
   if (Loc2 == nullptr) {
-    Env.setValue(*Loc1, createOptionalValue(Env, Env.makeAtomicBoolValue()));
+    createOptionalValue(*Loc1, Env.makeAtomicBoolValue(), Env);
     return;
   }
 
   // Both expressions have locations, though they may not have corresponding
   // values. In that case, we create a fresh value at this point. Note that if
   // two branches both do this, they will not share the value, but it at least
   // allows for local reasoning about the value. To avoid the above, we would
   // need *lazy* value allocation.
   // FIXME: allocate values lazily, instead of just creating a fresh value.
-  auto *Val1 = Env.getValue(*Loc1);
-  if (Val1 == nullptr)
-    Val1 = &createOptionalValue(Env, Env.makeAtomicBoolValue());
-
-  auto *Val2 = Env.getValue(*Loc2);
-  if (Val2 == nullptr)
-    Val2 = &createOptionalValue(Env, Env.makeAtomicBoolValue());
+  BoolValue *BoolVal1 = getHasValue(Env, Env.getValue(*Loc1));
+  if (BoolVal1 == nullptr)
+    BoolVal1 = &Env.makeAtomicBoolValue();
+
+  BoolValue *BoolVal2 = getHasValue(Env, Env.getValue(*Loc2));
+  if (BoolVal2 == nullptr)
+    BoolVal2 = &Env.makeAtomicBoolValue();
 
-  Env.setValue(*Loc1, *Val2);
-  Env.setValue(*Loc2, *Val1);
+  createOptionalValue(*Loc1, *BoolVal2, Env);
+  createOptionalValue(*Loc2, *BoolVal1, Env);
 }
 
 void transferSwapCall(const CXXMemberCallExpr *E,
                       const MatchFinder::MatchResult &,
                       LatticeTransferState &State) {
   assert(E->getNumArgs() == 1);
-  transferSwap(maybeSkipPointer(
-                   State.Env.getStorageLocation(*E->getImplicitObjectArgument(),
-                                                SkipPast::Reference),
-                   State.Env),
-               State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference),
-               State.Env);
+  auto *OtherLoc = cast_or_null<AggregateStorageLocation>(
+      State.Env.getStorageLocationStrict(*E->getArg(0)));
+  transferSwap(getImplicitObjectLocation(*E, State.Env), OtherLoc, State.Env);
 }
 
 void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
                          LatticeTransferState &State) {
   assert(E->getNumArgs() == 2);
-  transferSwap(State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference),
-               State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference),
-               State.Env);
+  auto *Arg0Loc = cast_or_null<AggregateStorageLocation>(
+      State.Env.getStorageLocationStrict(*E->getArg(0)));
+  auto *Arg1Loc = cast_or_null<AggregateStorageLocation>(
+      State.Env.getStorageLocationStrict(*E->getArg(1)));
+  transferSwap(Arg0Loc, Arg1Loc, State.Env);
 }
 
 void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &,
                             LatticeTransferState &State) {
   assert(E->getNumArgs() == 1);
 
   StorageLocation *LocRet = State.Env.getStorageLocation(*E, SkipPast::None);
   if (LocRet != nullptr)
     return;
 
   StorageLocation *LocArg =
       State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
 
   if (LocArg == nullptr)
     return;
 
   Value *ValArg = State.Env.getValue(*LocArg);
   if (ValArg == nullptr)
-    ValArg = &createOptionalValue(State.Env, State.Env.makeAtomicBoolValue());
+    ValArg = &createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env);
 
   // Create a new storage location
   LocRet = &State.Env.createStorageLocation(*E);
   State.Env.setStorageLocation(*E, *LocRet);
 
   State.Env.setValue(*LocRet, *ValArg);
 }
 
return CFGMatchSwitchBuilder<LatticeTransferState>()
       // make_optional
       .CaseOfCFGStmt<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall)
 
       // optional::optional (in place)
       .CaseOfCFGStmt<CXXConstructExpr>(
           isOptionalInPlaceConstructor(),
           [](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
-            assignOptionalValue(*E, State.Env,
+            constructOptionalValue(*E, State.Env,
                                    State.Env.getBoolLiteralValue(true));
           })
       // nullopt_t::nullopt_t
       .CaseOfCFGStmt<CXXConstructExpr>(
           isNulloptConstructor(),
           [](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
-            assignOptionalValue(*E, State.Env,
+            constructOptionalValue(*E, State.Env,
                                    State.Env.getBoolLiteralValue(false));
           })
       // optional::optional(nullopt_t)
       .CaseOfCFGStmt<CXXConstructExpr>(
           isOptionalNulloptConstructor(),
           [](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
-            assignOptionalValue(*E, State.Env,
+            constructOptionalValue(*E, State.Env,
                                    State.Env.getBoolLiteralValue(false));
           })
       // optional::optional (value/conversion)
       .CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(),
                                        transferValueOrConversionConstructor)
 
       // optional::operator=
       .CaseOfCFGStmt<CXXOperatorCallExpr>(
           isOptionalValueOrConversionAssignment(),
return CFGMatchSwitchBuilder<LatticeTransferState>()
           isOptionalMemberCallWithName("operator bool"),
           transferOptionalHasValueCall)
 
       // optional::emplace
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("emplace"),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
-            assignOptionalValue(*E->getImplicitObjectArgument(), State.Env,
-                                State.Env.getBoolLiteralValue(true));
+            if (AggregateStorageLocation *Loc =
+                    getImplicitObjectLocation(*E, State.Env)) {
+              createOptionalValue(*Loc, State.Env.getBoolLiteralValue(true),
+                                  State.Env);
+            }
           })
 
       // optional::reset
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("reset"),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
-            assignOptionalValue(*E->getImplicitObjectArgument(), State.Env,
-                                State.Env.getBoolLiteralValue(false));
+            if (AggregateStorageLocation *Loc =
+                    getImplicitObjectLocation(*E, State.Env)) {
+              createOptionalValue(*Loc, State.Env.getBoolLiteralValue(false),
+                                  State.Env);
+            }
           })
 
       // optional::swap
       .CaseOfCFGStmt<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
                                         transferSwapCall)
 
       // std::swap
       .CaseOfCFGStmt<CallExpr>(isStdSwapCall(), transferStdSwapCall)
if (auto *PrevHasVal =
       if (isa<TopBoolValue>(PrevHasVal))
         return &Prev;
     }
     if (auto *CurrentHasVal =
             cast_or_null<BoolValue>(Current.getProperty("has_value"))) {
       if (isa<TopBoolValue>(CurrentHasVal))
         return &Current;
     }
-    return &createOptionalValue(CurrentEnv, CurrentEnv.makeTopBoolValue());
+    return &createOptionalValue(CurrentEnv.makeTopBoolValue(), CurrentEnv);
   case ComparisonResult::Unknown:
     return nullptr;
   }
   llvm_unreachable("all cases covered in switch");
 }
 
 UncheckedOptionalAccessDiagnoser::UncheckedOptionalAccessDiagnoser(
     UncheckedOptionalAccessModelOptions Options)