From af0710ea16bae503e860eca1fd97e9d671ccfac3 Mon Sep 17 00:00:00 2001 From: Lucile Rose Nihlen Date: Tue, 29 Aug 2023 13:46:39 -0700 Subject: [PATCH] Add `addr self` binding support in destructors (#3117) Refactors `CallDestructor` and `CallFunction` to both call a new method `BindSelfIfPresent`, which includes support for binding `addr self` if specified. Fixes the associated unit test. Closes #2802. --- explorer/interpreter/interpreter.cpp | 124 ++++++++++-------- .../{fail_addr.carbon => self_addr.carbon} | 4 +- .../testdata/trace/phase_execution.carbon | 2 + 3 files changed, 75 insertions(+), 55 deletions(-) rename explorer/testdata/destructor/{fail_addr.carbon => self_addr.carbon} (77%) diff --git a/explorer/interpreter/interpreter.cpp b/explorer/interpreter/interpreter.cpp index c603974c9b45..8c4779987fbf 100644 --- a/explorer/interpreter/interpreter.cpp +++ b/explorer/interpreter/interpreter.cpp @@ -182,8 +182,17 @@ class Interpreter { std::optional location_received) -> ErrorOr; + // Call the destructor method in `fun`, with any self argument bound to + // `receiver`. auto CallDestructor(Nonnull fun, - Nonnull receiver) -> ErrorOr; + ExpressionResult receiver) -> ErrorOr; + + // If the given method or destructor `decl` has a self argument, bind it to + // `receiver`. + void BindSelfIfPresent(Nonnull decl, + ExpressionResult receiver, RuntimeScope& method_scope, + BindingMap& generic_args, + const SourceLocation& source_location); auto phase() const -> Phase { return phase_; } @@ -848,39 +857,6 @@ auto Interpreter::Convert(Nonnull value, } } -auto Interpreter::CallDestructor(Nonnull fun, - Nonnull receiver) - -> ErrorOr { - const DestructorDeclaration& method = *fun; - CARBON_CHECK(method.is_method()); - RuntimeScope method_scope(&heap_); - BindingMap generic_args; - - // TODO: move this logic into PatternMatch, and call it here. - const auto* p = &method.self_pattern().value(); - const auto* placeholder = dyn_cast(p); - if (!placeholder) { - // TODO: Fix this, probably merging logic with CallFunction. - // https://github.com/carbon-language/carbon-lang/issues/2802 - return ProgramError(fun->source_loc()) - << "destructors currently don't support `addr self` bindings"; - } - if (auto& value_node = placeholder->value_node()) { - if (value_node->expression_category() == ExpressionCategory::Value) { - method_scope.BindValue(*placeholder->value_node(), receiver); - } else { - CARBON_FATAL() - << "TODO: [self addr: Self*] destructors not implemented yet"; - } - } - CARBON_CHECK(method.body().has_value()) - << "Calling a method that's missing a body"; - - auto act = std::make_unique(*method.body(), std::nullopt); - return todo_.Spawn(std::unique_ptr(std::move(act)), - std::move(method_scope)); -} - auto Interpreter::CallFunction(const CallExpression& call, Nonnull fun, Nonnull arg, @@ -941,25 +917,9 @@ auto Interpreter::CallFunction(const CallExpression& call, // Bind the receiver to the `self` parameter, if there is one. if (const auto* method_val = dyn_cast(func_val)) { - CARBON_CHECK(function.is_method()); - const auto* self_pattern = &function.self_pattern().value(); - if (const auto* placeholder = - dyn_cast(self_pattern)) { - // Immutable self with `[self: Self]` - // TODO: move this logic into PatternMatch - if (placeholder->value_node().has_value()) { - function_scope.BindValue(*placeholder->value_node(), - method_val->receiver()); - } - } else { - // Mutable self with `[addr self: Self*]` - CARBON_CHECK(isa(self_pattern)); - bool success = PatternMatch( - self_pattern, ExpressionResult::Value(method_val->receiver()), - call.source_loc(), &function_scope, generic_args, trace_stream_, - this->arena_); - CARBON_CHECK(success) << "Failed to bind addr self"; - } + BindSelfIfPresent(&function, + ExpressionResult::Value(method_val->receiver()), + function_scope, generic_args, call.source_loc()); } CARBON_ASSIGN_OR_RETURN( @@ -1013,6 +973,60 @@ auto Interpreter::CallFunction(const CallExpression& call, } } +auto Interpreter::CallDestructor(Nonnull fun, + ExpressionResult receiver) + -> ErrorOr { + const DestructorDeclaration& method = *fun; + CARBON_CHECK(method.is_method()); + + RuntimeScope method_scope(&heap_); + BindingMap generic_args; + BindSelfIfPresent(fun, receiver, method_scope, generic_args, + SourceLocation::DiagnosticsIgnored()); + + CARBON_CHECK(method.body().has_value()) + << "Calling a method that's missing a body"; + + auto act = std::make_unique(*method.body(), std::nullopt); + return todo_.Spawn(std::unique_ptr(std::move(act)), + std::move(method_scope)); +} + +void Interpreter::BindSelfIfPresent(Nonnull decl, + ExpressionResult receiver, + RuntimeScope& method_scope, + BindingMap& generic_args, + const SourceLocation& source_location) { + CARBON_CHECK(decl->is_method()); + const auto* self_pattern = &decl->self_pattern().value(); + if (const auto* placeholder = + dyn_cast(self_pattern)) { + // Immutable self with `[self: Self]` + if (placeholder->value_node().has_value()) { + bool success = + PatternMatch(placeholder, receiver, source_location, &method_scope, + generic_args, trace_stream_, this->arena_); + CARBON_CHECK(success) << "Failed to bind self"; + } + } else { + // Mutable self with `[addr self: Self*]` + CARBON_CHECK(isa(self_pattern)); + ExpressionResult v = receiver; + // See if we need to make a LocationValue from the address provided in the + // ExpressionResult + if (receiver.value()->kind() != Value::Kind::LocationValue) { + CARBON_CHECK(receiver.expression_category() == + ExpressionCategory::Reference); + CARBON_CHECK(receiver.address().has_value()); + v = ExpressionResult::Value( + arena_->New(receiver.address().value())); + } + bool success = PatternMatch(self_pattern, v, source_location, &method_scope, + generic_args, trace_stream_, this->arena_); + CARBON_CHECK(success) << "Failed to bind addr self"; + } +} + // Returns true if the format string is okay to pass to formatv. This only // supports `{{` and `{N}` as special syntax. static auto ValidateFormatString(SourceLocation source_loc, @@ -2395,7 +2409,9 @@ auto Interpreter::StepDestroy() -> ErrorOr { if (act.pos() == 0) { // Run the destructor, if there is one. if (auto destructor = class_decl.destructor()) { - return CallDestructor(*destructor, class_obj); + return CallDestructor( + *destructor, ExpressionResult::Reference( + class_obj, destroy_act.location()->address())); } else { return todo_.RunAgain(); } diff --git a/explorer/testdata/destructor/fail_addr.carbon b/explorer/testdata/destructor/self_addr.carbon similarity index 77% rename from explorer/testdata/destructor/fail_addr.carbon rename to explorer/testdata/destructor/self_addr.carbon index 63270a0f15fd..1385e3021636 100644 --- a/explorer/testdata/destructor/fail_addr.carbon +++ b/explorer/testdata/destructor/self_addr.carbon @@ -10,7 +10,6 @@ class A { destructor[addr self: Self*] { self->n += 1; Print("DESTRUCTOR A {0}", self->n); - // CHECK:STDERR: RUNTIME ERROR: fail_addr.carbon:[[@LINE+1]]: destructors currently don't support `addr self` bindings } var n: i32; @@ -20,3 +19,6 @@ fn Main() -> i32 { var a: A = {.n = 2}; return 0; } + +// CHECK:STDOUT: DESTRUCTOR A 3 +// CHECK:STDOUT: result: 0 diff --git a/explorer/testdata/trace/phase_execution.carbon b/explorer/testdata/trace/phase_execution.carbon index a135610bad18..5b3e5d6fbf17 100644 --- a/explorer/testdata/trace/phase_execution.carbon +++ b/explorer/testdata/trace/phase_execution.carbon @@ -109,6 +109,8 @@ fn Main() -> i32 { // CHECK:STDOUT: <[] stack-pop: ValueExpressionAction pos: 1 `{}.(interface ImplicitAs(T = class T).Convert)` results: [`bound_method`] (phase_execution.carbon:10) // CHECK:STDOUT: ->> step ExpressionAction pos: 1 `{}.(interface ImplicitAs(T = class T).Convert)()` results: [`bound_method`] scope: [] (phase_execution.carbon:10) ---> // CHECK:STDOUT: -() calling function: bound_method +// CHECK:STDOUT: === match pattern `Placeholder` +// CHECK:STDOUT: from value expression with value `{}` // CHECK:STDOUT: === match pattern `()` // CHECK:STDOUT: from value expression with value `()` // CHECK:STDOUT: >[] stack-push: ScopeAction pos: 0 scope: [`self: Self`: `{}`] (None)