[clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

[mboehme]

Summary

After this change, StructValue is just a wrapper for an AggregateStorageLocation. For the wider context, see https://discourse.llvm.org/t/70086.

How to review

• Start by looking at the comments added / changed in Value.h, StorageLocation.h, and DataflowEnvironment.h. This will give you a good overview of the semantic changes.

• Look at the corresponding .cpp files that implement the semantic changes.

• Transfer.cpp, TypeErasedDataflowAnalysis.cpp, and RecordOps.cpp show how the core of the framework is affected by the semantic changes.

• UncheckedOptionalAccessModel.cpp shows how this complex model is affected by the changes.

• Many of the changes in the rest of the patch are mechanical in nature.

[mboehme]

Added bugfix for refreshStructValue() as a diffbase.

[ymandel]

I got up to Transfer.cpp and figured I should send along what I have. Looks very good so far! I should have the rest of the review in a few hours...

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

337–338

I found this sentence hard to parse. Lots of "of..." chained. I think it might be clearer with a bit of a preamble about record prvalues. Or, refer the reader to Value.h for details? Once I read that, this became a lot clearer.

340

353

Given that this is specific to record types, and that's not reflected in the argument type, should the name somehow reflect that?

413–416

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

97–99

When assertions are off this will be an odd for loop with no body. Will the optimizer delete it? If not, then consider putting the loop in a lambda inside the assert (or somesuch).

136

which constructor? Also, is this clause clarifying the "other fields cannot change" part of the sentence?

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

202–223

These comments are fantastic!

206–207

nit: consider adding a simple example. Is this one right?
MyStruct S = MyStruct(3);
The intiializer is a prvalue which will result in a StructValue wrapping an ASL. Then, we'll model the decl S using that ASL.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

127

Please add comment explaining why we can assume this invariant.

129

Why do we need a new StructValue rather than reusing one of the existing ones?

690–691

Maybe just an assert?

assert(!isa<StructValue>(&Val) || &cast<StructValue>(&Val)->getAggregateLoc() == &Loc);

690–694

is it worth exposing this in the header (inline) now that it's just one line?

708

How does this work, given that you never called setValue?

811–813

how does this interact with the fact that multiple StructValues can share the same ASL?

821

Consider adding a comment for this given that there's none in the header. the name is a bit confusing on its own since the function also creates a corresponding value, not just the loc.

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

351

What happens to ValueVal if we use createObject?

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

88–89

nit: use else if ?

[mboehme]

Various changes in response to review comments

[mboehme]

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

337–338

Reworded.

I've tried to avoid referring to Value.h because StructValue itself may be going away entirely in the not-too-distant future. Instead, I've adapted some material from there (which also means people don't need to cross-reference Value.h).

WDYT?

353

Done.

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

97–99

Hm -- I'm not sure. Changed to a lambda.

136

Reworded. Is this clearer now?

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

206–207

Yes, this looks right. Added this as an example below.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

129

Added a comment.

690–694

Good point, done.

708

Agree this is confusing.

The intent here was to check a _pre_condition of the function (i.e. that the caller must already have associated StructVal->getAggregateLoc() with Val in the environment before calling this function) rather than a _post_condition of the function. But looking at this again, this doesn't seem like a good contract. All current callers of the function satisfy this precondition, but it's not documented anywhere, and it's also just simply a strange and unnecessary precondition. So instead, I've simply replaced this with a setValue() that makes sure this is the case if it wasn't before.

811–813

Not sure exactly what you're asking?

First of all, I think the wording on the documentation for StructValue may have been confusing, so I've reworded that to make it explicit that when I say that multiple StructValues can share the same AggregateStorageLocation, what I mean, more precisely, is that the same AggregateStorageLocation can be associated with different StructValues in different environments.

Here, however, we've only just created the AggregateStorageLocation (and a StructValue to go with it), so it definitely makes sense to associate the two through a call to setValue(Loc, StructVal).

821

Yes, the name was bad. Renamed and added a comment in the header.

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

351

Oops -- this was left over from refactoring and was superfluous now. Thanks for catching. Removed.

[ymandel]

This is really impressive. Thank you!

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

337–338

Looks great!

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

136

Yes. thx

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

811–813

Thanks, that clarifies. I missed the "in different environments" bit. Makes sense now.

clang/lib/Analysis/FlowSensitive/Transfer.cpp

466

This diff makes me very happy. :)

496

I thought cast requires a nonnull argument, so this will always be true?

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

397

FIXME, here and below.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

30

fixme

[mboehme]

Changes in response to review comments

[mboehme]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

466

Me too! :)

496

Ah, good catch! Yes, the condition will always be true, so the if statement is superfluous. (We know S->getType() is a record type, and Env.createValue() will always create a value for record types.)

Removed the if statement.

(Confusingly, this comment now looks as if it's anchored to the if statement that does the Env.getValue(). The cast_or_null there _is_ necessary because we might not have a value for the argument.)

[mboehme]

CI failures look unrelated:

• Windows failure is in SemaCXX/static-assert-cxx26.cpp. I can't see a way in which this patch should affect the failure of this test (the dataflow framework isn't used in Clang itself)
• check-format failure is in clang/lib/Analysis/UnsafeBufferUsage.cpp, which this patch doesn't touch

[xazax.hun]

I did not do a thorough review checking every line, but I read the design paper and skimmed through this patch. Love the direction, and I am OK with landing this as is.

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

677

I am glad to see this gone.

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

201–233

Should we link to the design doc somewhere? The comments are great and self-contained, but I wonder if there is still some value for the doc to be mentioned somewhere in the code. I do not have a preference, just wondering.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

678

Is eliminating SkipPast coming in a follow-up patch?

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

534–549

This is more of a note for the future of these APIs. I wonder if this is the right level of abstraction in this framework. I think it would be great to have something like:

void transferCallReturningOptional(const CallExpr *E,
                                   StorageLocation* RetLoc,
                                   ArrayRef<StorageLocation> Args,
                                   const MatchFinder::MatchResult &Result,
                                   LatticeTransferState &State);

and all the values and storage locations could come from the framework. It would be great if the user would almost never need to get a value or a location from an expression, they would be readily available. What do you think?

[mboehme]

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

201–233

I'm hestitant do to this, as the doc is likely to become out of date at some point. We've already discussed plans on another patch to eliminate StructValue entirely, for example. So I'd rather make sure we have sufficient documentation with the code itself, where it's most likely to be kept up to date.

Anything in particular from the doc that you think would be worth capturing here? (I intend to submit shortly, but would address this in a followup patch.)

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

678

Yes! I've already got a draft patch, which I'll be submitting for review soon.

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

534–549

Hm, interesting idea! This definitely sounds like a way to avoid mistakes, though there would definitely be some details to work out -- for example, a StorageLocation for the arguments only works if they are passed by reference, not by value. Something to think about for sure though.

[xazax.hun]

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

201–233

I have nothing I would miss from the comments, but there were more code examples in the design doc that helped me understand the problem. I can imagine some people have easier time consuming that document for this reason. But I agree that this is not a strong enough argument to link to something that might go stale in the future.

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

534–549

Another potential advantage of that approach would be to amortize the lookup costs in case there are multiple checks participating in the same fixed-point iteration.

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

public:
   ///  `E` must be a glvalue or a `BuiltinType::BuiltinFn`
   StorageLocation *getStorageLocationStrict(const Expr &E) const;
 
   /// 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.
   AggregateStorageLocation *getThisPointeeStorageLocation() const;
 
+  /// Returns the location of the result object for a record-type prvalue.
+  ///
+  /// In C++, prvalues of record type serve only a limited purpose: They can
+  /// only be used to initialize a result object (e.g. a variable or a
+  /// temporary). This function returns the location of that result object.
+  ///
+  /// When creating a prvalue of record type, we already need the storage
+  /// location of the result object to pass in `this`, even though prvalues are
+  /// otherwise not associated with storage locations.
+  ///
+  /// FIXME: Currently, this simply returns a stable storage location for `E`,
+  /// but this doesn't do the right thing in scenarios like the following:
+  /// ```
+  /// MyClass c = some_condition()? MyClass(foo) : MyClass(bar);
+  /// ```
+  /// Here, `MyClass(foo)` and `MyClass(bar)` will have two different storage
+  /// locations, when in fact their storage locations should be the same.
+  /// Eventually, we want to propagate storage locations from result objects
+  /// down to the prvalues that initialize them, similar to the way that this is
+  /// done in Clang's CodeGen.
+  ///
+  /// Requirements:
+  ///  `E` must be a prvalue of record type.
+  AggregateStorageLocation &getResultObjectLocation(const Expr &RecordPRValue);
+
   /// Returns the return value of the current function. This can be null if:
   /// - The function has a void return type
   /// - No return value could be determined for the function, for example
   ///   because it calls a function without a body.
   ///
   /// Requirements:
   ///  The current function must have a non-reference return type.
   Value *getReturnValue() const {
void setReturnStorageLocation(StorageLocation *Loc) {
     ReturnLoc = Loc;
   }
 
   /// Returns a pointer value that represents a null pointer. Calls with
   /// `PointeeType` that are canonically equivalent will return the same result.
   PointerValue &getOrCreateNullPointerValue(QualType PointeeType);
 
   /// Creates a value appropriate for `Type`, if `Type` is supported, otherwise
-  /// return null. If `Type` is a pointer or reference type, creates all the
-  /// necessary storage locations and values for indirections until it finds a
+  /// returns null.
+  ///
+  /// If `Type` is a pointer or reference type, creates all the necessary
+  /// storage locations and values for indirections until it finds a
   /// non-pointer/non-reference type.
   ///
+  /// If `Type` is a class, struct, or union type, calls `setValue()` to
+  /// associate the `StructValue` with its storage location
+  /// (`StructValue::getAggregateLoc()`).
+  ///
   /// 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:
   ///
public:
   StorageLocation &createObject(const VarDecl &D, const Expr *InitExpr) {
     return createObjectInternal(&D, D.getType(), InitExpr);
   }
 
   /// Assigns `Val` as the value of `Loc` in the environment.
   void setValue(const StorageLocation &Loc, Value &Val);
 
   /// Clears any association between `Loc` and a value in the environment.
-  void clearValue(const StorageLocation &Loc);
+  void clearValue(const StorageLocation &Loc) { LocToVal.erase(&Loc); }
 
   /// Assigns `Val` as the value of the prvalue `E` in the environment.
   ///
   /// If `E` is not yet associated with a storage location, associates it with
   /// a newly created storage location. In any case, associates the storage
   /// location of `E` with `Val`.
   ///
   /// Once the migration to strict handling of value categories is complete
   /// (see https://discourse.llvm.org/t/70086), this function will be renamed to
   /// `setValue()`. At this point, prvalue expressions will be associated
   /// directly with `Value`s, and the legacy behavior of associating prvalue
   /// expressions with storage locations (as described above) will be
   /// eliminated.
   ///
   /// Requirements:
   ///
   ///  `E` must be a prvalue
   ///  `Val` must not be a `ReferenceValue`
+  ///  If `Val` is a `StructValue`, its `AggregateStorageLocation` must be the
+  ///  same as that of any `StructValue` that has already been associated with
+  ///  `E`. This is to guarantee that the result object initialized by a prvalue
+  ///  `StructValue` has a durable storage location.
   void setValueStrict(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.
   Value *getValue(const StorageLocation &Loc) const;
 
   /// Equivalent to `getValue(getStorageLocation(D, SP), SkipPast::None)` if `D`
   /// is assigned a storage location in the environment, otherwise returns null.
private:
   ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
   Value *createValueUnlessSelfReferential(QualType Type,
                                           llvm::DenseSet<QualType> &Visited,
                                           int Depth, int &CreatedValuesCount);
 
+  /// Creates a storage location for `Ty`. Also creates and associates a value
+  /// with the storage location, unless values of this type are not supported or
+  /// we hit one of the limits at which we stop producing values (controlled by
+  /// `Visited`, `Depth`, and `CreatedValuesCount`).
+  StorageLocation &createLocAndMaybeValue(QualType Ty,
+                                          llvm::DenseSet<QualType> &Visited,
+                                          int Depth, int &CreatedValuesCount);
+
   /// Shared implementation of `createObject()` overloads.
   /// `D` and `InitExpr` may be null.
   StorageLocation &createObjectInternal(const VarDecl *D, QualType Ty,
                                         const Expr *InitExpr);
 
   StorageLocation &skip(StorageLocation &Loc, SkipPast SP) const;
   const StorageLocation &skip(const StorageLocation &Loc, SkipPast SP) const;
 
private:
   // include only storage locations that are in scope for a particular basic
   // block.
   llvm::DenseMap<const ValueDecl *, StorageLocation *> DeclToLoc;
   llvm::DenseMap<const Expr *, StorageLocation *> ExprToLoc;
   // We preserve insertion order so that join/widen process values in
   // deterministic sequence. This in turn produces deterministic SAT formulas.
   llvm::MapVector<const StorageLocation *, Value *> LocToVal;
 
-  // Maps locations of struct members to symbolic values of the structs that own
-  // them and the decls of the struct members.
-  llvm::DenseMap<const StorageLocation *,
-                 std::pair<StructValue *, const ValueDecl *>>
-      MemberLocToStruct;
-
   Atom FlowConditionToken;
 };
 
 /// Returns the storage location for the implicit object of a
 /// `CXXMemberCallExpr`, or null if none is defined in the environment.
 /// Dereferences the pointer if the member call expression was written using
 /// `->`.
 AggregateStorageLocation *

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

 
 #ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_STORAGELOCATION_H
 #define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_STORAGELOCATION_H
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Debug.h"
+#include <cassert>
 
 #define DEBUG_TYPE "dataflow"
 
 namespace clang {
 namespace dataflow {
 
 /// Base class for elements of the local variable store and of the heap.
 ///
 /// Each storage location holds a value. The mapping from storage locations to
 /// values is stored in the environment.
 class StorageLocation {
 public:
   enum class Kind { Scalar, Aggregate };
 
-  StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) {}
+  StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) {
+    assert(Type.isNull() || !Type->isReferenceType());
+  }
 
   // Non-copyable because addresses of storage locations are used as their
   // identities throughout framework and user code. The framework is responsible
   // for construction and destruction of storage locations.
   StorageLocation(const StorageLocation &) = delete;
   StorageLocation &operator=(const StorageLocation &) = delete;
 
   virtual ~StorageLocation() = default;
public:
   }
 };
 
 /// A storage location which is subdivided into smaller storage locations that
 /// can be traced independently by abstract interpretation. For example: a
 /// struct with public members. The child map is flat, so when used for a struct
 /// or class type, all accessible members of base struct and class types are
 /// directly accesible as children of this location.
+///
+/// The storage location for a field of reference type may be null. This
+/// typically occurs in one of two situations:
+/// - The record has not been fully initialized.
+/// - The maximum depth for modelling a self-referential data structure has been
+///   reached.
+/// Storage locations for fields of all other types must be non-null.
+///
 /// FIXME: Currently, the storage location of unions is modelled the same way as
 /// that of structs or classes. Eventually, we need to change this modelling so
 /// that all of the members of a given union have the same storage location.
 class AggregateStorageLocation final : public StorageLocation {
 public:
   using FieldToLoc = llvm::DenseMap<const ValueDecl *, StorageLocation *>;
 
   explicit AggregateStorageLocation(QualType Type)
       : AggregateStorageLocation(Type, FieldToLoc()) {}
 
-  AggregateStorageLocation(QualType Type, FieldToLoc Children)
-      : StorageLocation(Kind::Aggregate, Type), Children(std::move(Children)) {}
+  AggregateStorageLocation(QualType Type, FieldToLoc TheChildren)
+      : StorageLocation(Kind::Aggregate, Type),
+        Children(std::move(TheChildren)) {
+    assert(!Type.isNull());
+    assert(Type->isRecordType());
+    assert([this] {
+      for (auto [Field, Loc] : Children) {
+        if (!Field->getType()->isReferenceType() && Loc == nullptr)
+          return false;
+      }
+      return true;
+    }());
+  }
 
   static bool classof(const StorageLocation *Loc) {
     return Loc->getKind() == Kind::Aggregate;
   }
 
   /// Returns the child storage location for `D`.
-  StorageLocation &getChild(const ValueDecl &D) const {
+  ///
+  /// May return null if `D` has reference type; guaranteed to return non-null
+  /// in all other cases.
+  ///
+  /// Note that it is an error to call this with a field that does not exist.
+  /// The function does not return null in this case.
+  StorageLocation *getChild(const ValueDecl &D) const {
     auto It = Children.find(&D);
     LLVM_DEBUG({
       if (It == Children.end()) {
         llvm::dbgs() << "Couldn't find child " << D.getNameAsString()
                      << " on StorageLocation " << this << " of type "
                      << getType() << "\n";
         llvm::dbgs() << "Existing children:\n";
         for ([[maybe_unused]] auto [Field, Loc] : Children) {
           llvm::dbgs() << Field->getNameAsString() << "\n";
         }
       }
     });
     assert(It != Children.end());
-    return *It->second;
+    return It->second;
+  }
+
+  /// Changes the child storage location for a field `D` of reference type.
+  /// All other fields cannot change their storage location and always retain
+  /// the storage location passed to the `AggregateStorageLocation` constructor.
+  ///
+  /// Requirements:
+  ///
+  ///  `D` must have reference type.
+  void setChild(const ValueDecl &D, StorageLocation *Loc) {
+    assert(D.getType()->isReferenceType());
+    Children[&D] = Loc;
   }
 
   llvm::iterator_range<FieldToLoc::const_iterator> children() const {
     return {Children.begin(), Children.end()};
   }
 
 private:
   FieldToLoc Children;
 };
 
 } // namespace dataflow
 } // namespace clang
 
 #undef DEBUG_TYPE
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_STORAGELOCATION_H

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

public:
   }
 
   StorageLocation &getPointeeLoc() const { return PointeeLoc; }
 
 private:
   StorageLocation &PointeeLoc;
 };
 
-/// Models a value of `struct` or `class` type, with a flat map of fields to
-/// child storage locations, containing all accessible members of base struct
-/// and class types.
+/// 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, `StructValue` also serves only two limited purposes:
+/// - 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. `StructValue` 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
+///   `StructValue` that wraps an `AbstractStorageLocation`. This
+//    `AbstractStorageLocation` is then used as the storage location for `S`.
+///
+/// - It allows properties to be associated with an object of class type.
+///   Note that when doing so, you should avoid mutating the properties of an
+///   existing `StructValue` in place, as these changes would be visible to
+///   other `Environment`s that share the same `StructValue`. Instead, associate
+///   a new `StructValue` with the `AggregateStorageLocation` and set the
+///   properties on this new `StructValue`. (See also `refreshStructValue()` in
+///   DataflowEnvironment.h, which makes this easy.)
+///   Note also that this implies that it is common for the same
+///   `AggregateStorageLocation` to be associated with different `StructValue`s
+///   in different environments.
+/// Over time, we may eliminate `StructValue` entirely. See also the discussion
+/// here: https://reviews.llvm.org/D155204#inline-1503204
 class StructValue final : public Value {
 public:
-  StructValue() : StructValue(llvm::DenseMap<const ValueDecl *, Value *>()) {}
-
-  explicit StructValue(llvm::DenseMap<const ValueDecl *, Value *> Children)
-      : Value(Kind::Struct), Children(std::move(Children)) {}
+  explicit StructValue(AggregateStorageLocation &Loc)
+      : Value(Kind::Struct), Loc(Loc) {}
 
   static bool classof(const Value *Val) {
     return Val->getKind() == Kind::Struct;
   }
 
-  /// Returns the child value that is assigned for `D` or null if the child is
-  /// not initialized.
-  Value *getChild(const ValueDecl &D) const { return Children.lookup(&D); }
-
-  /// Assigns `Val` as the child value for `D`.
-  void setChild(const ValueDecl &D, Value &Val) { Children[&D] = &Val; }
-
-  /// Clears any value assigned as the child value for `D`.
-  void clearChild(const ValueDecl &D) { Children.erase(&D); }
-
-  llvm::iterator_range<
-      llvm::DenseMap<const ValueDecl *, Value *>::const_iterator>
-  children() const {
-    return {Children.begin(), Children.end()};
+  /// Returns the storage location that this `StructValue` is associated with.
+  AggregateStorageLocation &getAggregateLoc() const { return Loc; }
+
+  /// Convenience function that returns the child storage location for `Field`.
+  /// See also the documentation for `AggregateStorageLocation::getChild()`.
+  StorageLocation *getChild(const ValueDecl &Field) const {
+    return Loc.getChild(Field);
   }
 
 private:
-  llvm::DenseMap<const ValueDecl *, Value *> Children;
+  AggregateStorageLocation &Loc;
 };
 
 raw_ostream &operator<<(raw_ostream &OS, const Value &Val);
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_VALUE_H

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

 void DataflowAnalysisContext::addModeledFields(const FieldSet &Fields) {
   ModeledFields.set_union(Fields);
 }
 
 StorageLocation &DataflowAnalysisContext::createStorageLocation(QualType Type) {
   if (!Type.isNull() && Type->isRecordType()) {
     llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
     for (const FieldDecl *Field : getModeledFields(Type))
-      FieldLocs.insert({Field, &createStorageLocation(Field->getType())});
+      if (Field->getType()->isReferenceType())
+        FieldLocs.insert({Field, nullptr});
+      else
+        FieldLocs.insert({Field, &createStorageLocation(
+                                     Field->getType().getNonReferenceType())});
     return arena().create<AggregateStorageLocation>(Type, std::move(FieldLocs));
   }
   return arena().create<ScalarStorageLocation>(Type);
 }
 
 StorageLocation &
 DataflowAnalysisContext::getStableStorageLocation(const VarDecl &D) {
   if (auto *Loc = getStorageLocation(D))

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

if (isa<BoolValue>(&Val1) && isa<BoolValue>(&Val2)) {
     MergedEnv.addToFlowCondition(
         A.makeOr(A.makeAnd(A.makeAtomRef(Env1.getFlowConditionToken()),
                            A.makeEquals(MergedVal, Expr1)),
                  A.makeAnd(A.makeAtomRef(Env2.getFlowConditionToken()),
                            A.makeEquals(MergedVal, Expr2))));
     return &A.makeBoolValue(MergedVal);
   }
 
+  Value *MergedVal = nullptr;
+  if (auto *StructVal1 = dyn_cast<StructValue>(&Val1)) {
+    auto *StructVal2 = cast<StructValue>(&Val2);
+
+    // Values to be merged are always associated with the same location in
+    // `LocToVal`. The location stored in `StructVal` should therefore also
+    // be the same.
+    assert(&StructVal1->getAggregateLoc() == &StructVal2->getAggregateLoc());
+
+    // `StructVal1` and `StructVal2` may have different properties associated
+    // with them. Create a new `StructValue` without any properties so that we
+    // soundly approximate both values. If a particular analysis needs to merge
+    // properties, it should do so in `DataflowAnalysis::merge()`.
+    MergedVal = &MergedEnv.create<StructValue>(StructVal1->getAggregateLoc());
+  } else {
+    MergedVal = MergedEnv.createValue(Type);
+  }
+
   // FIXME: Consider destroying `MergedValue` immediately if `ValueModel::merge`
   // returns false to avoid storing unneeded values in `DACtx`.
   // FIXME: Creating the value based on the type alone creates misshapen values
   // for lvalues, since the type does not reflect the need for `ReferenceValue`.
   // This issue will be resolved when `ReferenceValue` is eliminated as part
   // of the ongoing migration to strict handling of value categories (see
   // https://discourse.llvm.org/t/70086 for details).
-  if (Value *MergedVal = MergedEnv.createValue(Type))
+  if (MergedVal)
     if (Model.merge(Type, Val1, Env1, Val2, Env2, *MergedVal, MergedEnv))
       return MergedVal;
 
   return nullptr;
 }
 
 // When widening does not change `Current`, return value will equal `&Prev`.
 static Value &widenDistinctValues(QualType Type, Value &Prev,
if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) {
     auto *Parent = MethodDecl->getParent();
     assert(Parent != nullptr);
     if (Parent->isLambda())
       MethodDecl = dyn_cast<CXXMethodDecl>(Parent->getDeclContext());
 
     // FIXME: Initialize the ThisPointeeLoc of lambdas too.
     if (MethodDecl && !MethodDecl->isStatic()) {
       QualType ThisPointeeType = MethodDecl->getThisObjectType();
-      ThisPointeeLoc = &cast<AggregateStorageLocation>(
-          createStorageLocation(ThisPointeeType));
-      if (Value *ThisPointeeVal = createValue(ThisPointeeType))
-        setValue(*ThisPointeeLoc, *ThisPointeeVal);
+      ThisPointeeLoc =
+          &cast<StructValue>(createValue(ThisPointeeType))->getAggregateLoc();
     }
   }
 }
 
 bool Environment::canDescend(unsigned MaxDepth,
                              const DeclContext *Callee) const {
   return CallStack.size() <= MaxDepth && !llvm::is_contained(CallStack, Callee);
 }
Env.pushCallInternal(Call->getDirectCallee(),
                        llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
 
   return Env;
 }
 
 Environment Environment::pushCall(const CXXConstructExpr *Call) const {
   Environment Env(*this);
 
-  Env.ThisPointeeLoc = &cast<AggregateStorageLocation>(
-      Env.createStorageLocation(Call->getType()));
-  if (Value *Val = Env.createValue(Call->getType()))
-    Env.setValue(*Env.ThisPointeeLoc, *Val);
+  Env.ThisPointeeLoc = &Env.getResultObjectLocation(*Call);
 
   Env.pushCallInternal(Call->getConstructor(),
                        llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
 
   return Env;
 }
 
 void Environment::pushCallInternal(const FunctionDecl *FuncDecl,
void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) {
   // We ignore `DACtx` because it's already the same in both. We don't want the
   // callee's `DeclCtx`, `ReturnVal`, `ReturnLoc` or `ThisPointeeLoc`. We don't
   // bring back `DeclToLoc` and `ExprToLoc` because we want to be able to later
   // analyze the same callee in a different context, and `setStorageLocation`
   // requires there to not already be a storage location assigned. Conceptually,
   // these maps capture information from the local scope, so when popping that
   // scope, we do not propagate the maps.
   this->LocToVal = std::move(CalleeEnv.LocToVal);
-  this->MemberLocToStruct = std::move(CalleeEnv.MemberLocToStruct);
   this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken);
 
   if (Call->isGLValue()) {
     if (CalleeEnv.ReturnLoc != nullptr)
       setStorageLocationStrict(*Call, *CalleeEnv.ReturnLoc);
   } else if (!Call->getType()->isVoidType()) {
     if (CalleeEnv.ReturnVal != nullptr)
       setValueStrict(*Call, *CalleeEnv.ReturnVal);
   }
 }
 
 void Environment::popCall(const CXXConstructExpr *Call,
                           const Environment &CalleeEnv) {
   // See also comment in `popCall(const CallExpr *, const Environment &)` above.
   this->LocToVal = std::move(CalleeEnv.LocToVal);
-  this->MemberLocToStruct = std::move(CalleeEnv.MemberLocToStruct);
   this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken);
 
   if (Value *Val = CalleeEnv.getValue(*CalleeEnv.ThisPointeeLoc)) {
     setValueStrict(*Call, *Val);
   }
 }
 
 bool Environment::equivalentTo(const Environment &Other,
LatticeJoinEffect Environment::widen(const Environment &PrevEnv,
   auto Effect = LatticeJoinEffect::Unchanged;
 
   // By the API, `PrevEnv` is a previous version of the environment for the same
   // block, so we have some guarantees about its shape. In particular, it will
   // be the result of a join or widen operation on previous values for this
   // block. For `DeclToLoc` and `ExprToLoc`, join guarantees that these maps are
   // subsets of the maps in `PrevEnv`. So, as long as we maintain this property
   // here, we don't need change their current values to widen.
-  //
-  // FIXME: `MemberLocToStruct` does not share the above property, because
-  // `join` can cause the map size to increase (when we add fresh data in places
-  // of conflict). Once this issue with join is resolved, re-enable the
-  // assertion below or replace with something that captures the desired
-  // invariant.
   assert(DeclToLoc.size() <= PrevEnv.DeclToLoc.size());
   assert(ExprToLoc.size() <= PrevEnv.ExprToLoc.size());
-  // assert(MemberLocToStruct.size() <= PrevEnv.MemberLocToStruct.size());
 
   llvm::MapVector<const StorageLocation *, Value *> WidenedLocToVal;
   for (auto &Entry : LocToVal) {
     const StorageLocation *Loc = Entry.first;
     assert(Loc != nullptr);
 
     Value *Val = Entry.second;
     assert(Val != nullptr);
for (auto &Entry : LocToVal) {
 
     Value &WidenedVal = widenDistinctValues(Loc->getType(), *PrevIt->second,
                                             PrevEnv, *Val, *this, Model);
     WidenedLocToVal.insert({Loc, &WidenedVal});
     if (&WidenedVal != PrevIt->second)
       Effect = LatticeJoinEffect::Changed;
   }
   LocToVal = std::move(WidenedLocToVal);
-  // FIXME: update the equivalence calculation for `MemberLocToStruct`, once we
-  // have a systematic way of soundly comparing this map.
   if (DeclToLoc.size() != PrevEnv.DeclToLoc.size() ||
       ExprToLoc.size() != PrevEnv.ExprToLoc.size() ||
-      LocToVal.size() != PrevEnv.LocToVal.size() ||
-      MemberLocToStruct.size() != PrevEnv.MemberLocToStruct.size())
+      LocToVal.size() != PrevEnv.LocToVal.size())
     Effect = LatticeJoinEffect::Changed;
 
   return Effect;
 }
 
 Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
                               Environment::ValueModel &Model) {
   assert(EnvA.DACtx == EnvB.DACtx);
Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
   // FIXME: Once we're able to remove declarations from `DeclToLoc` when their
   // lifetime ends, add an assertion that there aren't any entries in
   // `DeclToLoc` and `Other.DeclToLoc` that map the same declaration to
   // different storage locations.
   JoinedEnv.DeclToLoc = intersectDenseMaps(EnvA.DeclToLoc, EnvB.DeclToLoc);
 
   JoinedEnv.ExprToLoc = intersectDenseMaps(EnvA.ExprToLoc, EnvB.ExprToLoc);
 
-  JoinedEnv.MemberLocToStruct =
-      intersectDenseMaps(EnvA.MemberLocToStruct, EnvB.MemberLocToStruct);
-
   // FIXME: update join to detect backedges and simplify the flow condition
   // accordingly.
   JoinedEnv.FlowConditionToken = EnvA.DACtx->joinFlowConditions(
       EnvA.FlowConditionToken, EnvB.FlowConditionToken);
 
   for (auto &Entry : EnvA.LocToVal) {
     const StorageLocation *Loc = Entry.first;
     assert(Loc != nullptr);
StorageLocation *Environment::getStorageLocationStrict(const Expr &E) const {
 
   return Loc;
 }
 
 AggregateStorageLocation *Environment::getThisPointeeStorageLocation() const {
   return ThisPointeeLoc;
 }
 
+AggregateStorageLocation &
+Environment::getResultObjectLocation(const Expr &RecordPRValue) {
+  assert(RecordPRValue.getType()->isRecordType());
+  assert(RecordPRValue.isPRValue());
+
+  if (StorageLocation *ExistingLoc =
+          getStorageLocation(RecordPRValue, SkipPast::None))
+    return *cast<AggregateStorageLocation>(ExistingLoc);
+  auto &Loc = cast<AggregateStorageLocation>(
+      DACtx->getStableStorageLocation(RecordPRValue));
+  setStorageLocation(RecordPRValue, Loc);
+  return Loc;
+}
+
 PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) {
   return DACtx->getOrCreateNullPointerValue(PointeeType);
 }
 
 void Environment::setValue(const StorageLocation &Loc, Value &Val) {
-  LocToVal[&Loc] = &Val;
-
-  if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
-    auto &AggregateLoc = *cast<AggregateStorageLocation>(&Loc);
-
-    const QualType Type = AggregateLoc.getType();
-    assert(Type->isRecordType());
-
-    for (const FieldDecl *Field : DACtx->getModeledFields(Type)) {
-      assert(Field != nullptr);
-      StorageLocation &FieldLoc = AggregateLoc.getChild(*Field);
-      MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field);
-      if (auto *FieldVal = StructVal->getChild(*Field))
-        setValue(FieldLoc, *FieldVal);
-    }
-  }
-
-  auto It = MemberLocToStruct.find(&Loc);
-  if (It != MemberLocToStruct.end()) {
-    // `Loc` is the location of a struct member so we need to also update the
-    // value of the member in the corresponding `StructValue`.
-
-    assert(It->second.first != nullptr);
-    StructValue &StructVal = *It->second.first;
-
-    assert(It->second.second != nullptr);
-    const ValueDecl &Member = *It->second.second;
-
-    StructVal.setChild(Member, Val);
-  }
-}
+  assert(!isa<StructValue>(&Val) ||
+         &cast<StructValue>(&Val)->getAggregateLoc() == &Loc);
 
-void Environment::clearValue(const StorageLocation &Loc) {
-  LocToVal.erase(&Loc);
-
-  if (auto It = MemberLocToStruct.find(&Loc); It != MemberLocToStruct.end()) {
-    // `Loc` is the location of a struct member so we need to also clear the
-    // member in the corresponding `StructValue`.
-
-    assert(It->second.first != nullptr);
-    StructValue &StructVal = *It->second.first;
-
-    assert(It->second.second != nullptr);
-    const ValueDecl &Member = *It->second.second;
-
-    StructVal.clearChild(Member);
-  }
+  LocToVal[&Loc] = &Val;
 }
 
 void Environment::setValueStrict(const Expr &E, Value &Val) {
   assert(E.isPRValue());
   assert(!isa<ReferenceValue>(Val));
 
+  if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
+    if (auto *ExistingVal = cast_or_null<StructValue>(getValueStrict(E)))
+      assert(&ExistingVal->getAggregateLoc() == &StructVal->getAggregateLoc());
+    if (StorageLocation *ExistingLoc = getStorageLocation(E, SkipPast::None))
+      assert(ExistingLoc == &StructVal->getAggregateLoc());
+    else
+      setStorageLocation(E, StructVal->getAggregateLoc());
+    setValue(StructVal->getAggregateLoc(), Val);
+    return;
+  }
+
   StorageLocation *Loc = getStorageLocation(E, SkipPast::None);
   if (Loc == nullptr) {
     Loc = &createStorageLocation(E);
     setStorageLocation(E, *Loc);
   }
   setValue(*Loc, Val);
 }
 
if (Type->isIntegerType()) {
     // prevent convergence.
     CreatedValuesCount++;
     return &arena().create<IntegerValue>();
   }
 
   if (Type->isReferenceType() || Type->isPointerType()) {
     CreatedValuesCount++;
     QualType PointeeType = Type->getPointeeType();
-    auto &PointeeLoc = createStorageLocation(PointeeType);
-
-    if (Visited.insert(PointeeType.getCanonicalType()).second) {
-      Value *PointeeVal = createValueUnlessSelfReferential(
-          PointeeType, Visited, Depth, CreatedValuesCount);
-      Visited.erase(PointeeType.getCanonicalType());
-
-      if (PointeeVal != nullptr)
-        setValue(PointeeLoc, *PointeeVal);
-    }
+    StorageLocation &PointeeLoc =
+        createLocAndMaybeValue(PointeeType, Visited, Depth, CreatedValuesCount);
 
     if (Type->isReferenceType())
       return &arena().create<ReferenceValue>(PointeeLoc);
     else
       return &arena().create<PointerValue>(PointeeLoc);
   }
 
   if (Type->isRecordType()) {
     CreatedValuesCount++;
-    llvm::DenseMap<const ValueDecl *, Value *> FieldValues;
+    llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
     for (const FieldDecl *Field : DACtx->getModeledFields(Type)) {
       assert(Field != nullptr);
 
       QualType FieldType = Field->getType();
-      if (Visited.contains(FieldType.getCanonicalType()))
-        continue;
 
-      Visited.insert(FieldType.getCanonicalType());
-      if (auto *FieldValue = createValueUnlessSelfReferential(
-              FieldType, Visited, Depth + 1, CreatedValuesCount))
-        FieldValues.insert({Field, FieldValue});
-      Visited.erase(FieldType.getCanonicalType());
+      FieldLocs.insert(
+          {Field, &createLocAndMaybeValue(FieldType, Visited, Depth + 1,
+                                          CreatedValuesCount)});
     }
 
-    return &arena().create<StructValue>(std::move(FieldValues));
+    AggregateStorageLocation &Loc =
+        arena().create<AggregateStorageLocation>(Type, std::move(FieldLocs));
+    StructValue &StructVal = create<StructValue>(Loc);
+
+    // As we already have a storage location for the `StructValue`, we can and
+    // should associate them in the environment.
+    setValue(Loc, StructVal);
+
+    return &StructVal;
   }
 
   return nullptr;
 }
 
+StorageLocation &
+Environment::createLocAndMaybeValue(QualType Ty,
+                                    llvm::DenseSet<QualType> &Visited,
+                                    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());
+
+  Ty = Ty.getNonReferenceType();
+
+  if (Val == nullptr)
+    return createStorageLocation(Ty);
+
+  if (Ty->isRecordType())
+    return cast<StructValue>(Val)->getAggregateLoc();
+
+  StorageLocation &Loc = createStorageLocation(Ty);
+  setValue(Loc, *Val);
+  return Loc;
+}
+
 StorageLocation &Environment::createObjectInternal(const VarDecl *D,
                                                    QualType Ty,
                                                    const Expr *InitExpr) {
   if (Ty->isReferenceType()) {
     // Although variables of reference type always need to be initialized, it
     // can happen that we can't see the initializer, so `InitExpr` may still
     // be null.
     if (InitExpr) {
if (InitExpr)
     // FIXME. If and when we interpret all language cases, change this to
     // assert that `InitExpr` is interpreted, rather than supplying a
     // default value (assuming we don't update the environment API to return
     // references).
     Val = getValueStrict(*InitExpr);
   if (!Val)
     Val = createValue(Ty);
 
+  if (Ty->isRecordType())
+    return cast<StructValue>(Val)->getAggregateLoc();
+
   StorageLocation &Loc =
       D ? createStorageLocation(*D) : createStorageLocation(Ty);
 
   if (Val)
     setValue(Loc, *Val);
 
   return Loc;
 }
std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD) {
   llvm::copy_if(
       RD->fields(), std::back_inserter(Fields),
       [](const FieldDecl *Field) { return !Field->isUnnamedBitfield(); });
   return Fields;
 }
 
 StructValue &refreshStructValue(AggregateStorageLocation &Loc,
                                 Environment &Env) {
-  auto &NewVal = *cast<StructValue>(Env.createValue(Loc.getType()));
+  auto &NewVal = Env.create<StructValue>(Loc);
   Env.setValue(Loc, NewVal);
   return NewVal;
 }
 
 StructValue &refreshStructValue(const Expr &Expr, Environment &Env) {
   assert(Expr.getType()->isRecordType());
 
-  auto &NewVal = *cast<StructValue>(Env.createValue(Expr.getType()));
-
   if (Expr.isPRValue()) {
+    if (auto *ExistingVal =
+            cast_or_null<StructValue>(Env.getValueStrict(Expr))) {
+      auto &NewVal = Env.create<StructValue>(ExistingVal->getAggregateLoc());
       Env.setValueStrict(Expr, NewVal);
-  } else {
-    StorageLocation *Loc = Env.getStorageLocationStrict(Expr);
-    if (Loc == nullptr) {
-      Loc = &Env.createStorageLocation(Expr);
-      Env.setStorageLocation(Expr, *Loc);
+      return NewVal;
     }
+
+    auto &NewVal = *cast<StructValue>(Env.createValue(Expr.getType()));
+    Env.setValueStrict(Expr, NewVal);
+    return NewVal;
+  }
+
+  if (auto *Loc = cast_or_null<AggregateStorageLocation>(
+          Env.getStorageLocationStrict(Expr))) {
+    auto &NewVal = Env.create<StructValue>(*Loc);
     Env.setValue(*Loc, NewVal);
+    return NewVal;
   }
 
+  auto &NewVal = *cast<StructValue>(Env.createValue(Expr.getType()));
+  Env.setStorageLocationStrict(Expr, NewVal.getAggregateLoc());
   return NewVal;
 }
 
 } // namespace dataflow
 } // namespace clang

clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp

case Value::Kind::Reference:
       JOS.attributeObject(
           "referent", [&] { dump(cast<ReferenceValue>(V).getReferentLoc()); });
       break;
     case Value::Kind::Pointer:
       JOS.attributeObject(
           "pointee", [&] { dump(cast<PointerValue>(V).getPointeeLoc()); });
       break;
     case Value::Kind::Struct:
-      for (const auto &Child : cast<StructValue>(V).children())
-        JOS.attributeObject("f:" + Child.first->getNameAsString(),
-                            [&] { dump(*Child.second); });
+      for (const auto &Child :
+           cast<StructValue>(V).getAggregateLoc().children())
+        JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {
+          if (Child.second)
+            if (Value *Val = Env.getValue(*Child.second))
+              dump(*Val);
+        });
       break;
     }
 
     for (const auto& Prop : V.properties())
       JOS.attributeObject(("p:" + Prop.first()).str(),
                           [&] { dump(*Prop.second); });
 
     // Running the SAT solver is expensive, but knowing which booleans are

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(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);
+  auto &OptionalVal = Env.create<StructValue>(Loc);
   Env.setValue(Loc, OptionalVal);
+  setHasValue(OptionalVal, HasValueVal);
   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 =
StorageLocation *maybeInitializeOptionalValueMember(QualType Q,
   // The "value" property represents a synthetic field. As such, it needs
   // `StorageLocation`, like normal fields (and other variables). So, we model
   // it with a `PointerValue`, 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 *ValuePtr = clang::cast<PointerValue>(ValueProp);
     auto &ValueLoc = ValuePtr->getPointeeLoc();
-    if (Env.getValue(ValueLoc) == nullptr) {
-      // The property was previously set, but the value has been lost. This can
-      // happen in various situations, for example:
-      // - Because of an environment merge (where the two environments mapped
-      //   the property to different values, which resulted in them both being
-      //   discarded).
-      // - When two blocks in the CFG, with neither a dominator of the other,
-      //   visit the same optional value. (FIXME: This is something we can and
-      //   should fix -- see also the lengthy FIXME below.)
-      // - 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
-      // between branches and the like. Practically, this lack of sharing
-      // reduces the precision of the model when the contents are relevant to
-      // the check, like another optional or a boolean that influences control
-      // flow.
+    if (Env.getValue(ValueLoc) != nullptr)
+      return &ValueLoc;
+
+    // - Because of an environment merge (where the two environments mapped the
+    //   property to different values, which resulted in them both being
+    if (ValueLoc.getType()->isRecordType()) {
+      refreshStructValue(cast<AggregateStorageLocation>(ValueLoc), Env);
+      return &ValueLoc;
+    } else {
       auto *ValueVal = Env.createValue(ValueLoc.getType());
       if (ValueVal == nullptr)
         return nullptr;
       Env.setValue(ValueLoc, *ValueVal);
-    }
       return &ValueLoc;
     }
+  }
 
   auto Ty = Q.getNonReferenceType();
-  auto *ValueVal = Env.createValue(Ty);
-  if (ValueVal == nullptr)
-    return nullptr;
-  auto &ValueLoc = Env.createStorageLocation(Ty);
-  Env.setValue(ValueLoc, *ValueVal);
+  auto &ValueLoc = Env.createObject(Ty);
   auto &ValuePtr = Env.create<PointerValue>(ValueLoc);
   // FIXME:
   // The change we make to the `value` property below may become visible to
   // other blocks that aren't successors of the current block and therefore
   // don't see the change we made above mapping `ValueLoc` to `ValueVal`. For
   // example:
   //
   //   void target(optional<int> oo, bool b) {
if (auto *Loc = maybeInitializeOptionalValueMember(
                                State.Env.create<PointerValue>(*Loc));
     }
   }
 }
 
 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.getBoolLiteralValue(true), State.Env));
+  createOptionalValue(State.Env.getResultObjectLocation(*E),
+                      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))) {
transferValueOrImpl(ComparisonExpr, Result, State,
                         auto &A = Env.arena();
                         // We know that if `(opt.value_or(X) != X)` then
                         // `opt.hasValue()`, even without knowing further
                         // details about the contents of `opt`.
                         return A.makeImplies(ExprVal, HasValueVal);
                       });
 }
 
 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.makeAtomicBoolValue(), State.Env));
+  AggregateStorageLocation *Loc = nullptr;
+  if (E->isPRValue()) {
+    Loc = &State.Env.getResultObjectLocation(*E);
+  } else {
+    Loc = &cast<AggregateStorageLocation>(State.Env.createStorageLocation(*E));
+    State.Env.setStorageLocationStrict(*E, *Loc);
+  }
+
+  createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
 }
 
 void constructOptionalValue(const Expr &E, Environment &Env,
                             BoolValue &HasValueVal) {
-  Env.setValueStrict(E, createOptionalValue(HasValueVal, Env));
+  AggregateStorageLocation &Loc = Env.getResultObjectLocation(E);
+  Env.setValueStrict(E, createOptionalValue(Loc, 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) {
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.makeTopBoolValue(), CurrentEnv);
+    return &createOptionalValue(cast<StructValue>(Current).getAggregateLoc(),
+                                CurrentEnv.makeTopBoolValue(), CurrentEnv);
   case ComparisonResult::Unknown:
     return nullptr;
   }
   llvm_unreachable("all cases covered in switch");
 }
 
 UncheckedOptionalAccessDiagnoser::UncheckedOptionalAccessDiagnoser(
     UncheckedOptionalAccessModelOptions Options)

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

if (Dst.getType().getCanonicalType().getUnqualifiedType() !=
       llvm::dbgs() << "Source type " << Src.getType() << "\n";
       llvm::dbgs() << "Destination type " << Dst.getType() << "\n";
     }
   });
   assert(Dst.getType().getCanonicalType().getUnqualifiedType() ==
          Src.getType().getCanonicalType().getUnqualifiedType());
 
   for (auto [Field, SrcFieldLoc] : Src.children()) {
-    assert(SrcFieldLoc != nullptr);
+    StorageLocation *DstFieldLoc = Dst.getChild(*Field);
 
-    StorageLocation &DstFieldLoc = Dst.getChild(*Field);
+    assert(Field->getType()->isReferenceType() ||
+           (SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
 
     if (Field->getType()->isRecordType()) {
       copyRecord(cast<AggregateStorageLocation>(*SrcFieldLoc),
-                 cast<AggregateStorageLocation>(DstFieldLoc), Env);
+                 cast<AggregateStorageLocation>(*DstFieldLoc), Env);
+    } else if (Field->getType()->isReferenceType()) {
+      Dst.setChild(*Field, SrcFieldLoc);
     } else {
       if (Value *Val = Env.getValue(*SrcFieldLoc))
-        Env.setValue(DstFieldLoc, *Val);
+        Env.setValue(*DstFieldLoc, *Val);
       else
-        Env.clearValue(DstFieldLoc);
+        Env.clearValue(*DstFieldLoc);
     }
   }
 
   StructValue *SrcVal = cast_or_null<StructValue>(Env.getValue(Src));
   StructValue *DstVal = cast_or_null<StructValue>(Env.getValue(Dst));
 
-  if (SrcVal == nullptr || DstVal == nullptr)
-    return;
-
-  auto DstChildren = DstVal->children();
-  DstVal = &Env.create<StructValue>(llvm::DenseMap<const ValueDecl *, Value *>(
-      DstChildren.begin(), DstChildren.end()));
+  DstVal = &Env.create<StructValue>(Dst);
   Env.setValue(Dst, *DstVal);
 
+  if (SrcVal == nullptr)
+    return;
+
   for (const auto &[Name, Value] : SrcVal->properties()) {
     if (Value != nullptr)
       DstVal->setProperty(Name, *Value);
   }
 }
 
 bool clang::dataflow::recordsEqual(const AggregateStorageLocation &Loc1,
                                    const Environment &Env1,
                                    const AggregateStorageLocation &Loc2,
                                    const Environment &Env2) {
   LLVM_DEBUG({
     if (Loc2.getType().getCanonicalType().getUnqualifiedType() !=
         Loc1.getType().getCanonicalType().getUnqualifiedType()) {
       llvm::dbgs() << "Loc1 type " << Loc1.getType() << "\n";
       llvm::dbgs() << "Loc2 type " << Loc2.getType() << "\n";
     }
   });
   assert(Loc2.getType().getCanonicalType().getUnqualifiedType() ==
          Loc1.getType().getCanonicalType().getUnqualifiedType());
 
   for (auto [Field, FieldLoc1] : Loc1.children()) {
-    assert(FieldLoc1 != nullptr);
+    StorageLocation *FieldLoc2 = Loc2.getChild(*Field);
 
-    StorageLocation &FieldLoc2 = Loc2.getChild(*Field);
+    assert(Field->getType()->isReferenceType() ||
+           (FieldLoc1 != nullptr && FieldLoc2 != nullptr));
 
     if (Field->getType()->isRecordType()) {
       if (!recordsEqual(cast<AggregateStorageLocation>(*FieldLoc1), Env1,
-                        cast<AggregateStorageLocation>(FieldLoc2), Env2))
+                        cast<AggregateStorageLocation>(*FieldLoc2), Env2))
         return false;
     } else if (Field->getType()->isReferenceType()) {
-      auto *RefVal1 = cast_or_null<ReferenceValue>(Env1.getValue(*FieldLoc1));
-      auto *RefVal2 = cast_or_null<ReferenceValue>(Env1.getValue(FieldLoc2));
-      if (RefVal1 && RefVal2) {
-        if (&RefVal1->getReferentLoc() != &RefVal2->getReferentLoc())
-          return false;
-      } else {
-        // If either of `RefVal1` and `RefVal2` is null, we only consider them
-        // equal if they're both null.
-        if (RefVal1 || RefVal2)
-          return false;
-      }
-    } else {
-      if (Env1.getValue(*FieldLoc1) != Env2.getValue(FieldLoc2))
-        return false;
+      if (FieldLoc1 != FieldLoc2)
+    } else if (Env1.getValue(*FieldLoc1) != Env2.getValue(*FieldLoc2)) {
     }
   }
 
   llvm::StringMap<Value *> Props1, Props2;
 
   if (StructValue *Val1 = cast_or_null<StructValue>(Env1.getValue(Loc1)))
     for (const auto &[Name, Value] : Val1->properties())
       Props1[Name] = Value;

clang/lib/Analysis/FlowSensitive/Transfer.cpp

if (auto *D = dyn_cast<VarDecl>(Member)) {
         return;
       }
     }
 
     AggregateStorageLocation *BaseLoc = getBaseObjectLocation(*S, Env);
     if (BaseLoc == nullptr)
       return;
 
-    auto &MemberLoc = BaseLoc->getChild(*Member);
-    if (MemberLoc.getType()->isReferenceType()) {
-      // Based on its type, `MemberLoc` must be mapped either to nothing or to a
-      // `ReferenceValue`. For the former, we won't set a storage location for
-      // this expression, so as to maintain an invariant lvalue expressions;
-      // namely, that their location maps to a `ReferenceValue`.  In this,
-      // lvalues are unlike other expressions, where it is valid for their
-      // location to map to nothing (because they are not modeled).
-      //
-      // Note: we need this invariant for lvalues so that, when accessing a
-      // value, we can distinguish an rvalue from an lvalue. An alternative
-      // design, which takes the expression's value category into account, would
-      // avoid the need for this invariant.
-      if (auto *V = Env.getValue(MemberLoc)) {
-        assert(isa<ReferenceValue>(V) &&
-               "reference-typed declarations map to `ReferenceValue`s");
-        Env.setStorageLocation(*S, MemberLoc);
-      }
-    } else {
-      auto &Loc = Env.createStorageLocation(*S);
-      Env.setStorageLocation(*S, Loc);
-      Env.setValue(Loc, Env.create<ReferenceValue>(MemberLoc));
-    }
+    auto *MemberLoc = BaseLoc->getChild(*Member);
+    if (MemberLoc == nullptr)
+      return;
+    Env.setStorageLocationStrict(*S, *MemberLoc);
   }
 
   void VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *S) {
     const Expr *InitExpr = S->getExpr();
     assert(InitExpr != nullptr);
     propagateValueOrStorageLocation(*InitExpr, *S, Env);
   }
 
if (ConstructorDecl->isCopyOrMoveConstructor()) {
 
       auto *ArgLoc = cast_or_null<AggregateStorageLocation>(
           Env.getStorageLocation(*Arg, SkipPast::Reference));
       if (ArgLoc == nullptr)
         return;
 
       if (S->isElidable()) {
         Env.setStorageLocation(*S, *ArgLoc);
-      } else {
-        auto &Loc =
-            cast<AggregateStorageLocation>(Env.createStorageLocation(*S));
-        Env.setStorageLocation(*S, Loc);
-        if (Value *Val = Env.createValue(S->getType())) {
-          Env.setValue(Loc, *Val);
-          copyRecord(*ArgLoc, Loc, Env);
+      } else if (auto *ArgVal = cast_or_null<StructValue>(
+                     Env.getValue(*Arg, SkipPast::Reference))) {
+        if (auto *Val = cast<StructValue>(Env.createValue(S->getType()))) {
+          Env.setValueStrict(*S, *Val);
+          copyRecord(ArgVal->getAggregateLoc(), Val->getAggregateLoc(), Env);
         }
       }
       return;
     }
 
-    auto &Loc = Env.createStorageLocation(*S);
-    Env.setStorageLocation(*S, Loc);
-    if (Value *Val = Env.createValue(S->getType()))
-      Env.setValue(Loc, *Val);
+    auto &InitialVal = *cast<StructValue>(Env.createValue(S->getType()));
+    copyRecord(InitialVal.getAggregateLoc(), Env.getResultObjectLocation(*S),
+               Env);
 
     transferInlineCall(S, ConstructorDecl);
   }
 
   void VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *S) {
     if (S->getOperator() == OO_Equal) {
       assert(S->getNumArgs() == 2);
 
       const Expr *Arg0 = S->getArg(0);
       assert(Arg0 != nullptr);
 
       const Expr *Arg1 = S->getArg(1);
       assert(Arg1 != nullptr);
 
       // Evaluate only copy and move assignment operators.
       const auto *Method =
           dyn_cast_or_null<CXXMethodDecl>(S->getDirectCallee());
       if (!Method)
         return;
       if (!Method->isCopyAssignmentOperator() &&
           !Method->isMoveAssignmentOperator())
         return;
 
-      auto *ObjectLoc = cast_or_null<AggregateStorageLocation>(
-          Env.getStorageLocation(*Arg0, SkipPast::Reference));
-      if (ObjectLoc == nullptr)
-        return;
-
-      auto *ArgLoc = cast_or_null<AggregateStorageLocation>(
-          Env.getStorageLocation(*Arg1, SkipPast::Reference));
-      if (ArgLoc == nullptr)
-        return;
-
-      copyRecord(*ArgLoc, *ObjectLoc, Env);
-
-      // FIXME: Add a test for the value of the whole expression.
-      // Assign a storage location for the whole expression.
-      Env.setStorageLocation(*S, *ObjectLoc);
+      auto *LocSrc = cast_or_null<AggregateStorageLocation>(
+          Env.getStorageLocationStrict(*Arg1));
+      auto *LocDst = cast_or_null<AggregateStorageLocation>(
+          Env.getStorageLocationStrict(*Arg0));
+
+      if (LocSrc != nullptr && LocDst != nullptr) {
+        copyRecord(*LocSrc, *LocDst, Env);
+        Env.setStorageLocationStrict(*S, *LocDst);
+      }
     }
   }
 
   void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *S) {
     if (S->getCastKind() == CK_ConstructorConversion) {
       const Expr *SubExpr = S->getSubExpr();
       assert(SubExpr != nullptr);
 
public:
   void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
     Value *SubExprVal = Env.getValueStrict(*SubExpr);
     if (SubExprVal == nullptr)
       return;
 
-    auto &Loc = Env.createStorageLocation(*S);
-    Env.setStorageLocationStrict(*S, Loc);
+    if (StructValue *StructVal = dyn_cast<StructValue>(SubExprVal)) {
+      Env.setStorageLocation(*S, StructVal->getAggregateLoc());
+      return;
+    }
+
+    StorageLocation &Loc = Env.createStorageLocation(*S);
     Env.setValue(Loc, *SubExprVal);
+    Env.setStorageLocation(*S, Loc);
   }
 
   void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
     propagateValue(*SubExpr, *S, Env);
   }
 
   void VisitCXXStaticCastExpr(const CXXStaticCastExpr *S) {
     if (S->getCastKind() == CK_NoOp) {
       const Expr *SubExpr = S->getSubExpr();
       assert(SubExpr != nullptr);
 
       propagateValueOrStorageLocation(*SubExpr, *S, Env);
     }
   }
 
   void VisitConditionalOperator(const ConditionalOperator *S) {
     // FIXME: Revisit this once flow conditions are added to the framework. For
     // `a = b ? c : d` we can add `b => a == c && !b => a == d` to the flow
     // condition.
-    if (S->isGLValue()) {
-      auto &Loc = Env.createStorageLocation(*S);
-      Env.setStorageLocationStrict(*S, Loc);
-      if (Value *Val = Env.createValue(S->getType()))
-        Env.setValue(Loc, *Val);
-    } else if (Value *Val = Env.createValue(S->getType())) {
+    if (S->isGLValue())
+      Env.setStorageLocationStrict(*S, Env.createObject(S->getType()));
+    else if (Value *Val = Env.createValue(S->getType()))
       Env.setValueStrict(*S, *Val);
-    }
-  }
 
   void VisitInitListExpr(const InitListExpr *S) {
     QualType Type = S->getType();
 
-    auto &Loc = Env.createStorageLocation(*S);
-    Env.setStorageLocation(*S, Loc);
+    if (!Type->isStructureOrClassType()) {
+      if (auto *Val = Env.createValue(Type))
+        Env.setValueStrict(*S, *Val);
 
-    auto *Val = Env.createValue(Type);
-    if (Val == nullptr)
       return;
+    }
 
-    Env.setValue(Loc, *Val);
-
-    if (Type->isStructureOrClassType()) {
-      std::vector<FieldDecl *> Fields =
-          getFieldsForInitListExpr(Type->getAsRecordDecl());
-      for (auto [Field, Init] : llvm::zip(Fields, S->inits())) {
-        assert(Field != nullptr);
-        assert(Init != nullptr);
+    llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
+
 
-        if (Field->getType()->isReferenceType()) {
-          if (StorageLocation *Loc = Env.getStorageLocationStrict(*Init))
-            cast<StructValue>(Val)->setChild(*Field,
-                                             Env.create<ReferenceValue>(*Loc));
-        } else if (Value *InitVal = Env.getValue(*Init, SkipPast::None))
-          cast<StructValue>(Val)->setChild(*Field, *InitVal);
-      }
+      FieldLocs.insert({Field, &Env.createObject(Field->getType(), Init)});
     }
+
+    auto &Loc =
+        Env.getDataflowAnalysisContext()
+            .arena()
+            .create<AggregateStorageLocation>(Type, std::move(FieldLocs));
+    StructValue &StructVal = Env.create<StructValue>(Loc);
+
+    Env.setValue(Loc, StructVal);
+
+    Env.setValueStrict(*S, StructVal);
+
     // FIXME: Implement array initialization.
   }
 
   void VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *S) {
     Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
   }
 
   void VisitIntegerLiteral(const IntegerLiteral *S) {

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Analysis/Analyses/PostOrderCFGView.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/DataflowLattice.h"
 #include "clang/Analysis/FlowSensitive/DataflowWorklist.h"
+#include "clang/Analysis/FlowSensitive/RecordOps.h"
 #include "clang/Analysis/FlowSensitive/Transfer.h"
 #include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
builtinTransferInitializer(const CFGInitializer &Elt,
 
   auto &Env = InputState.Env;
   auto &ThisLoc = *Env.getThisPointeeStorageLocation();
 
   if (!Init->isAnyMemberInitializer())
     // FIXME: Handle base initialization
     return;
 
-  auto *InitStmt = Init->getInit();
-  assert(InitStmt != nullptr);
+  auto *InitExpr = Init->getInit();
+  assert(InitExpr != nullptr);
 
   const FieldDecl *Member = nullptr;
+  AggregateStorageLocation *ParentLoc = &ThisLoc;
   StorageLocation *MemberLoc = nullptr;
   if (Init->isMemberInitializer()) {
     Member = Init->getMember();
-    MemberLoc = &ThisLoc.getChild(*Member);
+    MemberLoc = ThisLoc.getChild(*Member);
   } else {
     IndirectFieldDecl *IndirectField = Init->getIndirectMember();
     assert(IndirectField != nullptr);
     MemberLoc = &ThisLoc;
     for (const auto *I : IndirectField->chain()) {
       Member = cast<FieldDecl>(I);
-      MemberLoc = &cast<AggregateStorageLocation>(MemberLoc)->getChild(*Member);
+      ParentLoc = cast<AggregateStorageLocation>(MemberLoc);
+      MemberLoc = ParentLoc->getChild(*Member);
     }
   }
   assert(Member != nullptr);
   assert(MemberLoc != nullptr);
 
+  // TODO: Instead of these case distinctions, we would ideally want to be able
+  // to simply use `Environment::createObject()` here, the same way that we do
+  // this in `TransferVisitor::VisitInitListExpr()`. However, this would require
+  // us to be able to build a list of fields that we then use to initialize an
+  // `AggregateStorageLocation` -- and the problem is that, when we get here,
+  // the `AggregateStorageLocation` already exists. We should explore if there's
+  // anything that we can do to change this.
   if (Member->getType()->isReferenceType()) {
-    auto *InitStmtLoc = Env.getStorageLocationStrict(*InitStmt);
-    if (InitStmtLoc == nullptr)
+    auto *InitExprLoc = Env.getStorageLocationStrict(*InitExpr);
+    if (InitExprLoc == nullptr)
       return;
 
-    Env.setValue(*MemberLoc, Env.create<ReferenceValue>(*InitStmtLoc));
-  } else if (auto *InitStmtVal = Env.getValueStrict(*InitStmt)) {
-    Env.setValue(*MemberLoc, *InitStmtVal);
+    ParentLoc->setChild(*Member, InitExprLoc);
+  } else if (auto *InitExprVal = Env.getValueStrict(*InitExpr)) {
+    if (Member->getType()->isRecordType()) {
+      auto *InitValStruct = cast<StructValue>(InitExprVal);
+      // TODO: Rather than performing a copy here, we should really be
+      // initializing the field in place. This would require us to propagate the
+      // storage location of the field to the AST node that creates the
+      // `StructValue`.
+      copyRecord(InitValStruct->getAggregateLoc(),
+                 *cast<AggregateStorageLocation>(MemberLoc), Env);
+    } else {
+      Env.setValue(*MemberLoc, *InitExprVal);
+    }
   }
 }
 
 static void builtinTransfer(const CFGElement &Elt,
                             TypeErasedDataflowAnalysisState &State,
                             AnalysisContext &AC) {
   switch (Elt.getKind()) {
   case CFGElement::Statement:

clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp

runDataflow(
 
         const ValueDecl *OuterIntDecl = findValueDecl(ASTCtx, "outer_int");
         const ValueDecl *RefDecl = findValueDecl(ASTCtx, "ref");
         const ValueDecl *InnerDecl = findValueDecl(ASTCtx, "inner");
         const ValueDecl *InnerIntDecl = findValueDecl(ASTCtx, "inner_int");
 
         auto &S1 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s1");
         auto &S2 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s2");
-        auto &Inner1 = cast<AggregateStorageLocation>(S1.getChild(*InnerDecl));
-        auto &Inner2 = cast<AggregateStorageLocation>(S2.getChild(*InnerDecl));
+        auto &Inner1 = *cast<AggregateStorageLocation>(S1.getChild(*InnerDecl));
+        auto &Inner2 = *cast<AggregateStorageLocation>(S2.getChild(*InnerDecl));
 
         EXPECT_NE(getFieldValue(&S1, *OuterIntDecl, Env),
                   getFieldValue(&S2, *OuterIntDecl, Env));
-        EXPECT_NE(Env.getValue(S1.getChild(*RefDecl)),
-                  Env.getValue(S2.getChild(*RefDecl)));
+        EXPECT_NE(S1.getChild(*RefDecl), S2.getChild(*RefDecl));
         EXPECT_NE(getFieldValue(&Inner1, *InnerIntDecl, Env),
                   getFieldValue(&Inner2, *InnerIntDecl, Env));
 
         auto *S1Val = cast<StructValue>(Env.getValue(S1));
         auto *S2Val = cast<StructValue>(Env.getValue(S2));
         EXPECT_NE(S1Val, S2Val);
 
         S1Val->setProperty("prop", Env.getBoolLiteralValue(true));
 
         copyRecord(S1, S2, Env);
 
         EXPECT_EQ(getFieldValue(&S1, *OuterIntDecl, Env),
                   getFieldValue(&S2, *OuterIntDecl, Env));
-        EXPECT_EQ(Env.getValue(S1.getChild(*RefDecl)),
-                  Env.getValue(S2.getChild(*RefDecl)));
+        EXPECT_EQ(S1.getChild(*RefDecl), S2.getChild(*RefDecl));
         EXPECT_EQ(getFieldValue(&Inner1, *InnerIntDecl, Env),
                   getFieldValue(&Inner2, *InnerIntDecl, Env));
 
         S1Val = cast<StructValue>(Env.getValue(S1));
         S2Val = cast<StructValue>(Env.getValue(S2));
         EXPECT_NE(S1Val, S2Val);
 
         EXPECT_EQ(S2Val->getProperty("prop"), &Env.getBoolLiteralValue(true));
runDataflow(
 
         const ValueDecl *OuterIntDecl = findValueDecl(ASTCtx, "outer_int");
         const ValueDecl *RefDecl = findValueDecl(ASTCtx, "ref");
         const ValueDecl *InnerDecl = findValueDecl(ASTCtx, "inner");
         const ValueDecl *InnerIntDecl = findValueDecl(ASTCtx, "inner_int");
 
         auto &S1 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s1");
         auto &S2 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s2");
-        auto &Inner2 = cast<AggregateStorageLocation>(S2.getChild(*InnerDecl));
+        auto &Inner2 = *cast<AggregateStorageLocation>(S2.getChild(*InnerDecl));
 
         cast<StructValue>(Env.getValue(S1))
             ->setProperty("prop", Env.getBoolLiteralValue(true));
 
         // Strategy: Create two equal records, then verify each of the various
         // ways in which records can differ causes recordsEqual to return false.
         // changes we can make to the record.
 
         // This test reuses the same objects for multiple checks, which isn't
         // great, but seems better than duplicating the setup code for every
         // check.
 
         copyRecord(S1, S2, Env);
         EXPECT_TRUE(recordsEqual(S1, S2, Env));
 
         // S2 has a different outer_int.
-        Env.setValue(S2.getChild(*OuterIntDecl), Env.create<IntegerValue>());
+        Env.setValue(*S2.getChild(*OuterIntDecl), Env.create<IntegerValue>());
         EXPECT_FALSE(recordsEqual(S1, S2, Env));
         copyRecord(S1, S2, Env);
         EXPECT_TRUE(recordsEqual(S1, S2, Env));
 
         // S2 doesn't have outer_int at all.
-        Env.clearValue(S2.getChild(*OuterIntDecl));
+        Env.clearValue(*S2.getChild(*OuterIntDecl));
         EXPECT_FALSE(recordsEqual(S1, S2, Env));
         copyRecord(S1, S2, Env);
         EXPECT_TRUE(recordsEqual(S1, S2, Env));
 
         // S2 has a different ref.
-        Env.setValue(S2.getChild(*RefDecl),
-                     Env.create<ReferenceValue>(Env.createStorageLocation(
-                         RefDecl->getType().getNonReferenceType())));
-        EXPECT_FALSE(recordsEqual(S1, S2, Env));
-        copyRecord(S1, S2, Env);
-        EXPECT_TRUE(recordsEqual(S1, S2, Env));
-
-        // S2 doesn't have ref at all.
-        Env.clearValue(S2.getChild(*RefDecl));
+        S2.setChild(*RefDecl, &Env.createStorageLocation(
+                                  RefDecl->getType().getNonReferenceType()));
         EXPECT_FALSE(recordsEqual(S1, S2, Env));
         copyRecord(S1, S2, Env);
         EXPECT_TRUE(recordsEqual(S1, S2, Env));
 
         // S2 as a different inner_int.
-        Env.setValue(Inner2.getChild(*InnerIntDecl),
+        Env.setValue(*Inner2.getChild(*InnerIntDecl),
                      Env.create<IntegerValue>());
         EXPECT_FALSE(recordsEqual(S1, S2, Env));
         copyRecord(S1, S2, Env);
         EXPECT_TRUE(recordsEqual(S1, S2, Env));
 
         // S1 and S2 have the same property with different values.
         cast<StructValue>(Env.getValue(S2))
             ->setProperty("prop", Env.getBoolLiteralValue(false));

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

 }
 
 /// Returns the value of a `Field` on the record referenced by `Loc.`
 /// Returns null if `Loc` is null.
 inline Value *getFieldValue(const AggregateStorageLocation *Loc,
                             const ValueDecl &Field, const Environment &Env) {
   if (Loc == nullptr)
     return nullptr;
-  return Env.getValue(Loc->getChild(Field));
+  StorageLocation *FieldLoc = Loc->getChild(Field);
+  if (FieldLoc == nullptr)
+    return nullptr;
+  return Env.getValue(*FieldLoc);
 }
 
 /// Returns the value of a `Field` on a `Struct.
 /// Returns null if `Struct` is null.
 ///
 /// Note: This function currently does not use the `Env` parameter, but it will
 /// soon be needed to look up the `Value` when `setChild()` changes to return a
 /// `StorageLocation *`.
 inline Value *getFieldValue(const StructValue *Struct, const ValueDecl &Field,
                             const Environment &Env) {
   if (Struct == nullptr)
     return nullptr;
-  return Struct->getChild(Field);
+  StorageLocation *FieldLoc = Struct->getChild(Field);
+  if (FieldLoc == nullptr)
+    return nullptr;
+  return Env.getValue(*FieldLoc);
 }
 
 /// Creates and owns constraints which are boolean values.
 class ConstraintContext {
   unsigned NextAtom = 0;
   llvm::BumpPtrAllocator A;
 
   const Formula *make(Formula::Kind K,

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

 #include "llvm/Support/Casting.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <optional>
 #include <string>
 #include <utility>
 
+// TODO: There are still remaining checks here that check for consistency
+// between `StructValue` and `AggregateStorageLocation`. Now that the redundancy
+// between these two classes has been eliminated, these checks aren't needed any
+// more, so remove them.
+
 namespace {
 
 using namespace clang;
 using namespace dataflow;
 using namespace test;
 using ::testing::Eq;
 using ::testing::IsNull;
 using ::testing::Ne;
std::string Code = R"(
     S GlobalS;
     struct A { S &Unmodeled = GlobalS; };
     struct B { A F3; };
     struct C { B F2; };
     struct D { C F1; };
 
     void target() {
       D Bar;
-      A Foo = Bar.F1.F2.F3;
+      A &Foo = Bar.F1.F2.F3;
       int Zab = Foo.Unmodeled.X;
       // [[p]]
     }
   )";
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
         ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
-        ASSERT_TRUE(FooDecl->getType()->isStructureType());
-        auto FooFields = FooDecl->getType()->getAsRecordDecl()->fields();
+        QualType FooReferentType = FooDecl->getType()->getPointeeType();
+        ASSERT_TRUE(FooReferentType->isStructureType());
+        auto FooFields = FooReferentType->getAsRecordDecl()->fields();
 
         FieldDecl *UnmodeledDecl = nullptr;
         for (FieldDecl *Field : FooFields) {
           if (Field->getNameAsString() == "Unmodeled") {
             UnmodeledDecl = Field;
           } else {
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(UnmodeledDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
-        const auto *UnmodeledLoc = &FooLoc->getChild(*UnmodeledDecl);
-        ASSERT_TRUE(isa<ScalarStorageLocation>(UnmodeledLoc));
-        ASSERT_THAT(Env.getValue(*UnmodeledLoc), IsNull());
+        const auto *UnmodeledLoc = FooLoc->getChild(*UnmodeledDecl);
+        ASSERT_TRUE(isa<AggregateStorageLocation>(UnmodeledLoc));
+        EXPECT_THAT(Env.getValue(*UnmodeledLoc), IsNull());
 
         const ValueDecl *ZabDecl = findValueDecl(ASTCtx, "Zab");
         ASSERT_THAT(ZabDecl, NotNull());
         EXPECT_THAT(Env.getValue(*ZabDecl), NotNull());
       });
 }
 
 TEST(TransferTest, StructVarDecl) {
runDataflow(
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(FooLoc->getChild(*BarDecl));
 
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal =
             cast<IntegerValue>(getFieldValue(FooVal, *BarDecl, Env));
         EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
       });
 }
 
runDataflow(
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(FooLoc->getChild(*BarDecl));
 
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal =
             cast<IntegerValue>(getFieldValue(FooVal, *BarDecl, Env));
         EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
       });
 }
 
runDataflow(
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(FooLoc->getChild(*BarDecl));
 
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal =
             cast<IntegerValue>(getFieldValue(FooVal, *BarDecl, Env));
         EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
       });
 }
 
runDataflow(Code, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>>
     }
     ASSERT_THAT(FooRefDecl, NotNull());
     ASSERT_THAT(FooPtrDecl, NotNull());
     ASSERT_THAT(BazRefDecl, NotNull());
     ASSERT_THAT(BazPtrDecl, NotNull());
 
     const auto &FooLoc =
         *cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
-    const auto &FooReferentVal = *cast<StructValue>(Env.getValue(FooLoc));
 
-    const auto &BarVal =
-        *cast<ReferenceValue>(FooReferentVal.getChild(*BarDecl));
-    const auto &BarReferentVal =
-        *cast<StructValue>(Env.getValue(BarVal.getReferentLoc()));
+    const auto &BarLoc =
+        *cast<AggregateStorageLocation>(FooLoc.getChild(*BarDecl));
 
-    const auto &FooRefVal =
-        *cast<ReferenceValue>(getFieldValue(&BarReferentVal, *FooRefDecl, Env));
     const auto &FooReferentLoc =
-        cast<AggregateStorageLocation>(FooRefVal.getReferentLoc());
+        *cast<AggregateStorageLocation>(BarLoc.getChild(*FooRefDecl));
     EXPECT_THAT(Env.getValue(FooReferentLoc), NotNull());
     EXPECT_THAT(getFieldValue(&FooReferentLoc, *BarDecl, Env), IsNull());
 
     const auto &FooPtrVal =
-        *cast<PointerValue>(getFieldValue(&BarReferentVal, *FooPtrDecl, Env));
+        *cast<PointerValue>(getFieldValue(&BarLoc, *FooPtrDecl, Env));
     const auto &FooPtrPointeeLoc =
         cast<AggregateStorageLocation>(FooPtrVal.getPointeeLoc());
     EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), NotNull());
     EXPECT_THAT(getFieldValue(&FooPtrPointeeLoc, *BarDecl, Env), IsNull());
 
-    const auto &BazRefVal =
-        *cast<ReferenceValue>(getFieldValue(&BarReferentVal, *BazRefDecl, Env));
-    const StorageLocation &BazReferentLoc = BazRefVal.getReferentLoc();
-    EXPECT_THAT(Env.getValue(BazReferentLoc), NotNull());
+    EXPECT_THAT(getFieldValue(&BarLoc, *BazRefDecl, Env), NotNull());
 
     const auto &BazPtrVal =
-        *cast<PointerValue>(getFieldValue(&BarReferentVal, *BazPtrDecl, Env));
+        *cast<PointerValue>(getFieldValue(&BarLoc, *BazPtrDecl, Env));
     const StorageLocation &BazPtrPointeeLoc = BazPtrVal.getPointeeLoc();
     EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull());
   });
 }
 
 TEST(TransferTest, PointerVarDecl) {
   std::string Code = R"(
     struct A {};
runDataflow(
         const auto &FooPointeeVal =
             *cast<StructValue>(Env.getValue(FooVal.getPointeeLoc()));
 
         const auto &BarVal =
             *cast<PointerValue>(getFieldValue(&FooPointeeVal, *BarDecl, Env));
         const auto &BarPointeeVal =
             *cast<StructValue>(Env.getValue(BarVal.getPointeeLoc()));
 
-        const auto &FooRefVal = *cast<ReferenceValue>(
-            getFieldValue(&BarPointeeVal, *FooRefDecl, Env));
-        const StorageLocation &FooReferentLoc = FooRefVal.getReferentLoc();
-        EXPECT_THAT(Env.getValue(FooReferentLoc), IsNull());
+        EXPECT_THAT(getFieldValue(&BarPointeeVal, *FooRefDecl, Env), NotNull());
 
         const auto &FooPtrVal = *cast<PointerValue>(
             getFieldValue(&BarPointeeVal, *FooPtrDecl, Env));
         const auto &FooPtrPointeeLoc =
             cast<AggregateStorageLocation>(FooPtrVal.getPointeeLoc());
         EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull());
 
-        const auto &BazRefVal = *cast<ReferenceValue>(
-            getFieldValue(&BarPointeeVal, *BazRefDecl, Env));
-        const StorageLocation &BazReferentLoc = BazRefVal.getReferentLoc();
-        EXPECT_THAT(Env.getValue(BazReferentLoc), NotNull());
+        EXPECT_THAT(getFieldValue(&BarPointeeVal, *BazRefDecl, Env), NotNull());
 
         const auto &BazPtrVal = *cast<PointerValue>(
             getFieldValue(&BarPointeeVal, *BazPtrDecl, Env));
         const StorageLocation &BazPtrPointeeLoc = BazPtrVal.getPointeeLoc();
         EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull());
       });
 }
 
TEST(TransferTest, DirectlySelfReferentialReference) {
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
         const ValueDecl *SelfDecl = findValueDecl(ASTCtx, "self");
 
         auto *ThisLoc = Env.getThisPointeeStorageLocation();
-        auto *RefVal =
-            cast<ReferenceValue>(Env.getValue(ThisLoc->getChild(*SelfDecl)));
-        ASSERT_EQ(&RefVal->getReferentLoc(), ThisLoc);
+        ASSERT_EQ(ThisLoc->getChild(*SelfDecl), ThisLoc);
       });
 }
 
 TEST(TransferTest, MultipleVarsDecl) {
   std::string Code = R"(
     void target() {
       int Foo, Bar;
       (void)0;
runDataflow(
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(FooLoc->getChild(*BarDecl));
 
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal =
             cast<IntegerValue>(getFieldValue(FooVal, *BarDecl, Env));
         EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
       });
 }
 
runDataflow(
             }
           }
         }
         ASSERT_THAT(ADefaultDecl, NotNull());
         ASSERT_THAT(AProtectedDecl, NotNull());
         ASSERT_THAT(APrivateDecl, NotNull());
         ASSERT_THAT(APublicDecl, NotNull());
 
-        const auto &FooLoc =
-            *cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
-        const auto &FooVal = *cast<StructValue>(Env.getValue(FooLoc));
-
-        // Note: we can't test presence of children in `FooLoc`, because
-        // `getChild` requires its argument be present (or fails an assert). So,
-        // we limit to testing presence in `FooVal` and coherence between the
-        // two.
-
-        // Base-class fields.
-        EXPECT_THAT(FooVal.getChild(*ADefaultDecl), NotNull());
-        EXPECT_THAT(FooVal.getChild(*APrivateDecl), NotNull());
-
-        EXPECT_THAT(FooVal.getChild(*AProtectedDecl), NotNull());
-        EXPECT_EQ(Env.getValue(FooLoc.getChild(*APublicDecl)),
-                  FooVal.getChild(*APublicDecl));
-        EXPECT_THAT(FooVal.getChild(*APublicDecl), NotNull());
-        EXPECT_EQ(Env.getValue(FooLoc.getChild(*AProtectedDecl)),
-                  FooVal.getChild(*AProtectedDecl));
-
-        // Derived-class fields.
-        EXPECT_THAT(FooVal.getChild(*BDefaultDecl), NotNull());
-        EXPECT_EQ(Env.getValue(FooLoc.getChild(*BDefaultDecl)),
-                  FooVal.getChild(*BDefaultDecl));
-        EXPECT_THAT(FooVal.getChild(*BProtectedDecl), NotNull());
-        EXPECT_EQ(Env.getValue(FooLoc.getChild(*BProtectedDecl)),
-                  FooVal.getChild(*BProtectedDecl));
-        EXPECT_THAT(FooVal.getChild(*BPrivateDecl), NotNull());
-        EXPECT_EQ(Env.getValue(FooLoc.getChild(*BPrivateDecl)),
-                  FooVal.getChild(*BPrivateDecl));
+        ASSERT_TRUE(
+            isa<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl)));
       });
 }
 
 static void derivedBaseMemberExpectations(
     const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
     ASTContext &ASTCtx) {
   ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
   const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
for (const FieldDecl *Field : BaseType->getAsRecordDecl()->fields()) {
       }
     }
   }
   ASSERT_THAT(BarDecl, NotNull());
 
   const auto &FooLoc =
       *cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
   const auto &FooVal = *cast<StructValue>(Env.getValue(FooLoc));
-  EXPECT_THAT(FooVal.getChild(*BarDecl), NotNull());
-  EXPECT_EQ(Env.getValue(FooLoc.getChild(*BarDecl)), FooVal.getChild(*BarDecl));
+  EXPECT_EQ(&FooVal.getAggregateLoc(), &FooLoc);
 }
 
 TEST(TransferTest, DerivedBaseMemberStructDefault) {
   std::string Code = R"(
     struct A {
       int Bar;
     };
     struct B : public A {
runDataflow(
           } else {
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
-        const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
-        const auto *BarVal = cast<ReferenceValue>(FooVal->getChild(*BarDecl));
         const auto *BarReferentVal =
-            cast<IntegerValue>(Env.getValue(BarVal->getReferentLoc()));
+            cast<IntegerValue>(getFieldValue(FooLoc, *BarDecl, Env));
 
         const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
         ASSERT_THAT(BazDecl, NotNull());
 
         EXPECT_EQ(Env.getValue(*BazDecl), BarReferentVal);
       });
 }
 
runDataflow(
 
         const auto *ThisLoc = Env.getThisPointeeStorageLocation();
         ASSERT_THAT(ThisLoc, NotNull());
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&ThisLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(ThisLoc->getChild(*BarDecl));
         ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(BarLoc));
 
         const Value *BarVal = Env.getValue(*BarLoc);
         ASSERT_TRUE(isa_and_nonnull<IntegerValue>(BarVal));
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
         EXPECT_EQ(Env.getValue(*FooDecl), BarVal);
runDataflow(
             BazDecl = Field;
           } else {
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(BazDecl, NotNull());
 
         const auto *QuxLoc =
-            cast<AggregateStorageLocation>(&ThisLoc->getChild(*QuxDecl));
+            cast<AggregateStorageLocation>(ThisLoc->getChild(*QuxDecl));
         const auto *QuxVal = dyn_cast<StructValue>(Env.getValue(*QuxLoc));
         ASSERT_THAT(QuxVal, NotNull());
 
         const auto *BazLoc =
-            cast<ScalarStorageLocation>(&QuxLoc->getChild(*BazDecl));
+            cast<ScalarStorageLocation>(QuxLoc->getChild(*BazDecl));
         const auto *BazVal =
             cast<IntegerValue>(getFieldValue(QuxVal, *BazDecl, Env));
         EXPECT_EQ(Env.getValue(*BazLoc), BazVal);
 
         const ValueDecl *QuuxDecl = findValueDecl(ASTCtx, "Quux");
         ASSERT_THAT(QuuxDecl, NotNull());
         EXPECT_EQ(Env.getValue(*QuuxDecl), BazVal);
       });
runDataflow(
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
 
         const auto *ThisLoc = Env.getThisPointeeStorageLocation();
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&ThisLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(ThisLoc->getChild(*BarDecl));
         ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(BarLoc));
 
         const Value *BarVal = Env.getValue(*BarLoc);
         ASSERT_TRUE(isa_and_nonnull<IntegerValue>(BarVal));
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
         EXPECT_EQ(Env.getValue(*FooDecl), BarVal);
runDataflow(
             BazDecl = Field;
           } else {
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }
         }
         ASSERT_THAT(BazDecl, NotNull());
 
         const auto *QuxLoc =
-            cast<AggregateStorageLocation>(&ThisLoc->getChild(*QuxDecl));
+            cast<AggregateStorageLocation>(ThisLoc->getChild(*QuxDecl));
         const auto *QuxVal = dyn_cast<StructValue>(Env.getValue(*QuxLoc));
         ASSERT_THAT(QuxVal, NotNull());
 
         const auto *BazLoc =
-            cast<ScalarStorageLocation>(&QuxLoc->getChild(*BazDecl));
+            cast<ScalarStorageLocation>(QuxLoc->getChild(*BazDecl));
         const auto *BazVal =
             cast<IntegerValue>(getFieldValue(QuxVal, *BazDecl, Env));
         EXPECT_EQ(Env.getValue(*BazLoc), BazVal);
 
         const ValueDecl *QuuxDecl = findValueDecl(ASTCtx, "Quux");
         ASSERT_THAT(QuuxDecl, NotNull());
         EXPECT_EQ(Env.getValue(*QuuxDecl), BazVal);
       });
runDataflow(
 
         const auto *ThisLoc = Env.getThisPointeeStorageLocation();
         ASSERT_THAT(ThisLoc, NotNull());
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
 
         const auto *FooLoc =
-            cast<ScalarStorageLocation>(&ThisLoc->getChild(*FooDecl));
+            cast<ScalarStorageLocation>(ThisLoc->getChild(*FooDecl));
         ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc));
 
         const Value *FooVal = Env.getValue(*FooLoc);
         ASSERT_TRUE(isa_and_nonnull<IntegerValue>(FooVal));
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&ThisLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(ThisLoc->getChild(*BarDecl));
         ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(BarLoc));
 
         const Value *BarVal = Env.getValue(*BarLoc);
         ASSERT_TRUE(isa_and_nonnull<IntegerValue>(BarVal));
       });
 }
 
 TEST(TransferTest, StructThisInLambda) {
runDataflow(
 
         const auto *ThisLoc = Env.getThisPointeeStorageLocation();
         ASSERT_THAT(ThisLoc, NotNull());
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&ThisLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(ThisLoc->getChild(*BarDecl));
         ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(BarLoc));
 
         const Value *BarVal = Env.getValue(*BarLoc);
         ASSERT_TRUE(isa_and_nonnull<IntegerValue>(BarVal));
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
         EXPECT_EQ(Env.getValue(*FooDecl), BarVal);
runDataflow(
 
         const auto *ThisLoc = Env.getThisPointeeStorageLocation();
         ASSERT_THAT(ThisLoc, NotNull());
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&ThisLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(ThisLoc->getChild(*BarDecl));
         ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(BarLoc));
 
         const Value *BarVal = Env.getValue(*BarLoc);
         ASSERT_TRUE(isa_and_nonnull<IntegerValue>(BarVal));
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
         EXPECT_EQ(Env.getValue(*FooDecl), BarVal);
runDataflow(
         ASSERT_THAT(FooDecl, NotNull());
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(FooLoc->getChild(*BarDecl));
 
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal =
             cast<IntegerValue>(getFieldValue(FooVal, *BarDecl, Env));
         EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
       });
 }
 
runDataflow(
         ASSERT_THAT(FooDecl, NotNull());
 
         const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooLoc =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto *BarLoc =
-            cast<ScalarStorageLocation>(&FooLoc->getChild(*BarDecl));
+            cast<ScalarStorageLocation>(FooLoc->getChild(*BarDecl));
 
         const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
         const auto *BarVal =
             cast<IntegerValue>(getFieldValue(FooVal, *BarDecl, Env));
         EXPECT_EQ(Env.getValue(*BarLoc), BarVal);
       },
       LangStandard::lang_cxx14);
 }
runDataflow(
         const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
         ASSERT_THAT(BazDecl, NotNull());
 
         const auto *FooLoc1 =
             cast<AggregateStorageLocation>(Env1.getStorageLocation(*FooDecl));
         const auto *BarLoc1 =
             cast<AggregateStorageLocation>(Env1.getStorageLocation(*BarDecl));
 
+        EXPECT_FALSE(recordsEqual(*FooLoc1, *BarLoc1, Env1));
+
         const auto *FooVal1 = cast<StructValue>(Env1.getValue(*FooLoc1));
         const auto *BarVal1 = cast<StructValue>(Env1.getValue(*BarLoc1));
         EXPECT_NE(FooVal1, BarVal1);
 
-        EXPECT_FALSE(recordsEqual(*FooLoc1, *BarLoc1, Env1));
-
         const auto *FooBazVal1 =
             cast<IntegerValue>(getFieldValue(FooLoc1, *BazDecl, Env1));
         const auto *BarBazVal1 =
             cast<IntegerValue>(getFieldValue(BarLoc1, *BazDecl, Env1));
         EXPECT_NE(FooBazVal1, BarBazVal1);
 
         const auto *FooLoc2 =
             cast<AggregateStorageLocation>(Env2.getStorageLocation(*FooDecl));
runDataflow(
          ASTContext &ASTCtx) {
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
 
         const ValueDecl *RefFieldDecl = findValueDecl(ASTCtx, "RefField");
 
         auto &ILoc = getLocForDecl<StorageLocation>(ASTCtx, Env, "i");
         auto &SLoc = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s");
 
-        auto &RefValue =
-            *cast<ReferenceValue>(getFieldValue(&SLoc, *RefFieldDecl, Env));
-        EXPECT_EQ(&RefValue.getReferentLoc(), &ILoc);
+        EXPECT_EQ(SLoc.getChild(*RefFieldDecl), &ILoc);
       });
 }
 
 TEST(TransferTest, AggregateInitialization_NotExplicitlyInitializedField) {
   std::string Code = R"(
     struct S {
       int i1;
       int i2;
runDataflow(
 
         const ValueDecl *OuterField = findValueDecl(ASTCtx, "OuterField");
         const ValueDecl *InnerField = findValueDecl(ASTCtx, "InnerField");
 
         auto &P = getValueForDecl<PointerValue>(ASTCtx, Env, "p");
 
         auto &OuterLoc = cast<AggregateStorageLocation>(P.getPointeeLoc());
         auto &OuterFieldLoc =
-            cast<AggregateStorageLocation>(OuterLoc.getChild(*OuterField));
-        auto &InnerFieldLoc = OuterFieldLoc.getChild(*InnerField);
+            *cast<AggregateStorageLocation>(OuterLoc.getChild(*OuterField));
+        auto &InnerFieldLoc = *OuterFieldLoc.getChild(*InnerField);
 
         // Values for the struct and all fields exist after the new.
         EXPECT_THAT(Env.getValue(OuterLoc), NotNull());
         EXPECT_THAT(Env.getValue(OuterFieldLoc), NotNull());
         EXPECT_THAT(Env.getValue(InnerFieldLoc), NotNull());
       });
 }
 
runDataflow(
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
         const ValueDecl *SDecl = findValueDecl(ASTCtx, "s");
         const ValueDecl *BDecl = findValueDecl(ASTCtx, "b");
         const IndirectFieldDecl *IndirectField =
             findIndirectFieldDecl(ASTCtx, "b");
 
         auto *S =
             cast<AggregateStorageLocation>(Env.getStorageLocation(*SDecl));
-        auto &AnonStruct = cast<AggregateStorageLocation>(
+        auto &AnonStruct = *cast<AggregateStorageLocation>(
             S->getChild(*cast<ValueDecl>(IndirectField->chain().front())));
 
         auto *B = cast<BoolValue>(getFieldValue(&AnonStruct, *BDecl, Env));
         ASSERT_TRUE(Env.flowConditionImplies(B->formula()));
       });
 }
 
 TEST(TransferTest, AnonymousStructWithInitializer) {
runDataflow(
          ASTContext &ASTCtx) {
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
         const ValueDecl *BDecl = findValueDecl(ASTCtx, "b");
         const IndirectFieldDecl *IndirectField =
             findIndirectFieldDecl(ASTCtx, "b");
 
         auto *ThisLoc =
             cast<AggregateStorageLocation>(Env.getThisPointeeStorageLocation());
-        auto &AnonStruct = cast<AggregateStorageLocation>(ThisLoc->getChild(
+        auto &AnonStruct = *cast<AggregateStorageLocation>(ThisLoc->getChild(
             *cast<ValueDecl>(IndirectField->chain().front())));
 
         auto *B = cast<BoolValue>(getFieldValue(&AnonStruct, *BDecl, Env));
         ASSERT_TRUE(Env.flowConditionImplies(B->formula()));
       });
 }
 
 TEST(TransferTest, AnonymousStructWithReferenceField) {
runDataflow(
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
         const ValueDecl *GlobalIDecl = findValueDecl(ASTCtx, "global_i");
         const ValueDecl *IDecl = findValueDecl(ASTCtx, "i");
         const IndirectFieldDecl *IndirectField =
             findIndirectFieldDecl(ASTCtx, "i");
 
         auto *ThisLoc =
             cast<AggregateStorageLocation>(Env.getThisPointeeStorageLocation());
-        auto &AnonStruct = cast<AggregateStorageLocation>(ThisLoc->getChild(
+        auto &AnonStruct = *cast<AggregateStorageLocation>(ThisLoc->getChild(
             *cast<ValueDecl>(IndirectField->chain().front())));
 
-        auto *RefVal =
-            cast<ReferenceValue>(Env.getValue(AnonStruct.getChild(*IDecl)));
-
-        ASSERT_EQ(&RefVal->getReferentLoc(),
+        ASSERT_EQ(AnonStruct.getChild(*IDecl),
                   Env.getStorageLocation(*GlobalIDecl));
       });
 }
 
 } // namespace