[clang][dataflow] Rename `getPointeeLoc` to `getReferentLoc` for ReferenceValue.

[wyt]

Summary

We distinguish between the referent location for ReferenceValue and pointee location for PointerValue. The former must be non-empty but the latter may be empty in the case of a nullptr

[sgatev]

On a high level, what's wrong with having getPointeeLoc members with different return types in ReferenceValue and PointerValue?

[wyt]

Modified renaming: getPointeeLoc of ReferenceValue is modified to getReferentLoc, getPointeeLoc remains unchanged for PointerValue

[wyt]

In D127745#3582085, @sgatev wrote:

On a high level, what's wrong with having getPointeeLoc members with different return types in ReferenceValue and PointerValue?

IMO, it would be nice to distinguish the type of Value we are working on.
As per Dmitri's suggestion, I updated the patch so that getPointeeLoc of ReferenceValue is now renamed to getReferentLoc, getPointeeLoc is remains the same for PointerValue.

clang/include/clang/Analysis/FlowSensitive/Value.h

static bool classof(const Value *Val) {
     return Val->getKind() == Kind::Integer;
   }
 };
 
 /// Models a dereferenced pointer. For example, a reference in C++ or an lvalue
 /// in C.
 class ReferenceValue final : public Value {
 public:
-  explicit ReferenceValue(StorageLocation &PointeeLoc)
-      : Value(Kind::Reference), PointeeLoc(PointeeLoc) {}
+  explicit ReferenceValue(StorageLocation &ReferentLoc)
+      : Value(Kind::Reference), ReferentLoc(ReferentLoc) {}
 
   static bool classof(const Value *Val) {
     return Val->getKind() == Kind::Reference;
   }
 
-  StorageLocation &getPointeeLoc() const { return PointeeLoc; }
+  StorageLocation &getReferentLoc() const { return ReferentLoc; }
 
 private:
-  StorageLocation &PointeeLoc;
+  StorageLocation &ReferentLoc;
 };
 
 /// Models a symbolic pointer. Specifically, any value of type `T*`.
 class PointerValue final : public Value {
 public:
   explicit PointerValue(StorageLocation &PointeeLoc)
       : Value(Kind::Pointer), PointeeLoc(PointeeLoc) {}
 

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

if (It != Map2.end() && Entry.second == It->second)
       Result.insert({Entry.first, Entry.second});
   }
   return Result;
 }
 
 static bool areEquivalentIndirectionValues(Value *Val1, Value *Val2) {
   if (auto *IndVal1 = dyn_cast<ReferenceValue>(Val1)) {
     auto *IndVal2 = cast<ReferenceValue>(Val2);
-    return &IndVal1->getPointeeLoc() == &IndVal2->getPointeeLoc();
+    return &IndVal1->getReferentLoc() == &IndVal2->getReferentLoc();
   }
   if (auto *IndVal1 = dyn_cast<PointerValue>(Val1)) {
     auto *IndVal2 = cast<PointerValue>(Val2);
     return &IndVal1->getPointeeLoc() == &IndVal2->getPointeeLoc();
   }
   return false;
 }
 

 StorageLocation &Environment::skip(StorageLocation &Loc, SkipPast SP) const {
   switch (SP) {
   case SkipPast::None:
     return Loc;
   case SkipPast::Reference:
     // References cannot be chained so we only need to skip past one level of
     // indirection.
     if (auto *Val = dyn_cast_or_null<ReferenceValue>(getValue(Loc)))
-      return Val->getPointeeLoc();
+      return Val->getReferentLoc();
     return Loc;
   case SkipPast::ReferenceThenPointer:
     StorageLocation &LocPastRef = skip(Loc, SkipPast::Reference);
     if (auto *Val = dyn_cast_or_null<PointerValue>(getValue(LocPastRef)))
       return Val->getPointeeLoc();
     return LocPastRef;
   }
   llvm_unreachable("bad SkipPast kind");

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

StorageLocation *maybeInitializeOptionalValueMember(QualType Q,
                                                     Environment &Env) {
   // The "value" property represents a synthetic field. As such, it needs
   // `StorageLocation`, like normal fields (and other variables). So, we model
   // it with a `ReferenceValue`, since that includes a storage location.  Once
   // the property is set, it will be shared by all environments that access the
   // `Value` representing the optional (here, `OptionalVal`).
   if (auto *ValueProp = OptionalVal.getProperty("value")) {
     auto *ValueRef = clang::cast<ReferenceValue>(ValueProp);
-    auto &ValueLoc = ValueRef->getPointeeLoc();
+    auto &ValueLoc = ValueRef->getReferentLoc();
     if (Env.getValue(ValueLoc) == nullptr) {
       // The property was previously set, but the value has been lost. This can
       // happen, for example, because of an environment merge (where the two
       // environments mapped the property to different values, which resulted in
       // them both being discarded), or when two blocks in the CFG, with neither
       // a dominator of the other, visit the same optional value, or even when a
       // block is revisited during testing to collect per-statement state.
       // FIXME: This situation means that the optional contents are not shared

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

std::string Code = R"(
 
     A &getA();
 
     void target() {
       A &Foo = getA();
       // [[p]]
     }
   )";
-  runDataflow(Code,
-              [](llvm::ArrayRef<
-                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
-                     Results,
-                 ASTContext &ASTCtx) {
-                ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
-                const Environment &Env = Results[0].second.Env;
+  runDataflow(
+      Code, [](llvm::ArrayRef<
 
-                const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-                ASSERT_THAT(FooDecl, NotNull());
 
-                const StorageLocation *FooLoc =
-                    Env.getStorageLocation(*FooDecl, SkipPast::None);
-                ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc));
 
-                const ReferenceValue *FooVal =
-                    cast<ReferenceValue>(Env.getValue(*FooLoc));
-                const StorageLocation &FooPointeeLoc = FooVal->getPointeeLoc();
-                EXPECT_TRUE(isa<AggregateStorageLocation>(&FooPointeeLoc));
+        const StorageLocation &FooReferentLoc = FooVal->getReferentLoc();
+        EXPECT_TRUE(isa<AggregateStorageLocation>(&FooReferentLoc));
 
-                const Value *FooPointeeVal = Env.getValue(FooPointeeLoc);
-                EXPECT_TRUE(isa_and_nonnull<StructValue>(FooPointeeVal));
-              });
+        const Value *FooReferentVal = Env.getValue(FooReferentLoc);
+        EXPECT_TRUE(isa_and_nonnull<StructValue>(FooReferentVal));
 }
 
 TEST_F(TransferTest, SelfReferentialReferenceVarDecl) {
   std::string Code = R"(
     struct A;
 
     struct B {};
 
runDataflow(Code, [](llvm::ArrayRef<std::pair<
     ASSERT_THAT(FooRefDecl, NotNull());
     ASSERT_THAT(FooPtrDecl, NotNull());
     ASSERT_THAT(BazRefDecl, NotNull());
     ASSERT_THAT(BazPtrDecl, NotNull());
 
     const auto *FooLoc = cast<ScalarStorageLocation>(
         Env.getStorageLocation(*FooDecl, SkipPast::None));
     const auto *FooVal = cast<ReferenceValue>(Env.getValue(*FooLoc));
-    const auto *FooPointeeVal =
-        cast<StructValue>(Env.getValue(FooVal->getPointeeLoc()));
+    const auto *FooReferentVal =
+        cast<StructValue>(Env.getValue(FooVal->getReferentLoc()));
 
     const auto *BarVal =
-        cast<ReferenceValue>(FooPointeeVal->getChild(*BarDecl));
-    const auto *BarPointeeVal =
-        cast<StructValue>(Env.getValue(BarVal->getPointeeLoc()));
+        cast<ReferenceValue>(FooReferentVal->getChild(*BarDecl));
+    const auto *BarReferentVal =
+        cast<StructValue>(Env.getValue(BarVal->getReferentLoc()));
 
     const auto *FooRefVal =
-        cast<ReferenceValue>(BarPointeeVal->getChild(*FooRefDecl));
-    const StorageLocation &FooRefPointeeLoc = FooRefVal->getPointeeLoc();
-    EXPECT_THAT(Env.getValue(FooRefPointeeLoc), IsNull());
+        cast<ReferenceValue>(BarReferentVal->getChild(*FooRefDecl));
+    const StorageLocation &FooReferentLoc = FooRefVal->getReferentLoc();
+    EXPECT_THAT(Env.getValue(FooReferentLoc), IsNull());
 
     const auto *FooPtrVal =
-        cast<PointerValue>(BarPointeeVal->getChild(*FooPtrDecl));
+        cast<PointerValue>(BarReferentVal->getChild(*FooPtrDecl));
     const StorageLocation &FooPtrPointeeLoc = FooPtrVal->getPointeeLoc();
     EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull());
 
     const auto *BazRefVal =
-        cast<ReferenceValue>(BarPointeeVal->getChild(*BazRefDecl));
-    const StorageLocation &BazRefPointeeLoc = BazRefVal->getPointeeLoc();
-    EXPECT_THAT(Env.getValue(BazRefPointeeLoc), NotNull());
+        cast<ReferenceValue>(BarReferentVal->getChild(*BazRefDecl));
+    const StorageLocation &BazReferentLoc = BazRefVal->getReferentLoc();
+    EXPECT_THAT(Env.getValue(BazReferentLoc), NotNull());
 
     const auto *BazPtrVal =
-        cast<PointerValue>(BarPointeeVal->getChild(*BazPtrDecl));
+        cast<PointerValue>(BarReferentVal->getChild(*BazPtrDecl));
     const StorageLocation &BazPtrPointeeLoc = BazPtrVal->getPointeeLoc();
     EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull());
   });
 }
 
 TEST_F(TransferTest, PointerVarDecl) {
   std::string Code = R"(
     struct A {};
runDataflow(
 
         const auto *BarVal =
             cast<PointerValue>(FooPointeeVal->getChild(*BarDecl));
         const auto *BarPointeeVal =
             cast<StructValue>(Env.getValue(BarVal->getPointeeLoc()));
 
         const auto *FooRefVal =
             cast<ReferenceValue>(BarPointeeVal->getChild(*FooRefDecl));
-        const StorageLocation &FooRefPointeeLoc = FooRefVal->getPointeeLoc();
-        EXPECT_THAT(Env.getValue(FooRefPointeeLoc), IsNull());
+        const StorageLocation &FooReferentLoc = FooRefVal->getReferentLoc();
+        EXPECT_THAT(Env.getValue(FooReferentLoc), IsNull());
 
         const auto *FooPtrVal =
             cast<PointerValue>(BarPointeeVal->getChild(*FooPtrDecl));
         const StorageLocation &FooPtrPointeeLoc = FooPtrVal->getPointeeLoc();
         EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull());
 
         const auto *BazRefVal =
             cast<ReferenceValue>(BarPointeeVal->getChild(*BazRefDecl));
-        const StorageLocation &BazRefPointeeLoc = BazRefVal->getPointeeLoc();
-        EXPECT_THAT(Env.getValue(BazRefPointeeLoc), NotNull());
+        const StorageLocation &BazReferentLoc = BazRefVal->getReferentLoc();
+        EXPECT_THAT(Env.getValue(BazReferentLoc), NotNull());
 
         const auto *BazPtrVal =
             cast<PointerValue>(BarPointeeVal->getChild(*BazPtrDecl));
         const StorageLocation &BazPtrPointeeLoc = BazPtrVal->getPointeeLoc();
         EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull());
       });
 }
 
TEST_F(TransferTest, ReferenceParamDecl) {
   std::string Code = R"(
     struct A {};
 
     void target(A &Foo) {
       (void)0;
       // [[p]]
     }
   )";
-  runDataflow(Code,
-              [](llvm::ArrayRef<
-                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
-                     Results,
-                 ASTContext &ASTCtx) {
-                ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
-                const Environment &Env = Results[0].second.Env;
+  runDataflow(
+      Code, [](llvm::ArrayRef<
 
-                const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-                ASSERT_THAT(FooDecl, NotNull());
 
-                const StorageLocation *FooLoc =
-                    Env.getStorageLocation(*FooDecl, SkipPast::None);
-                ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc));
 
-                const ReferenceValue *FooVal =
-                    dyn_cast<ReferenceValue>(Env.getValue(*FooLoc));
-                ASSERT_THAT(FooVal, NotNull());
 
-                const StorageLocation &FooPointeeLoc = FooVal->getPointeeLoc();
-                EXPECT_TRUE(isa<AggregateStorageLocation>(&FooPointeeLoc));
+        const StorageLocation &FooReferentLoc = FooVal->getReferentLoc();
+        EXPECT_TRUE(isa<AggregateStorageLocation>(&FooReferentLoc));
 
-                const Value *FooPointeeVal = Env.getValue(FooPointeeLoc);
-                EXPECT_TRUE(isa_and_nonnull<StructValue>(FooPointeeVal));
-              });
+        const Value *FooReferentVal = Env.getValue(FooReferentLoc);
+        EXPECT_TRUE(isa_and_nonnull<StructValue>(FooReferentVal));
 }
 
 TEST_F(TransferTest, PointerParamDecl) {
   std::string Code = R"(
     struct A {};
 
     void target(A *Foo) {
       (void)0;
runDataflow(
           }
         }
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc = cast<AggregateStorageLocation>(
             Env.getStorageLocation(*FooDecl, SkipPast::None));
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal = cast<ReferenceValue>(FooVal->getChild(*BarDecl));
-        const auto *BarPointeeVal =
-            cast<IntegerValue>(Env.getValue(BarVal->getPointeeLoc()));
+        const auto *BarReferentVal =
+            cast<IntegerValue>(Env.getValue(BarVal->getReferentLoc()));
 
         const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
         ASSERT_THAT(BazDecl, NotNull());
 
-        EXPECT_EQ(Env.getValue(*BazDecl, SkipPast::None), BarPointeeVal);
+        EXPECT_EQ(Env.getValue(*BazDecl, SkipPast::None), BarReferentVal);
       });
 }
 
 TEST_F(TransferTest, StructThisMember) {
   std::string Code = R"(
     struct A {
       int Bar;
 
runDataflow(
         const auto *FooVal =
             cast<ReferenceValue>(Env.getValue(*FooDecl, SkipPast::None));
 
         const ValueDecl *QuxDecl = findValueDecl(ASTCtx, "Qux");
         ASSERT_THAT(QuxDecl, NotNull());
 
         const auto *QuxVal =
             cast<ReferenceValue>(Env.getValue(*QuxDecl, SkipPast::None));
-        EXPECT_EQ(&QuxVal->getPointeeLoc(), &FooVal->getPointeeLoc());
+        EXPECT_EQ(&QuxVal->getReferentLoc(), &FooVal->getReferentLoc());
       });
 }
 
 TEST_F(TransferTest, TemporaryObject) {
   std::string Code = R"(
     struct A {
       int Bar;
     };
runDataflow(
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooVal =
             cast<PointerValue>(Env.getValue(*FooDecl, SkipPast::None));
         const auto *BarVal =
             cast<ReferenceValue>(Env.getValue(*BarDecl, SkipPast::None));
-        EXPECT_EQ(&BarVal->getPointeeLoc(), &FooVal->getPointeeLoc());
+        EXPECT_EQ(&BarVal->getReferentLoc(), &FooVal->getPointeeLoc());
       });
 }
 
 TEST_F(TransferTest, VarDeclInitAssignConditionalOperator) {
   std::string Code = R"(
     struct A {};
 
     void target(A Foo, A Bar, bool Cond) {