Distinguish between the symbolic .Self value for an associated constant and the value found when referencing one. (#2348)

Within the type of an associated constant, references to `.Self` should resolve symbolically to that associated constant as a `GenericBinding` so that it can be substituted for the actual value when using its type. However, when the associated constant is referenced from elsewhere in the same interface, the value we want is a symbolic value naming the constant as a member of `Self`.

explorer/ast/declaration.h

@@ -599,6 +599,8 @@ class InterfaceImplDeclaration : public Declaration {
 
 class AssociatedConstantDeclaration : public Declaration {
  public:
+  using ImplementsCarbonValueNode = void;
+
   AssociatedConstantDeclaration(SourceLocation source_loc,
                                 Nonnull<GenericBinding*> binding)
       : Declaration(AstNodeKind::AssociatedConstantDeclaration, source_loc),

explorer/interpreter/resolve_names.cpp

@@ -77,8 +77,7 @@ static auto AddExposedNames(const Declaration& declaration,
     case DeclarationKind::AssociatedConstantDeclaration: {
       const auto& let = cast<AssociatedConstantDeclaration>(declaration);
       if (let.binding().name() != AnonymousName) {
-        CARBON_RETURN_IF_ERROR(
-            enclosing_scope.Add(let.binding().name(), &let.binding()));
+        CARBON_RETURN_IF_ERROR(enclosing_scope.Add(let.binding().name(), &let));
       }
       break;
     }
@@ -666,7 +665,9 @@ static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope,
     }
     case DeclarationKind::AssociatedConstantDeclaration: {
       auto& let = cast<AssociatedConstantDeclaration>(declaration);
-      CARBON_RETURN_IF_ERROR(ResolveNames(let.binding(), enclosing_scope));
+      StaticScope constant_scope;
+      constant_scope.AddParent(&enclosing_scope);
+      CARBON_RETURN_IF_ERROR(ResolveNames(let.binding(), constant_scope));
       break;
     }
 

explorer/interpreter/type_checker.cpp

@@ -3552,9 +3552,7 @@ auto TypeChecker::TypeCheckPattern(
                << binding;
       }
 
-      auto* val = arena_->New<VariableType>(&binding);
-      return TypeCheckGenericBinding(binding, "generic binding", val,
-                                     impl_scope);
+      return TypeCheckGenericBinding(binding, "generic binding", impl_scope);
     }
     case PatternKind::TuplePattern: {
       auto& tuple = cast<TuplePattern>(*p);
@@ -3675,11 +3673,11 @@ auto TypeChecker::TypeCheckPattern(
 
 auto TypeChecker::TypeCheckGenericBinding(GenericBinding& binding,
                                           std::string_view context,
-                                          Nonnull<const Value*> symbolic_value,
                                           ImplScope& impl_scope)
     -> ErrorOr<Success> {
   // The binding can be referred to in its own type via `.Self`, so set up
   // its symbolic identity before we type-check and interpret the type.
+  auto* symbolic_value = arena_->New<VariableType>(&binding);
   binding.set_symbolic_identity(symbolic_value);
   SetValue(&binding, symbolic_value);
 
@@ -3705,9 +3703,8 @@ auto TypeChecker::TypeCheckGenericBinding(GenericBinding& binding,
     // Substitute the VariableType as `.Self` of the constraint to form the
     // resolved type of the binding. Eg, `T:! X where .Self is Y` resolves
     // to `T:! <constraint T is X and T is Y>`.
-    auto* binding_self_type = arena_->New<VariableType>(&binding);
     ConstraintTypeBuilder builder(arena_, &binding, impl_binding);
-    builder.AddAndSubstitute(*this, constraint, binding_self_type, witness,
+    builder.AddAndSubstitute(*this, constraint, symbolic_value, witness,
                              Bindings(), /*add_lookup_contexts=*/true);
     if (trace_stream_) {
       **trace_stream_ << "resolving constraint type for " << binding << " from "
@@ -4503,13 +4500,20 @@ auto TypeChecker::DeclareInterfaceDeclaration(
 
       case DeclarationKind::AssociatedConstantDeclaration: {
         auto* assoc = cast<AssociatedConstantDeclaration>(m);
-        auto* assoc_value = arena_->New<AssociatedConstant>(
-            &iface_decl->self()->value(), iface_type, assoc, impl_witness);
         CARBON_RETURN_IF_ERROR(TypeCheckGenericBinding(
-            assoc->binding(), "associated constant", assoc_value, iface_scope));
+            assoc->binding(), "associated constant", iface_scope));
         Nonnull<const Value*> constraint = &assoc->binding().static_type();
         assoc->set_static_type(constraint);
 
+        // The constant value is used if the constant is named later in the
+        // same interface. Note that this differs from the symbolic identity of
+        // the binding, which was set in TypeCheckGenericBinding to a
+        // VariableType naming the binding so that .Self resolves to the
+        // binding itself.
+        auto* assoc_value = arena_->New<AssociatedConstant>(
+            &iface_decl->self()->value(), iface_type, assoc, impl_witness);
+        assoc->set_constant_value(assoc_value);
+
         // The type specified for the associated constant becomes a
         // constraint for the interface: `let X:! Interface` adds a `Self.X
         // is Interface` constraint that `impl`s must satisfy and users of

explorer/interpreter/type_checker.h

@@ -165,9 +165,8 @@ class TypeChecker {
   // which this generic binding is known in its scope. `impl_scope` is updated
   // with the impl implied by the binding, if any.
   auto TypeCheckGenericBinding(GenericBinding& binding,
-                               std::string_view context,
-                               Nonnull<const Value*> symbolic_value,
-                               ImplScope& impl_scope) -> ErrorOr<Success>;
+                               std::string_view context, ImplScope& impl_scope)
+      -> ErrorOr<Success>;
 
   // Equivalent to TypeCheckExp, but operates on the AST rooted at `s`.
   //

explorer/testdata/assoc_const/fail_implied_constraints.carbon

@@ -18,7 +18,7 @@ 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 = (Self).(Z.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 = N) for i32
   let N:! Y(.Self) where .M = i32;
 }
 

explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon → explorer/testdata/assoc_const/impl_used_by_later_rewrite.carbon

@@ -3,8 +3,9 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 // AUTOUPDATE
-// RUN: %{not} %{explorer-run}
-// RUN: %{not} %{explorer-run-trace}
+// RUN: %{explorer-run}
+// RUN: %{explorer-run-trace}
+// CHECK:STDOUT: result: 0
 
 package ExplorerTest api;
 
@@ -22,10 +23,6 @@ interface Z {
 
 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; }