Calling =destroy on non-var fields with stack based collections (Table, HashSet, etc)

[elcritch]

Description

type
  AgentObj = object of RootObj
    subscribers*: Table[int, int]

proc `=destroy`*(agent: AgentObj) =
  `=destroy`(agent.subscribers)

Nim Version

Since the var variant of destroy was added, but currently:

Nim Compiler Version 2.0.14 [Linux: arm64]
Compiled at 2024-12-23
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: bf4de6a394e040d9810cba8c69fb2829ff04dcc6
active boot switches: -d:release -d:danger

Current Output

➜  sigils git:(thread-lifetimes-alt1) ✗ nim c -r "/home/elcritch.linux/sigils/tests/tslotsThread.nim"
Hint: used config file '/home/elcritch.linux/.asdf/installs/nim/2.0.14/config/nim.cfg' [Conf]
Hint: used config file '/home/elcritch.linux/.asdf/installs/nim/2.0.14/config/config.nims' [Conf]
Hint: used config file '/home/elcritch.linux/sigils/tests/nim.cfg' [Conf]
Hint: used config file '/home/elcritch.linux/sigils/tests/config.nims' [Conf]
.....................................................................................................................................
/home/elcritch.linux/sigils/sigils/agents.nim(4, 20) Hint: duplicate import of 'options'; previous import here: /home/elcritch.linux/sigils/sigils/agents.nim(1, 13) [DuplicateModuleImport]
....
/home/elcritch.linux/.asdf/installs/nim/2.0.14/nimble/pkgs2/stack_strings-1.1.4-1c5d7fa85b69c771cba02393aff571cfd36713e6/stack_strings.nim(331, 85) Hint: '[]' cannot raise 'IndexDefect' [XCannotRaiseY]
/home/elcritch.linux/sigils/sigils/agents.nim(138, 13) Error: type mismatch
Expression: `=destroy`(agent.subscribers)
  [1] agent.subscribers: Table[protocol.SigilName, OrderedSet[agents.Subscription]]

Expected one of (first mismatch at [position]):
[1] proc `=destroy`(agent: AgentObj)
[1] proc `=destroy`(x: string)
[1] proc `=destroy`[T](dest: var Isolated[T])
[1] proc `=destroy`[T](x: ref T)
[1] proc `=destroy`[T](x: seq[T])
[1] proc `=destroy`[T](x: var T)
  expression 'agent.subscribers' is immutable, not 'var'

### Expected Output

```text
It compiles!

Known Workarounds

proc `=destroy`*(agent: AgentObj) =
  `=destroy`(addr(agent)[].subscribers)

Maybe:

proc `=destroy`*(agent: AgentObj) =
  var subscribers = ensureMoved agent[].subscribers 
  `=destroy`(subscribers)

Additional Information

No response

[forchid]

Don't manually call the proc =destroy, and actually it will be called at an appropriate timing by nim compiler.

[elcritch]

Don't manually call the proc =destroy, and actually it will be called at an appropriate timing by nim compiler.

When you provide a custom =destroy for a type you need to manually call destroy each field that manages memory. According to the docs at least. The default =destroy provided by the compiler will do this, but I need to override it for my object to handle some custom behaviors.

[Araq]

According to the docs at least.

The docs are correct.

[forchid]

each field that manages memory

It should say these fields for manual memory management such as objects allocated by alloc() , file descriptors etc, but not nim traced objects.

[Araq]

Why should it say something that is wrong? If you override the destructor you need to destroy everything inside the destructor. This design choice allows us to avoid recursions more easily for linked data structures.

[elcritch]

each field that manages memory

It should say these fields for manual memory management such as objects allocated by alloc() , file descriptors etc, but not nim traced objects.

I believe you may misunderstand the details of how the destructors work with ARC/ORC. The documentation only shows resources created by alloc, file descriptors, etc. However all objects need to have their destructors called.

Containers like Table or HashSet are non-ref object but manage their own memory and need to have their destructors called to free any resources. This also includes objects which contain refs as well as the refs need to be decremented. The odd ball is handling cycles which are traced by ORC. The docs probably need to describe these details and/or add more examples.

Overall Nim with ARC doesn't trace objects. ORC only traces cycles of refs. Otherwise all memory is mamanged in the RAII style based on the destructors being called.

[forchid]

Overall Nim with ARC doesn't trace objects.

Really?
I think all objects traced except ptr objects in nim, whatever gc or mm. And the traced objects should be assigned as nil in the overried proc destroy instead of manually calling the =destroy, because these objects may also be referenced by other objects.

[forchid]

@elcritch Here is a simple demo destroytest.nim for the overrided proc =destroy in traced objects, both the object field foo in Bar and the ref field fref in Bar2 are destroyed automatically by nim compiler.

The mm arc test

>%nim20_home%\bin\nim c -d:release --mm:arc destroytest.nim
Hint: used config file '..\nim-2.0.14\config\nim.cfg' [Conf]
Hint: used config file '..\nim-2.0.14\config\config.nims' [Conf]
......................................................................
CC: ../../../Programs/nim-2.0.14/lib/system.nim
CC: destroytest.nim
Hint:  [Link]
Hint: mm: arc; threads: on; opt: speed; options: -d:release
27515 lines; 2.750s; 31.238MiB peakmem; proj: ..destroytest.nim; out: ..\destroytest.exe [SuccessX]

>destroytest
Bar2#4 destroyed.
Foo#3 destroyed.
Bar#2 destroyed.
Foo#1 destroyed.

The orc test

>%nim20_home%\bin\nim c -d:release --mm:orc destroytest.nim
Hint: used config file '..\nim-2.0.14\config\nim.cfg' [Conf]
Hint: used config file '..\nim-2.0.14\config\config.nims' [Conf]
........................................................................
CC: ../../../Programs/nim-2.0.14/lib/system/exceptions.nim
CC: ../../../Programs/nim-2.0.14/lib/std/private/digitsutils.nim
CC: ../../../Programs/nim-2.0.14/lib/system/dollars.nim
CC: ../../../Programs/nim-2.0.14/lib/std/typedthreads.nim
CC: ../../../Programs/nim-2.0.14/lib/std/exitprocs.nim
CC: ../../../Programs/nim-2.0.14/lib/std/syncio.nim
CC: ../../../Programs/nim-2.0.14/lib/system.nim
CC: destroytest.nim
Hint:  [Link]
Hint: mm: orc; threads: on; opt: speed; options: -d:release
28092 lines; 3.500s; 31.254MiB peakmem; proj: ..\destroytest.nim; out: ..\destroytest.exe [SuccessX]

>destroytest
Bar2#4 destroyed.
Foo#3 destroyed.
Bar#2 destroyed.
Foo#1 destroyed.

The test source

# destroytest.nim
type
    Foo = object
        id: int
    Bar = object
        id: int
        foo: Foo
    Bar2 = object
        id: int
        fref: ref Foo
        
proc `=destroy`(foo: Foo) =
    echo "Foo#" & $(foo.id) & " destroyed."

proc `=destroy`(bar: Bar) =
    echo "Bar#" & $(bar.id) & " destroyed."
    
proc `=destroy`(bar2: Bar2) =
    echo "Bar2#" & $(bar2.id) & " destroyed."

proc main() =
    discard Bar(id: 2, foo: Foo(id: 1))
    discard Bar2(id: 4, fref: (ref Foo)(id: 3))

main()

[beef331]

Heh seems to be a bug that was introduced between 2.0.0 and 2.0.2. It very much should not call the builtin destructor.

[elcritch]

Heh seems to be a bug that was introduced between 2.0.0 and 2.0.2. It very much should not call the builtin destructor.

Yeah I just filed a bug. Appears to be something with the discard Bar(...) bit #24586

[elcritch]

Also could relate to the lowered performance in Nim 2.x on some of those Fibonnaci benchmarks? If a bunch of extra copies/destroys or whatnot are being added.

[elcritch]

@elcritch Here is a simple demo destroytest.nim for the overrided proc =destroy in traced objects, both the object field foo in Bar and the ref field fref in Bar2 are destroyed automatically by nim compiler.

It appears you found a nice bug with discarded variables. What happens with your example is that the 2.x compiler is doing some funky extra creations/destruction which shouldn't be there.

Run this version with variables and you'll see that if you don't call =destroy(x.fref) in Baz's destructor it doesn't free fref.

# destroytest.nim
# run with nim c -d:debug -d:useMalloc --mm:arc -d:traceArc -d:nimArcDebug -r "destroytest.nim"
import std/strutils

type
  Foo = object
    id: int
  Bar = object
    id: int
    foo: Foo
  Baz = object
    id: int
    fref: ref Foo

var depth = 1

template printDestroying(x) = 
  depth.inc
  echo "\t".repeat(depth), "Destroying: ", x.repr, " addr: 0x", addr(x).pointer.repr 
template printDestroyed(x) = 
  echo "\t".repeat(depth), "Destroyed: ", x.repr, " addr: 0x", addr(x).pointer.repr 
  depth.dec

proc `=destroy`*(x: var Foo) =
  printDestroying(x)
  addr(x)[].id = -1
  printDestroyed(x)

proc `=destroy`*(x: var Bar) =
  printDestroying(x)
  `=destroy`(x.foo)
  addr(x)[].id = -1
  printDestroyed(x)

proc `=destroy`*(x: var Baz) =
  printDestroying(x)
  if not x.fref.isNil:
    `=destroy`(x.fref)
  addr(x)[].id = -1
  printDestroyed(x)

proc test2() =
  echo "\n=== Test2 ==="
  let x1 = Foo(id: 1)
  let x2 = Bar(id: 2, foo: x1)
  let x3 = (ref Foo)(id: 3)
  let x4 = Baz(id: 4, fref: x3)
  echo "addr x1: ", $(x1), " ", x1.unsafeAddr.pointer.repr
  echo "addr x2: ", $(x2), " ", x2.unsafeAddr.pointer.repr
  echo "addr x2.foo: ", $(x2.foo), " ", x2.foo.unsafeAddr.pointer.repr
  echo "addr x3: ref Foo: ", $(x3[]), " ", cast[pointer](x3).repr
  echo "addr x4: Baz #", $(x4), " ", x4.unsafeAddr.pointer.repr
  echo "addr x4.fref: ", $(x4.fref[]), " ", cast[pointer](x4.fref).repr
  echo ""

test2()

[forchid]

I think that impllicitly calling =destroy in nim is a correct behaviour for traced references as C++ RAII, unique_ptr or shared_ptr. In nim 2.x, it works well, but nim 1.6.x has a bug for traced objects and ok for traced references in nim 1.6.x.

[Araq]

It doesn't matter what you think, it matters what the spec says.

[forchid]

it matters what the spec says.

The spec says that variables are destroyed via this hook when they go out of scope or when the routine they were declared in is about to return. So the behavior is same as C++ RAII or Rust.

[metagn]

About the original issue, the generic non-var overload of =destroy is only declared if -d:nimPreviewNonVarDestructor is defined. So the destructor as written would work if gated behind when defined(nimPreviewNonVarDestructor) and defined(gcDestructors):, and otherwise if the signature is changed so that agent is var AgentObj.

when defined(nimPreviewNonVarDestructor) and defined(gcDestructors):
  proc `=destroy`*(agent: AgentObj) =
    `=destroy`(agent.subscribers)
else:
  proc `=destroy`*(agent: var AgentObj) =
    `=destroy`(agent.subscribers)

[forchid]

It doesn't matter what you think

@Araq But nim 2.x(2.0.8, 2.0.14, 2.2.0 etc) also proved my this thinking.

[elcritch]

I think that impllicitly calling =destroy in nim is a correct behaviour for traced references as C++ RAII, unique_ptr or shared_ptr. In nim 2.x, it works well, but nim 1.6.x has a bug for traced objects and ok for traced references in nim 1.6.x.

@Araq But nim 2.x(2.0.8, 2.0.14, 2.2.0 etc) also proved my this thinking.

@forchid I believe you are misunderstanding something with "tracing" and how destructors work with RAII. Yes when an object goes out of scope their =destroy proc is called implicitly for any objects on the stack in the current scope. Similarly in ARC ref types also have a =destroy which when they go out of scope. It's the same as C++ smart_ptrs.

However, if an object isn't on the current stack, but exists inside another object Bar that is on the stack then it's =destroy won't be called directly since the compiler doesn't know about it. It's up to Bar's destructor to call =destroy on it. When you override a =destroy you need to ensure this happens.

What you're seeing in you example with discard Bar(id: 2, foo: Foo(id: 1)) are the =destroy being called on the temporary objects it looks like the compiler creates. The generated code would look something like:

block:
  let foo'gensym1 = Foo(id: 1)
  let bar'gensym2 = Bar(id: 2, foo'gensym1)
  
  `=destroy`(tmp'gensym1)
  `=destroy`(tmp'gensym2)

However, it's up to =destroy(tmp'gensym2) to call destroy on it's copy of foo. The same with ref Foo as well. If you run my code example above you'll see that this happens for each copy of Foo that gets made, but only if you call =destroy on it. Otherwise they're missed and you'll leak a ref Foo.

[elcritch]

About the original issue, the generic non-var overload of =destroy is only declared if -d:nimPreviewNonVarDestructor is defined. So the destructor as written would work if gated behind when defined(nimPreviewNonVarDestructor) and defined(gcDestructors):, and otherwise if the signature is changed so that agent is var AgentObj.

Nice! Though in Nim 2.x the var form is deprecated. So it's probably just the logic or destructors are gated behind nimPreviewNonVarDestructor still even in 2.x.

Though it'd be great if the var form stuck around, it's better to work with without weird special rules IMHO. But as long as it works.