Merge nim-lang#604

604: language: remove new-with-finalize r=saem a=zerbina

## Summary

The new-with-finalize (`new(x, finalizer)`) feature had unintuitive behaviour, worked different when using `--gc:arc|orc`, and was unsupported both in compile-time execution contexts and by the JavaScript and VM backend.

Due to the amount of issues the feature had, and destructors being able to replace it in all valid use-cases, the feature is removed.

## Details

- remove the `mNewFinalize` magic and adjust all usage sites
- replace finalizer usage with destructors for the `nre` and `re` module
- remove tests for new-with-finalize feature
- adjust tests that use new-with-finalize but don't depend on it



Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>

compiler/ast/ast_query.nim

@@ -120,7 +120,7 @@ const
   defaultAlignment* = -1
   defaultOffset* = -1
 
-  FakeVarParams* = {mNew, mNewFinalize, mInc, mDec, mIncl, mExcl,
+  FakeVarParams* = {mNew, mInc, mDec, mIncl, mExcl,
     mSetLengthStr, mSetLengthSeq, mAppendStrCh, mAppendStrStr, mSwap,
     mAppendSeqElem, mNewSeq, mReset, mShallowCopy, mDeepCopy, mMove,
     mWasMoved}

compiler/ast/ast_types.nim

@@ -725,7 +725,7 @@ type
     mPlugin, mEcho, mShallowCopy, mSlurp, mStaticExec, mStatic,
     mParseExprToAst, mParseStmtToAst, mExpandToAst, mQuoteAst,
     mInc, mDec, mOrd,
-    mNew, mNewFinalize, mNewSeq, mNewSeqOfCap,
+    mNew, mNewSeq, mNewSeqOfCap,
     mLengthOpenArray, mLengthStr, mLengthArray, mLengthSeq,
     mIncl, mExcl, mCard, mChr,
     mGCref, mGCunref,

compiler/ast/report_enums.nim

@@ -592,7 +592,6 @@ type
     rsemOldTakesParameterName
     rsemOldDoesNotBelongTo
     rsemCannotFindPlugin
-    rsemExpectedProcReferenceForFinalizer
     rsemCannotIsolate
     rsemRecursiveDependencyIterator
     rsemIllegalNimvmContext

compiler/backend/ccgcalls.nim

@@ -308,7 +308,7 @@ proc getPotentialWrites(n: PNode; mutate: bool; result: var seq[PNode]) =
   of nkCallKinds:
     case n.getMagic:
     of mIncl, mExcl, mInc, mDec, mAppendStrCh, mAppendStrStr, mAppendSeqElem,
-        mAddr, mNew, mNewFinalize, mWasMoved, mDestroy, mReset:
+        mAddr, mNew, mWasMoved, mDestroy, mReset:
       getPotentialWrites(n[1], true, result)
       for i in 2..<n.len:
         getPotentialWrites(n[i], mutate, result)

compiler/backend/ccgexprs.nim

@@ -1709,26 +1709,6 @@ proc genArrToSeq(p: BProc, n: PNode, d: var TLoc) =
     genAssignment(p, elem, arr, {needToCopy})
     lineF(p, cpsStmts, "}$n", [])
 
-
-proc genNewFinalize(p: BProc, e: PNode) =
-  var
-    a, b, f: TLoc
-    refType, bt: PType
-    ti: Rope
-  refType = skipTypes(e[1].typ, abstractVarRange)
-  initLocExpr(p, e[1], a)
-  initLocExpr(p, e[2], f)
-  initLoc(b, locExpr, a.lode, OnHeap)
-  ti = genTypeInfo(p.config, p.module, refType, e.info)
-  p.module.s[cfsTypeInit3].addf("$1->finalizer = (void*)$2;$n", [ti, rdLoc(f)])
-  b.r = ropecg(p.module, "($1) #newObj($2, sizeof($3))", [
-      getTypeDesc(p.module, refType),
-      ti, getTypeDesc(p.module, skipTypes(refType.lastSon, abstractRange))])
-  genAssignment(p, a, b, {})  # set the object type:
-  bt = skipTypes(refType.lastSon, abstractRange)
-  genObjectInit(p, cpsStmts, bt, a, constructRefObj)
-  gcUsage(p.config, e)
-
 proc genOfHelper(p: BProc; dest: PType; a: Rope; info: TLineInfo): Rope =
   if optTinyRtti in p.config.globalOptions:
     result = ropecg(p.module, "#isObj($1.m_type, $2)",
@@ -2452,14 +2432,6 @@ proc genMagicExpr(p: BProc, e: PNode, d: var TLoc, op: TMagic) =
       genRepr(p, e, d)
   of mOf: genOf(p, e, d)
   of mNew: genNew(p, e)
-  of mNewFinalize:
-    if optTinyRtti in p.config.globalOptions:
-      var a: TLoc
-      initLocExpr(p, e[1], a)
-      rawGenNew(p, a, "", needsInit = true)
-      gcUsage(p.config, e)
-    else:
-      genNewFinalize(p, e)
   of mNewSeq:
     if optSeqDestructors in p.config.globalOptions:
       e[1] = makeAddr(e[1], p.module.idgen)

compiler/backend/jsgen.nim

@@ -2064,7 +2064,7 @@ proc genMagic(p: PProc, n: PNode, r: var TCompRes) =
       gen(p, n[1], x)
       r.res = "($# == null && $# === 0)" % [x.address, x.res]
   of mEnumToStr: genRepr(p, n, r)
-  of mNew, mNewFinalize: genNew(p, n)
+  of mNew: genNew(p, n)
   of mChr: gen(p, n[1], r)
   of mArrToSeq:
     # only array literals doesn't need copy

compiler/front/cli_reporter.nim

@@ -873,9 +873,6 @@ proc reportBody*(conf: ConfigRef, r: SemReport): string =
     of rsemCannotFindPlugin:
       result = "cannot find plugin " & r.symstr
 
-    of rsemExpectedProcReferenceForFinalizer:
-      result = "finalizer must be a direct reference to a proc"
-
     of rsemUnsafeSetLen:
       result = "setLen can potentially expand the sequence, " &
         "but the element type '$1' doesn't have a valid default value" %

compiler/mir/mirgen.nim

@@ -987,17 +987,16 @@ proc genMagic(c: var TCtx, n: PNode; m: TMagic): EValue =
   of mDefault:
     # use the canonical form:
     genDefault(c, n.typ)
-  of mNew, mNewFinalize:
-    # ``new`` has 3 variants. The standard one with a single argument, the
-    # unsafe version that also takes a ``size`` argument, and the finalizer
-    # version
+  of mNew:
+    # ``new`` has 2 variants. The standard one with a single argument, and the
+    # unsafe version that also takes an extra ``size`` argument
     assert n.len == 3 or n.len == 2
     argBlock(c.stmts):
       # the first argument is the location storing the ``ref``. A new value is
       # assigned to it by ``new``, so the 'out' tag is used
       chain: genArgExpression(c, n[0].typ[1], n[1]) => outOp(c) => name(c)
       if n.len == 3:
-        # the finalizer or size argument
+        # the size argument
         chain: genArgExpression(c, n[0].typ[2], n[2]) => arg(c)
 
     magicCall(c, m, typeOrVoid(c, n.typ))

compiler/sem/dfa.nim

@@ -764,7 +764,7 @@ proc genCall(c: var Con; n: PNode) =
 proc genMagic(c: var Con; n: PNode; m: TMagic) =
   case m
   of mAnd, mOr: c.genAndOr(n)
-  of mNew, mNewFinalize:
+  of mNew:
     genDef(c, n[1])
     for i in 2..<n.len: gen(c, n[i])
   else:

compiler/sem/injectdestructors.nim

@@ -1350,7 +1350,7 @@ proc deferGlobalDestructors(tree: MirTree, g: ModuleGraph, idgen: IdGenerator,
 proc lowerNew(tree: MirTree, g: ModuleGraph, c: var Changeset) =
   ## Lower calls to the ``new(x)`` into a ``=destroy(x); new(x)``
   for i, n in tree.pairs:
-    if n.kind == mnkMagic and n.magic in {mNew, mNewFinalize}:
+    if n.kind == mnkMagic and n.magic == mNew:
       c.seek(i)
       c.replaceMulti(buf):
         buf.subTree MirNode(kind: mnkRegion):

compiler/sem/semexprs.nim

@@ -1028,7 +1028,7 @@ proc fixVarArgumentsAndAnalyse(c: PContext, n: PNode): PNode =
 
   var hasError = false
 
-  if magic in {mNew, mNewFinalize, mNewSeq}:
+  if magic in {mNew, mNewSeq}:
     # XXX: this check doesn't really fit here. ``magicsAfterOverloadResolution``
     #       would be a better place for it
     # bug #5113: disallow newSeq(result) where result is a 'var T':

compiler/sem/semmagic.nim

@@ -346,41 +346,6 @@ proc semOf(c: PContext, n: PNode): PNode =
   n.typ = getSysType(c.graph, n.info, tyBool)
   result = n
 
-proc turnFinalizerIntoDestructor(c: PContext; orig: PSym; info: TLineInfo): PSym =
-  # We need to do 2 things: Replace n.typ which is a 'ref T' by a 'var T' type.
-  # Replace nkDerefExpr by nkHiddenDeref
-  # nkDeref is for 'ref T':  x[].field
-  # nkHiddenDeref is for 'var T': x<hidden deref [] here>.field
-  proc transform(c: PContext; procSym: PSym; n: PNode; old, fresh: PType; oldParam, newParam: PSym): PNode =
-    result = shallowCopy(n)
-    if sameTypeOrNil(n.typ, old):
-      result.typ = fresh
-    if n.kind == nkSym:
-      if n.sym == oldParam:
-        result.sym = newParam
-      elif n.sym.owner == orig:
-        result.sym = copySym(n.sym, nextSymId c.idgen)
-        result.sym.owner = procSym
-    for i in 0 ..< safeLen(n):
-      result[i] = transform(c, procSym, n[i], old, fresh, oldParam, newParam)
-    #if n.kind == nkDerefExpr and sameType(n[0].typ, old):
-    #  result =
-
-  result = copySym(orig, nextSymId c.idgen)
-  result.info = info
-  result.flags.incl sfFromGeneric
-  result.owner = orig
-  let origParamType = orig.typ[1]
-  let newParamType = makeVarType(result, origParamType.skipTypes(abstractPtrs), c.idgen)
-  let oldParam = orig.typ.n[1].sym
-  let newParam = newSym(skParam, oldParam.name, nextSymId c.idgen, result, result.info)
-  newParam.typ = newParamType
-  # proc body:
-  result.ast = transform(c, result, orig.ast, origParamType, newParamType, oldParam, newParam)
-  # proc signature:
-  result.typ = newProcType(result.info, nextTypeId c.idgen, result)
-  result.typ.addParam newParam
-
 proc semPrivateAccess(c: PContext, n: PNode): PNode =
   let t = n[1].typ[0].toObjectFromRefPtrGeneric
   c.currentScope.allowPrivateAccess.add t.sym
@@ -448,31 +413,6 @@ proc magicsAfterOverloadResolution(c: PContext, n: PNode,
       result = n
     else:
       result = plugin(c, n)
-  of mNewFinalize:
-    # Make sure the finalizer procedure refers to a procedure
-    if n[^1].kind == nkSym and n[^1].sym.kind notin {skProc, skFunc}:
-      localReport(c.config, n, reportSem rsemExpectedProcReferenceForFinalizer)
-    elif optTinyRtti in c.config.globalOptions:
-      let nfin = skipConvCastAndClosure(n[^1])
-      let fin = case nfin.kind
-        of nkSym: nfin.sym
-        of nkLambda, nkDo: nfin[namePos].sym
-        else:
-          localReport(c.config, n, reportSem rsemExpectedProcReferenceForFinalizer)
-          nil
-      if fin != nil:
-        if fin.kind notin {skProc, skFunc}:
-          # calling convention is checked in codegen
-          localReport(c.config, n, reportSem rsemExpectedProcReferenceForFinalizer)
-
-        # check if we converted this finalizer into a destructor already:
-        let t = whereToBindTypeHook(c, fin.typ[1].skipTypes(abstractInst+{tyRef}))
-        if t != nil and getAttachedOp(c.graph, t, attachedDestructor) != nil and
-            getAttachedOp(c.graph, t, attachedDestructor).owner == fin:
-          discard "already turned this one into a finalizer"
-        else:
-          bindTypeHook(c, turnFinalizerIntoDestructor(c, fin, n.info), n, attachedDestructor)
-    result = n
   of mDestroy:
     result = n
     let t = n[1].typ.skipTypes(abstractVar)

compiler/sem/sempass2.nim

@@ -985,7 +985,7 @@ proc trackCall(tracked: PEffects; n: PNode) =
     if a.kind != nkSym or a.sym.magic notin {mFinished}:
       for i in 1..<n.len:
         trackOperandForIndirectCall(tracked, n[i], op, i, a)
-    if a.kind == nkSym and a.sym.magic in {mNew, mNewFinalize, mNewSeq}:
+    if a.kind == nkSym and a.sym.magic in {mNew, mNewSeq}:
       # may not look like an assignment, but it is:
       let arg = n[1]
       initVarViaNew(tracked, arg)
@@ -1001,7 +1001,6 @@ proc trackCall(tracked: PEffects; n: PNode) =
       if n[1].typ.len > 0:
         createTypeBoundOps(tracked, n[1].typ.lastSon, n.info)
         createTypeBoundOps(tracked, n[1].typ, n.info)
-        # new(x, finalizer): Problem: how to move finalizer into 'createTypeBoundOps'?
 
     elif a.kind == nkSym and a.sym.magic in {mArrGet, mArrPut} and
         optStaticBoundsCheck in tracked.currOptions:

compiler/sem/semstmts.nim

@@ -2405,15 +2405,6 @@ proc prevDestructor(c: PContext; prevOp: PSym; obj: PType; info: TLineInfo) =
     localReport(c.config, info, reportSym(
       rsemRebidingDestructor, prevOp, typ = obj))
 
-proc whereToBindTypeHook(c: PContext; t: PType): PType =
-  result = t
-  while true:
-    if result.kind in {tyGenericBody, tyGenericInst}: result = result.lastSon
-    elif result.kind == tyGenericInvocation: result = result[0]
-    else: break
-  if result.kind in {tyObject, tyDistinct, tySequence, tyString}:
-    result = canonType(c, result)
-
 proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) =
   let t = s.typ
   var noError = false

compiler/vm/vmgen.nim

@@ -1504,7 +1504,7 @@ proc genMagic(c: var TCtx; n: PNode; dest: var TDest; m: TMagic) =
     c.freeTemp(L)
   of mIsolate:
     genCall(c, n, dest)
-  of mNew, mNewFinalize:
+  of mNew:
     unused(c, n, dest)
     c.genNew(n)
   of mNewSeq:

lib/impure/nre.nim

@@ -211,7 +211,9 @@ type
     ## for whatever reason. The message contains the error
     ## code.
 
-proc destroyRegex(pattern: Regex) =
+template objectOf[T](x: typedesc[ref T]): typedesc = T
+
+proc `=destroy`(pattern: var objectOf(Regex)) =
   pcre.free_substring(cast[cstring](pattern.pcreObj))
   if pattern.pcreExtra != nil:
     pcre.free_study(pattern.pcreExtra)
@@ -244,7 +246,7 @@ proc getNameToNumberTable(pattern: Regex): Table[string, int] =
     result[name] = num
 
 proc initRegex(pattern: string, flags: int, study = true): Regex =
-  new(result, destroyRegex)
+  new(result)
   result.pattern = pattern
 
   var errorMsg: cstring

lib/impure/re.nim

@@ -57,12 +57,6 @@ type
   RegexError* = object of ValueError
     ## is raised if the pattern is no valid regular expression.
 
-when defined(gcDestructors):
-  proc `=destroy`(x: var RegexDesc) =
-    pcre.free_substring(cast[cstring](x.h))
-    if not isNil(x.e):
-      pcre.free_study(x.e)
-
 proc raiseInvalidRegex(msg: string) {.noinline, noreturn.} =
   var e: ref RegexError
   new(e)
@@ -77,7 +71,7 @@ proc rawCompile(pattern: string, flags: cint): ptr Pcre =
   if result == nil:
     raiseInvalidRegex($msg & "\n" & pattern & "\n" & spaces(offset) & "^\n")
 
-proc finalizeRegEx(x: Regex) =
+proc `=destroy`(x: var RegexDesc) =
   # XXX This is a hack, but PCRE does not export its "free" function properly.
   # Sigh. The hack relies on PCRE's implementation (see `pcre_get.c`).
   # Fortunately the implementation is unlikely to change.
@@ -95,10 +89,7 @@ proc re*(s: string, flags = {reStudy}): Regex =
   ## avoid putting it directly in the arguments of the functions like
   ## the examples show below if you plan to use it a lot of times, as
   ## this will hurt performance immensely. (e.g. outside a loop, ...)
-  when defined(gcDestructors):
-    result = Regex()
-  else:
-    new(result, finalizeRegEx)
+  result = Regex()
   result.h = rawCompile(s, cast[cint](flags - {reStudy}))
   if reStudy in flags:
     var msg: cstring = ""

lib/system.nim

@@ -268,20 +268,6 @@ const ThisIsSystem = true
 proc internalNew*[T](a: var ref T) {.magic: "New", noSideEffect.}
   ## Leaked implementation detail. Do not use.
 
-when 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!
-
 proc wasMoved*[T](obj: var T) {.magic: "WasMoved", noSideEffect.} =
   ## Resets an object `obj` to its initial (binary zero) value to signify
   ## it was "moved" and to signify its destructor should do nothing and

tests/arc/t14383.nim

@@ -52,21 +52,6 @@ echo x
 import std/os
 discard getFileInfo(".")
 
-
-#------------------------------------------------------------------------------
-# Issue #15707
-#------------------------------------------------------------------------------
-
-type
-  JVMObject = ref object
-proc freeJVMObject(o: JVMObject) =
-  discard
-proc fromJObject(T: typedesc[JVMObject]): T =
-  result.new(cast[proc(r: T) {.nimcall.}](freeJVMObject))
-
-discard JVMObject.fromJObject()
-
-
 #------------------------------------------------------------------------------
 # Issue #15910
 #------------------------------------------------------------------------------