lifter: destroy an inheritable object through its vtable, even an emp… (#2532)

…ty one

A closure's environment is a ref to a compiler-made object deriving from
RootObj, and the closure value knows it only as `(ref RootObj)`. Its
=destroy decremented the count and freed the block, but never ran the
env's OWN destructor: RootObj has no fields, `isTrivialTypeDecl` called
it trivial, and `emitRefDestructor` skipped the payload. Every captured
string, ref or resource in every closure leaked.

An object with RTTI is never trivial for `=destroy` now: the value may
be a DERIVED object that owns something, and its `=destroy` is a method.
The synthesized `=destroy_RootObj` is an empty method — the vtable slot
is the point — and the `ref RootObj` hook dispatches through it to the
env's synthesized destroy, which releases the captures. Same mechanism
the existing user-hierarchy dispatch (txdestroy) relies on; `=wasMoved`
already had this exception for the vtable field.

Test: tclosure_env_release — a captured resource with a loud destructor
is released when the last reference to the closure goes, and not before.

Co-authored-by: Ryan Walklin <ryan@kaitakeradiology.co.nz>

Author: Araq

src/hexer/lifter.nim

@@ -236,9 +236,21 @@ proc isTrivialTypeDecl(c: var LiftingCtx; n: Cursor): bool =
     result = false
   of ObjectT:
     result = isTrivialObjectBody(c, r.body)
-    if result and c.op == attachedWasMoved and hasRtti(r.pragmas):
-      # We set the RTTI field in `=wasMoved` so objects with a vtable are not trivial.
-      result = false
+    if result and hasRtti(r.pragmas):
+      if c.op == attachedWasMoved:
+        # We set the RTTI field in `=wasMoved` so objects with a vtable are not trivial.
+        result = false
+      elif c.op == attachedDestroy:
+        # A value of an inheritable type may really be a DERIVED object that
+        # owns resources, and its `=destroy` is a method: the destroy has to
+        # be dispatched through the vtable even when this class itself has
+        # nothing to release. `RootObj` is the case that matters: a closure's
+        # environment is `(ref RootObj)` to everyone but its maker, and
+        # treating `RootObj` as trivial freed the env block without ever
+        # running the env's own destructor — every captured string, ref or
+        # resource leaked. The synthesized `=destroy_RootObj` is an empty
+        # method; the vtable slot is the point.
+        result = false
   of DistinctT:
     # A `distinct T` with no hooks of its own inherits the triviality of its
     # base type: `distinct string` is non-trivial (it owns the same heap

tests/nimony/closures/deps/mclosureenvleak.nim

@@ -0,0 +1,18 @@
+# The foreign half of tclosure_env_release: a resource with a loud destructor,
+# and an object holding a closure in a field of the semchecked (never lowered
+# here) `.closure` type.
+{.feature: "lenientnils".}
+import std/syncio
+
+type
+  Resource* = object
+    name*: string
+
+proc `=destroy`*(r: Resource) =
+  if r.name.len > 0: echo "released ", r.name
+
+type
+  Holder* = object
+    onEvent*: proc() {.closure.}
+
+proc clear*(h: var Holder) = h.onEvent = nil

tests/nimony/closures/tclosure_env_release.nim

@@ -0,0 +1,38 @@
+# A closure's environment must release what it captured when the last
+# reference to the closure goes away. The env is a `ref` to a compiler-made
+# object deriving from RootObj, and the closure value only knows it as
+# `(ref RootObj)`: its `=destroy` decremented the count and freed the block,
+# but never ran the env's OWN destructor (RootObj has no fields, so the
+# payload counted as trivial) — every captured string, ref or resource in
+# every closure leaked. `=destroy(RootObj)` is a (empty) virtual method now,
+# so the `ref RootObj` hook dispatches through the env's vtable.
+{.feature: "lenientnils".}
+import std/syncio
+import deps/mclosureenvleak
+
+proc touch(what: string; r: Resource) = echo what, r.name
+
+proc fire(h: Holder) =
+  # its own scope: the temp the call takes on the closure value dies here,
+  # not at the end of the caller
+  h.onEvent()
+
+proc attach(h: var Holder; tag: string) =
+  var res = Resource(name: tag)
+  let cb = proc() {.closure.} = touch("event ", res)
+  h.onEvent = cb
+
+proc main() =
+  var h = Holder()
+  attach(h, "alpha")
+  fire(h)
+  clear(h) # last reference: the env goes, and with it "alpha"
+  echo "cleared"
+  attach(h, "beta")
+  let copy = h.onEvent # a second reference
+  clear(h)
+  copy() # still alive through `copy`
+  echo "scope end next"
+
+main()
+echo "ok"

tests/nimony/closures/tclosure_env_release.output

@@ -0,0 +1,7 @@
+event alpha
+released alpha
+cleared
+event beta
+scope end next
+released beta
+ok