[clang][dataflow] Add Environment::get<>(). (llvm#76027)

This template function casts the result of `getValue()` or
`getStorageLocation()` to a given subclass of `Value` or
`StorageLocation` (using `cast_or_null`).

It's a common pattern to do something like this:

```cxx
auto *Val = cast_or_null<PointerValue>(Env.getValue(E));
```

This can now be expressed more concisely like this:

```cxx
auto *Val = Env.get<PointerValue>(E);
```

Instead of adding a new method `get()`, I had originally considered
simply adding a template parameter to `getValue()` and
`getStorageLocation()` (with a default argument of `Value` or
`StorageLocation`), but this results in an undesirable repetition at the
callsite, e.g. `getStorageLocation<RecordStorageLocation>(...)`. The
`Value` and `StorageLocation` in the method name adds nothing of value
when the template argument already contains this information, so it
seemed best to shorten the method name to simply `get()`.

Author: martinboehme

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

@@ -289,6 +289,22 @@ class Environment {
   ///  `E` must be a glvalue or a `BuiltinType::BuiltinFn`
   StorageLocation *getStorageLocation(const Expr &E) const;
 
+  /// Returns the result of casting `getStorageLocation(...)` to a subclass of
+  /// `StorageLocation` (using `cast_or_null<T>`).
+  /// This assert-fails if the result of `getStorageLocation(...)` is not of
+  /// type `T *`; if the storage location is not guaranteed to have type `T *`,
+  /// consider using `dyn_cast_or_null<T>(getStorageLocation(...))` instead.
+  template <typename T>
+  std::enable_if_t<std::is_base_of_v<StorageLocation, T>, T *>
+  get(const ValueDecl &D) const {
+    return cast_or_null<T>(getStorageLocation(D));
+  }
+  template <typename T>
+  std::enable_if_t<std::is_base_of_v<StorageLocation, T>, T *>
+  get(const Expr &E) const {
+    return cast_or_null<T>(getStorageLocation(E));
+  }
+
   /// Returns the storage location assigned to the `this` pointee in the
   /// environment or null if the `this` pointee has no assigned storage location
   /// in the environment.
@@ -457,6 +473,26 @@ class Environment {
   /// storage location in the environment, otherwise returns null.
   Value *getValue(const Expr &E) const;
 
+  /// Returns the result of casting `getValue(...)` to a subclass of `Value`
+  /// (using `cast_or_null<T>`).
+  /// This assert-fails if the result of `getValue(...)` is not of type `T *`;
+  /// if the value is not guaranteed to have type `T *`, consider using
+  /// `dyn_cast_or_null<T>(getValue(...))` instead.
+  template <typename T>
+  std::enable_if_t<std::is_base_of_v<Value, T>, T *>
+  get(const StorageLocation &Loc) const {
+    return cast_or_null<T>(getValue(Loc));
+  }
+  template <typename T>
+  std::enable_if_t<std::is_base_of_v<Value, T>, T *>
+  get(const ValueDecl &D) const {
+    return cast_or_null<T>(getValue(D));
+  }
+  template <typename T>
+  std::enable_if_t<std::is_base_of_v<Value, T>, T *> get(const Expr &E) const {
+    return cast_or_null<T>(getValue(E));
+  }
+
   // FIXME: should we deprecate the following & call arena().create() directly?
 
   /// Creates a `T` (some subclass of `Value`), forwarding `args` to the

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

@@ -1034,7 +1034,7 @@ RecordStorageLocation *getImplicitObjectLocation(const CXXMemberCallExpr &MCE,
   if (ImplicitObject == nullptr)
     return nullptr;
   if (ImplicitObject->getType()->isPointerType()) {
-    if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*ImplicitObject)))
+    if (auto *Val = Env.get<PointerValue>(*ImplicitObject))
       return &cast<RecordStorageLocation>(Val->getPointeeLoc());
     return nullptr;
   }
@@ -1048,11 +1048,11 @@ RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
   if (Base == nullptr)
     return nullptr;
   if (ME.isArrow()) {
-    if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*Base)))
+    if (auto *Val = Env.get<PointerValue>(*Base))
       return &cast<RecordStorageLocation>(Val->getPointeeLoc());
     return nullptr;
   }
-  return cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Base));
+  return Env.get<RecordStorageLocation>(*Base);
 }
 
 std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD) {
@@ -1077,7 +1077,7 @@ RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) {
   assert(Expr.getType()->isRecordType());
 
   if (Expr.isPRValue()) {
-    if (auto *ExistingVal = cast_or_null<RecordValue>(Env.getValue(Expr))) {
+    if (auto *ExistingVal = Env.get<RecordValue>(Expr)) {
       auto &NewVal = Env.create<RecordValue>(ExistingVal->getLoc());
       Env.setValue(Expr, NewVal);
       Env.setValue(NewVal.getLoc(), NewVal);
@@ -1089,8 +1089,7 @@ RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) {
     return NewVal;
   }
 
-  if (auto *Loc =
-          cast_or_null<RecordStorageLocation>(Env.getStorageLocation(Expr))) {
+  if (auto *Loc = Env.get<RecordStorageLocation>(Expr)) {
     auto &NewVal = Env.create<RecordValue>(*Loc);
     Env.setValue(*Loc, NewVal);
     return NewVal;

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

@@ -226,7 +226,7 @@ auto isComparisonOperatorCall(L lhs_arg_matcher, R rhs_arg_matcher) {
 
 /// Ensures that `Expr` is mapped to a `BoolValue` and returns its formula.
 const Formula &forceBoolValue(Environment &Env, const Expr &Expr) {
-  auto *Value = cast_or_null<BoolValue>(Env.getValue(Expr));
+  auto *Value = Env.get<BoolValue>(Expr);
   if (Value != nullptr)
     return Value->formula();
 
@@ -267,7 +267,7 @@ BoolValue *getHasValue(Environment &Env, RecordStorageLocation *OptionalLoc) {
   if (OptionalLoc == nullptr)
     return nullptr;
   StorageLocation &HasValueLoc = locForHasValue(*OptionalLoc);
-  auto *HasValueVal = cast_or_null<BoolValue>(Env.getValue(HasValueLoc));
+  auto *HasValueVal = Env.get<BoolValue>(HasValueLoc);
   if (HasValueVal == nullptr) {
     HasValueVal = &Env.makeAtomicBoolValue();
     Env.setValue(HasValueLoc, *HasValueVal);
@@ -406,7 +406,7 @@ void transferCallReturningOptional(const CallExpr *E,
   if (E->isPRValue()) {
     Loc = &State.Env.getResultObjectLocation(*E);
   } else {
-    Loc = cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(*E));
+    Loc = State.Env.get<RecordStorageLocation>(*E);
     if (Loc == nullptr) {
       Loc = &cast<RecordStorageLocation>(State.Env.createStorageLocation(*E));
       State.Env.setStorageLocation(*E, *Loc);
@@ -449,8 +449,7 @@ BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
 
   // This is a constructor/assignment call for `optional<T>` with argument of
   // type `optional<U>` such that `T` is constructible from `U`.
-  auto *Loc =
-      cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(E));
+  auto *Loc = State.Env.get<RecordStorageLocation>(E);
   if (auto *HasValueVal = getHasValue(State.Env, Loc))
     return *HasValueVal;
   return State.Env.makeAtomicBoolValue();
@@ -471,8 +470,7 @@ void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal,
                         LatticeTransferState &State) {
   assert(E->getNumArgs() > 0);
 
-  if (auto *Loc = cast_or_null<RecordStorageLocation>(
-          State.Env.getStorageLocation(*E->getArg(0)))) {
+  if (auto *Loc = State.Env.get<RecordStorageLocation>(*E->getArg(0))) {
     createOptionalValue(*Loc, HasValueVal, State.Env);
 
     // Assign a storage location for the whole expression.
@@ -534,18 +532,15 @@ void transferSwapCall(const CXXMemberCallExpr *E,
                       const MatchFinder::MatchResult &,
                       LatticeTransferState &State) {
   assert(E->getNumArgs() == 1);
-  auto *OtherLoc = cast_or_null<RecordStorageLocation>(
-      State.Env.getStorageLocation(*E->getArg(0)));
+  auto *OtherLoc = State.Env.get<RecordStorageLocation>(*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);
-  auto *Arg0Loc = cast_or_null<RecordStorageLocation>(
-      State.Env.getStorageLocation(*E->getArg(0)));
-  auto *Arg1Loc = cast_or_null<RecordStorageLocation>(
-      State.Env.getStorageLocation(*E->getArg(1)));
+  auto *Arg0Loc = State.Env.get<RecordStorageLocation>(*E->getArg(0));
+  auto *Arg1Loc = State.Env.get<RecordStorageLocation>(*E->getArg(1));
   transferSwap(Arg0Loc, Arg1Loc, State.Env);
 }
 
@@ -585,11 +580,9 @@ void transferOptionalAndOptionalCmp(const clang::CXXOperatorCallExpr *CmpExpr,
   Environment &Env = State.Env;
   auto &A = Env.arena();
   auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
-  auto *Arg0Loc = cast_or_null<RecordStorageLocation>(
-      Env.getStorageLocation(*CmpExpr->getArg(0)));
+  auto *Arg0Loc = Env.get<RecordStorageLocation>(*CmpExpr->getArg(0));
   if (auto *LHasVal = getHasValue(Env, Arg0Loc)) {
-    auto *Arg1Loc = cast_or_null<RecordStorageLocation>(
-        Env.getStorageLocation(*CmpExpr->getArg(1)));
+    auto *Arg1Loc = Env.get<RecordStorageLocation>(*CmpExpr->getArg(1));
     if (auto *RHasVal = getHasValue(Env, Arg1Loc)) {
       if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
         CmpValue = &A.makeNot(*CmpValue);
@@ -603,7 +596,7 @@ void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr,
                                  const clang::Expr *E, Environment &Env) {
   auto &A = Env.arena();
   auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
-  auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E));
+  auto *Loc = Env.get<RecordStorageLocation>(*E);
   if (auto *HasVal = getHasValue(Env, Loc)) {
     if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
       CmpValue = &A.makeNot(*CmpValue);
@@ -616,7 +609,7 @@ void transferOptionalAndNulloptCmp(const clang::CXXOperatorCallExpr *CmpExpr,
                                    const clang::Expr *E, Environment &Env) {
   auto &A = Env.arena();
   auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
-  auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E));
+  auto *Loc = Env.get<RecordStorageLocation>(*E);
   if (auto *HasVal = getHasValue(Env, Loc)) {
     if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
       CmpValue = &A.makeNot(*CmpValue);

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

@@ -66,8 +66,8 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src,
     }
   }
 
-  RecordValue *SrcVal = cast_or_null<RecordValue>(Env.getValue(Src));
-  RecordValue *DstVal = cast_or_null<RecordValue>(Env.getValue(Dst));
+  RecordValue *SrcVal = Env.get<RecordValue>(Src);
+  RecordValue *DstVal = Env.get<RecordValue>(Dst);
 
   DstVal = &Env.create<RecordValue>(Dst);
   Env.setValue(Dst, *DstVal);
@@ -127,10 +127,10 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
 
   llvm::StringMap<Value *> Props1, Props2;
 
-  if (RecordValue *Val1 = cast_or_null<RecordValue>(Env1.getValue(Loc1)))
+  if (RecordValue *Val1 = Env1.get<RecordValue>(Loc1))
     for (const auto &[Name, Value] : Val1->properties())
       Props1[Name] = Value;
-  if (RecordValue *Val2 = cast_or_null<RecordValue>(Env2.getValue(Loc2)))
+  if (RecordValue *Val2 = Env2.get<RecordValue>(Loc2))
     for (const auto &[Name, Value] : Val2->properties())
       Props2[Name] = Value;
 

clang/lib/Analysis/FlowSensitive/Transfer.cpp

@@ -339,8 +339,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 
     switch (S->getOpcode()) {
     case UO_Deref: {
-      const auto *SubExprVal =
-          cast_or_null<PointerValue>(Env.getValue(*SubExpr));
+      const auto *SubExprVal = Env.get<PointerValue>(*SubExpr);
       if (SubExprVal == nullptr)
         break;
 
@@ -467,8 +466,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       const Expr *Arg = S->getArg(0);
       assert(Arg != nullptr);
 
-      auto *ArgLoc =
-          cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Arg));
+      auto *ArgLoc = Env.get<RecordStorageLocation>(*Arg);
       if (ArgLoc == nullptr)
         return;
 
@@ -515,14 +513,12 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 
       RecordStorageLocation *LocSrc = nullptr;
       if (Arg1->isPRValue()) {
-        if (auto *Val = cast_or_null<RecordValue>(Env.getValue(*Arg1)))
+        if (auto *Val = Env.get<RecordValue>(*Arg1))
           LocSrc = &Val->getLoc();
       } else {
-        LocSrc =
-            cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Arg1));
+        LocSrc = Env.get<RecordStorageLocation>(*Arg1);
       }
-      auto *LocDst =
-          cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Arg0));
+      auto *LocDst = Env.get<RecordStorageLocation>(*Arg0);
 
       if (LocSrc == nullptr || LocDst == nullptr)
         return;
@@ -676,7 +672,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
         auto Init = Inits[InitIdx++];
         assert(Base.getType().getCanonicalType() ==
                Init->getType().getCanonicalType());
-        auto* BaseVal = cast_or_null<RecordValue>(Env.getValue(*Init));
+        auto *BaseVal = Env.get<RecordValue>(*Init);
         if (!BaseVal)
           BaseVal = cast<RecordValue>(Env.createValue(Init->getType()));
         // Take ownership of the fields of the `RecordValue` for the base class

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

@@ -130,7 +130,7 @@ class TerminatorVisitor
     if (Env.getValue(Cond) == nullptr)
       transfer(StmtToEnv, Cond, Env);
 
-    auto *Val = cast_or_null<BoolValue>(Env.getValue(Cond));
+    auto *Val = Env.get<BoolValue>(Cond);
     // Value merging depends on flow conditions from different environments
     // being mutually exclusive -- that is, they cannot both be true in their
     // entirety (even if they may share some clauses). So, we need *some* value

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

@@ -133,7 +133,7 @@ void transferBinary(const BinaryOperator *BO, const MatchFinder::MatchResult &M,
                     LatticeTransferState &State) {
   auto &A = State.Env.arena();
   const Formula *Comp;
-  if (BoolValue *V = cast_or_null<BoolValue>(State.Env.getValue(*BO))) {
+  if (BoolValue *V = State.Env.get<BoolValue>(*BO)) {
     Comp = &V->formula();
   } else {
     Comp = &A.makeAtomRef(A.makeAtom());