From df289efac4e7bf5373d24c6d6542568429becdbf Mon Sep 17 00:00:00 2001 From: Adrien Leravat Date: Wed, 22 Mar 2023 09:58:30 -0700 Subject: [PATCH] Explorer: Add virtual destructor support (#2695) ### Features * Add virtual `destructor`s support (Closes #2521) * Check virtual override for virtual destructors * Error if attempting to `Delete` a class that does not have virtual destructors from a base class pointer ### Implementation * Update parser to support virtual override introducers for destructors * Check virtual override for class destructor and add to class vtable if necessary * Add corresponding tests ### Notes Contrary to initial implementation, this implementation leverages the `Address` structure and implements a new `Address::DowncastedAddress()` method to get address from child most class from a base class address. This avoids the need to use `GetAllocationId` and its issues when it comes to having multiple values for an `AllocationId`. ### Next work Following this PR, we need to: * Check when using `Delete` that the class was allocated with `New` (WIP) * Drop the old `GetAllocationId(Value*)` in favor of a better system (WIP) --- explorer/ast/address.h | 9 +++ explorer/ast/declaration.cpp | 5 +- explorer/ast/declaration.h | 10 +-- explorer/ast/element_path.h | 14 ++++ explorer/interpreter/heap.cpp | 6 +- explorer/interpreter/interpreter.cpp | 43 +++++++++--- explorer/interpreter/type_checker.cpp | 33 +++++++++ explorer/syntax/parser.ypp | 12 +++- ...ete_base_without_virtual_destructor.carbon | 29 ++++++++ .../destructor/fail_override_abstract.carbon | 19 ++++++ .../destructor/fail_override_impl_none.carbon | 19 ++++++ .../fail_override_virtual_virtual.carbon | 23 +++++++ .../destructor/virtual_destructor.carbon | 51 ++++++++++++++ .../virtual_destructor_nested.carbon | 67 +++++++++++++++++++ 14 files changed, 320 insertions(+), 20 deletions(-) create mode 100644 explorer/testdata/destructor/fail_delete_base_without_virtual_destructor.carbon create mode 100644 explorer/testdata/destructor/fail_override_abstract.carbon create mode 100644 explorer/testdata/destructor/fail_override_impl_none.carbon create mode 100644 explorer/testdata/destructor/fail_override_virtual_virtual.carbon create mode 100644 explorer/testdata/destructor/virtual_destructor.carbon create mode 100644 explorer/testdata/destructor/virtual_destructor_nested.carbon diff --git a/explorer/ast/address.h b/explorer/ast/address.h index a7bb6e4a7f81..4e3cd43347b1 100644 --- a/explorer/ast/address.h +++ b/explorer/ast/address.h @@ -9,6 +9,7 @@ #include #include +#include "common/check.h" #include "common/ostream.h" #include "explorer/ast/element_path.h" #include "llvm/Support/Compiler.h" @@ -72,6 +73,14 @@ class Address { return result; } + // Drop all trailing BaseElements from the element path, returning the + // downcasted address. + auto DowncastedAddress() const -> Address { + Address address = *this; + address.element_path_.RemoveTrailingBaseElements(); + return address; + } + private: // The representation of Address describes how to locate an object within // the Heap, so its implementation details are tied to the implementation diff --git a/explorer/ast/declaration.cpp b/explorer/ast/declaration.cpp index 2295677d98aa..cdc6458893b4 100644 --- a/explorer/ast/declaration.cpp +++ b/explorer/ast/declaration.cpp @@ -359,14 +359,15 @@ auto DestructorDeclaration::CreateDestructor( Nonnull arena, SourceLocation source_loc, std::vector> deduced_params, Nonnull param_pattern, ReturnTerm return_term, - std::optional> body) + std::optional> body, VirtualOverride virt_override) -> ErrorOr> { DeducedParameters split_params; CARBON_ASSIGN_OR_RETURN(split_params, SplitDeducedParameters(source_loc, deduced_params)); return arena->New( source_loc, std::move(split_params.resolved_params), - split_params.self_pattern, param_pattern, return_term, body); + split_params.self_pattern, param_pattern, return_term, body, + virt_override); } auto FunctionDeclaration::Create(Nonnull arena, diff --git a/explorer/ast/declaration.h b/explorer/ast/declaration.h index 0463942cd76c..a5328c489aca 100644 --- a/explorer/ast/declaration.h +++ b/explorer/ast/declaration.h @@ -316,7 +316,8 @@ class DestructorDeclaration : public CallableDeclaration { std::vector> deduced_params, Nonnull param_pattern, ReturnTerm return_term, - std::optional> body) + std::optional> body, + VirtualOverride virt_override) -> ErrorOr>; // Use `Create()` instead. This is public only so Arena::New() can call it. @@ -325,12 +326,11 @@ class DestructorDeclaration : public CallableDeclaration { std::optional> self_pattern, Nonnull param_pattern, ReturnTerm return_term, - std::optional> body) + std::optional> body, + VirtualOverride virt_override) : CallableDeclaration(AstNodeKind::DestructorDeclaration, source_loc, std::move(deduced_params), self_pattern, - param_pattern, return_term, body, - // TODO: Add virtual destructors - VirtualOverride::None) {} + param_pattern, return_term, body, virt_override) {} explicit DestructorDeclaration(CloneContext& context, const DestructorDeclaration& other) diff --git a/explorer/ast/element_path.h b/explorer/ast/element_path.h index 25988c4aa7f5..79c6bb995478 100644 --- a/explorer/ast/element_path.h +++ b/explorer/ast/element_path.h @@ -5,11 +5,13 @@ #ifndef CARBON_EXPLORER_AST_ELEMENT_PATH_H_ #define CARBON_EXPLORER_AST_ELEMENT_PATH_H_ +#include #include #include #include #include +#include "common/check.h" #include "common/ostream.h" #include "explorer/ast/element.h" #include "explorer/ast/value_node.h" @@ -88,6 +90,18 @@ class ElementPath { components_.push_back(Component(element)); } + // Removes all trailing `BaseElement`s, errors if there are no base elements. + auto RemoveTrailingBaseElements() -> void { + CARBON_CHECK(!components_.empty() && components_.back().element()->kind() == + ElementKind::BaseElement) + << "No base elements to remove."; + const auto r_it = std::find_if( + components_.rbegin(), components_.rend(), [](const Component& c) { + return c.element()->kind() != ElementKind::BaseElement; + }); + components_.erase(r_it.base(), components_.end()); + } + void Print(llvm::raw_ostream& out) const { for (const Component& component : components_) { out << "." << component; diff --git a/explorer/interpreter/heap.cpp b/explorer/interpreter/heap.cpp index 7c8181b4dad8..196694ced619 100644 --- a/explorer/interpreter/heap.cpp +++ b/explorer/interpreter/heap.cpp @@ -13,9 +13,9 @@ namespace Carbon { auto Heap::AllocateValue(Nonnull v) -> AllocationId { // Putting the following two side effects together in this function - // ensures that we don't do anything else in between, which is really bad! - // Consider whether to include a copy of the input v in this function - // or to leave it up to the caller. + // ensures that we don't do anything else in between, which would be really + // bad! Consider whether to include a copy of the input v in this function or + // to leave it up to the caller. AllocationId a(values_.size()); values_.push_back(v); if (v->kind() == Carbon::Value::Kind::UninitializedValue) { diff --git a/explorer/interpreter/interpreter.cpp b/explorer/interpreter/interpreter.cpp index 8047dbb5cbcc..19d0e377480f 100644 --- a/explorer/interpreter/interpreter.cpp +++ b/explorer/interpreter/interpreter.cpp @@ -1449,15 +1449,42 @@ auto Interpreter::StepExp() -> ErrorOr { CARBON_CHECK(args.size() == 1); CARBON_CHECK(act.pos() > 0); const auto* ptr = cast(args[0]); - if (act.pos() == 1) { - CARBON_ASSIGN_OR_RETURN( - const auto* pointee, - this->heap_.Read(ptr->address(), exp.source_loc())); - return todo_.Spawn(std::make_unique( - arena_->New(ptr->address()), pointee)); + CARBON_ASSIGN_OR_RETURN(const auto* pointee, + heap_.Read(ptr->address(), exp.source_loc())); + if (const auto* class_value = dyn_cast(pointee)) { + // Handle destruction from base class pointer. + const auto* child_class_value = *class_value->class_value_ptr(); + bool is_subtyped = child_class_value != class_value; + if (is_subtyped) { + // Error if destructor is not virtual. + const auto& class_type = + cast(class_value->type()); + const auto& class_decl = class_type.declaration(); + if ((*class_decl.destructor())->virt_override() == + VirtualOverride::None) { + return ProgramError(exp.source_loc()) + << "Deallocating a derived class from base class " + "pointer requires a virtual destructor"; + } + } + const Address obj_addr = is_subtyped + ? ptr->address().DowncastedAddress() + : ptr->address(); + if (act.pos() == 1) { + return todo_.Spawn(std::make_unique( + arena_->New(obj_addr), child_class_value)); + } else { + heap_.Deallocate(obj_addr); + return todo_.FinishAction(TupleValue::Empty()); + } } else { - heap_.Deallocate(ptr->address()); - return todo_.FinishAction(TupleValue::Empty()); + if (act.pos() == 1) { + return todo_.Spawn(std::make_unique( + arena_->New(ptr->address()), pointee)); + } else { + heap_.Deallocate(ptr->address()); + return todo_.FinishAction(TupleValue::Empty()); + } } } case IntrinsicExpression::Intrinsic::Rand: { diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index fdff55b889ac..64a4fa58cf48 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -4920,6 +4920,39 @@ auto TypeChecker::DeclareClassDeclaration(Nonnull class_decl, class_vtable[fun->name().inner_name()] = {fun, class_level}; } + // Check destructor's virtual override, add to vtable if necessary. + if (const auto destructor = class_decl->destructor()) { + const auto* fun = (*destructor); + static constexpr llvm::StringRef DestructorName = "destructor"; + bool has_vtable_entry = + class_vtable.find(DestructorName) != class_vtable.end(); + switch (fun->virt_override()) { + case VirtualOverride::None: + break; + case VirtualOverride::Abstract: + return ProgramError(fun->source_loc()) + << "Cannot declare abstract destructor."; + case VirtualOverride::Virtual: + if (has_vtable_entry) { + return ProgramError(fun->source_loc()) + << "Error declaring destructor for `" << class_decl->name() + << "`: use `impl` to implement virtual destructor in child " + "class."; + } + class_vtable[DestructorName] = {fun, class_level}; + break; + case VirtualOverride::Impl: + if (!has_vtable_entry) { + return ProgramError(fun->source_loc()) + << "Error declaring destructor for `" << class_decl->name() + << "`: cannot override a destructor that is not declared " + "`virtual` in base class."; + } + class_vtable[DestructorName] = {fun, class_level}; + break; + } + } + // For class declaration `class MyType(T:! type, U:! AnInterface)`, `Self` // should have the value `MyType(T, U)`. Nonnull self_type = arena_->New( diff --git a/explorer/syntax/parser.ypp b/explorer/syntax/parser.ypp index 3c4311f4519b..8580d17c0252 100644 --- a/explorer/syntax/parser.ypp +++ b/explorer/syntax/parser.ypp @@ -105,6 +105,7 @@ %type optional_library_path %type api_or_impl %type fn_virtual_override_intro +%type destructor_virtual_override_intro %type class_declaration_extensibility %type >> class_declaration_extends %type > declaration @@ -1328,16 +1329,23 @@ match_first_declaration_list: $$ = std::move($1); $$.push_back($2); } +destructor_virtual_override_intro: + DESTRUCTOR + { $$ = VirtualOverride::None; } +| VIRTUAL DESTRUCTOR + { $$ = VirtualOverride::Virtual; } +| IMPL DESTRUCTOR + { $$ = VirtualOverride::Impl; } ; destructor_declaration: - DESTRUCTOR deduced_params block + destructor_virtual_override_intro deduced_params block { ErrorOr fn = DestructorDeclaration::CreateDestructor( arena, context.source_loc(), $2, arena->New(context.source_loc(), std::vector>()), - ReturnTerm::Omitted(context.source_loc()), $3); + ReturnTerm::Omitted(context.source_loc()), $3, $1); if (fn.ok()) { $$ = *fn; } else { diff --git a/explorer/testdata/destructor/fail_delete_base_without_virtual_destructor.carbon b/explorer/testdata/destructor/fail_delete_base_without_virtual_destructor.carbon new file mode 100644 index 000000000000..24533d34f4dc --- /dev/null +++ b/explorer/testdata/destructor/fail_delete_base_without_virtual_destructor.carbon @@ -0,0 +1,29 @@ +// 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: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} +// CHECK:STDERR: RUNTIME ERROR: {{.*}}/explorer/data/prelude.carbon:{{.*}}: Deallocating a derived class from base class pointer requires a virtual destructor + +package ExplorerTest api; + + +base class A{ + destructor[self: Self]{} +} + +class B extends A { + fn Create() -> Self { + return {.base={}}; + } + destructor[self: Self]{} +} + +fn Main() -> i32 { + var pb: B* = heap.New(B.Create()); + var pa: A* = pb; + heap.Delete(pa); + return 0; +} diff --git a/explorer/testdata/destructor/fail_override_abstract.carbon b/explorer/testdata/destructor/fail_override_abstract.carbon new file mode 100644 index 000000000000..a7539ddaeacb --- /dev/null +++ b/explorer/testdata/destructor/fail_override_abstract.carbon @@ -0,0 +1,19 @@ +// 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: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + + +base class A { + // CHECK:STDERR: SYNTAX ERROR: {{.*}}/explorer/testdata/destructor/fail_override_abstract.carbon:[[@LINE+1]]: syntax error, unexpected DESTRUCTOR, expecting CLASS + abstract destructor[self: Self]{} +} + +fn Main() -> i32 { + return 0; +} diff --git a/explorer/testdata/destructor/fail_override_impl_none.carbon b/explorer/testdata/destructor/fail_override_impl_none.carbon new file mode 100644 index 000000000000..f3a4734c93c3 --- /dev/null +++ b/explorer/testdata/destructor/fail_override_impl_none.carbon @@ -0,0 +1,19 @@ +// 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: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + + +base class A { + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/destructor/fail_override_impl_none.carbon:[[@LINE+1]]: Error declaring destructor for `A`: cannot override a destructor that is not declared `virtual` in base class. + impl destructor[self: Self]{} +} + +fn Main() -> i32 { + return 0; +} diff --git a/explorer/testdata/destructor/fail_override_virtual_virtual.carbon b/explorer/testdata/destructor/fail_override_virtual_virtual.carbon new file mode 100644 index 000000000000..45ba9cb81032 --- /dev/null +++ b/explorer/testdata/destructor/fail_override_virtual_virtual.carbon @@ -0,0 +1,23 @@ +// 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: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + + +base class A { + virtual destructor[self: Self]{} +} + +class B extends A { + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/destructor/fail_override_virtual_virtual.carbon:[[@LINE+1]]: Error declaring destructor for `B`: use `impl` to implement virtual destructor in child class. + virtual destructor[self: Self]{} +} + +fn Main() -> i32 { + return 0; +} diff --git a/explorer/testdata/destructor/virtual_destructor.carbon b/explorer/testdata/destructor/virtual_destructor.carbon new file mode 100644 index 000000000000..5fe4f6ea1f15 --- /dev/null +++ b/explorer/testdata/destructor/virtual_destructor.carbon @@ -0,0 +1,51 @@ +// 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: Allocate C +// CHECK:STDOUT: DESTRUCTOR C +// CHECK:STDOUT: DESTRUCTOR B +// CHECK:STDOUT: DESTRUCTOR A +// CHECK:STDOUT: Delete C from A* +// CHECK:STDOUT: DESTRUCTOR C +// CHECK:STDOUT: DESTRUCTOR B +// CHECK:STDOUT: DESTRUCTOR A +// CHECK:STDOUT: result: 0 + +package ExplorerTest api; + + +base class A { + virtual destructor[self: Self] { + Print("DESTRUCTOR A"); + } +} + +base class B extends A { + impl destructor[self: Self] { + Print("DESTRUCTOR B"); + } +} + +class C extends B { + fn Create() -> Self{ + return {.base={.base={}}}; + } + impl destructor[self: Self] { + Print("DESTRUCTOR C"); + } +} + +fn Main() -> i32 { + Print("Allocate C"); + var pc: C* = heap.New(C.Create()); + var pa: A* = pc; + + Print("Delete C from A*"); + heap.Delete(pa); + + return 0; +} diff --git a/explorer/testdata/destructor/virtual_destructor_nested.carbon b/explorer/testdata/destructor/virtual_destructor_nested.carbon new file mode 100644 index 000000000000..09cb2184b8bb --- /dev/null +++ b/explorer/testdata/destructor/virtual_destructor_nested.carbon @@ -0,0 +1,67 @@ +// 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: Allocate D +// CHECK:STDOUT: DESTRUCTOR B +// CHECK:STDOUT: DESTRUCTOR A +// CHECK:STDOUT: DESTRUCTOR D +// CHECK:STDOUT: DESTRUCTOR C +// CHECK:STDOUT: Delete B from A* +// CHECK:STDOUT: DESTRUCTOR B +// CHECK:STDOUT: DESTRUCTOR A +// CHECK:STDOUT: Delete D from C* +// CHECK:STDOUT: DESTRUCTOR D +// CHECK:STDOUT: DESTRUCTOR C +// CHECK:STDOUT: result: 0 + +package ExplorerTest api; + + +base class A { + virtual destructor[self: Self] { + Print("DESTRUCTOR A"); + } +} + +class B extends A { + fn Create() -> Self{ + return {.base={}}; + } + impl destructor[self: Self] { + Print("DESTRUCTOR B"); + } +} + +base class C { + virtual destructor[self: Self] { + Print("DESTRUCTOR C"); + } +} + +class D extends C { + fn Create() -> Self{ + return {.base={}, .d_pa=heap.New(B.Create())}; + } + impl destructor[self: Self] { + Print("DESTRUCTOR D"); + } + var d_pa: A*; +} + +fn Main() -> i32 { + Print("Allocate D"); + var pd: D* = heap.New(D.Create()); + var pc: C* = pd; + + Print("Delete B from A*"); + heap.Delete(pd->d_pa); + + Print("Delete D from C*"); + heap.Delete(pc); + + return 0; +}