From bc7bf325d6416307e3ef0fbf8541b95cb2dbcf1c Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 25 Oct 2022 11:59:42 -0700 Subject: [PATCH] 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 | 12 ++++ explorer/interpreter/interpreter.cpp | 5 +- explorer/interpreter/resolve_names.cpp | 8 +++ explorer/interpreter/type_checker.cpp | 65 +++++++++++-------- explorer/interpreter/value.cpp | 4 +- .../fail_impl_used_by_later_rewrite.carbon | 7 +- .../fail_implied_constraints.carbon | 10 +-- ...il_rewrite_depends_on_later_rewrite.carbon | 2 +- 8 files changed, 69 insertions(+), 44 deletions(-) diff --git a/explorer/ast/expression.h b/explorer/ast/expression.h index 1274839c85e5..0698f8b3f644 100644 --- a/explorer/ast/expression.h +++ b/explorer/ast/expression.h @@ -914,6 +914,17 @@ class WhereExpression : public RewritableMixin { auto self_binding() const -> const GenericBinding& { return *self_binding_; } auto self_binding() -> GenericBinding& { return *self_binding_; } + auto enclosing_dot_self() const + -> std::optional> { + return enclosing_dot_self_; + } + // Sets the enclosing value of `.Self`. Can only be called during name + // resolution. + void set_enclosing_dot_self(Nonnull dot_self) { + CARBON_CHECK(!enclosing_dot_self_ || enclosing_dot_self_ == dot_self); + enclosing_dot_self_ = dot_self; + } + auto clauses() const -> llvm::ArrayRef> { return clauses_; } @@ -922,6 +933,7 @@ class WhereExpression : public RewritableMixin { private: Nonnull self_binding_; std::vector> clauses_; + std::optional> enclosing_dot_self_; }; // An expression whose semantics have not been implemented. This can be used diff --git a/explorer/interpreter/interpreter.cpp b/explorer/interpreter/interpreter.cpp index ffe5ee82685b..33944410f69b 100644 --- a/explorer/interpreter/interpreter.cpp +++ b/explorer/interpreter/interpreter.cpp @@ -1157,12 +1157,9 @@ auto Interpreter::StepExp() -> ErrorOr { 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(exp); - return todo_.FinishAction( - arena_->New(&dot_self.self_binding())); + return todo_.FinishAction(*dot_self.self_binding().symbolic_identity()); } case ExpressionKind::IntLiteral: CARBON_CHECK(act.pos() == 0); diff --git a/explorer/interpreter/resolve_names.cpp b/explorer/interpreter/resolve_names.cpp index 6f32d6b5cd43..4010b7a6a5fc 100644 --- a/explorer/interpreter/resolve_names.cpp +++ b/explorer/interpreter/resolve_names.cpp @@ -239,6 +239,14 @@ static auto ResolveNames(Expression& expression, auto& where = cast(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(enclosing_dot_self->base())); + } // Introduce `.Self` into scope on the right of the `where` keyword. StaticScope where_scope; where_scope.AddParent(&enclosing_scope); diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index 8df4a5187627..3def63a32c7d 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -549,6 +549,11 @@ auto TypeChecker::ImplicitlyConvert(std::string_view context, destination)); CARBON_ASSIGN_OR_RETURN(Nonnull 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 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 { - 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, Nonnull self_binding) : arena_(arena), - self_binding_(PrepareSelfBinding(arena, self_binding)), + self_binding_(self_binding), impl_binding_(AddImplBinding(arena, self_binding_)) {} ConstraintTypeBuilder(Nonnull arena, Nonnull 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, + Nonnull self_binding) { + Nonnull self = arena->New(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, SourceLocation source_loc) -> Nonnull { // Note, the type-of-type here is a placeholder and isn't really // meaningful. - return arena->New(source_loc, ".Self", - arena->New(source_loc)); - } - - // Sets up a `.Self` binding to act as the self type of this constraint. - static auto PrepareSelfBinding(Nonnull arena, - Nonnull self_binding) - -> Nonnull { - Nonnull self = arena->New(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( + source_loc, ".Self", arena->New(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(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 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 e, CARBON_ASSIGN_OR_RETURN( Nonnull converted_expression, ImplicitlyConvert( - "rewrite constraint", impl_scope, replacement_literal, + "rewrite constraint", inner_impl_scope, replacement_literal, GetTypeForAssociatedConstant(constant_value))); CARBON_ASSIGN_OR_RETURN( Nonnull 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); diff --git a/explorer/interpreter/value.cpp b/explorer/interpreter/value.cpp index c5360ae12534..dc938d25d382 100644 --- a/explorer/interpreter/value.cpp +++ b/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 value : equality.values) { diff --git a/explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon b/explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon index 28bf1afa8a8c..5dd6ecb4a7ff 100644 --- a/explorer/testdata/assoc_const/fail_impl_used_by_later_rewrite.carbon +++ b/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; } diff --git a/explorer/testdata/assoc_const/fail_implied_constraints.carbon b/explorer/testdata/assoc_const/fail_implied_constraints.carbon index 4907db55b77d..62ea135831cb 100644 --- a/explorer/testdata/assoc_const/fail_implied_constraints.carbon +++ b/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; } diff --git a/explorer/testdata/assoc_const/fail_rewrite_depends_on_later_rewrite.carbon b/explorer/testdata/assoc_const/fail_rewrite_depends_on_later_rewrite.carbon index 71ad34fde34a..f1f99c35457d 100644 --- a/explorer/testdata/assoc_const/fail_rewrite_depends_on_later_rewrite.carbon +++ b/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 {}