From fb02e9831d36f79ab8c78fb4827f345c3407fff6 Mon Sep 17 00:00:00 2001 From: metagn Date: Sun, 12 Apr 2026 08:05:11 +0300 Subject: [PATCH] only generate called hook for explicit or generated destructor calls [backport] (#25729) fixes #25727, regression from #24627 which was backported to 2.2.2 and 2.0.16 Instead of calling `createTypeBoundOps` for explicit hook calls and when generating default hooks, only the called destructor is generated at a time. This allows defining more than 1 hook for recursive types. `=sink` for `useSeqOrStrOp` and also `atomicRefOp` always need a `=destroy` hook generated so that is also generated separately. There might be more that I missed, only the atomicRefOp one failed `trtree` in CI, and it was just from a compiler assert that got triggered, otherwise it would still have functioned. --- compiler/liftdestructors.nim | 42 +++++++++++++-- compiler/sempass2.nim | 8 +-- tests/destructor/trecursivedestructor.nim | 63 +++++++++++++++++++++++ 3 files changed, 105 insertions(+), 8 deletions(-) create mode 100644 tests/destructor/trecursivedestructor.nim diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index 15c60363f8..748e0ac201 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -42,6 +42,8 @@ proc fillBody(c: var TLiftCtx; t: PType; body, x, y: PNode) proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp; info: TLineInfo; idgen: IdGenerator): PSym +proc createSingleTypeBoundOp*(g: ModuleGraph; c: PContext; orig: PType; op: TTypeAttachedOp; + info: TLineInfo; idgen: IdGenerator) proc createTypeBoundOps*(g: ModuleGraph; c: PContext; orig: PType; info: TLineInfo; idgen: IdGenerator) @@ -684,7 +686,7 @@ proc fillSeqOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc useSeqOrStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = - createTypeBoundOps(c.g, c.c, t, body.info, c.idgen) + createSingleTypeBoundOp(c.g, c.c, t, c.kind, body.info, c.idgen) # recursions are tricky, so we might need to forward the generated # operation here: var t = t @@ -703,8 +705,12 @@ proc useSeqOrStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = # we always inline the move for better performance: let moveCall = genBuiltin(c, mMove, "move", x) moveCall.add y - doAssert t.destructor != nil - moveCall.add destructorCall(c, t.destructor, x) + var destructor = t.destructor + if destructor == nil or destructor.ast.isGenericRoutine: + createSingleTypeBoundOp(c.g, c.c, t, attachedDestructor, body.info, c.idgen) + destructor = t.destructor + doAssert destructor != nil + moveCall.add destructorCall(c, destructor, x) body.add moveCall # alternatively we could do this: when false: @@ -789,7 +795,7 @@ proc atomicRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = var actions = newNodeI(nkStmtList, c.info) let elemType = t.elementType - createTypeBoundOps(c.g, c.c, elemType, c.info, c.idgen) + createSingleTypeBoundOp(c.g, c.c, elemType, c.kind, c.info, c.idgen) # YRC uses dedicated runtime procs for the entire write barrier: if c.g.config.selectedGC == gcYrc: @@ -822,6 +828,8 @@ proc atomicRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = else: x + if t.destructor == nil or t.destructor.ast.isGenericRoutine: + createSingleTypeBoundOp(c.g, c.c, elemType, attachedDestructor, body.info, c.idgen) if isFinal(elemType): addDestructorCall(c, elemType, actions, genDeref(tmp, nkDerefExpr)) var alignOf = genBuiltin(c, mAlignOf, "alignof", newNodeIT(nkType, c.info, elemType)) @@ -1415,6 +1423,32 @@ proc inst(g: ModuleGraph; c: PContext; t: PType; kind: TTypeAttachedOp; idgen: I proc isTrivial*(s: PSym): bool {.inline.} = s == nil or (s.ast != nil and s.ast[bodyPos].len == 0) +proc createSingleTypeBoundOp(g: ModuleGraph; c: PContext; orig: PType; op: TTypeAttachedOp; + info: TLineInfo; idgen: IdGenerator) = + ## like `createTypeBoundOps` but only generates a single hook + if orig == nil or {tfCheckedForDestructor, tfHasMeta} * orig.flags != {}: return + + let skipped = orig.skipTypes({tyGenericInst, tyAlias, tySink}) + if isEmptyContainer(skipped) or skipped.kind == tyStatic: return + + let h = sighashes.hashType(skipped, g.config, {CoType, CoConsiderOwned, CoDistinct}) + var canon = g.canonTypes.getOrDefault(h) + if canon == nil: + g.canonTypes[h] = skipped + canon = skipped + + let generic = getAttachedOp(g, canon, op) != nil + if not generic: + setAttachedOp(g, idgen.module, canon, op, + symPrototype(g, canon, canon.owner, op, info, idgen)) + + if not generic: + discard produceSym(g, c, canon, op, info, idgen) + else: + inst(g, c, canon, op, idgen, info) + if canon != orig: + setAttachedOp(g, idgen.module, orig, op, getAttachedOp(g, canon, op)) + proc createTypeBoundOps(g: ModuleGraph; c: PContext; orig: PType; info: TLineInfo; idgen: IdGenerator) = ## In the semantic pass this is called in strategic places diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim index 35901ed960..9530abdd06 100644 --- a/compiler/sempass2.nim +++ b/compiler/sempass2.nim @@ -127,11 +127,10 @@ proc collectObjectTree(graph: ModuleGraph, n: PNode) = else: graph.objectTree[root].add (depthLevel, typ) -proc createTypeBoundOps(tracked: PEffects, typ: PType; info: TLineInfo; explicit = false) = - if typ == nil or (sfGeneratedOp in tracked.owner.flags and not explicit): +proc createTypeBoundOps(tracked: PEffects, typ: PType; info: TLineInfo) = + if typ == nil or sfGeneratedOp in tracked.owner.flags: # don't create type bound ops for anything in a function with a `nodestroy` pragma # bug #21987 - # unless this is an explicit call, bug #24626 return when false: let realType = typ.skipTypes(abstractInst) @@ -1166,7 +1165,8 @@ proc trackCall(tracked: PEffects; n: PNode) = # rebind type bounds operations after createTypeBoundOps call let t = n[1].typ.skipTypes({tyAlias, tyVar, tySink}) if a.sym != getAttachedOp(tracked.graph, t, opKind): - createTypeBoundOps(tracked, t, n.info, explicit = true) + # generate called hook regardless of `nodestroy` for explicit call, bug #24626 + createSingleTypeBoundOp(tracked.graph, tracked.c, t, opKind, n.info, tracked.c.idgen) # replace builtin hooks with lifted ones n = replaceHookMagic(tracked.c, n, opKind) diff --git a/tests/destructor/trecursivedestructor.nim b/tests/destructor/trecursivedestructor.nim new file mode 100644 index 0000000000..9a8159aa72 --- /dev/null +++ b/tests/destructor/trecursivedestructor.nim @@ -0,0 +1,63 @@ +discard """ + matrix: "--mm:refc; --mm:orc; --mm:none" +""" + +# issue #25727 + +type ObjWithSeq = object + x: seq[ObjWithSeq] + +when not defined(gcDestructors): + proc `=destroy`(a: var ObjWithSeq) {.nodestroy.} = + `=destroy`(a.x) +else: + proc `=destroy`(a: ObjWithSeq) {.nodestroy.} = + `=destroy`(a.x) + +proc `=copy`(a: var ObjWithSeq, b: ObjWithSeq) {.nodestroy.} = + `=copy`(a.x, b.x) + +proc `=sink`(a: var ObjWithSeq, b: ObjWithSeq) {.nodestroy.} = + `=sink`(a.x, b.x) + +proc `=dup`(a: ObjWithSeq): ObjWithSeq {.nodestroy.} = + ObjWithSeq(x: a.x) + +proc `=trace`(a: var ObjWithSeq, env: pointer) {.nodestroy.} = + `=trace`(a.x, env) + +proc fooSeq() = + let a = ObjWithSeq(x: @[ObjWithSeq()]) + let b = a + let c = a + let d = b +fooSeq() + +type ObjWithRef = object + x: ref ObjWithRef + +when not defined(gcDestructors): + proc `=destroy`(a: var ObjWithRef) {.nodestroy.} = + `=destroy`(a.x) +else: + proc `=destroy`(a: ObjWithRef) {.nodestroy.} = + `=destroy`(a.x) + +proc `=copy`(a: var ObjWithRef, b: ObjWithRef) {.nodestroy.} = + `=copy`(a.x, b.x) + +proc `=sink`(a: var ObjWithRef, b: ObjWithRef) {.nodestroy.} = + `=sink`(a.x, b.x) + +proc `=dup`(a: ObjWithRef): ObjWithRef {.nodestroy.} = + ObjWithRef(x: a.x) + +proc `=trace`(a: var ObjWithRef, env: pointer) {.nodestroy.} = + `=trace`(a.x, env) + +proc fooRef() = + let a = ObjWithRef(x: (ref ObjWithRef)()) + let b = a + let c = a + let d = b +fooRef()