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.})
```

(cherry picked from commit 2af602a)

Author: ringabout

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]), {})
-
-        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)
+
+        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
+
+          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 =

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)

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".} =

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)