Propagate errors out of Substitute. (#2694)

This is in preparation for template instantiation being triggered during substitution, and being able to fail.

Fix rule-of-three violation (missing assignment operator) in `Error` that got in the way of using it to hold an error temporarily in a failed transformation.

common/error.h

@@ -36,6 +36,12 @@ class [[nodiscard]] Error {
       : location_(std::move(other.location_)),
         message_(std::move(other.message_)) {}
 
+  Error& operator=(Error&& other) noexcept {
+    location_ = std::move(other.location_);
+    message_ = std::move(other.message_);
+    return *this;
+  }
+
   // Prints the error string.
   void Print(llvm::raw_ostream& out) const {
     if (!location().empty()) {
@@ -73,9 +79,6 @@ class [[nodiscard]] ErrorOr {
   // NOLINTNEXTLINE(google-explicit-constructor)
   ErrorOr(T val) : val_(std::move(val)) {}
 
-  // Moves held state.
-  ErrorOr(ErrorOr&& other) noexcept : val_(std::move(other.val_)) {}
-
   // Returns true for success.
   auto ok() const -> bool { return std::holds_alternative<T>(val_); }
 

explorer/ast/value_transform.h

@@ -5,6 +5,7 @@
 #ifndef CARBON_EXPLORER_AST_VALUE_TRANSFORM_H_
 #define CARBON_EXPLORER_AST_VALUE_TRANSFORM_H_
 
+#include "common/error.h"
 #include "explorer/ast/value.h"
 
 namespace Carbon {
@@ -40,17 +41,61 @@ class TransformBase {
  public:
   explicit TransformBase(Nonnull<Arena*> arena) : arena_(arena) {}
 
+  // Transform the given value, and produce either the transformed value or an
+  // error.
   template <typename T>
-  auto Transform(T&& v) -> decltype(auto) {
-    return static_cast<Derived&>(*this)(std::forward<T>(v));
+  auto Transform(const T& v) -> ErrorOr<T> {
+    auto result = TransformOrOriginal(v);
+    if (!status_.ok()) {
+      Error error = std::move(status_).error();
+      status_ = Success();
+      return error;
+    }
+    return result;
+  }
+
+ protected:
+  // Given an original value and the result of calling `operator()`, find the
+  // transformed value we should use.
+  //
+  // If `operator()` returns `ErrorOr<T>`, then on failure, collect the error
+  // and return the untransformed value; otherwise, return the transformed
+  // value.
+  template <typename T, typename U>
+  auto CollectError(const T& /*original*/, const U& transformed) -> U {
+    return transformed;
+  }
+  template <typename T, typename U>
+  auto CollectError(const T& original, ErrorOr<U> transformed) -> U {
+    if (!transformed.ok()) {
+      status_ = std::move(transformed).error();
+      return original;
+    }
+    return std::move(*transformed);
+  }
+
+  // Transform the given value, or return the original if transformation fails.
+  template <typename T>
+  auto TransformOrOriginal(const T& v)
+      -> decltype(CollectError(v, std::declval<Derived>()(v))) {
+    // If we've already failed, don't do any more transformations.
+    if (!status_.ok()) {
+      return v;
+    }
+    return CollectError(v, static_cast<Derived&>(*this)(v));
   }
 
   // Transformable values are recursively transformed by default.
   template <typename T,
             std::enable_if_t<IsRecursivelyTransformable<T>, void*> = nullptr>
   auto operator()(const T& value) -> T {
-    return value.Decompose([&](auto&&... elements) {
-      return T{Transform(decltype(elements)(elements))...};
+    return value.Decompose([&](const auto&... elements) {
+      return [&](auto&&... transformed_elements) {
+        if (status_.ok()) {
+          return T{decltype(transformed_elements)(transformed_elements)...};
+        }
+        return value;
+      }(TransformOrOriginal(elements)...);
     });
   }
 
@@ -59,9 +104,17 @@ class TransformBase {
   template <typename T,
             std::enable_if_t<IsRecursivelyTransformable<T>, void*> = nullptr>
   auto operator()(Nonnull<const T*> value) -> auto{
-    return value->Decompose([&](auto&&... elements) {
-      return AllocateTrait<T>::New(arena_,
-                                   Transform(decltype(elements)(elements))...);
+    return value->Decompose([&](const auto&... elements) {
+      return [&](auto&&... transformed_elements)
+                 -> decltype(AllocateTrait<T>::New(
+                     arena_,
+                     decltype(transformed_elements)(transformed_elements)...)) {
+        if (status_.ok()) {
+          return AllocateTrait<T>::New(
+              arena_, decltype(transformed_elements)(transformed_elements)...);
+        }
+        return value;
+      }(TransformOrOriginal(elements)...);
     });
   }
 
@@ -79,13 +132,14 @@ class TransformBase {
     if (!v) {
       return std::nullopt;
     }
-    return Transform(*v);
+    return TransformOrOriginal(*v);
   }
 
   // Transform `pair<T, U>` by transforming T and U.
   template <typename T, typename U>
   auto operator()(const std::pair<T, U>& pair) -> std::pair<T, U> {
-    return std::pair<T, U>{Transform(pair.first), Transform(pair.second)};
+    return std::pair<T, U>{TransformOrOriginal(pair.first),
+                           TransformOrOriginal(pair.second)};
   }
 
   // Transform `vector<T>` by transforming its elements.
@@ -94,7 +148,7 @@ class TransformBase {
     std::vector<T> result;
     result.reserve(vec.size());
     for (auto& value : vec) {
-      result.push_back(Transform(value));
+      result.push_back(TransformOrOriginal(value));
     }
     return result;
   }
@@ -104,7 +158,7 @@ class TransformBase {
   auto operator()(const std::map<T, U>& map) -> std::map<T, U> {
     std::map<T, U> result;
     for (auto& [key, value] : map) {
-      result.insert({Transform(key), Transform(value)});
+      result.insert({TransformOrOriginal(key), TransformOrOriginal(value)});
     }
     return result;
   }
@@ -114,13 +168,17 @@ class TransformBase {
   auto operator()(const llvm::StringMap<T>& map) -> llvm::StringMap<T> {
     llvm::StringMap<T> result;
     for (const auto& it : map) {
-      result.insert({Transform(it.first()), Transform(it.second)});
+      result.insert(
+          {TransformOrOriginal(it.first()), TransformOrOriginal(it.second)});
     }
     return result;
   }
 
  private:
   Nonnull<Arena*> arena_;
+  // Temporary storage for an error that was produced during transformation
+  // that has not yet been handed back to the caller.
+  ErrorOr<Success> status_ = Success();
 };
 
 // Base class for transforms of `Value`s.
@@ -159,7 +217,7 @@ class ValueTransform : public TransformBase<Derived> {
     return value->template Visit<R>([&](const auto* derived_value) {
       using DerivedType = std::remove_pointer_t<decltype(derived_value)>;
       static_assert(IsRecursivelyTransformable<DerivedType>);
-      return this->Transform(derived_value);
+      return this->TransformOrOriginal(derived_value);
     });
   }
 
@@ -170,7 +228,8 @@ class ValueTransform : public TransformBase<Derived> {
 
   // Provide a more precise type from transforming a `Witness`.
   auto operator()(Nonnull<const Witness*> value) -> Nonnull<const Witness*> {
-    return llvm::cast<Witness>(this->Transform(llvm::cast<Value>(value)));
+    return llvm::cast<Witness>(
+        this->TransformOrOriginal(llvm::cast<Value>(value)));
   }
 
   // For elements, dispatch on the element kind and recursively transform.

explorer/interpreter/impl_scope.cpp

@@ -121,8 +121,9 @@ auto ImplScope::TryResolve(Nonnull<const Value*> constraint_type,
                            bool diagnose_missing_impl) const
     -> ErrorOr<std::optional<Nonnull<const Witness*>>> {
   if (const auto* iface_type = dyn_cast<InterfaceType>(constraint_type)) {
-    iface_type =
-        cast<InterfaceType>(type_checker.Substitute(bindings, iface_type));
+    CARBON_ASSIGN_OR_RETURN(
+        iface_type,
+        type_checker.SubstituteCast<InterfaceType>(bindings, iface_type));
     return TryResolveInterface(iface_type, impl_type, source_loc, type_checker,
                                diagnose_missing_impl);
   }
@@ -144,13 +145,17 @@ auto ImplScope::TryResolve(Nonnull<const Value*> constraint_type,
       }
       Bindings local_bindings = bindings;
       local_bindings.Add(constraint->self_binding(), impl_type, witness);
+
+      CARBON_ASSIGN_OR_RETURN(const auto* subst_interface,
+                              type_checker.SubstituteCast<InterfaceType>(
+                                  local_bindings, impl.interface));
+      CARBON_ASSIGN_OR_RETURN(
+          Nonnull<const Value*> subst_type,
+          type_checker.Substitute(local_bindings, impl.type));
       CARBON_ASSIGN_OR_RETURN(
           std::optional<Nonnull<const Witness*>> result,
-          TryResolveInterface(
-              cast<InterfaceType>(
-                  type_checker.Substitute(local_bindings, impl.interface)),
-              type_checker.Substitute(local_bindings, impl.type), source_loc,
-              type_checker, diagnose_missing_impl));
+          TryResolveInterface(subst_interface, subst_type, source_loc,
+                              type_checker, diagnose_missing_impl));
       if (!result) {
         return {std::nullopt};
       }
@@ -174,16 +179,22 @@ auto ImplScope::TryResolve(Nonnull<const Value*> constraint_type,
       local_bindings.Add(constraint->self_binding(), impl_type, witness);
       SingleStepEqualityContext equality_ctx(this);
       for (const auto& intrinsic : intrinsics) {
+        CARBON_ASSIGN_OR_RETURN(
+            Nonnull<const Value*> type,
+            type_checker.Substitute(local_bindings, intrinsic.type));
         IntrinsicConstraint converted = {
-            .type = type_checker.Substitute(local_bindings, intrinsic.type),
-            .kind = intrinsic.kind,
-            .arguments = {}};
+            .type = type, .kind = intrinsic.kind, .arguments = {}};
         converted.arguments.reserve(intrinsic.arguments.size());
         for (Nonnull<const Value*> argument : intrinsic.arguments) {
-          converted.arguments.push_back(
+          CARBON_ASSIGN_OR_RETURN(
+              Nonnull<const Value*> subst_arg,
               type_checker.Substitute(local_bindings, argument));
+          converted.arguments.push_back(subst_arg);
         }
-        if (!type_checker.IsIntrinsicConstraintSatisfied(converted, *this)) {
+        CARBON_ASSIGN_OR_RETURN(
+            bool intrinsic_satisfied,
+            type_checker.IsIntrinsicConstraintSatisfied(converted, *this));
+        if (!intrinsic_satisfied) {
           if (!diagnose_missing_impl) {
             return {std::nullopt};
           }
@@ -193,11 +204,11 @@ auto ImplScope::TryResolve(Nonnull<const Value*> constraint_type,
       }
       for (const auto& equal : equals) {
         auto it = equal.values.begin();
-        Nonnull<const Value*> first =
-            type_checker.Substitute(local_bindings, *it++);
+        CARBON_ASSIGN_OR_RETURN(Nonnull<const Value*> first,
+                                type_checker.Substitute(local_bindings, *it++));
         for (; it != equal.values.end(); ++it) {
-          Nonnull<const Value*> current =
-              type_checker.Substitute(local_bindings, *it);
+          CARBON_ASSIGN_OR_RETURN(Nonnull<const Value*> current,
+                                  type_checker.Substitute(local_bindings, *it));
           if (!ValueEqual(first, current, &equality_ctx)) {
             if (!diagnose_missing_impl) {
               return {std::nullopt};
@@ -208,10 +219,13 @@ auto ImplScope::TryResolve(Nonnull<const Value*> constraint_type,
         }
       }
       for (const auto& rewrite : rewrites) {
-        Nonnull<const Value*> constant =
-            type_checker.Substitute(local_bindings, rewrite.constant);
-        Nonnull<const Value*> value = type_checker.Substitute(
-            local_bindings, rewrite.converted_replacement);
+        CARBON_ASSIGN_OR_RETURN(
+            Nonnull<const Value*> constant,
+            type_checker.Substitute(local_bindings, rewrite.constant));
+        CARBON_ASSIGN_OR_RETURN(
+            Nonnull<const Value*> value,
+            type_checker.Substitute(local_bindings,
+                                    rewrite.converted_replacement));
         if (!ValueEqual(constant, value, &equality_ctx)) {
           if (!diagnose_missing_impl) {
             return {std::nullopt};

explorer/interpreter/type_checker.h

@@ -12,6 +12,7 @@
 #include <unordered_map>
 #include <vector>
 
+#include "common/error.h"
 #include "common/ostream.h"
 #include "explorer/ast/ast.h"
 #include "explorer/ast/expression.h"
@@ -24,6 +25,7 @@
 #include "explorer/interpreter/interpreter.h"
 #include "explorer/interpreter/matching_impl_set.h"
 #include "explorer/interpreter/trace_stream.h"
+#include "llvm/ADT/identity.h"
 
 namespace Carbon {
 
@@ -45,11 +47,24 @@ class TypeChecker {
   // processed.
   auto TypeCheck(AST& ast) -> ErrorOr<Success>;
 
-  // Construct a type that is the same as `type` except that occurrences
-  // of type variables (aka. `GenericBinding` and references to `ImplBinding`)
-  // are replaced by their corresponding type or witness in `dict`.
-  auto Substitute(const Bindings& bindings, Nonnull<const Value*> type) const
-      -> Nonnull<const Value*>;
+  // Construct a value that is the same as `value` except that occurrences
+  // of generic parameters (aka. `GenericBinding` and references to
+  // `ImplBinding`) are replaced by their corresponding value or witness in
+  // `bindings`.
+  auto Substitute(const Bindings& bindings, Nonnull<const Value*> value) const
+      -> ErrorOr<Nonnull<const Value*>>;
+
+  // Same as `Substitute`, but cast the result to the type given as a template
+  // argument, which must be explicitly specified. The `remove_cv_t` here
+  // blocks template argument deduction.
+  template <typename T>
+  auto SubstituteCast(const Bindings& bindings,
+                      Nonnull<const std::remove_cv_t<T>*> value) const
+      -> ErrorOr<Nonnull<const T*>> {
+    CARBON_ASSIGN_OR_RETURN(Nonnull<const Value*> subst_value,
+                            Substitute(bindings, value));
+    return llvm::cast<T>(subst_value);
+  }
 
   // Attempts to refine a witness that might be symbolic into an impl witness,
   // using `impl` declarations that have been declared and type-checked so far.
@@ -57,7 +72,7 @@ class TypeChecker {
   auto RefineWitness(Nonnull<const Witness*> witness,
                      Nonnull<const Value*> type,
                      Nonnull<const Value*> constraint) const
-      -> Nonnull<const Witness*>;
+      -> ErrorOr<Nonnull<const Witness*>>;
 
   // If `impl` can be an implementation of interface `iface` for the given
   // `type`, then return the witness for this `impl`. Otherwise return
@@ -103,7 +118,7 @@ class TypeChecker {
   // in the given scope.
   auto IsIntrinsicConstraintSatisfied(const IntrinsicConstraint& constraint,
                                       const ImplScope& impl_scope) const
-      -> bool;
+      -> ErrorOr<bool>;
 
  private:
   class ConstraintTypeBuilder;
@@ -359,11 +374,11 @@ class TypeChecker {
 
   // Returns the field names of the class together with their types.
   auto FieldTypes(const NominalClassType& class_type) const
-      -> std::vector<NamedValue>;
+      -> ErrorOr<std::vector<NamedValue>>;
 
   // Returns the field names and types of the class and its parents.
   auto FieldTypesWithBase(const NominalClassType& class_type) const
-      -> std::vector<NamedValue>;
+      -> ErrorOr<std::vector<NamedValue>>;
 
   // Returns true if source_fields and destination_fields contain the same set
   // of names, and each value in source_fields is implicitly convertible to
@@ -372,7 +387,7 @@ class TypeChecker {
   auto FieldTypesImplicitlyConvertible(
       llvm::ArrayRef<NamedValue> source_fields,
       llvm::ArrayRef<NamedValue> destination_fields,
-      const ImplScope& impl_scope) const -> bool;
+      const ImplScope& impl_scope) const -> ErrorOr<bool>;
 
   // Returns true if *source is implicitly convertible to *destination. *source
   // and *destination must be concrete types.
@@ -384,7 +399,7 @@ class TypeChecker {
                                Nonnull<const Value*> destination,
                                const ImplScope& impl_scope,
                                bool allow_user_defined_conversions) const
-      -> bool;
+      -> ErrorOr<bool>;
 
   // Attempt to implicitly convert type-checked expression `source` to the type
   // `destination`.
@@ -421,13 +436,14 @@ class TypeChecker {
   // Rebuild a value in the current type-checking context. Applies any rewrites
   // that are in scope and attempts to resolve associated constants using impls
   // that have been declared since the value was formed.
-  auto RebuildValue(Nonnull<const Value*> value) const -> Nonnull<const Value*>;
+  auto RebuildValue(Nonnull<const Value*> value) const
+      -> ErrorOr<Nonnull<const Value*>>;
 
   // Implementation of Substitute and RebuildValue. Does not check that
   // bindings are nonempty, nor does it trace its progress.
   auto SubstituteImpl(const Bindings& bindings,
                       Nonnull<const Value*> type) const
-      -> Nonnull<const Value*>;
+      -> ErrorOr<Nonnull<const Value*>>;
 
   // The name of a builtin interface, with any arguments.
   struct BuiltinInterfaceName {
@@ -468,8 +484,8 @@ class TypeChecker {
       -> ErrorOr<Nonnull<const ConstraintType*>>;
 
   // Gets the type for the given associated constant.
-  auto GetTypeForAssociatedConstant(
-      Nonnull<const AssociatedConstant*> assoc) const -> Nonnull<const Value*>;
+  auto GetTypeForAssociatedConstant(Nonnull<const AssociatedConstant*> assoc)
+      const -> ErrorOr<Nonnull<const Value*>>;
 
   // Look up a member name in a constraint, which might be a single interface or
   // a compound constraint.
@@ -484,14 +500,14 @@ class TypeChecker {
   auto LookupRewriteInTypeOf(Nonnull<const Value*> type,
                              Nonnull<const InterfaceType*> interface,
                              Nonnull<const Declaration*> member) const
-      -> std::optional<const RewriteConstraint*>;
+      -> ErrorOr<std::optional<const RewriteConstraint*>>;
 
   // Given a witness value, look for a rewrite for the given associated
   // constant.
   auto LookupRewriteInWitness(Nonnull<const Witness*> witness,
                               Nonnull<const InterfaceType*> interface,
                               Nonnull<const Declaration*> member) const
-      -> std::optional<const RewriteConstraint*>;
+      -> ErrorOr<std::optional<const RewriteConstraint*>>;
 
   // Adds a member of a declaration to collected_members_
   auto CollectMember(Nonnull<const Declaration*> enclosing_decl,