deprecate NewFinalize with the ref T finalizer (#24354)

pre-existing issues: ```nim block: type FooObj = object data: int Foo = ref ref FooObj proc delete(self: Foo) = echo self.data var s: Foo new(s, delete) ``` it crashed with arc/orc in 1.6.x and 2.x.x ```nim block: type Foo = ref int proc delete(self: Foo) = echo self[] var s: Foo new(s, delete) ``` The simple fix is to add a type restriction for the type `T` for arc/orc versions ```nim proc new*[T: object](a: var ref T, finalizer: proc (x: T) {.nimcall.}) ```
2025-12-28 17:04:41 +00:00 · 2024-10-26 04:35:26 +08:00
parent d303c289fa
commit 2af602a5c8
4 changed files with 86 additions and 39 deletions
--- a/compiler/semmagic.nim
+++ b/compiler/semmagic.nim
@@ -536,31 +536,33 @@ proc semNewFinalize(c: PContext; n: PNode): PNode =
      else:
        if fin.instantiatedFrom != nil and fin.instantiatedFrom != fin.owner: #undo move
          setOwner(fin, fin.instantiatedFrom)
-        let wrapperSym = newSym(skProc, getIdent(c.graph.cache, fin.name.s & "FinalizerWrapper"), c.idgen, fin.owner, fin.info)
-        let selfSymNode = newSymNode(copySym(fin.ast[paramsPos][1][0].sym, c.idgen))
-        selfSymNode.typ() = fin.typ.firstParamType
-        wrapperSym.flags.incl sfUsed

-        let wrapper = c.semExpr(c, newProcNode(nkProcDef, fin.info, body = newTree(nkCall, newSymNode(fin), selfSymNode),
-          params = nkFormalParams.newTree(c.graph.emptyNode,
-                  newTree(nkIdentDefs, selfSymNode, newNodeIT(nkType,
-                  fin.ast[paramsPos][1][1].info, fin.typ.firstParamType), c.graph.emptyNode)
-                  ),
-          name = newSymNode(wrapperSym), pattern = fin.ast[patternPos],
-          genericParams = fin.ast[genericParamsPos], pragmas = fin.ast[pragmasPos], exceptions = fin.ast[miscPos]), {})
+        if fin.typ[1].skipTypes(abstractInst).kind != tyRef:
+          bindTypeHook(c, fin, n, attachedDestructor)
+        else:
+          let wrapperSym = newSym(skProc, getIdent(c.graph.cache, fin.name.s & "FinalizerWrapper"), c.idgen, fin.owner, fin.info)
+          let selfSymNode = newSymNode(copySym(fin.ast[paramsPos][1][0].sym, c.idgen))
+          selfSymNode.typ() = fin.typ.firstParamType
+          wrapperSym.flags.incl sfUsed

-        var transFormedSym = turnFinalizerIntoDestructor(c, wrapperSym, wrapper.info)
-        setOwner(transFormedSym, fin)
-        if c.config.backend == backendCpp or sfCompileToCpp in c.module.flags:
-          let origParamType = transFormedSym.ast[bodyPos][1].typ
-          let selfSymbolType = makePtrType(c, origParamType.skipTypes(abstractPtrs))
-          let selfPtr = newNodeI(nkHiddenAddr, transFormedSym.ast[bodyPos][1].info)
-          selfPtr.add transFormedSym.ast[bodyPos][1]
-          selfPtr.typ() = selfSymbolType
-          transFormedSym.ast[bodyPos][1] = c.semExpr(c, selfPtr)
-        # TODO: suppress var destructor warnings; if newFinalizer is not
-        # TODO: deprecated, try to implement plain T destructor
-        bindTypeHook(c, transFormedSym, n, attachedDestructor, suppressVarDestructorWarning = true)
+          let wrapper = c.semExpr(c, newProcNode(nkProcDef, fin.info, body = newTree(nkCall, newSymNode(fin), selfSymNode),
+            params = nkFormalParams.newTree(c.graph.emptyNode,
+                    newTree(nkIdentDefs, selfSymNode, newNodeIT(nkType,
+                    fin.ast[paramsPos][1][1].info, fin.typ.firstParamType), c.graph.emptyNode)
+                    ),
+            name = newSymNode(wrapperSym), pattern = fin.ast[patternPos],
+            genericParams = fin.ast[genericParamsPos], pragmas = fin.ast[pragmasPos], exceptions = fin.ast[miscPos]), {})
+
+          var transFormedSym = turnFinalizerIntoDestructor(c, wrapperSym, wrapper.info)
+          setOwner(transFormedSym, fin)
+          if c.config.backend == backendCpp or sfCompileToCpp in c.module.flags:
+            let origParamType = transFormedSym.ast[bodyPos][1].typ
+            let selfSymbolType = makePtrType(c, origParamType.skipTypes(abstractPtrs))
+            let selfPtr = newNodeI(nkHiddenAddr, transFormedSym.ast[bodyPos][1].info)
+            selfPtr.add transFormedSym.ast[bodyPos][1]
+            selfPtr.typ() = selfSymbolType
+            transFormedSym.ast[bodyPos][1] = c.semExpr(c, selfPtr)
+          bindTypeHook(c, transFormedSym, n, attachedDestructor)
  result = addDefaultFieldForNew(c, n)

 proc semPrivateAccess(c: PContext, n: PNode): PNode =
--- a/compiler/semstmts.nim
+++ b/compiler/semstmts.nim
@@ -2092,7 +2092,7 @@ proc bindDupHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) =
  incl(s.flags, sfUsed)
  incl(s.flags, sfOverridden)

-proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp; suppressVarDestructorWarning = false) =
+proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) =
  let t = s.typ
  var noError = false
  let cond = case op
@@ -2116,7 +2116,7 @@ proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp; suppressV
      elif obj.kind == tyGenericInvocation: obj = obj.genericHead
      else: break
    if obj.kind in {tyObject, tyDistinct, tySequence, tyString}:
-      if (not suppressVarDestructorWarning) and op == attachedDestructor and t.firstParamType.kind == tyVar and
+      if op == attachedDestructor and t.firstParamType.kind == tyVar and
          c.config.selectedGC in {gcArc, gcAtomicArc, gcOrc}:
        message(c.config, n.info, warnDeprecated, "A custom '=destroy' hook which takes a 'var T' parameter is deprecated; it should take a 'T' parameter")
      obj = canonType(c, obj)
--- a/lib/system.nim
+++ b/lib/system.nim
@@ -125,18 +125,38 @@ proc unsafeAddr*[T](x: T): ptr T {.magic: "Addr", noSideEffect.} =

 const ThisIsSystem = true

-proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {.
-  magic: "NewFinalize", noSideEffect.}
-  ## Creates a new object of type `T` and returns a safe (traced)
-  ## reference to it in `a`.
-  ##
-  ## When the garbage collector frees the object, `finalizer` is called.
-  ## The `finalizer` may not keep a reference to the
-  ## object pointed to by `x`. The `finalizer` cannot prevent the GC from
-  ## freeing the object.
-  ##
-  ## **Note**: The `finalizer` refers to the type `T`, not to the object!
-  ## This means that for each object of type `T` the finalizer will be called!
+const arcLikeMem = defined(gcArc) or defined(gcAtomicArc) or defined(gcOrc)
+
+when defined(nimAllowNonVarDestructor) and arcLikeMem:
+  proc new*[T](a: var ref T, finalizer: proc (x: T) {.nimcall.}) {.
+    magic: "NewFinalize", noSideEffect.}
+    ## Creates a new object of type `T` and returns a safe (traced)
+    ## reference to it in `a`.
+    ##
+    ## When the garbage collector frees the object, `finalizer` is called.
+    ## The `finalizer` may not keep a reference to the
+    ## object pointed to by `x`. The `finalizer` cannot prevent the GC from
+    ## freeing the object.
+    ##
+    ## **Note**: The `finalizer` refers to the type `T`, not to the object!
+    ## This means that for each object of type `T` the finalizer will be called!
+
+  proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {.
+    magic: "NewFinalize", noSideEffect, deprecated: "pass a finalizer of the 'proc (x: T) {.nimcall.}' type".}
+
+else:
+  proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {.
+    magic: "NewFinalize", noSideEffect.}
+    ## Creates a new object of type `T` and returns a safe (traced)
+    ## reference to it in `a`.
+    ##
+    ## When the garbage collector frees the object, `finalizer` is called.
+    ## The `finalizer` may not keep a reference to the
+    ## object pointed to by `x`. The `finalizer` cannot prevent the GC from
+    ## freeing the object.
+    ##
+    ## **Note**: The `finalizer` refers to the type `T`, not to the object!
+    ## This means that for each object of type `T` the finalizer will be called!

 proc `=wasMoved`*[T](obj: var T) {.magic: "WasMoved", noSideEffect.} =
  ## Generic `wasMoved`:idx: implementation that can be overridden.
@@ -362,8 +382,6 @@ proc arrGet[I: Ordinal;T](a: T; i: I): T {.
 proc arrPut[I: Ordinal;T,S](a: T; i: I;
  x: S) {.noSideEffect, magic: "ArrPut".}

-const arcLikeMem = defined(gcArc) or defined(gcAtomicArc) or defined(gcOrc)
-

 when defined(nimAllowNonVarDestructor) and arcLikeMem and defined(nimPreviewNonVarDestructor):
  proc `=destroy`*[T](x: T) {.inline, magic: "Destroy".} =
--- a/tests/arc/tarcmisc.nim
+++ b/tests/arc/tarcmisc.nim
@@ -834,3 +834,30 @@ block: # bug #24141
    doAssert abc == "fbc"

  main()
+
+block:
+  type
+    FooObj = object
+      data: int
+    Foo = ref FooObj
+
+
+  proc delete(self: FooObj) =
+    discard
+
+  var s = Foo()
+  new(s, delete)
+
+block:
+  type
+    FooObj = object
+      data: int
+      i1, i2, i3, i4: float
+    Foo = ref FooObj
+
+
+  proc delete(self: FooObj) =
+    discard
+
+  var s = Foo()
+  new(s, delete)