Implement canonicalization of Value and Element (#3024)

See arena.h for discussion of what canonicalization means in this
context. This is primarily intended to support implementing a memo table
of template instantiations to resolve #2951, but could be useful for
other purposes as well.

Additional changes:
- Pass `VTable` constructor parameters by pointer, to avoid the need to
define `operator==` and `hash_value` for it.
- Clean up the recurring pattern of allocating identical `NamedElement`s
on the stack and heap. Instead we always allocate it on the heap and
pass it by pointer.
- Add `Print()` and `Dump()` to `Bindings` as a debugging convenience.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>

explorer/ast/BUILD

@@ -52,6 +52,7 @@ cc_library(
         "//common:indirect_value",
         "//common:ostream",
         "//explorer/common:arena",
+        "//explorer/common:decompose",
         "//explorer/common:error_builders",
         "//explorer/common:nonnull",
         "//explorer/common:source_location",

explorer/ast/address.h

@@ -24,6 +24,13 @@ class AllocationId {
   AllocationId(const AllocationId&) = default;
   auto operator=(const AllocationId&) -> AllocationId& = default;
 
+  inline friend auto operator==(AllocationId lhs, AllocationId rhs) -> bool {
+    return lhs.index_ == rhs.index_;
+  }
+  inline friend auto hash_value(AllocationId id) {
+    return llvm::hash_combine(id.index_);
+  }
+
   // Prints a human-readable representation of *this to `out`.
   //
   // Currently that representation consists of an integer index.
@@ -81,6 +88,15 @@ class Address {
     return address;
   }
 
+  inline friend auto operator==(const Address& lhs, const Address& rhs)
+      -> bool {
+    return lhs.allocation_ == rhs.allocation_ &&
+           lhs.element_path_ == rhs.element_path_;
+  }
+  inline friend auto hash_value(const Address& a) -> llvm::hash_code {
+    return llvm::hash_combine(a.allocation_, a.element_path_);
+  }
+
  private:
   // The representation of Address describes how to locate an object within
   // the Heap, so its implementation details are tied to the implementation

explorer/ast/bindings.cpp

@@ -8,6 +8,7 @@
 #include "explorer/ast/impl_binding.h"
 #include "explorer/ast/pattern.h"
 #include "explorer/ast/value.h"
+#include "llvm/ADT/StringExtras.h"
 
 namespace Carbon {
 
@@ -41,6 +42,18 @@ auto Bindings::None() -> Nonnull<const Bindings*> {
   return bindings;
 }
 
+void Bindings::Print(llvm::raw_ostream& out) const {
+  out << "{";
+  llvm::ListSeparator sep;
+  for (const auto& [name, value] : args_) {
+    out << sep << *name << " -> " << *value;
+  }
+  for (const auto& [name, value] : witnesses_) {
+    out << sep << *name << " -> " << *value;
+  }
+  out << "}";
+}
+
 auto Bindings::SymbolicIdentity(
     Nonnull<Arena*> arena,
     llvm::ArrayRef<Nonnull<const GenericBinding*>> bindings)

explorer/ast/bindings.h

@@ -63,6 +63,9 @@ class Bindings {
     return f(args_, witnesses_);
   }
 
+  void Print(llvm::raw_ostream& out) const;
+  LLVM_DUMP_METHOD void Dump() const { Print(llvm::errs()); }
+
   // Add a value, and perhaps a witness, for a generic binding.
   void Add(Nonnull<const GenericBinding*> binding, Nonnull<const Value*> value,
            std::optional<Nonnull<const Value*>> witness);

explorer/ast/element.h

@@ -11,6 +11,7 @@
 
 #include "common/ostream.h"
 #include "explorer/ast/ast_rtti.h"
+#include "explorer/common/decompose.h"
 #include "explorer/common/nonnull.h"
 #include "llvm/ADT/PointerUnion.h"
 
@@ -20,7 +21,7 @@ class Declaration;
 class Value;
 
 // A NamedValue represents a value with a name, such as a single struct field.
-struct NamedValue {
+struct NamedValue : HashFromDecompose<NamedValue> {
   NamedValue(std::string name, Nonnull<const Value*> value)
       : name(std::move(name)), value(value) {}
 
@@ -36,14 +37,19 @@ struct NamedValue {
   Nonnull<const Value*> value;
 };
 
-// A generic member of a type.
+// A generic member of a type. This is can be a named, positional or other type
+// of member.
 //
-// This is can be a named, positional or other type of member.
+// Arena's canonicalization support is enabled for Element and all derived
+// types. As a result, all Elements must be immutable, and all their constructor
+// arguments must be copyable, equality-comparable, and hashable. See
+// Arena's documentation for details.
 class Element {
  protected:
   explicit Element(ElementKind kind) : kind_(kind) {}
 
  public:
+  using EnableCanonicalizedAllocation = void;
   virtual ~Element() = default;
 
   // Call `f` on this value, cast to its most-derived type. `R` specifies the

explorer/ast/element_path.h

@@ -50,6 +50,17 @@ class ElementPath {
               std::optional<Nonnull<const Witness*>> witness)
         : element_(element), interface_(interface), witness_(witness) {}
 
+    inline friend auto operator==(const Component& lhs, const Component& rhs)
+        -> bool {
+      return lhs.element_ == rhs.element_ && lhs.interface_ == rhs.interface_ &&
+             lhs.witness_ == rhs.witness_;
+    }
+    inline friend auto hash_value(const Component& component)
+        -> llvm::hash_code {
+      return llvm::hash_combine(component.element_, component.interface_,
+                                component.witness_);
+    }
+
     auto element() const -> Nonnull<const Element*> { return element_; }
 
     auto IsNamed(std::string_view name) const -> bool {
@@ -82,6 +93,15 @@ class ElementPath {
   auto operator=(const ElementPath&) -> ElementPath& = default;
   auto operator=(ElementPath&&) -> ElementPath& = default;
 
+  inline friend auto operator==(const ElementPath& lhs, const ElementPath& rhs)
+      -> bool {
+    return lhs.components_ == rhs.components_;
+  }
+  inline friend auto hash_value(const ElementPath& path) -> llvm::hash_code {
+    return llvm::hash_combine_range(path.components_.begin(),
+                                    path.components_.end());
+  }
+
   // Returns whether *this is empty.
   auto IsEmpty() const -> bool { return components_.empty(); }
 

explorer/ast/value.cpp

@@ -97,7 +97,7 @@ struct NestedValueVisitor {
     // which is not "within" this value, so we shouldn't visit it.
     return true;
   }
-  auto Visit(const VTable&) -> bool { return true; }
+  auto Visit(const VTable*) -> bool { return true; }
 
   llvm::function_ref<bool(const Value*)> callback;
 };

explorer/ast/value.h

@@ -41,6 +41,16 @@ struct AllocateTrait {
 using VTable =
     llvm::StringMap<std::pair<Nonnull<const CallableDeclaration*>, int>>;
 
+// Returns a pointer to an empty VTable that will never be deallocated.
+//
+// Using this instead of `new VTable()` avoids unnecessary allocations, and
+// takes better advantage of Arena canonicalization when a VTable pointer is
+// used as a constructor argument.
+inline auto EmptyVTable() -> Nonnull<const VTable*> {
+  static Nonnull<const VTable*> result = new VTable();
+  return result;
+}
+
 // Abstract base class of all AST nodes representing values.
 //
 // Value and its derived classes support LLVM-style RTTI, including
@@ -49,8 +59,14 @@ using VTable =
 // every concrete derived class must have a corresponding enumerator
 // in `Kind`; see https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html for
 // details.
+//
+// Arena's canonicalization support is enabled for Value and all derived types.
+// As a result, all Values must be immutable, and all their constructor
+// arguments must be copyable, equality-comparable, and hashable. See
+// Arena's documentation for details.
 class Value {
  public:
+  using EnableCanonicalizedAllocation = void;
   enum class Kind {
 #define CARBON_VALUE_KIND(kind) kind,
 #include "explorer/ast/value_kinds.def"
@@ -679,7 +695,7 @@ class FunctionType : public Value {
   // An explicit function parameter that is a `:!` binding:
   //
   //     fn MakeEmptyVector(T:! type) -> Vector(T);
-  struct GenericParameter {
+  struct GenericParameter : public HashFromDecompose<GenericParameter> {
     template <typename F>
     auto Decompose(F f) const {
       return f(index, binding);
@@ -690,7 +706,7 @@ class FunctionType : public Value {
   };
 
   // For methods with unbound `self` parameters.
-  struct MethodSelf {
+  struct MethodSelf : public HashFromDecompose<MethodSelf> {
     template <typename F>
     auto Decompose(F f) const {
       return f(addr_self, self_type);
@@ -724,11 +740,18 @@ class FunctionType : public Value {
         impl_bindings_(std::move(impl_bindings)),
         is_initializing_(is_initializing) {}
 
-  struct ExceptSelf {};
-  FunctionType(ExceptSelf, const FunctionType& clone)
-      : FunctionType(std::nullopt, clone.parameters_, clone.generic_parameters_,
-                     clone.return_type_, clone.deduced_bindings_,
-                     clone.impl_bindings_, clone.is_initializing_) {}
+  struct ExceptSelf : public HashFromDecompose<ExceptSelf> {
+    template <typename F>
+    auto Decompose(F f) const {
+      return f();
+    }
+  };
+
+  FunctionType(ExceptSelf, const FunctionType* clone)
+      : FunctionType(std::nullopt, clone->parameters_,
+                     clone->generic_parameters_, clone->return_type_,
+                     clone->deduced_bindings_, clone->impl_bindings_,
+                     clone->is_initializing_) {}
 
   static auto classof(const Value* value) -> bool {
     return value->kind() == Kind::FunctionType;
@@ -841,12 +864,13 @@ class NominalClassType : public Value {
   explicit NominalClassType(
       Nonnull<const ClassDeclaration*> declaration,
       Nonnull<const Bindings*> bindings,
-      std::optional<Nonnull<const NominalClassType*>> base, VTable class_vtable)
+      std::optional<Nonnull<const NominalClassType*>> base,
+      Nonnull<const VTable*> class_vtable)
       : Value(Kind::NominalClassType),
         declaration_(declaration),
         bindings_(bindings),
         base_(base),
-        vtable_(std::move(class_vtable)),
+        vtable_(class_vtable),
         hierarchy_level_(base ? (*base)->hierarchy_level() + 1 : 0) {}
 
   static auto classof(const Value* value) -> bool {
@@ -873,7 +897,7 @@ class NominalClassType : public Value {
     return bindings_->witnesses();
   }
 
-  auto vtable() const -> const VTable& { return vtable_; }
+  auto vtable() const -> const VTable& { return *vtable_; }
 
   // Returns how many levels from the top ancestor class it is. i.e. a class
   // with no base returns `0`, while a class with a `.base` and `.base.base`
@@ -893,7 +917,7 @@ class NominalClassType : public Value {
   Nonnull<const ClassDeclaration*> declaration_;
   Nonnull<const Bindings*> bindings_ = Bindings::None();
   const std::optional<Nonnull<const NominalClassType*>> base_;
-  const VTable vtable_;
+  Nonnull<const VTable*> vtable_;
   int hierarchy_level_;
 };
 
@@ -1025,7 +1049,7 @@ class NamedConstraintType : public Value {
 };
 
 // A constraint that requires implementation of an interface.
-struct ImplsConstraint {
+struct ImplsConstraint : public HashFromDecompose<ImplsConstraint> {
   template <typename F>
   auto Decompose(F f) const {
     return f(type, interface);
@@ -1038,7 +1062,7 @@ struct ImplsConstraint {
 };
 
 // A constraint that requires an intrinsic property of a type.
-struct IntrinsicConstraint {
+struct IntrinsicConstraint : public HashFromDecompose<IntrinsicConstraint> {
   template <typename F>
   auto Decompose(F f) const {
     return f(type, kind, arguments);
@@ -1063,7 +1087,7 @@ struct IntrinsicConstraint {
 };
 
 // A constraint that a collection of values are known to be the same.
-struct EqualityConstraint {
+struct EqualityConstraint : public HashFromDecompose<EqualityConstraint> {
   template <typename F>
   auto Decompose(F f) const {
     return f(values);
@@ -1086,7 +1110,7 @@ struct EqualityConstraint {
 
 // A constraint indicating that access to an associated constant should be
 // replaced by another value.
-struct RewriteConstraint {
+struct RewriteConstraint : public HashFromDecompose<RewriteConstraint> {
   template <typename F>
   auto Decompose(F f) const {
     return f(constant, unconverted_replacement, unconverted_replacement_type,
@@ -1104,7 +1128,7 @@ struct RewriteConstraint {
 };
 
 // A context in which we might look up a name.
-struct LookupContext {
+struct LookupContext : public HashFromDecompose<LookupContext> {
   template <typename F>
   auto Decompose(F f) const {
     return f(context);
@@ -1446,11 +1470,11 @@ class MemberName : public Value {
  public:
   MemberName(std::optional<Nonnull<const Value*>> base_type,
              std::optional<Nonnull<const InterfaceType*>> interface,
-             NamedElement member)
+             Nonnull<const NamedElement*> member)
       : Value(Kind::MemberName),
         base_type_(base_type),
         interface_(interface),
-        member_(std::move(member)) {
+        member_(member) {
     CARBON_CHECK(base_type || interface)
         << "member name must be in a type, an interface, or both";
   }
@@ -1465,7 +1489,7 @@ class MemberName : public Value {
   }
 
   // Prints the member name or identifier.
-  void Print(llvm::raw_ostream& out) const { member_.Print(out); }
+  void Print(llvm::raw_ostream& out) const { member_->Print(out); }
 
   // The type for which `name` is a member or a member of an `impl`.
   auto base_type() const -> std::optional<Nonnull<const Value*>> {
@@ -1476,14 +1500,14 @@ class MemberName : public Value {
     return interface_;
   }
   // The member.
-  auto member() const -> const NamedElement& { return member_; }
+  auto member() const -> const NamedElement& { return *member_; }
   // The name of the member.
   auto name() const -> std::string_view { return member().name(); }
 
  private:
   std::optional<Nonnull<const Value*>> base_type_;
   std::optional<Nonnull<const InterfaceType*>> interface_;
-  NamedElement member_;
+  Nonnull<const NamedElement*> member_;
 };
 
 // A symbolic value representing an associated constant.
@@ -1622,8 +1646,8 @@ class TypeOfParameterizedEntityName : public Value {
 // as the member name in a compound member access.
 class TypeOfMemberName : public Value {
  public:
-  explicit TypeOfMemberName(NamedElement member)
-      : Value(Kind::TypeOfMemberName), member_(std::move(member)) {}
+  explicit TypeOfMemberName(Nonnull<const NamedElement*> member)
+      : Value(Kind::TypeOfMemberName), member_(member) {}
 
   static auto classof(const Value* value) -> bool {
     return value->kind() == Kind::TypeOfMemberName;
@@ -1636,10 +1660,10 @@ class TypeOfMemberName : public Value {
 
   // TODO: consider removing this or moving it elsewhere in the AST,
   // since it's arguably part of the expression value rather than its type.
-  auto member() const -> NamedElement { return member_; }
+  auto member() const -> const NamedElement& { return *member_; }
 
  private:
-  NamedElement member_;
+  Nonnull<const NamedElement*> member_;
 };
 
 // The type of a namespace name.

explorer/ast/value_node.h

@@ -137,6 +137,11 @@ class ValueNodeView {
     return std::less<>()(lhs.base_, rhs.base_);
   }
 
+  friend auto hash_value(const ValueNodeView& view) -> llvm::hash_code {
+    using llvm::hash_value;
+    return hash_value(view.base_);
+  }
+
  private:
   Nonnull<const AstNode*> base_;
   std::function<std::optional<Nonnull<const Value*>>(const AstNode&)>

explorer/ast/value_transform.h

@@ -11,12 +11,30 @@
 
 namespace Carbon {
 
+// Constructs a T instance by direct-list-initialization from the given
+// components (which must have been produced by Decompose).
+template <typename T, typename... Args>
+auto ConstructFromComponents(Args&&... args)
+    -> decltype(T{std::declval<Args>()...}) {
+  return T{std::forward<Args>(args)...};
+}
+
+// Overload of the above to accommodate the case where T is an aggregate and
+// has a CRTP base class, in which case the initializer list must start with
+// an empty initializer for the base class.
+template <typename T, typename... Args>
+auto ConstructFromComponents(Args&&... args)
+    -> decltype(T{{}, std::declval<Args>()...}) {
+  return T{{}, std::forward<Args>(args)...};
+}
+
 template <typename T, typename, typename... Args>
-constexpr bool is_list_constructible_impl = false;
+constexpr bool IsConstructibleFromComponentsImpl = false;
 
 template <typename T, typename... Args>
-constexpr bool is_list_constructible_impl<
-    T, decltype(T{std::declval<Args>()...}), Args...> = true;
+constexpr bool IsConstructibleFromComponentsImpl<
+    T, decltype(ConstructFromComponents<T>(std::declval<Args>()...)), Args...> =
+    true;
 
 // A no-op visitor used to implement `IsRecursivelyTransformable`. The
 // `operator()` function returns `true_type` if it's called with arguments that
@@ -24,9 +42,8 @@ constexpr bool is_list_constructible_impl<
 template <typename T>
 struct IsRecursivelyTransformableVisitor {
   template <typename... Args>
-  auto operator()(Args&&... args)
-      -> std::integral_constant<bool,
-                                is_list_constructible_impl<T, T, Args...>>;
+  auto operator()(Args&&... args) -> std::integral_constant<
+      bool, IsConstructibleFromComponentsImpl<T, T, Args...>>;
 };
 
 // A type trait that indicates whether `T` is transformable. A transformable
@@ -132,7 +149,8 @@ class TransformBase {
         if (unwrapper_.failed()) {
           return value;
         }
-        return T{decltype(transformed_elements)(transformed_elements)...};
+        return ConstructFromComponents<T>(
+            decltype(transformed_elements)(transformed_elements)...);
       }(TransformOrOriginal(elements)...);
     });
   }

explorer/common/BUILD

@@ -9,6 +9,35 @@ cc_library(
     hdrs = ["arena.h"],
     deps = [
         ":nonnull",
+        "@llvm-project//llvm:Support",
+    ],
+)
+
+cc_test(
+    name = "arena_test",
+    srcs = ["arena_test.cpp"],
+    deps = [
+        ":arena",
+        "//testing/util:gtest_main",
+        "@com_google_googletest//:gtest",
+    ],
+)
+
+cc_library(
+    name = "decompose",
+    hdrs = ["decompose.h"],
+    deps = [
+        "@llvm-project//llvm:Support",
+    ],
+)
+
+cc_test(
+    name = "decompose_test",
+    srcs = ["decompose_test.cpp"],
+    deps = [
+        ":decompose",
+        "//testing/util:gtest_main",
+        "@com_google_googletest//:gtest",
     ],
 )
 

explorer/common/arena.h

@@ -5,15 +5,43 @@
 #ifndef CARBON_EXPLORER_COMMON_ARENA_H_
 #define CARBON_EXPLORER_COMMON_ARENA_H_
 
+#include <any>
+#include <map>
 #include <memory>
 #include <type_traits>
+#include <unordered_map>
+#include <utility>
 #include <vector>
 
 #include "explorer/common/nonnull.h"
+#include "llvm/ADT/Hashing.h"
 
 namespace Carbon {
 
+// Adapter metafunction that converts T to a form that is usable as part of
+// a key in a hash map.
+//
+// ArgKey<T>::type must be implicitly convertible from T, equality-comparable,
+// and have a hash_value overload as defined in llvm/ADT/Hashing.h. This
+// should only be customized in cases where we cannot modify T itself to
+// satisfy those requirements.
+template <typename T, typename = void>
+struct ArgKey {
+  using type = T;
+};
+
+template <typename T>
+using ArgKeyType = typename ArgKey<T>::type;
+
+// Allocates and maintains ownership of arbitrary objects, so that their
+// lifetimes all end at the same time. It can also canonicalize the allocated
+// objects (see the documentation of New).
 class Arena {
+  // CanonicalizeAllocation<T>::value is true if canonicalization is enabled
+  // for T, and false otherwise.
+  template <typename T, typename = void>
+  struct CanonicalizeAllocation;
+
  public:
   // Values of this type can be passed as the first argument to New in order to
   // have the address of the created object written to the given pointer before
@@ -26,28 +54,45 @@ class Arena {
   template <typename T>
   WriteAddressTo(T** target) -> WriteAddressTo<T>;
 
-  // Allocates an object in the arena, returning a pointer to it.
+  // Returns a pointer to an object constructed as if by `T(args...)`, owned
+  // by this Arena.
+  //
+  // If T::EnableCanonicalizedAllocation exists and names a type, this method
+  // will canonicalize the allocated objects, meaning that two calls to this
+  // method with the same T and equal arguments will return pointers to the same
+  // object. If canonicalization is enabled, all types in Args... must be
+  // copyable, equality-comparable, and have a hash_value overload as defined in
+  // llvm/ADT/Hashing.h. If it's not possible to modify an argument type A to
+  // satisfy those requirements, the ArgKey<A> customization point can be used
+  // instead.
+  //
+  // Canonically-allocated objects must not be mutated, because those mutations
+  // would be visible to all users that happened to allocate a T object with
+  // the same constructor arguments. To help enforce this, the returned pointer
+  // will be const when canonicalization is enabled. Since that means there
+  // is no way to allocate a mutable instance of T, canonicalization should
+  // only be enabled for types that are inherently immutable.
+  //
+  // Canonicalization does not guarantee that equal objects will be identical,
+  // but it can substantially reduce the incidence of equal-but-not-identical
+  // objects, which can facilitate various optimizations.
   template <
       typename T, typename... Args,
-      typename std::enable_if_t<std::is_constructible_v<T, Args...>>* = nullptr>
-  auto New(Args&&... args) -> Nonnull<T*> {
-    auto smart_ptr =
-        std::make_unique<ArenaEntryTyped<T>>(std::forward<Args>(args)...);
-    Nonnull<T*> ptr = smart_ptr->Instance();
-    arena_.push_back(std::move(smart_ptr));
-    allocated_ += sizeof(T);
-    return ptr;
-  }
+      typename std::enable_if_t<std::is_constructible_v<T, Args...> &&
+                                !CanonicalizeAllocation<T>::value>* = nullptr>
+  auto New(Args&&... args) -> Nonnull<T*>;
+
+  template <
+      typename T, typename... Args,
+      typename std::enable_if_t<std::is_constructible_v<T, Args...> &&
+                                CanonicalizeAllocation<T>::value>* = nullptr>
+  auto New(Args&&... args) -> Nonnull<const T*>;
 
   // Allocates an object in the arena, writing its address to the given pointer.
   template <
       typename T, typename U, typename... Args,
       typename std::enable_if_t<std::is_constructible_v<T, Args...>>* = nullptr>
-  void New(WriteAddressTo<U> addr, Args&&... args) {
-    arena_.push_back(std::make_unique<ArenaEntryTyped<T>>(
-        addr, std::forward<Args>(args)...));
-    allocated_ += sizeof(T);
-  }
+  void New(WriteAddressTo<U> addr, Args&&... args);
 
   auto allocated() -> int64_t { return allocated_; }
 
@@ -61,35 +106,175 @@ class Arena {
 
   // Templated destruction of a pointer.
   template <typename T>
-  class ArenaEntryTyped : public ArenaEntry {
-   public:
-    struct WriteAddressHelper {};
+  class ArenaEntryTyped;
 
-    template <typename... Args>
-    explicit ArenaEntryTyped(Args&&... args)
-        : instance_(std::forward<Args>(args)...) {}
+  // Hash functor implemented in terms of hash_value (see llvm/ADT/Hashing.h).
+  struct LlvmHasher {
+    template <typename T>
+    auto operator()(const T& t) const -> size_t {
+      using llvm::hash_value;
+      return hash_value(t);
+    }
+  };
 
-    template <typename... Args>
-    explicit ArenaEntryTyped(WriteAddressHelper, Args&&... args)
-        : ArenaEntryTyped(std::forward<Args>(args)...) {}
+  // Factory metafunction for globally unique type IDs.
+  // &TypeId<T>::id == &TypeId<U>::id if and only if std::is_same_v<T,U>.
+  //
+  // Inspired by llvm::Any::TypeId.
+  template <typename T>
+  struct TypeId {
+    // This is only used for an address to compare; the value is unimportant.
+    static char id;
+  };
 
-    template <typename U, typename... Args>
-    explicit ArenaEntryTyped(WriteAddressTo<U> write_address, Args&&... args)
-        : ArenaEntryTyped(
-              (*write_address.target = &instance_, WriteAddressHelper{}),
-              std::forward<Args>(args)...) {}
+  // A canonicalization table maps a tuple of constructor argument values to
+  // a non-null pointer to a T object constructed with those arguments.
+  template <typename T, typename... Args>
+  using CanonicalizationTable =
+      std::unordered_map<std::tuple<ArgKeyType<Args>...>, Nonnull<const T*>,
+                         LlvmHasher>;
 
-    auto Instance() -> Nonnull<T*> { return Nonnull<T*>(&instance_); }
+  // Allocates an object in the arena. Unlike New, this will always allocate
+  // and construct a new object.
+  template <typename T, typename... Args>
+  auto UniqueNew(Args&&... args) -> Nonnull<T*>;
 
-   private:
-    T instance_;
-  };
+  // Returns a pointer to the canonical instance of T constructed from
+  // `args...`, or null if there is no such instance yet. Returns a mutable
+  // reference so that a null entry can be updated.
+  template <typename T, typename... Args>
+  auto CanonicalInstance(const Args&... args) -> const T*&;
 
   // Manages allocations in an arena for destruction at shutdown.
   std::vector<std::unique_ptr<ArenaEntry>> arena_;
   int64_t allocated_ = 0;
+
+  // Maps a CanonicalizationTable type to a unique instance of that type for
+  // this arena. For a key equal to &TypeId<T>::id for some T, the corresponding
+  // value contains a T*.
+  std::map<char*, std::any> canonical_tables_;
+};
+
+// ---------------------------------------
+// Implementation details only below here.
+// ---------------------------------------
+
+template <>
+struct ArgKey<std::nullopt_t> {
+  using type = struct NulloptProxy {
+    NulloptProxy(std::nullopt_t) {}
+    friend auto operator==(NulloptProxy, NulloptProxy) -> bool { return true; }
+    friend auto hash_value(NulloptProxy) -> llvm::hash_code {
+      return llvm::hash_combine();
+    }
+  };
+};
+
+template <typename T>
+struct ArgKey<std::vector<T>> {
+  using type = class VectorProxy {
+   public:
+    VectorProxy(std::vector<T> vec) : vec_(std::move(vec)) {}
+    friend auto operator==(const VectorProxy& lhs, const VectorProxy& rhs) {
+      return lhs.vec_ == rhs.vec_;
+    }
+    friend auto hash_value(const VectorProxy& v) {
+      return llvm::hash_combine(
+          llvm::hash_combine_range(v.vec_.begin(), v.vec_.end()),
+          v.vec_.size());
+    }
+
+   private:
+    std::vector<T> vec_;
+  };
 };
 
+template <typename T, typename... Args,
+          typename std::enable_if_t<std::is_constructible_v<T, Args...> &&
+                                    !Arena::CanonicalizeAllocation<T>::value>*>
+auto Arena::New(Args&&... args) -> Nonnull<T*> {
+  return UniqueNew<T>(std::forward<Args>(args)...);
+}
+
+template <typename T, typename... Args,
+          typename std::enable_if_t<std::is_constructible_v<T, Args...> &&
+                                    Arena::CanonicalizeAllocation<T>::value>*>
+auto Arena::New(Args&&... args) -> Nonnull<const T*> {
+  const T*& canonical_instance = CanonicalInstance<T>(args...);
+  if (canonical_instance == nullptr) {
+    canonical_instance = UniqueNew<T>(std::forward<Args>(args)...);
+  }
+  return canonical_instance;
+}
+
+template <typename T, typename U, typename... Args,
+          typename std::enable_if_t<std::is_constructible_v<T, Args...>>*>
+void Arena::New(WriteAddressTo<U> addr, Args&&... args) {
+  static_assert(!CanonicalizeAllocation<T>::value,
+                "This form of New does not support canonicalization yet");
+  arena_.push_back(
+      std::make_unique<ArenaEntryTyped<T>>(addr, std::forward<Args>(args)...));
+  allocated_ += sizeof(T);
+}
+
+template <typename T, typename... Args>
+auto Arena::UniqueNew(Args&&... args) -> Nonnull<T*> {
+  auto smart_ptr =
+      std::make_unique<ArenaEntryTyped<T>>(std::forward<Args>(args)...);
+  Nonnull<T*> ptr = smart_ptr->Instance();
+  arena_.push_back(std::move(smart_ptr));
+  allocated_ += sizeof(T);
+  return ptr;
+}
+
+template <typename T, typename>
+struct Arena::CanonicalizeAllocation : public std::false_type {};
+
+template <typename T>
+struct Arena::CanonicalizeAllocation<
+    T, std::void_t<typename T::EnableCanonicalizedAllocation>>
+    : public std::true_type {};
+
+template <typename T, typename... Args>
+auto Arena::CanonicalInstance(const Args&... args) -> const T*& {
+  using MapType = CanonicalizationTable<T, Args...>;
+  std::any& wrapped_table = canonical_tables_[&TypeId<MapType>::id];
+  if (!wrapped_table.has_value()) {
+    wrapped_table.emplace<MapType>();
+  }
+  MapType& table = std::any_cast<MapType&>(wrapped_table);
+  return table[typename MapType::key_type(args...)];
+}
+
+// Templated destruction of a pointer.
+template <typename T>
+class Arena::ArenaEntryTyped : public ArenaEntry {
+ public:
+  struct WriteAddressHelper {};
+
+  template <typename... Args>
+  explicit ArenaEntryTyped(Args&&... args)
+      : instance_(std::forward<Args>(args)...) {}
+
+  template <typename... Args>
+  explicit ArenaEntryTyped(WriteAddressHelper, Args&&... args)
+      : ArenaEntryTyped(std::forward<Args>(args)...) {}
+
+  template <typename U, typename... Args>
+  explicit ArenaEntryTyped(WriteAddressTo<U> write_address, Args&&... args)
+      : ArenaEntryTyped(
+            (*write_address.target = &instance_, WriteAddressHelper{}),
+            std::forward<Args>(args)...) {}
+
+  auto Instance() -> Nonnull<T*> { return Nonnull<T*>(&instance_); }
+
+ private:
+  T instance_;
+};
+
+template <typename T>
+char Arena::TypeId<T>::id = 1;
+
 }  // namespace Carbon
 
 #endif  // CARBON_EXPLORER_COMMON_ARENA_H_

explorer/common/arena_test.cpp

@@ -0,0 +1,96 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "explorer/common/arena.h"
+
+#include <gtest/gtest.h>
+
+#include <optional>
+#include <vector>
+
+namespace Carbon {
+
+class ReportDestruction {
+ public:
+  explicit ReportDestruction(bool* destroyed) : destroyed_(destroyed) {}
+
+  ~ReportDestruction() { *destroyed_ = true; }
+
+ private:
+  bool* destroyed_;
+};
+
+TEST(ArenaTest, BasicAllocation) {
+  bool destroyed = false;
+  {
+    Arena arena;
+    (void)arena.New<ReportDestruction>(&destroyed);
+  }
+  EXPECT_TRUE(destroyed);
+}
+
+struct CanonicalizedDummy {
+  explicit CanonicalizedDummy(int) {}
+  explicit CanonicalizedDummy(int*) {}
+  explicit CanonicalizedDummy(int, int*) {}
+  explicit CanonicalizedDummy(std::vector<int>, std::nullopt_t) {}
+  using EnableCanonicalizedAllocation = void;
+};
+
+TEST(ArenaTest, Canonicalize) {
+  Arena arena;
+  auto* dummy1 = arena.New<CanonicalizedDummy>(1);
+  EXPECT_TRUE(std::is_const_v<std::remove_pointer_t<decltype(dummy1)>>);
+  auto* dummy2 = arena.New<CanonicalizedDummy>(1);
+  EXPECT_TRUE(dummy1 == dummy2);
+}
+
+TEST(ArenaTest, CanonicalizeArgMismatch) {
+  Arena arena;
+  auto* dummy1 = arena.New<CanonicalizedDummy>(1);
+  auto* dummy2 = arena.New<CanonicalizedDummy>(2);
+  EXPECT_TRUE(dummy1 != dummy2);
+}
+
+TEST(ArenaTest, CanonicalizeDifferentArenas) {
+  Arena arena1;
+  Arena arena2;
+  auto* dummy1 = arena1.New<CanonicalizedDummy>(1);
+  auto* dummy2 = arena2.New<CanonicalizedDummy>(1);
+
+  EXPECT_TRUE(dummy1 != dummy2);
+}
+
+TEST(ArenaTest, CanonicalizeIsShallow) {
+  Arena arena;
+  int i1 = 1;
+  int i2 = 1;
+  auto* dummy1 = arena.New<CanonicalizedDummy>(&i1);
+  auto* dummy2 = arena.New<CanonicalizedDummy>(&i2);
+  EXPECT_TRUE(dummy1 != dummy2);
+}
+
+TEST(ArenaTest, CanonicalizeMultipleArgs) {
+  Arena arena;
+  int i;
+  auto* dummy1 = arena.New<CanonicalizedDummy>(1, &i);
+  auto* dummy2 = arena.New<CanonicalizedDummy>(1, &i);
+  EXPECT_TRUE(dummy1 == dummy2);
+}
+
+TEST(ArenaTest, CanonicalizeStdTypes) {
+  Arena arena;
+  std::vector<int> v1 = {1, 2, 3};
+  std::vector<int> v2 = {1, 2, 3};
+  std::vector<int> v3 = {1, 2, 3, 4};
+
+  auto* dummy1 = arena.New<CanonicalizedDummy>(v1, std::nullopt);
+  auto* dummy2 = arena.New<CanonicalizedDummy>(v2, std::nullopt);
+  EXPECT_TRUE(dummy1 == dummy2);
+
+  auto* dummy3 = arena.New<CanonicalizedDummy>(v3, std::nullopt);
+  EXPECT_TRUE(dummy1 != dummy3);
+}
+
+}  // namespace Carbon

explorer/common/decompose.h

@@ -0,0 +1,54 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// Utilities for types that support the `Decompose` API.
+
+#ifndef CARBON_EXPLORER_COMMON_DECOMPOSE_H_
+#define CARBON_EXPLORER_COMMON_DECOMPOSE_H_
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Hashing.h"
+
+namespace Carbon {
+
+// CRTP base class which extends `Base` to support hashing and equality
+// comparison. `Base` must support the `Decompose` API.
+template <typename Base>
+class HashFromDecompose {
+ public:
+  friend auto operator==(const HashFromDecompose& lhs,
+                         const HashFromDecompose& rhs) -> bool {
+    return static_cast<const Base*>(&lhs)->Decompose(
+        [&](auto&&... lhs_elements) {
+          return static_cast<const Base*>(&rhs)->Decompose(
+              [&](auto&&... rhs_elements) {
+                return ((lhs_elements == rhs_elements) && ...);
+              });
+        });
+  }
+
+  friend auto hash_value(const HashFromDecompose& self) -> llvm::hash_code {
+    return static_cast<const Base*>(&self)->Decompose(
+        [&](auto&&... lhs_elements) {
+          return llvm::hash_combine(WrapForHash(lhs_elements)...);
+        });
+  }
+
+ private:
+  // Wraps T in a form that supports `hash_value`. Used for adapting types that
+  // we can't extend directly.
+  template <typename T>
+  static auto WrapForHash(const T& t) -> const T& {
+    return t;
+  }
+
+  template <typename T>
+  static auto WrapForHash(const std::vector<T>& vec) -> llvm::ArrayRef<T> {
+    return vec;
+  }
+};
+
+}  // namespace Carbon
+
+#endif  // CARBON_EXPLORER_COMMON_DECOMPOSE_H_

explorer/common/decompose_test.cpp

@@ -0,0 +1,39 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "explorer/common/decompose.h"
+
+#include <gtest/gtest.h>
+
+namespace Carbon {
+namespace {
+
+struct Decomposeable : public HashFromDecompose<Decomposeable> {
+  template <typename F>
+  auto Decompose(F f) const {
+    return f(i, s);
+  }
+
+  int i = 0;
+  std::string s;
+};
+
+TEST(HashFromDecomposeTest, EqualValues) {
+  Decomposeable d1 = {.i = 42, .s = "foo"};
+  Decomposeable d2 = {.i = 42, .s = "foo"};
+
+  EXPECT_TRUE(d1 == d2);
+  EXPECT_TRUE(hash_value(d1) == hash_value(d2));
+}
+
+TEST(HashFromDecomposeTest, NonEqualValues) {
+  Decomposeable d1 = {.i = 42, .s = "foo"};
+  Decomposeable d2 = {.i = 42, .s = "bar"};
+
+  EXPECT_FALSE(d1 == d2);
+  EXPECT_FALSE(hash_value(d1) == hash_value(d2));
+}
+
+}  // namespace
+}  // namespace Carbon

explorer/interpreter/interpreter.cpp

@@ -123,7 +123,7 @@ class Interpreter {
   auto ConvertStructToClass(Nonnull<const StructValue*> init,
                             Nonnull<const NominalClassType*> class_type,
                             SourceLocation source_loc)
-      -> ErrorOr<Nonnull<NominalClassValue*>>;
+      -> ErrorOr<Nonnull<const NominalClassValue*>>;
 
   // Evaluate an expression immediately, recursively, and return its result.
   //
@@ -780,7 +780,7 @@ auto Interpreter::InstantiateWitness(Nonnull<const Witness*> witness,
 auto Interpreter::ConvertStructToClass(
     Nonnull<const StructValue*> init_struct,
     Nonnull<const NominalClassType*> class_type, SourceLocation source_loc)
-    -> ErrorOr<Nonnull<NominalClassValue*>> {
+    -> ErrorOr<Nonnull<const NominalClassValue*>> {
   std::vector<NamedValue> struct_values;
   std::optional<Nonnull<const NominalClassValue*>> base_instance;
   // Instantiate the `destination_type` to obtain the runtime
@@ -1191,7 +1191,7 @@ auto Interpreter::CallFunction(const CallExpression& call,
         case DeclarationKind::ClassDeclaration: {
           const auto& class_decl = cast<ClassDeclaration>(decl);
           return todo_.FinishAction(arena_->New<NominalClassType>(
-              &class_decl, bindings, class_decl.base_type(), VTable()));
+              &class_decl, bindings, class_decl.base_type(), EmptyVTable()));
         }
         case DeclarationKind::InterfaceDeclaration:
           return todo_.FinishAction(arena_->New<InterfaceType>(
@@ -1292,7 +1292,7 @@ auto Interpreter::StepInstantiateType() -> ErrorOr<Success> {
             Nonnull<const Bindings*> bindings,
             InstantiateBindings(&class_type.bindings(), source_loc));
         return todo_.FinishAction(arena_->New<NominalClassType>(
-            &class_type.declaration(), bindings, base, class_type.vtable()));
+            &class_type.declaration(), bindings, base, &class_type.vtable()));
       }
     }
     case Value::Kind::PointerType: {
@@ -1437,8 +1437,8 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
                     result)) {
               type_result = result;
             }
-            MemberName* member_name = arena_->New<MemberName>(
-                type_result, found_in_interface, member_name_type->member());
+            const auto* member_name = arena_->New<MemberName>(
+                type_result, found_in_interface, &member_name_type->member());
             return todo_.FinishAction(member_name);
           }
         } else {
@@ -1553,8 +1553,9 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
             CARBON_CHECK(!access.member().base_type().has_value())
                 << "compound member access forming a member name should be "
                    "performing impl lookup";
-            auto* member_name = arena_->New<MemberName>(
-                act.results()[0], found_in_interface, access.member().member());
+            auto* member_name =
+                arena_->New<MemberName>(act.results()[0], found_in_interface,
+                                        &access.member().member());
             return todo_.FinishAction(member_name);
           }
         } else {

explorer/interpreter/type_checker.cpp

@@ -2311,15 +2311,8 @@ auto TypeChecker::Substitute(const Bindings& bindings,
   CARBON_ASSIGN_OR_RETURN(const auto* result, SubstituteImpl(bindings, type));
 
   if (trace_stream_->is_enabled()) {
-    *trace_stream_ << "substitution of {";
-    llvm::ListSeparator sep;
-    for (const auto& [name, value] : bindings.args()) {
-      *trace_stream_ << sep << *name << " -> " << *value;
-    }
-    for (const auto& [name, value] : bindings.witnesses()) {
-      *trace_stream_ << sep << *name << " -> " << *value;
-    }
-    *trace_stream_ << "}\n  old: " << *type << "\n  new: " << *result << "\n";
+    *trace_stream_ << "substitution of " << bindings << "\n  old: " << *type
+                   << "\n  new: " << *result << "\n";
   }
   return result;
 }
@@ -3131,7 +3124,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
                   // Remove `self` from type since now bound.
                   auto* function_type = cast<FunctionType>(field_type);
                   access.set_static_type(arena_->New<FunctionType>(
-                      FunctionType::ExceptSelf{}, *function_type));
+                      FunctionType::ExceptSelf{}, function_type));
                 }
                 access.set_expression_category(ExpressionCategory::Value);
                 break;
@@ -3214,7 +3207,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
               // Remove `self` from type since now bound.
               auto* function_type = cast<FunctionType>(inst_member_type);
               access.set_static_type(arena_->New<FunctionType>(
-                  FunctionType::ExceptSelf{}, *function_type));
+                  FunctionType::ExceptSelf{}, function_type));
             }
           } else {
             access.set_static_type(inst_member_type);
@@ -3272,7 +3265,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
             // declarations to be member name types, rather than special-casing
             // member accesses that name them.
             access.set_static_type(
-                arena_->New<TypeOfMemberName>(NamedElement(result.member)));
+                arena_->New<TypeOfMemberName>(&access.member()));
             access.set_expression_category(ExpressionCategory::Value);
           } else {
             // This is a non-instance member whose value is found directly via
@@ -3301,7 +3294,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
                 if (access.member_name() == field.name) {
                   access.set_member(arena_->New<NamedElement>(&field));
                   access.set_static_type(
-                      arena_->New<TypeOfMemberName>(NamedElement(&field)));
+                      arena_->New<TypeOfMemberName>(&access.member()));
                   access.set_expression_category(ExpressionCategory::Value);
                   return Success();
                 }
@@ -3373,7 +3366,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
                     break;
                 }
                 access.set_static_type(
-                    arena_->New<TypeOfMemberName>(NamedElement(member)));
+                    arena_->New<TypeOfMemberName>(&access.member()));
                 access.set_expression_category(ExpressionCategory::Value);
                 return Success();
               } else {
@@ -3392,7 +3385,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
               access.set_member(arena_->New<NamedElement>(result.member));
               access.set_found_in_interface(result.interface);
               access.set_static_type(
-                  arena_->New<TypeOfMemberName>(NamedElement(result.member)));
+                  arena_->New<TypeOfMemberName>(&access.member()));
               access.set_expression_category(ExpressionCategory::Value);
               return Success();
             }
@@ -3520,7 +3513,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
                                 Substitute(bindings_for_member(), member_type));
         auto* function_type = cast<FunctionType>(member_type);
         access.set_static_type(arena_->New<FunctionType>(
-            FunctionType::ExceptSelf{}, *function_type));
+            FunctionType::ExceptSelf{}, function_type));
         return Success();
       };
 
@@ -3568,7 +3561,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
       }
 
       access.set_static_type(
-          arena_->New<TypeOfMemberName>(member_name.member()));
+          arena_->New<TypeOfMemberName>(&access.member().member()));
       access.set_expression_category(ExpressionCategory::Value);
       return Success();
     }
@@ -3861,7 +3854,7 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
           for (size_t i = 0; i != params.size(); ++i) {
             // TODO: Should we disallow all other kinds of top-level params?
             if (const auto* binding = dyn_cast<GenericBinding>(params[i])) {
-              generic_parameters.push_back({i, binding});
+              generic_parameters.push_back({{}, i, binding});
             }
           }
 
@@ -5136,8 +5129,8 @@ auto TypeChecker::DeclareCallableDeclaration(Nonnull<CallableDeclaration*> f,
     CollectAndNumberGenericBindingsInPattern(&f->self_pattern(), all_bindings);
     CollectImplBindingsInPattern(&f->self_pattern(), impl_bindings);
     FunctionType::MethodSelf method_self_present = {
-        (f->self_pattern().kind() == PatternKind::AddrPattern),
-        &f->self_pattern().static_type()};
+        .addr_self = (f->self_pattern().kind() == PatternKind::AddrPattern),
+        .self_type = &f->self_pattern().static_type()};
     method_self = method_self_present;
   }
   // Type check the parameter pattern.
@@ -5160,7 +5153,7 @@ auto TypeChecker::DeclareCallableDeclaration(Nonnull<CallableDeclaration*> f,
     CollectAndNumberGenericBindingsInPattern(param_pattern, all_bindings);
 
     if (const auto* binding = dyn_cast<GenericBinding>(param_pattern)) {
-      generic_parameters.push_back({i, binding});
+      generic_parameters.push_back({.index = i, .binding = binding});
     } else {
       deduced_bindings.insert(deduced_bindings.end(),
                               all_bindings.begin() + old_size,
@@ -5279,7 +5272,7 @@ auto TypeChecker::DeclareClassDeclaration(Nonnull<ClassDeclaration*> class_decl,
   // set the static type before we start processing them. We can't set the
   // constant value until later, but the base class declaration doesn't need it.
   self->set_static_type(arena_->New<TypeType>());
-  std::optional<Nonnull<ParameterizedEntityName*>> param_name;
+  std::optional<Nonnull<const ParameterizedEntityName*>> param_name;
   if (class_decl->type_params().has_value()) {
     // TODO: The `enclosing_bindings` should be tracked in the parameterized
     // entity name so that they can be included in the eventual type.
@@ -5445,9 +5438,9 @@ auto TypeChecker::DeclareClassDeclaration(Nonnull<ClassDeclaration*> class_decl,
 
   // For class declaration `class MyType(T:! type, U:! AnInterface)`, `Self`
   // should have the value `MyType(T, U)`.
-  Nonnull<NominalClassType*> self_type = arena_->New<NominalClassType>(
+  const auto* self_type = arena_->New<NominalClassType>(
       class_decl, Bindings::SymbolicIdentity(arena_, bindings), base_class,
-      std::move(class_vtable));
+      arena_->New<VTable>(std::move(class_vtable)));
   self->set_constant_value(self_type);
 
   // The declarations of the members may refer to the class, so we must set the
@@ -5516,14 +5509,13 @@ auto TypeChecker::DeclareMixinDeclaration(Nonnull<MixinDeclaration*> mixin_decl,
       *trace_stream_ << mixin_scope;
     }
 
-    Nonnull<ParameterizedEntityName*> param_name =
+    const auto* param_name =
         arena_->New<ParameterizedEntityName>(mixin_decl, *mixin_decl->params());
     mixin_decl->set_static_type(
         arena_->New<TypeOfParameterizedEntityName>(param_name));
     mixin_decl->set_constant_value(param_name);
   } else {
-    Nonnull<MixinPseudoType*> mixin_type =
-        arena_->New<MixinPseudoType>(mixin_decl);
+    const auto* mixin_type = arena_->New<MixinPseudoType>(mixin_decl);
     mixin_decl->set_static_type(arena_->New<TypeOfMixinPseudoType>(mixin_type));
     mixin_decl->set_constant_value(mixin_type);
   }
@@ -5665,9 +5657,8 @@ auto TypeChecker::DeclareConstraintTypeDeclaration(
 
   // Set up the meaning of the declaration when used as an identifier.
   if (constraint_decl->params().has_value()) {
-    Nonnull<ParameterizedEntityName*> param_name =
-        arena_->New<ParameterizedEntityName>(constraint_decl,
-                                             *constraint_decl->params());
+    const auto* param_name = arena_->New<ParameterizedEntityName>(
+        constraint_decl, *constraint_decl->params());
     constraint_decl->set_static_type(
         arena_->New<TypeOfParameterizedEntityName>(param_name));
     constraint_decl->set_constant_value(param_name);
@@ -6229,7 +6220,7 @@ auto TypeChecker::DeclareChoiceDeclaration(Nonnull<ChoiceDeclaration*> choice,
   }
 
   if (choice->type_params().has_value()) {
-    Nonnull<ParameterizedEntityName*> param_name =
+    const auto* param_name =
         arena_->New<ParameterizedEntityName>(choice, *choice->type_params());
     choice->set_static_type(
         arena_->New<TypeOfParameterizedEntityName>(param_name));

explorer/interpreter/type_structure.cpp

@@ -81,10 +81,10 @@ struct TypeStructureBuilder {
   void Visit(Nonnull<const AstNode*>) {}
   void Visit(const ValueNodeView&) {}
   void Visit(const Address&) {}
-  void Visit(const VTable&) {}
+  void Visit(const VTable*) {}
   void Visit(const FunctionType::GenericParameter&) {}
   void Visit(const FunctionType::MethodSelf&) {}
-  void Visit(const NamedElement&) {}
+  void Visit(const NamedElement*) {}
 
   // Constraint types can contain mentions of VariableTypes, but they aren't
   // deducible so it's not important to look for them.