From e557d4af3b2ac879cafde8993d02015453ccb78d Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 7 Nov 2022 08:11:19 -0800 Subject: [PATCH] Make `addr me: Self*` work for interface methods. (#2374) Fixes #2223. --- explorer/ast/expression.h | 19 +++--- explorer/interpreter/interpreter.cpp | 10 +++- explorer/interpreter/type_checker.cpp | 60 ++++++++++++------- explorer/interpreter/type_checker.h | 8 +++ explorer/interpreter/value.cpp | 2 +- explorer/testdata/addr/fail_method_let.carbon | 2 +- .../testdata/addr/fail_method_me_type.carbon | 4 +- .../destructor/dont_call_in_method.carbon | 1 - explorer/testdata/interface/addr_me.carbon | 50 ++++++++++++++++ 9 files changed, 118 insertions(+), 38 deletions(-) create mode 100644 explorer/testdata/interface/addr_me.carbon diff --git a/explorer/ast/expression.h b/explorer/ast/expression.h index ee9ddae7b631..d9dc7827f6ff 100644 --- a/explorer/ast/expression.h +++ b/explorer/ast/expression.h @@ -263,6 +263,13 @@ class MemberAccessExpression : public Expression { // Can only be called once, during typechecking. void set_is_type_access(bool type_access) { is_type_access_ = type_access; } + // Returns true if the member is a method that has a "me" declaration in an + // AddrPattern. + auto is_addr_me_method() const -> bool { return is_addr_me_method_; } + + // Can only be called once, during typechecking. + void set_is_addr_me_method() { is_addr_me_method_ = true; } + // If `object` has a generic type, returns the witness value, which might be // either concrete or symbolic. Otherwise, returns `std::nullopt`. Should not // be called before typechecking. @@ -292,6 +299,7 @@ class MemberAccessExpression : public Expression { private: Nonnull object_; bool is_type_access_ = false; + bool is_addr_me_method_ = false; std::optional> impl_; std::optional> constant_value_; }; @@ -324,16 +332,6 @@ class SimpleMemberAccessExpression : public MemberAccessExpression { member_ = member; } - // Returns true if the field is a method that has a "me" declaration in an - // AddrPattern. - // TODO: Should be in MemberAccessExpression. - auto is_field_addr_me_method() const -> bool { - return is_field_addr_me_method_; - } - - // Can only be called once, during typechecking. - void set_is_field_addr_me_method() { is_field_addr_me_method_ = true; } - // If `object` is a constrained type parameter and `member` was found in an // interface, returns that interface. Should not be called before // typechecking. @@ -351,7 +349,6 @@ class SimpleMemberAccessExpression : public MemberAccessExpression { private: std::string member_name_; std::optional member_; - bool is_field_addr_me_method_ = false; std::optional> found_in_interface_; }; diff --git a/explorer/interpreter/interpreter.cpp b/explorer/interpreter/interpreter.cpp index b653824cd714..2119cf7108aa 100644 --- a/explorer/interpreter/interpreter.cpp +++ b/explorer/interpreter/interpreter.cpp @@ -1043,7 +1043,7 @@ auto Interpreter::StepExp() -> ErrorOr { bool forming_member_name = isa(&access.static_type()); if (act.pos() == 0) { // First, evaluate the first operand. - if (access.is_field_addr_me_method()) { + if (access.is_addr_me_method()) { return todo_.Spawn(std::make_unique(&access.object())); } else { return todo_.Spawn( @@ -1119,8 +1119,12 @@ auto Interpreter::StepExp() -> ErrorOr { bool forming_member_name = isa(&access.static_type()); if (act.pos() == 0) { // First, evaluate the first operand. - return todo_.Spawn( - std::make_unique(&access.object())); + if (access.is_addr_me_method()) { + return todo_.Spawn(std::make_unique(&access.object())); + } else { + return todo_.Spawn( + std::make_unique(&access.object())); + } } else if (act.pos() == 1 && access.impl().has_value() && !forming_member_name) { // Next, if we're accessing an interface member, evaluate the `impl` diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index 2cfdab64dc47..c915caeb60cc 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -2430,6 +2430,27 @@ static auto IsInstanceMember(Member member) { } } +auto TypeChecker::CheckAddrMeAccess( + Nonnull access, + Nonnull func_decl, const Bindings& bindings, + const ImplScope& impl_scope) -> ErrorOr { + if (func_decl->is_method() && + func_decl->me_pattern().kind() == PatternKind::AddrPattern) { + access->set_is_addr_me_method(); + Nonnull me_type = + Substitute(bindings, &func_decl->me_pattern().static_type()); + CARBON_RETURN_IF_ERROR( + ExpectExactType(access->source_loc(), "method access, receiver type", + me_type, &access->object().static_type(), impl_scope)); + if (access->object().value_category() != ValueCategory::Var) { + return ProgramError(access->source_loc()) + << "method " << *access + << " requires its receiver to be an lvalue"; + } + } + return Success(); +} + auto TypeChecker::TypeCheckExp(Nonnull e, const ImplScope& impl_scope) -> ErrorOr { @@ -2568,21 +2589,8 @@ auto TypeChecker::TypeCheckExp(Nonnull e, break; case DeclarationKind::FunctionDeclaration: { const auto* func_decl = cast(member); - if (func_decl->is_method() && func_decl->me_pattern().kind() == - PatternKind::AddrPattern) { - access.set_is_field_addr_me_method(); - Nonnull me_type = - Substitute(t_class.bindings(), - &func_decl->me_pattern().static_type()); - CARBON_RETURN_IF_ERROR(ExpectType( - e->source_loc(), "method access, receiver type", me_type, - &access.object().static_type(), impl_scope)); - if (access.object().value_category() != ValueCategory::Var) { - return ProgramError(e->source_loc()) - << "method " << access.member_name() - << " requires its receiver to be an lvalue"; - } - } + CARBON_RETURN_IF_ERROR(CheckAddrMeAccess( + &access, func_decl, t_class.bindings(), impl_scope)); access.set_value_category(ValueCategory::Let); break; } @@ -2650,6 +2658,11 @@ auto TypeChecker::TypeCheckExp(Nonnull e, access.set_is_type_access(!IsInstanceMember(access.member())); access.set_static_type(inst_member_type); + if (auto* func_decl = dyn_cast(result.member)) { + CARBON_RETURN_IF_ERROR( + CheckAddrMeAccess(&access, func_decl, bindings, impl_scope)); + } + // TODO: This is just a ConstraintImplWitness into the // iface_constraint. If we can compute the right index, we can avoid // re-resolving it. @@ -2904,19 +2917,23 @@ auto TypeChecker::TypeCheckExp(Nonnull e, access.set_impl(impl); } - auto substitute_into_member_type = [&]() { - Nonnull member_type = &member_name.member().type(); + auto bindings_for_member = [&]() -> Bindings { if (member_name.interface()) { Nonnull iface_type = *member_name.interface(); Bindings bindings = iface_type->bindings(); bindings.Add(iface_type->declaration().self(), *base_type, witness); - return Substitute(bindings, member_type); + return bindings; } if (const auto* class_type = dyn_cast(base_type.value())) { - return Substitute(class_type->bindings(), member_type); + return class_type->bindings(); } - return member_type; + return Bindings(); + }; + + auto substitute_into_member_type = [&]() { + Nonnull member_type = &member_name.member().type(); + return Substitute(bindings_for_member(), member_type); }; switch (std::optional> decl = @@ -2939,6 +2956,9 @@ auto TypeChecker::TypeCheckExp(Nonnull e, << "vacuous compound member access"; access.set_static_type(substitute_into_member_type()); access.set_value_category(ValueCategory::Let); + CARBON_RETURN_IF_ERROR( + CheckAddrMeAccess(&access, cast(*decl), + bindings_for_member(), impl_scope)); return Success(); } break; diff --git a/explorer/interpreter/type_checker.h b/explorer/interpreter/type_checker.h index 6dbcc768a18f..ba9b89b2fd9f 100644 --- a/explorer/interpreter/type_checker.h +++ b/explorer/interpreter/type_checker.h @@ -129,6 +129,14 @@ class TypeChecker { Nonnull member; }; + // Checks a member access that might be accessing a function taking `addr me: + // Self*`. If it does, this function marks the member access accordingly and + // ensures the object argument is an lvalue. + auto CheckAddrMeAccess(Nonnull access, + Nonnull func_decl, + const Bindings& bindings, const ImplScope& impl_scope) + -> ErrorOr; + // Traverses the AST rooted at `e`, populating the static_type() of all nodes // and ensuring they follow Carbon's typing rules. // diff --git a/explorer/interpreter/value.cpp b/explorer/interpreter/value.cpp index a476ce9a63b1..2eaa40869401 100644 --- a/explorer/interpreter/value.cpp +++ b/explorer/interpreter/value.cpp @@ -59,7 +59,7 @@ static auto GetMember(Nonnull arena, Nonnull v, mem_decl.has_value()) { const auto& fun_decl = cast(**mem_decl); if (fun_decl.is_method()) { - return arena->New(&fun_decl, v, + return arena->New(&fun_decl, me_value, &impl_witness->bindings()); } else { // Class function. diff --git a/explorer/testdata/addr/fail_method_let.carbon b/explorer/testdata/addr/fail_method_let.carbon index 293b6e01c795..5e72f9455333 100644 --- a/explorer/testdata/addr/fail_method_let.carbon +++ b/explorer/testdata/addr/fail_method_let.carbon @@ -26,7 +26,7 @@ class Point { fn Main() -> i32 { let p: Point = Point.Origin(); - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/addr/fail_method_let.carbon:[[@LINE+1]]: method GetSetX requires its receiver to be an lvalue + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/addr/fail_method_let.carbon:[[@LINE+1]]: method p.GetSetX requires its receiver to be an lvalue var x: auto = p.GetSetX(42); if (p.x == 42) { return x; diff --git a/explorer/testdata/addr/fail_method_me_type.carbon b/explorer/testdata/addr/fail_method_me_type.carbon index bdd5c64a798f..584309d7a716 100644 --- a/explorer/testdata/addr/fail_method_me_type.carbon +++ b/explorer/testdata/addr/fail_method_me_type.carbon @@ -30,7 +30,9 @@ class Point { fn Main() -> i32 { var p: Point = Point.Origin(); - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/addr/fail_method_me_type.carbon:[[@LINE+1]]: type error in method access, receiver type: 'class Point' is not implicitly convertible to 'class Shape' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/addr/fail_method_me_type.carbon:[[@LINE+3]]: type error in method access, receiver type + // CHECK:STDERR: expected: class Shape + // CHECK:STDERR: actual: class Point var x: auto = p.GetSetX(42); if (p.x == 42) { return x; diff --git a/explorer/testdata/destructor/dont_call_in_method.carbon b/explorer/testdata/destructor/dont_call_in_method.carbon index f0d05acf03f4..3e116bf2d731 100644 --- a/explorer/testdata/destructor/dont_call_in_method.carbon +++ b/explorer/testdata/destructor/dont_call_in_method.carbon @@ -5,7 +5,6 @@ // AUTOUPDATE // RUN: %{explorer-run} // RUN: %{explorer-run-trace} -// AUTOUPDATE: %{explorer} %s // CHECK:STDOUT: TEST // CHECK:STDOUT: TEST 2 // CHECK:STDOUT: DESTRUCTOR A 1 diff --git a/explorer/testdata/interface/addr_me.carbon b/explorer/testdata/interface/addr_me.carbon new file mode 100644 index 000000000000..b153f2bbb3f6 --- /dev/null +++ b/explorer/testdata/interface/addr_me.carbon @@ -0,0 +1,50 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// RUN: %{explorer-run} +// RUN: %{explorer-run-trace} +// CHECK:STDOUT: 75 +// CHECK:STDOUT: 100 +// CHECK:STDOUT: result: 0 + +package ExplorerTest api; + +interface Vector { + fn Zero() -> Self; + fn Add[addr me: Self*](b: Self); + fn Scale[addr me: Self*](v: i32); +} + +class Point { + var x: i32; + var y: i32; + impl as Vector { + fn Zero() -> Self { + return {.x = 1, .y = 1}; + } + fn Add[addr me: Self*](b: Self) { + (*me).x = (*me).x + b.x; + (*me).y = (*me).y + b.y; + } + fn Scale[addr me: Self*](v: i32) { + (*me).x = (*me).x * v; + (*me).y = (*me).y * v; + } + } +} + +fn AddAndScaleGeneric[T:! Vector](p: T*, s: i32) { + (*p).Add(T.Zero()); + (*p).(Vector.Scale)(s); + (*p).(T.(Vector.Scale))(s); +} + +fn Main() -> i32 { + var a: Point = {.x = 2, .y = 3}; + AddAndScaleGeneric(&a, 5); + Print("{0}", a.x); + Print("{0}", a.y); + return 0; +}