[clang][nullability] Remove RecordValue.

This class no longer serves any purpose; see also the discussion here:
https://reviews.llvm.org/D155204#inline-1503204

A lot of existing tests in TransferTest.cpp check for the existence of
`RecordValue`s. Some of these checks are now simply redundant and have been
removed. In other cases, tests were checking for the existence of a
`RecordValue` as a way of testing whether a record has been initialized. I have
typically changed these test to instead check whether a field of the record has
a value.

Author: martinboehme

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

@@ -84,7 +84,7 @@ class Environment {
     virtual ComparisonResult compare(QualType Type, const Value &Val1,
                                      const Environment &Env1, const Value &Val2,
                                      const Environment &Env2) {
-      // FIXME: Consider adding QualType to RecordValue and removing the Type
+      // FIXME: Consider adding `QualType` to `Value` and removing the `Type`
       // argument here.
       return ComparisonResult::Unknown;
     }
@@ -407,20 +407,15 @@ class Environment {
   /// storage locations and values for indirections until it finds a
   /// non-pointer/non-reference type.
   ///
-  /// If `Type` is a class, struct, or union type, creates values for all
-  /// modeled fields (including synthetic fields) and calls `setValue()` to
-  /// associate the `RecordValue` with its storage location
-  /// (`RecordValue::getLoc()`).
-  ///
   /// If `Type` is one of the following types, this function will always return
   /// a non-null pointer:
   /// - `bool`
   /// - Any integer type
-  /// - Any class, struct, or union type
   ///
   /// Requirements:
   ///
-  ///  `Type` must not be null.
+  ///  - `Type` must not be null.
+  ///  - `Type` must not be a reference type or record type.
   Value *createValue(QualType Type);
 
   /// Creates an object (i.e. a storage location with an associated value) of
@@ -452,6 +447,7 @@ class Environment {
   /// Initializes the fields (including synthetic fields) of `Loc` with values,
   /// unless values of the field type are not supported or we hit one of the
   /// limits at which we stop producing values.
+  /// If a field already has a value, that value is preserved.
   /// If `Type` is provided, initializes only those fields that are modeled for
   /// `Type`; this is intended for use in cases where `Loc` is a derived type
   /// and we only want to initialize the fields of a base type.
@@ -461,6 +457,10 @@ class Environment {
   }
 
   /// Assigns `Val` as the value of `Loc` in the environment.
+  ///
+  /// Requirements:
+  ///
+  ///  `Loc` must not be a `RecordStorageLocation`.
   void setValue(const StorageLocation &Loc, Value &Val);
 
   /// Clears any association between `Loc` and a value in the environment.
@@ -470,20 +470,24 @@ class Environment {
   ///
   /// Requirements:
   ///
-  ///  - `E` must be a prvalue
-  ///  - If `Val` is a `RecordValue`, its `RecordStorageLocation` must be
-  ///    `getResultObjectLocation(E)`. An exception to this is if `E` is an
-  ///    expression that originally creates a `RecordValue` (such as a
-  ///    `CXXConstructExpr` or `CallExpr`), as these establish the location of
-  ///    the result object in the first place.
+  ///  - `E` must be a prvalue.
+  ///  - `E` must not have record type.
   void setValue(const Expr &E, Value &Val);
 
   /// Returns the value assigned to `Loc` in the environment or null if `Loc`
   /// isn't assigned a value in the environment.
+  ///
+  /// Requirements:
+  ///
+  ///  `Loc` must not be a `RecordStorageLocation`.
   Value *getValue(const StorageLocation &Loc) const;
 
   /// Equivalent to `getValue(getStorageLocation(D))` if `D` is assigned a
   /// storage location in the environment, otherwise returns null.
+  ///
+  /// Requirements:
+  ///
+  ///  `D` must not have record type.
   Value *getValue(const ValueDecl &D) const;
 
   /// Equivalent to `getValue(getStorageLocation(E, SP))` if `E` is assigned a
@@ -811,12 +815,6 @@ class RecordInitListHelper {
   std::optional<ImplicitValueInitExpr> ImplicitValueInitForUnion;
 };
 
-/// Associates a new `RecordValue` with `Loc` and returns the new value.
-RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env);
-
-/// Associates a new `RecordValue` with `Expr` and returns the new value.
-RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env);
-
 } // namespace dataflow
 } // namespace clang
 

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

@@ -35,7 +35,6 @@ class Value {
   enum class Kind {
     Integer,
     Pointer,
-    Record,
 
     // TODO: Top values should not be need to be type-specific.
     TopBool,
@@ -67,7 +66,6 @@ class Value {
   /// Properties may not be set on `RecordValue`s; use synthetic fields instead
   /// (for details, see documentation for `RecordStorageLocation`).
   void setProperty(llvm::StringRef Name, Value &Val) {
-    assert(getKind() != Kind::Record);
     Properties.insert_or_assign(Name, &Val);
   }
 
@@ -184,45 +182,6 @@ class PointerValue final : public Value {
   StorageLocation &PointeeLoc;
 };
 
-/// Models a value of `struct` or `class` type.
-/// In C++, prvalues of class type serve only a limited purpose: They can only
-/// be used to initialize a result object. It is not possible to access member
-/// variables or call member functions on a prvalue of class type.
-/// Correspondingly, `RecordValue` also serves only a limited purpose: It
-/// conveys a prvalue of class type from the place where the object is
-/// constructed to the result object that it initializes.
-///
-/// When creating a prvalue of class type, we already need a storage location
-/// for `this`, even though prvalues are otherwise not associated with storage
-/// locations. `RecordValue` is therefore essentially a wrapper for a storage
-/// location, which is then used to set the storage location for the result
-/// object when we process the AST node for that result object.
-///
-/// For example:
-///    MyStruct S = MyStruct(3);
-///
-/// In this example, `MyStruct(3) is a prvalue, which is modeled as a
-/// `RecordValue` that wraps a `RecordStorageLocation`. This
-/// `RecordStorageLocation` is then used as the storage location for `S`.
-///
-/// Over time, we may eliminate `RecordValue` entirely. See also the discussion
-/// here: https://reviews.llvm.org/D155204#inline-1503204
-class RecordValue final : public Value {
-public:
-  explicit RecordValue(RecordStorageLocation &Loc)
-      : Value(Kind::Record), Loc(Loc) {}
-
-  static bool classof(const Value *Val) {
-    return Val->getKind() == Kind::Record;
-  }
-
-  /// Returns the storage location that this `RecordValue` is associated with.
-  RecordStorageLocation &getLoc() const { return Loc; }
-
-private:
-  RecordStorageLocation &Loc;
-};
-
 raw_ostream &operator<<(raw_ostream &OS, const Value &Val);
 
 } // namespace dataflow

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

@@ -23,6 +23,7 @@
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <utility>
@@ -79,7 +80,6 @@ static bool equateUnknownValues(Value::Kind K) {
   switch (K) {
   case Value::Kind::Integer:
   case Value::Kind::Pointer:
-  case Value::Kind::Record:
     return true;
   default:
     return false;
@@ -144,25 +144,7 @@ static Value *joinDistinctValues(QualType Type, Value &Val1,
     return &A.makeBoolValue(JoinedVal);
   }
 
-  Value *JoinedVal = nullptr;
-  if (auto *RecordVal1 = dyn_cast<RecordValue>(&Val1)) {
-    auto *RecordVal2 = cast<RecordValue>(&Val2);
-
-    if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
-      // `RecordVal1` and `RecordVal2` may have different properties associated
-      // with them. Create a new `RecordValue` with the same location but
-      // without any properties so that we soundly approximate both values. If a
-      // particular analysis needs to join properties, it should do so in
-      // `DataflowAnalysis::join()`.
-      JoinedVal = &JoinedEnv.create<RecordValue>(RecordVal1->getLoc());
-    else
-      // If the locations for the two records are different, need to create a
-      // completely new value.
-      JoinedVal = JoinedEnv.createValue(Type);
-  } else {
-    JoinedVal = JoinedEnv.createValue(Type);
-  }
-
+  Value *JoinedVal = JoinedEnv.createValue(Type);
   if (JoinedVal)
     Model.join(Type, Val1, Env1, Val2, Env2, *JoinedVal, JoinedEnv);
 
@@ -627,7 +609,6 @@ void Environment::initialize() {
       auto &ThisLoc =
           cast<RecordStorageLocation>(createStorageLocation(ThisPointeeType));
       setThisPointeeStorageLocation(ThisLoc);
-      refreshRecordValue(ThisLoc, *this);
       // Initialize fields of `*this` with values, but only if we're not
       // analyzing a constructor; after all, it's the constructor's job to do
       // this (and we want to be able to test that).
@@ -794,10 +775,6 @@ void Environment::popCall(const CXXConstructExpr *Call,
   // See also comment in `popCall(const CallExpr *, const Environment &)` above.
   this->LocToVal = std::move(CalleeEnv.LocToVal);
   this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken);
-
-  if (Value *Val = CalleeEnv.getValue(*CalleeEnv.ThisPointeeLoc)) {
-    setValue(*Call, *Val);
-  }
 }
 
 bool Environment::equivalentTo(const Environment &Other,
@@ -1021,24 +998,23 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
 }
 
 void Environment::setValue(const StorageLocation &Loc, Value &Val) {
-  assert(!isa<RecordValue>(&Val) || &cast<RecordValue>(&Val)->getLoc() == &Loc);
-
+  // Records should not be associated with values.
+  assert(!isa<RecordStorageLocation>(Loc));
   LocToVal[&Loc] = &Val;
 }
 
 void Environment::setValue(const Expr &E, Value &Val) {
   const Expr &CanonE = ignoreCFGOmittedNodes(E);
 
-  if (auto *RecordVal = dyn_cast<RecordValue>(&Val)) {
-    assert(&RecordVal->getLoc() == &getResultObjectLocation(CanonE));
-    (void)RecordVal;
-  }
-
   assert(CanonE.isPRValue());
+  // Records should not be associated with values.
+  assert(!CanonE.getType()->isRecordType());
   ExprToVal[&CanonE] = &Val;
 }
 
 Value *Environment::getValue(const StorageLocation &Loc) const {
+  // Records should not be associated with values.
+  assert(!isa<RecordStorageLocation>(Loc));
   return LocToVal.lookup(&Loc);
 }
 
@@ -1050,6 +1026,9 @@ Value *Environment::getValue(const ValueDecl &D) const {
 }
 
 Value *Environment::getValue(const Expr &E) const {
+  // Records should not be associated with values.
+  assert(!E.getType()->isRecordType());
+
   if (E.isPRValue()) {
     auto It = ExprToVal.find(&ignoreCFGOmittedNodes(E));
     return It == ExprToVal.end() ? nullptr : It->second;
@@ -1078,6 +1057,7 @@ Value *Environment::createValueUnlessSelfReferential(
     int &CreatedValuesCount) {
   assert(!Type.isNull());
   assert(!Type->isReferenceType());
+  assert(!Type->isRecordType());
 
   // Allow unlimited fields at depth 1; only cap at deeper nesting levels.
   if ((Depth > 1 && CreatedValuesCount > MaxCompositeValueSize) ||
@@ -1106,15 +1086,6 @@ Value *Environment::createValueUnlessSelfReferential(
     return &arena().create<PointerValue>(PointeeLoc);
   }
 
-  if (Type->isRecordType()) {
-    CreatedValuesCount++;
-    auto &Loc = cast<RecordStorageLocation>(createStorageLocation(Type));
-    initializeFieldsWithValues(Loc, Loc.getType(), Visited, Depth,
-                               CreatedValuesCount);
-
-    return &refreshRecordValue(Loc, *this);
-  }
-
   return nullptr;
 }
 
@@ -1124,20 +1095,23 @@ Environment::createLocAndMaybeValue(QualType Ty,
                                     int Depth, int &CreatedValuesCount) {
   if (!Visited.insert(Ty.getCanonicalType()).second)
     return createStorageLocation(Ty.getNonReferenceType());
-  Value *Val = createValueUnlessSelfReferential(
-      Ty.getNonReferenceType(), Visited, Depth, CreatedValuesCount);
-  Visited.erase(Ty.getCanonicalType());
+  auto EraseVisited = llvm::make_scope_exit(
+      [&Visited, Ty] { Visited.erase(Ty.getCanonicalType()); });
 
   Ty = Ty.getNonReferenceType();
 
-  if (Val == nullptr)
-    return createStorageLocation(Ty);
-
-  if (Ty->isRecordType())
-    return cast<RecordValue>(Val)->getLoc();
+  if (Ty->isRecordType()) {
+    auto &Loc = cast<RecordStorageLocation>(createStorageLocation(Ty));
+    initializeFieldsWithValues(Loc, Ty, Visited, Depth, CreatedValuesCount);
+    return Loc;
+  }
 
   StorageLocation &Loc = createStorageLocation(Ty);
-  setValue(Loc, *Val);
+
+  if (Value *Val = createValueUnlessSelfReferential(Ty, Visited, Depth,
+                                                    CreatedValuesCount))
+    setValue(Loc, *Val);
+
   return Loc;
 }
 
@@ -1149,10 +1123,11 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
   auto initField = [&](QualType FieldType, StorageLocation &FieldLoc) {
     if (FieldType->isRecordType()) {
       auto &FieldRecordLoc = cast<RecordStorageLocation>(FieldLoc);
-      setValue(FieldRecordLoc, create<RecordValue>(FieldRecordLoc));
       initializeFieldsWithValues(FieldRecordLoc, FieldRecordLoc.getType(),
                                  Visited, Depth + 1, CreatedValuesCount);
     } else {
+      if (getValue(FieldLoc) != nullptr)
+        return;
       if (!Visited.insert(FieldType.getCanonicalType()).second)
         return;
       if (Value *Val = createValueUnlessSelfReferential(
@@ -1210,7 +1185,6 @@ StorageLocation &Environment::createObjectInternal(const ValueDecl *D,
     auto &RecordLoc = cast<RecordStorageLocation>(Loc);
     if (!InitExpr)
       initializeFieldsWithValues(RecordLoc);
-    refreshRecordValue(RecordLoc, *this);
   } else {
     Value *Val = nullptr;
     if (InitExpr)
@@ -1407,25 +1381,5 @@ RecordInitListHelper::RecordInitListHelper(const InitListExpr *InitList) {
   }
 }
 
-RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env) {
-  auto &NewVal = Env.create<RecordValue>(Loc);
-  Env.setValue(Loc, NewVal);
-  return NewVal;
-}
-
-RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) {
-  assert(Expr.getType()->isRecordType());
-
-  if (Expr.isPRValue())
-    refreshRecordValue(Env.getResultObjectLocation(Expr), Env);
-
-  if (auto *Loc = Env.get<RecordStorageLocation>(Expr))
-    refreshRecordValue(*Loc, Env);
-
-  auto &NewVal = *cast<RecordValue>(Env.createValue(Expr.getType()));
-  Env.setStorageLocation(Expr, NewVal.getLoc());
-  return NewVal;
-}
-
 } // namespace dataflow
 } // namespace clang

clang/lib/Analysis/FlowSensitive/DebugSupport.cpp

@@ -28,8 +28,6 @@ llvm::StringRef debugString(Value::Kind Kind) {
     return "Integer";
   case Value::Kind::Pointer:
     return "Pointer";
-  case Value::Kind::Record:
-    return "Record";
   case Value::Kind::AtomicBool:
     return "AtomicBool";
   case Value::Kind::TopBool: