From 5c3f48b0fbfa0e5983e14f2a4f7bf86b552b9686 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 25 Oct 2022 12:29:51 -0700 Subject: [PATCH] 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 | 2 ++ explorer/interpreter/resolve_names.cpp | 7 +++--- explorer/interpreter/type_checker.cpp | 22 +++++++++++-------- explorer/interpreter/type_checker.h | 5 ++--- .../fail_implied_constraints.carbon | 2 +- ...rbon => impl_used_by_later_rewrite.carbon} | 9 +++----- 6 files changed, 25 insertions(+), 22 deletions(-) rename explorer/testdata/assoc_const/{fail_impl_used_by_later_rewrite.carbon => impl_used_by_later_rewrite.carbon} (59%) diff --git a/explorer/ast/declaration.h b/explorer/ast/declaration.h index 404b6e81a7da..cee411c9f406 100644 --- a/explorer/ast/declaration.h +++ b/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 binding) : Declaration(AstNodeKind::AssociatedConstantDeclaration, source_loc), diff --git a/explorer/interpreter/resolve_names.cpp b/explorer/interpreter/resolve_names.cpp index 4010b7a6a5fc..05240ab6f7c6 100644 --- a/explorer/interpreter/resolve_names.cpp +++ b/explorer/interpreter/resolve_names.cpp @@ -77,8 +77,7 @@ static auto AddExposedNames(const Declaration& declaration, case DeclarationKind::AssociatedConstantDeclaration: { const auto& let = cast(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(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; } diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index 3def63a32c7d..8abaf8bd8982 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -3552,9 +3552,7 @@ auto TypeChecker::TypeCheckPattern( << binding; } - auto* val = arena_->New(&binding); - return TypeCheckGenericBinding(binding, "generic binding", val, - impl_scope); + return TypeCheckGenericBinding(binding, "generic binding", impl_scope); } case PatternKind::TuplePattern: { auto& tuple = cast(*p); @@ -3675,11 +3673,11 @@ auto TypeChecker::TypeCheckPattern( auto TypeChecker::TypeCheckGenericBinding(GenericBinding& binding, std::string_view context, - Nonnull symbolic_value, ImplScope& impl_scope) -> ErrorOr { // 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(&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:! `. - auto* binding_self_type = arena_->New(&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(m); - auto* assoc_value = arena_->New( - &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 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( + &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 diff --git a/explorer/interpreter/type_checker.h b/explorer/interpreter/type_checker.h index 1693fa4a5df6..b612d0d8e049 100644 --- a/explorer/interpreter/type_checker.h +++ b/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 symbolic_value, - ImplScope& impl_scope) -> ErrorOr; + std::string_view context, ImplScope& impl_scope) + -> ErrorOr; // Equivalent to TypeCheckExp, but operates on the AST rooted at `s`. // diff --git a/explorer/testdata/assoc_const/fail_implied_constraints.carbon b/explorer/testdata/assoc_const/fail_implied_constraints.carbon index 62ea135831cb..e00540b8d14a 100644 --- a/explorer/testdata/assoc_const/fail_implied_constraints.carbon +++ b/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; } diff --git a/explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon b/explorer/testdata/assoc_const/impl_used_by_later_rewrite.carbon similarity index 59% rename from explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon rename to explorer/testdata/assoc_const/impl_used_by_later_rewrite.carbon index 5dd6ecb4a7ff..d7830712a1ca 100644 --- a/explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon +++ b/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; }