delete not called with ARC/ORC due to forward proc declaration + RootObj

[arkanoid87]

Following #42

Using forward declaration of finalizer (delete) function causes bug that silently skips finalizer call with --gc:arc
Possible solutions:

• fix compiler: not backward compatible unless backport
• refactor code to avoid forward declaration: possibly backward compatible

I see that the code does not follow import pattern but is just a single large module. I think it would make it easier to think about the code by converting the include into import + export and handle the dependency tree

---

@arkanoid87 Switching from finalizers to destructors is impossible without breaking existing code. The problem is due to destructors taking a var T but finalizers ref T.

I'm already doing some experiments on this

case n==0: destroy can be declared on base non-ref type and it would be called on ref object dealloc, it is possible to call parent destroy
case n==1: finalizer is called on leaf (real) type only and seems not possible to easily call parent finalizer without additional code

type
  FooObj = object of RootObj
    fooValue: int
  BarObj = object of FooObj
    barValue: int
  Foo = ref FooObj
  Bar = ref BarObj

const n = 0

when n == 0:
  proc `=destroy`(x: var FooObj) =
    echo "Goodbye Foo " & $x.fooValue
  
  proc `=destroy`(x: var BarObj) = # `=destroy`(x: var FooObj) would be called if this proc was not be declared 
    x.FooObj.`=destroy`
    echo "Goodbye Bar " & $x.barValue

  proc newFoo: Foo =
    new(result)
    result.fooValue = 99

  proc newBar: Bar =
    new(result)
    result.fooValue = 42
    result.barValue = 19

when n == 1:
  proc finalize(x: Foo) =
    echo "Goodbye Foo " & $x.fooValue

  proc finalize(x: Bar) =
    x.Foo.finalize() # Error: cannot bind another '=destroy' to: FooObj
    echo "Goodbye Bar " & $x.barValue

  proc newFoo: Foo =
    new(result, finalize)
    result.fooValue = 99

  proc newBar: Bar =
    new(result, finalize)
    result.fooValue = 42
    result.barValue = 19


proc main =
  var foo = newFoo()
  echo foo.fooValue
  var bar = newBar()
  echo bar.fooValue
  echo bar.barValue

if isMainModule:
  main()
  GC_fullCollect()

[arkanoid87]

position of forward declarations:

$ rg "proc [A-Za-z]+\*?\([: a-zA-Z]+\)\$"
examples/connections/main.nim
8:  proc delete*(self: Contact)
9:  proc setup(self: Contact)

examples/contactapp/contactlist.nim
13:  proc delete(self: ContactList)
14:  proc setup(self: ContactList)

examples/slotsandproperties/contact.nim
7:  proc delete*(self: Contact)
8:  proc setup(self: Contact)

examples/abstractitemmodel/mylistmodel.nim
12:  proc delete(self: MyListModel)
13:  proc setup(self: MyListModel)

src/nimqml/private/constructors.nim
17:proc setup*(self: QAbstractItemModel)
18:proc delete*(self: QAbstractItemModel)
26:proc setup*(self: QAbstractListModel)
27:proc delete*(self: QAbstractListModel)
35:proc setup*(self: QAbstractTableModel)
36:proc delete*(self: QAbstractTableModel)
44:proc setup*(variant: QVariant)
53:proc delete*(variant: QVariant)
97:proc delete*(self: QUrl)
104:proc setup*(self: QQuickView)
105:proc delete*(self: QQuickView)
112:proc setup*(self: QQmlApplicationEngine)
113:proc delete*(self: QQmlApplicationEngine)
120:proc setup*(self: QModelIndex)
122:proc delete*(self: QModelIndex)
135:proc delete*(self: QMetaObjectConnection)
146:proc delete*(metaObject: QMetaObject)
185:proc setup*(self: QHashIntByteArray)
186:proc delete*(self: QHashIntByteArray)
193:proc setup*(self: QGuiApplication)
194:proc delete*(self: QGuiApplication)

examples/charts/mylistmodel.nim
15:  proc delete(self: MyListModel)
16:  proc setup(self: MyListModel)

[filcuc]

Client users that use NimQml already created their delete methods that are called from the base class finalizers. This delete methods take a ref T argument thus they're not callable from a destroy destructors. It's not possible to switch to destructors without breaking existing client code. The problem is probably in the compiler. I would wait for a fix for nim-lang/Nim#19402

[arkanoid87]

do we really have to wait a fix in nim compiler if a working solution seems to exist just by avoiding forward declarations of finalizers?

Custom finalizer not being called

--gc:arc

type Foo = ref object of RootObj
proc delete(self: Foo) 
proc newFoo: Foo = new(result, delete)
proc delete(self: Foo) = echo("delete Foo")

if isMainModule:
  let mem0 = getOccupiedMem()
  proc test() = discard newFoo()
  test()
  GC_fullCollect()
  echo (getOccupiedMem() - mem0)

0

Working code

--gc:arc

type Foo = ref object of RootObj
proc delete(self: Foo) = echo("delete Foo")
proc newFoo: Foo = new(result, delete)

if isMainModule:
  let mem0 = getOccupiedMem()
  proc test() = discard newFoo()
  test()
  GC_fullCollect()
  echo (getOccupiedMem() - mem0)

delete foo
0

[filcuc]

@arkanoid87 No i'm ok in a workaround. Maybe i'm wrong but i recall having some issue in removing the finalizers forward declarations. Compiler seemed buggy in there. I had some bikeshedding and for example i had to remove the forwad declaration for the QObject (see

nimqml/src/nimqml/private/constructors.nim

Line 3 in 4f237b4

proc delete*(self: QObject) =

)
I can give a try by moving all the finalizers definitions in constructors.nim

[arkanoid87]

I think this has to be tackled first

Compiles and runs with refc, doesn't compile with arc.
Is this the source of forward declarations / bikeshedding?

type
  Parent* = ref object of RootObj
  Child* = ref object of Parent

proc setup*(self: Parent) =
  echo "Parent setup"
  
proc delete*(self: Parent) =
  echo "Parent delete"

proc newParent*(): Parent =
  echo "Parent newParent"
  new(result, delete)
  result.setup

proc setup(self: Child) =
  self.Parent.setup
  echo "Child setup"

proc delete*(self: Child) =
  echo "Child delete"
  self.Parent.delete

proc newChild*(): Child =
  echo "Child newChild"
  new(result, delete)
  result.setup

proc main = discard newChild()

if isMainModule:
  main()
  GC_fullCollect()

--gc:refc

Child newChild
Parent setup
Child setup
Child delete
Parent delete

--gc:arc

Error: cannot bind another '=destroy' to: Child:ObjectType; previous declaration was constructed here implicitly

[arkanoid87]

good news, the nim issue has been tagged as showstopper

[arkanoid87]

@xflywind suggested to add {.experimental: "codeReordering".} on module to fix, this would also remove the need for forward declarations
nim-lang/Nim#19402 (comment)

problem is that I've tried to add it on top of constructors.nim and qqmlapplicationengine.nim but this code:

--gc:arc
--d:debug

import nimqml


proc main =
  var app = newQApplication()
  var engine = newQQmlApplicationEngine()

  engine.load("qml/main.qml")
  app.exec()
  echo "main scope end"

when isMainModule:
  main()
  echo "goodbye"
  GC_fullCollect()

doesn't print NimQml: QQmlApplicationEngine: delete, so the problem has to be extended to multiple file context and extended use of the include feature

EDIT: I've also tried adding it on top of nimqml.nim but yet no cake

[filcuc]

the feature is experimental for a reason i would say...we need a proper fix with forward declarations imho

[arkanoid87]

before I try removing the forward declarations by lot of refactoring, would you consider the change of setup and delete from proc to method or using destructors instead of finalizers?

[filcuc]

i don't get why you want those changes and why it metters here. Again this is a compiler problem not of NimQml. If ORC/ARC is broken is not our fault

[arkanoid87]

because current nimqml code is silently leaking memory with ORC/ARC.

We can wait a compiler fix, or actually handle this.
I think it is possible to avoid forwarding declarations and replace the include approach with an proper import one that's more nim style

[filcuc]

The leaks are due to the compiler generated code not again due to NimQml. Just use refc since it's the default GC even for Nim 1.6. The use of include has been necessary due to Nim not handling cyclic dependencies and cyclic imports. The NimQml codebase has changed quite a bit since the time i had to switch using includes. Feel free to try but i can assure you that the use includes was due to the compiler not being able to support cycling imports.

[filcuc]

I would not accept:

• changing setup and delete to be methods
• changing the signature of setup/delete due to destructors needing a var argument instead of a ref
• having different setup/methods specific for supporting arc/orc

[arkanoid87]

spotted the showstopper I was not expecting:
Error: type bound operation `delete` can be defined only in the same module with its type

This triggers a chain reaction that stops me from willing to refactor the code:
a module re-design without cyclic dependencies is probably possible by splitting the shared code between two modules into a third new module that the two imports, but the requirement to split nimqmltypes.nim or move all the deletes in the types modules makes it smell anyway

This is where a dependency graph tools would come handy

Still working on this, I want at least to learn something or end up with a possible improvement

[arkanoid87]

thanks to the help received from nim community, I've learned that is possible to use delayed import to overcome the recursive module dependency issue, and that mostly worked apart from a couple of modules. This has enabled the possibility to return all type declarations to each module, and replace include with imports.

The show stopper is now this pattern that wants user to call parent destructor/finalizer

type
  MyListModel* = ref object of QAbstractListModel
    names*: seq[string]

proc delete(self: MyListModel)
proc setup(self: MyListModel)
proc newMyListModel*(): MyListModel =
  new(result, delete)
  result.names = @["John", "Max", "Paul", "Anna"]
  result.setup

proc delete(self: MyListModel) =
  self.QAbstractListModel.delete

proc setup(self: MyListModel) =
  self.QAbstractListModel.setup

minized:

# --gc:refc OK
# --gc:arc  Error: cannot bind another '=destroy' to: Child:ObjectType; previous declaration was constructed here implicitly: finalizer1.nim(11, 3)

type
  Parent = ref object of RootObj
  Child = ref object of Parent

proc delete(self: Parent) =
  echo "delete parent"

proc delete(self: Child) =
  self.Parent.delete()

proc newChild: Child =
  new(result, delete) # error here

that I have yet to find a way to prevent breaking changes, even by splitting logic between ARC/ORC and refc at compile time. Here a full example on how to call contructor/destructor hierarchy in both cases

type
  ParentObj = object of RootObj
  Parent = ref ParentObj
  ChildObj = object of ParentObj
  Child = ref ChildObj

proc setup(self: Parent)
proc setup(self: Child)

when defined(gcDestructors):
  proc `=destroy`(self: var ParentObj) = echo "destroy Parent"
  proc `=destroy`(self: var ChildObj) = 
    echo "destroy Child"
    self.ParentObj.`=destroy`()

  proc newParent: Parent =
    result = new(Parent)
    result.setup()

  proc newChild: Child =
    result = new(Child)
    result.Parent.setup()
    result.setup()
else:
  proc delete(self: Parent) = echo "delete parent"
  proc delete(self: Child) =
    echo "delete child"
    self.Parent.delete()

  proc newParent: Parent =
    new(result, delete)
    result.setup()

  proc newChild: Child =
    new(result, delete)
    result.Parent.setup()
    result.setup()


proc setup(self: Parent) = echo "setup Parent"
proc setup(self: Child) = echo "setup Child"


proc test =
  discard newChild()

test()
GC_fullCollect()

[arkanoid87]

In the meantime I suggest to use defined(gcDestructors) to trigger compile error if ARC/ORC is used, as nimqml it will silently leak memory and trigger hard-to-find problems otherwise

[filcuc]

we can close this one with recent nim versions

[arkanoid87]

is nimqml tested for orc on nim 2.0?