Set the value of `.Self` in a where to that of the outer `.Self`. (#2344)

Prior to this change, declarations like `let N:! X(.Self) where .(X(.Self).Y) == 5;` have the surprising behavior of the two `.Self` expressions resolving to two different symbolic values. Fix this by forcing the inner one to have the same symbolic value as the outer one, albeit with a different type.

explorer/ast/expression.h

@@ -914,6 +914,17 @@ class WhereExpression : public RewritableMixin<Expression> {
   auto self_binding() const -> const GenericBinding& { return *self_binding_; }
   auto self_binding() -> GenericBinding& { return *self_binding_; }
 
+  auto enclosing_dot_self() const
+      -> std::optional<Nonnull<const GenericBinding*>> {
+    return enclosing_dot_self_;
+  }
+  // Sets the enclosing value of `.Self`. Can only be called during name
+  // resolution.
+  void set_enclosing_dot_self(Nonnull<const GenericBinding*> dot_self) {
+    CARBON_CHECK(!enclosing_dot_self_ || enclosing_dot_self_ == dot_self);
+    enclosing_dot_self_ = dot_self;
+  }
+
   auto clauses() const -> llvm::ArrayRef<Nonnull<const WhereClause*>> {
     return clauses_;
   }
@@ -922,6 +933,7 @@ class WhereExpression : public RewritableMixin<Expression> {
  private:
   Nonnull<GenericBinding*> self_binding_;
   std::vector<Nonnull<WhereClause*>> clauses_;
+  std::optional<Nonnull<const GenericBinding*>> enclosing_dot_self_;
 };
 
 // An expression whose semantics have not been implemented. This can be used

explorer/interpreter/interpreter.cpp

@@ -1157,12 +1157,9 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
       return todo_.FinishAction(value);
     }
     case ExpressionKind::DotSelfExpression: {
-      // `.Self` always symbolically resolves to the self binding, even if it's
-      // not yet been type-checked.
       CARBON_CHECK(act.pos() == 0);
       const auto& dot_self = cast<DotSelfExpression>(exp);
-      return todo_.FinishAction(
-          arena_->New<VariableType>(&dot_self.self_binding()));
+      return todo_.FinishAction(*dot_self.self_binding().symbolic_identity());
     }
     case ExpressionKind::IntLiteral:
       CARBON_CHECK(act.pos() == 0);

explorer/interpreter/resolve_names.cpp

@@ -239,6 +239,14 @@ static auto ResolveNames(Expression& expression,
       auto& where = cast<WhereExpression>(expression);
       CARBON_RETURN_IF_ERROR(
           ResolveNames(where.self_binding().type(), enclosing_scope));
+      // If we're already in a `.Self` context, remember it so that we can
+      // reuse its value for the inner `.Self`.
+      if (auto enclosing_dot_self =
+              enclosing_scope.Resolve(".Self", where.source_loc());
+          enclosing_dot_self.ok()) {
+        where.set_enclosing_dot_self(
+            &cast<GenericBinding>(enclosing_dot_self->base()));
+      }
       // Introduce `.Self` into scope on the right of the `where` keyword.
       StaticScope where_scope;
       where_scope.AddParent(&enclosing_scope);

explorer/interpreter/type_checker.cpp

@@ -549,6 +549,11 @@ auto TypeChecker::ImplicitlyConvert(std::string_view context,
                                 destination));
     CARBON_ASSIGN_OR_RETURN(Nonnull<const Value*> source_value,
                             InterpExp(source, arena_, trace_stream_));
+    if (trace_stream_) {
+      **trace_stream_ << "converting type " << *source_value
+                      << " to constraint " << *destination_constraint << " for "
+                      << context << " in scope " << impl_scope << "\n";
+    }
     // Note, we discard the witness. We don't actually need it in order to
     // perform the conversion, but we do want to know it exists.
     CARBON_RETURN_IF_ERROR(impl_scope.Resolve(
@@ -756,15 +761,6 @@ auto TypeChecker::ArgumentDeduction::Deduce(Nonnull<const Value*> param,
   // different forms. In this case, we require an implicit conversion to exist,
   // or for an exact type match if implicit conversions are not permitted.
   auto handle_non_deduced_type = [&]() -> ErrorOr<Success> {
-    if (!IsConcreteType(param)) {
-      // Parameter type contains a nested `auto` and argument type isn't the
-      // same kind of type.
-      // TODO: This seems like something we should be able to accept.
-      return ProgramError(source_loc_) << "type error in " << context_ << "\n"
-                                       << "expected: " << *param << "\n"
-                                       << "actual: " << *arg;
-    }
-
     if (ValueEqual(param, arg, std::nullopt)) {
       return Success();
     }
@@ -1112,7 +1108,7 @@ class TypeChecker::ConstraintTypeBuilder {
   ConstraintTypeBuilder(Nonnull<Arena*> arena,
                         Nonnull<GenericBinding*> self_binding)
       : arena_(arena),
-        self_binding_(PrepareSelfBinding(arena, self_binding)),
+        self_binding_(self_binding),
         impl_binding_(AddImplBinding(arena, self_binding_)) {}
   ConstraintTypeBuilder(Nonnull<Arena*> arena,
                         Nonnull<GenericBinding*> self_binding,
@@ -1392,25 +1388,24 @@ class TypeChecker::ConstraintTypeBuilder {
     return result;
   }
 
+  // Sets up a `.Self` binding to act as the self type of a constraint.
+  static void PrepareSelfBinding(Nonnull<Arena*> arena,
+                                 Nonnull<GenericBinding*> self_binding) {
+    Nonnull<const Value*> self = arena->New<VariableType>(self_binding);
+    self_binding->set_symbolic_identity(self);
+    self_binding->set_value(self);
+  }
+
  private:
-  // Makes a generic binding to serve as the `.Self` of this constraint type.
+  // Makes a generic binding to serve as the `.Self` of a constraint type.
   static auto MakeSelfBinding(Nonnull<Arena*> arena, SourceLocation source_loc)
       -> Nonnull<GenericBinding*> {
     // Note, the type-of-type here is a placeholder and isn't really
     // meaningful.
-    return arena->New<GenericBinding>(source_loc, ".Self",
-                                      arena->New<TypeTypeLiteral>(source_loc));
-  }
-
-  // Sets up a `.Self` binding to act as the self type of this constraint.
-  static auto PrepareSelfBinding(Nonnull<Arena*> arena,
-                                 Nonnull<GenericBinding*> self_binding)
-      -> Nonnull<GenericBinding*> {
-    Nonnull<const Value*> self = arena->New<VariableType>(self_binding);
-    // TODO: Do we really need both of these?
-    self_binding->set_symbolic_identity(self);
-    self_binding->set_value(self);
-    return self_binding;
+    auto* result = arena->New<GenericBinding>(
+        source_loc, ".Self", arena->New<TypeTypeLiteral>(source_loc));
+    PrepareSelfBinding(arena, result);
+    return result;
   }
 
   // Adds an impl binding to the given self binding.
@@ -2063,6 +2058,7 @@ auto TypeChecker::LookupRewriteInTypeOf(
       // We looked for a rewrite before we finished type-checking the generic
       // binding. This happens when forming the type of a generic binding. Just
       // say there are no rewrites yet.
+      // TODO: `.Self` substitution should fix this.
       return std::nullopt;
     }
     return LookupRewrite(&var_type->binding().static_type(), interface, member);
@@ -2074,8 +2070,10 @@ auto TypeChecker::LookupRewriteInTypeOf(
   if (const auto* assoc_const = dyn_cast<AssociatedConstant>(type)) {
     if (!assoc_const->constant().has_static_type()) {
       // We looked for a rewrite before we finished type-checking the
-      // associated constant. This can happens when a use of `.Self` occurs
-      // within the constant's type. Just say there are no rewrites yet.
+      // associated constant. This happens when forming the type of the
+      // associated constant, if `.Self` is used to access an associated
+      // constant. Just say that there are not rewrites yet.
+      // TODO: `.Self` substitution should fix this.
       return std::nullopt;
     }
     // The following is an expanded version of
@@ -3208,6 +3206,18 @@ auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e,
       inner_impl_scope.AddParent(&impl_scope);
 
       auto& self = where.self_binding();
+
+      // If there's some enclosing `.Self` value, our self is symbolically
+      // equal to that. Otherwise it's a new type variable.
+      if (auto enclosing_dot_self = where.enclosing_dot_self()) {
+        // TODO: We need to also enforce that our `.Self` does end up being the
+        // same as the enclosing type.
+        self.set_symbolic_identity(*(*enclosing_dot_self)->symbolic_identity());
+        self.set_value(&(*enclosing_dot_self)->value());
+      } else {
+        ConstraintTypeBuilder::PrepareSelfBinding(arena_, &self);
+      }
+
       ConstraintTypeBuilder builder(arena_, &self);
       ConstraintTypeBuilder::ConstraintsInScopeTracker constraint_tracker;
 
@@ -3322,7 +3332,7 @@ auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e,
             CARBON_ASSIGN_OR_RETURN(
                 Nonnull<Expression*> converted_expression,
                 ImplicitlyConvert(
-                    "rewrite constraint", impl_scope, replacement_literal,
+                    "rewrite constraint", inner_impl_scope, replacement_literal,
                     GetTypeForAssociatedConstant(constant_value)));
             CARBON_ASSIGN_OR_RETURN(
                 Nonnull<const Value*> converted_value,
@@ -4434,6 +4444,7 @@ auto TypeChecker::DeclareInterfaceDeclaration(
   self_type->set_constant_value(iface_type);
 
   // Build a constraint corresponding to this interface.
+  ConstraintTypeBuilder::PrepareSelfBinding(arena_, iface_decl->self());
   ConstraintTypeBuilder builder(arena_, iface_decl->self());
   ConstraintTypeBuilder::ConstraintsInScopeTracker constraint_tracker;
   iface_decl->self()->set_static_type(iface_type);

explorer/interpreter/value.cpp

@@ -430,6 +430,9 @@ void Value::Print(llvm::raw_ostream& out) const {
       for (const LookupContext& ctx : constraint.lookup_contexts()) {
         out << combine << *ctx.context;
       }
+      if (constraint.lookup_contexts().empty()) {
+        out << "Type";
+      }
       out << " where ";
       llvm::ListSeparator sep(" and ");
       for (const RewriteConstraint& rewrite :
@@ -447,7 +450,6 @@ void Value::Print(llvm::raw_ostream& out) const {
       }
       for (const EqualityConstraint& equality :
            constraint.equality_constraints()) {
-        // TODO: Skip cases matching something in `rewrite_constraints()`.
         out << sep;
         llvm::ListSeparator equal(" == ");
         for (Nonnull<const Value*> value : equality.values) {

explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon

@@ -17,14 +17,15 @@ interface Y(T:! Type) {
 interface Z {
   // The `i32 is X(.Self)` constraint is indirectly required by
   // specifying that `.M = i32`.
-  // TODO: This testcase should be accepted, but is currently not because the
-  // two `.Self`s here refer to different symbolic types.
-  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon:[[@LINE+1]]: could not find implementation of interface X(T = N) for i32
   let N:! Y(.Self) where i32 is X(.Self) and .M = i32;
 }
 
 impl i32 as X(i32) {}
 impl i32 as Y(i32) where .M = i32 {}
+// TODO: This testcase should be accepted, but is currently not because the
+// rewrite for `.N` is not properly applied to impl constraints within the type
+// of N.
+// CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon:[[@LINE+1]]: could not find implementation of interface Y(T = (.Self).(Z.N)) for i32
 impl i32 as Z where .N = i32 {}
 
 fn F[A:! Z](a: A) -> A { return a; }

explorer/testdata/assoc_const/fail_implied_constraints.carbon

@@ -18,16 +18,10 @@ interface Z {
   // We reject this even though it is the responsibility of the `impl as Z` to
   // provide a type `N` such that `i32 is X(N)`. We might want to treat this as
   // an implied constraint and allow this in the future.
-  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/assoc_const/fail_implied_constraints.carbon:[[@LINE+1]]: could not find implementation of interface X(T = N) for i32
+  // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/assoc_const/fail_implied_constraints.carbon:[[@LINE+1]]: could not find implementation of interface X(T = (Self).(Z.N)) for i32
   let N:! Y(.Self) where .M = i32;
 }
 
-impl i32 as X(i32) {}
-impl i32 as Y(i32) where .M = i32 {}
-impl i32 as Z where .N = i32 {}
-
-fn F[A:! Z](a: A) -> A { return a; }
-
 fn Main() -> i32 {
-  return F(0);
+  return 0;
 }

explorer/testdata/assoc_const/fail_rewrite_depends_on_later_rewrite.carbon

@@ -13,7 +13,7 @@ interface HasTypeAndValue {
   let V:! T;
 }
 
-// CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/assoc_const/fail_rewrite_depends_on_later_rewrite.carbon:[[@LINE+1]]: type error in rewrite constraint: 'i32' is not implicitly convertible to '(.Self).(HasTypeAndValue.T)'
+// CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/assoc_const/fail_rewrite_depends_on_later_rewrite.carbon:[[@LINE+1]]: type error in rewrite constraint: 'i32' is not implicitly convertible to '(X).(HasTypeAndValue.T)'
 fn F(X:! HasTypeAndValue where .V = 5 and .T = i32) -> i32 { return X.V; }
 
 impl i32 as HasTypeAndValue where .T = i32 and .V = 5 {}