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,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#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

explorer/ast/declaration.cpp

@@ -359,14 +359,15 @@ auto DestructorDeclaration::CreateDestructor(
     Nonnull<Arena*> arena, SourceLocation source_loc,
     std::vector<Nonnull<AstNode*>> deduced_params,
     Nonnull<TuplePattern*> param_pattern, ReturnTerm return_term,
-    std::optional<Nonnull<Block*>> body)
+    std::optional<Nonnull<Block*>> body, VirtualOverride virt_override)
     -> ErrorOr<Nonnull<DestructorDeclaration*>> {
   DeducedParameters split_params;
   CARBON_ASSIGN_OR_RETURN(split_params,
                           SplitDeducedParameters(source_loc, deduced_params));
   return arena->New<DestructorDeclaration>(
       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*> arena,

explorer/ast/declaration.h

@@ -316,7 +316,8 @@ class DestructorDeclaration : public CallableDeclaration {
                                std::vector<Nonnull<AstNode*>> deduced_params,
                                Nonnull<TuplePattern*> param_pattern,
                                ReturnTerm return_term,
-                               std::optional<Nonnull<Block*>> body)
+                               std::optional<Nonnull<Block*>> body,
+                               VirtualOverride virt_override)
       -> ErrorOr<Nonnull<DestructorDeclaration*>>;
 
   // Use `Create()` instead. This is public only so Arena::New() can call it.
@@ -325,12 +326,11 @@ class DestructorDeclaration : public CallableDeclaration {
                         std::optional<Nonnull<Pattern*>> self_pattern,
                         Nonnull<TuplePattern*> param_pattern,
                         ReturnTerm return_term,
-                        std::optional<Nonnull<Block*>> body)
+                        std::optional<Nonnull<Block*>> 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)

explorer/ast/element_path.h

@@ -5,11 +5,13 @@
 #ifndef CARBON_EXPLORER_AST_ELEMENT_PATH_H_
 #define CARBON_EXPLORER_AST_ELEMENT_PATH_H_
 
+#include <algorithm>
 #include <optional>
 #include <string>
 #include <string_view>
 #include <vector>
 
+#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;

explorer/interpreter/heap.cpp

@@ -13,9 +13,9 @@ namespace Carbon {
 
 auto Heap::AllocateValue(Nonnull<const Value*> 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) {

explorer/interpreter/interpreter.cpp

@@ -1449,15 +1449,42 @@ auto Interpreter::StepExp() -> ErrorOr<Success> {
           CARBON_CHECK(args.size() == 1);
           CARBON_CHECK(act.pos() > 0);
           const auto* ptr = cast<PointerValue>(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<DestroyAction>(
-                arena_->New<LValue>(ptr->address()), pointee));
+          CARBON_ASSIGN_OR_RETURN(const auto* pointee,
+                                  heap_.Read(ptr->address(), exp.source_loc()));
+          if (const auto* class_value = dyn_cast<NominalClassValue>(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<NominalClassType>(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<DestroyAction>(
+                  arena_->New<LValue>(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<DestroyAction>(
+                  arena_->New<LValue>(ptr->address()), pointee));
+            } else {
+              heap_.Deallocate(ptr->address());
+              return todo_.FinishAction(TupleValue::Empty());
+            }
           }
         }
         case IntrinsicExpression::Intrinsic::Rand: {

explorer/interpreter/type_checker.cpp

@@ -4920,6 +4920,39 @@ auto TypeChecker::DeclareClassDeclaration(Nonnull<ClassDeclaration*> 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<NominalClassType*> self_type = arena_->New<NominalClassType>(

explorer/syntax/parser.ypp

@@ -105,6 +105,7 @@
 %type <std::string> optional_library_path
 %type <bool> api_or_impl
 %type <VirtualOverride> fn_virtual_override_intro
+%type <VirtualOverride> destructor_virtual_override_intro
 %type <ClassExtensibility> class_declaration_extensibility
 %type <std::optional<Nonnull<Expression*>>> class_declaration_extends
 %type <Nonnull<Declaration*>> 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<DestructorDeclaration*> fn =
         DestructorDeclaration::CreateDestructor(
             arena, context.source_loc(), $2,
             arena->New<TuplePattern>(context.source_loc(),
                                      std::vector<Nonnull<Pattern*>>()),
-            ReturnTerm::Omitted(context.source_loc()), $3);
+            ReturnTerm::Omitted(context.source_loc()), $3, $1);
     if (fn.ok()) {
       $$ = *fn;
     } else {

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;
+}

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;
+}

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;
+}

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;
+}

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;
+}

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;
+}