destructor not called (leak) for seq elements; destructor called for default-initialized object

[timotheecour]

• bug1: with default gc, destructor is not called with seq when seq is accessed through a field
• bug2: with default gc and --gc:arc, destructor is called for a default-initialized object (wasteful at best)

Example

when true: # gitissue D20200608T005906:here
  type Foo = object
    x: int
    p: ptr int
  type Vec = object
    vals: seq[Foo]
  proc `=destroy`*(a: var Foo) {.inline.} =
    echo ("in destroy", a.x, a.p == nil)
    if a.p != nil:
      dealloc(a.p)
  proc initFoo(x: int): Foo =
    result = Foo(x: x, p: create(int))
  proc add2(v: var Vec, a: Foo) = v.vals.add a
  proc add3(v: var Vec, a: sink Foo) = v.vals.add a
  proc main1()=
    echo "main1"
    var a = newSeqOfCap[Foo](2)
    a.add initFoo(10)
    a.add initFoo(11)
  proc main2()=
    echo "main2"
    var a: Vec
    a.vals = newSeqOfCap[Foo](2) # ditto with `a.vals.setLen 2; a.vals.setLen 0` or even with nothing
    a.vals.add initFoo(12)
    a.add2 initFoo(13)
    a.add3 initFoo(14)
  main1()
  main2()
  echo "done"

Current Output

nim r t10917.nim

main1
("in destroy", 10, false)
("in destroy", 11, false)
main2
("in destroy", 0, true)
("in destroy", 0, true)
done

=> ("in destroy", 0, true) should not be called
=> destructor not called for 12,13,14

nim r --gc:arc t10917.nim

main1
("in destroy", 10, false)
("in destroy", 11, false)
main2
("in destroy", 0, true)
("in destroy", 0, true)
("in destroy", 12, false)
("in destroy", 13, false)
("in destroy", 14, false)
done

=> ("in destroy", 0, true) should not be called (and there are 2 instead of 3 (or 0) of them)

Expected Output

both with and without --gc:arc should produce:

main1
("in destroy", 10, false)
("in destroy", 11, false)
main2
("in destroy", 12, false)
("in destroy", 13, false)
("in destroy", 14, false)
done

Additional Information

• devel 1.3.5 52841db

proposal

• bug1 should be fixed, even if --gc:arc doesn't have that issue (--gc:arc has other issues and isn't the default yet)
• don't call destructor for a variable that was not initialized or default-initialized (would fix bug2)
  This requires tracking which variables are default-initialized or un-initialized, which would also be useful for cgen creates redundant nimZeroMem calls #14602

proc main() =
  var a: Foo
  a = Foo(x: 10)
main() # this should only call 1 destructor (for Foo(x:10)), not for the default initialized `Foo()`

for seq, the simplest correct way is to assume that all elements from 0..<len are initialized, and shall have their destructor called when seq goes out of scope; potential optimizations to track which range of indexes are default initialized would be possible but not needed, eg:

proc main()=
  var a = newSeqOfCap[Foo](1)
  a.add Foo(x: 10)
  a.setLen 3
main()
# only 3 `=destroy(a: var Foo)` shall be called

[Clyybber]

bug2: with default gc and --gc:arc, destructor is called for a default-initialized object (wasteful at best)

Thats not a bug. Its why you put the nilcheck in the destructor. We can do better here, but that counts as optimization.

Right now with --gc:arc

main1
("in destroy", 10, false)
("in destroy", 11, false)
main2
("in destroy", 12, false)
("in destroy", 13, false)
("in destroy", 14, false)
done

[Araq]

Well then it works now as Timothee wants it to work.

[timotheecour]

bug 2 was fixed indeed but bug 1 is still there:

with default gc:

main1
("in destroy", 10, false)
("in destroy", 11, false)
main2
done

@timotheecour what if you add GC_fullCollect() ? Default GC needs that if you want to make sure it runs all destructors

[timotheecour]

I just tried adding GC_fullCollect at the end, it gives same results; but destructor should run deterministically in this case (GC_fullCollect should not be needed to get the desired results, ie what's shown in #14604 (comment)).

see comment in top post:

for seq, the simplest correct way is to assume that all elements from 0..<len are initialized, and shall have their destructor called when seq goes out of scope [...]

I just tried adding GC_fullCollect at the end, it gives same results; but destructor should run deterministically in this case (GC_fullCollect should not be needed to get the desired results, ie what's shown in #14604 (comment)).

see comment in top post:

for seq, the simplest correct way is to assume that all elements from 0..<len are initialized, and shall have their destructor called when seq goes out of scope [...]

I agree that the destructors should be called for refc too(e.g. after GC_fullCollect), but I don't agree that they should be called deterministically - how can that be possible (I mean refc giving the same results as arc here) with refc if it's not deterministic by itself? And refc is being deprecated anyway, issues about refc get a Low Priority label nowadays :)