system: lower assigning-new directly (#917)

## Summary

Instead of special-casing the assigning-`new` (i.e., `new(x)`)
throughout the compiler and only turning it into an assignment during
code generation, directly turn it into an assignment within library
code. This simplifies the magic's implementation within the compiler.

**Breaking changes**:
- `new(result)` now enforces a void context, meaning that
  `(new(result); expr)` is no longer treated as an expression

## Details

* change the `new` magic into a procedure that returns a `ref`
  instead of accepting a `var` parameter
* the new `new` magic procedure is kept as a non-exported procedure.
  User code should continue to use the `new(typedesc)` procedure
* turn the `new` routine with the `var` parameter into a template
  that calls the new `new` magic and performs the assignment
* adjust `unsafeNew` in the same way that `new` is
* remove the obsolete `internalNew` procedure; `refc` support is already
  remove from the csources compiler

In order to support instantiated generic `ref` types, the assigning
`new` template cannot continue to use `var ref T`, but instead needs to
use `var T` where `T: ref`. This was previously not necessary because no
distinction between a normal `ref T` and one coming from a generic was
made at the point were the assignment was generated (i.e., during code
generation).

**Compiler changes/simplification**:
- remove `mNew` from the `FakeVarParams` set as it no longer has a `var`
  parameter
- remove the now obsolete `lowerNew` MIR pass. The pass was previously
  needed to ensure that the previous `ref` value was cleaned up, but
  the assignment rewriting automatically takes care of that now
- update the places treating `mNew` as an assignment; it no longer is
  one
- update the code generators
- since `new(result)` expands to `result = new()`, `new(result)` forces
  a void context without any special handling being required

The `system` changes are guarded with the new conditional symbol
`nimskullNoMagicNewAssign`. This is required for bootstrapping to keep
working.

### Misc

- update the `tmovebug.nim` test, which relied on `new(result)` not
  forcing a void context. The transformation that `sem` previously
  applied is now applied manually

compiler/ast/ast_query.nim

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

compiler/backend/ccgexprs.nim

@@ -936,13 +936,11 @@ proc rawGenNew(p: BProc, a: var TLoc, sizeExpr: Rope; needsInit: bool; doInitObj
     # set the object type:
     genObjectInit(p, cpsStmts, bt, a, constructRefObj)
 
-proc genNew(p: BProc, e: CgNode) =
-  var a: TLoc
-  initLocExpr(p, e[1], a)
+proc genNew(p: BProc, e: CgNode, a: var TLoc) =
   # 'genNew' also handles 'unsafeNew':
-  if e.len == 3:
+  if e.len == 2:
     var se: TLoc
-    initLocExpr(p, e[2], se)
+    initLocExpr(p, e[1], se)
     rawGenNew(p, a, se.rdLoc, needsInit = true)
   else:
     rawGenNew(p, a, "", needsInit = true)
@@ -1792,7 +1790,7 @@ proc genMagicExpr(p: BProc, e: CgNode, d: var TLoc, op: TMagic) =
   of mFinished: genBreakState(p, e, d)
   of mEnumToStr: genCall(p, e, d)
   of mOf: genOf(p, e, d)
-  of mNew: genNew(p, e)
+  of mNew: genNew(p, e, d)
   of mNewSeqOfCap: genNewSeqOfCap(p, e, d)
   of mSizeOf:
     let t = e[1].typ.skipTypes({tyTypeDesc})

compiler/backend/jsgen.nim

@@ -1757,16 +1757,20 @@ proc genConstant*(g: PGlobals, m: BModule, c: PSym) =
   # all constants need a name:
   g.names[c.id] = name
 
-proc genNew(p: PProc, n: CgNode) =
-  var a: TCompRes
-  gen(p, n[1], a)
-  var t = skipTypes(n[1].typ, abstractVar)[0]
-  if mapType(t) == etyObject:
-    lineF(p, "$1 = $2;$n", [a.rdLoc, createVar(p, t, false)])
-  elif a.typ == etyBaseIndex:
-    lineF(p, "$1 = [$3]; $2 = 0;$n", [a.address, a.res, createVar(p, t, false)])
+proc genNew(p: PProc, n: CgNode, r: var TCompRes) =
+  ## Updates `r` with the result of a ``new`` magic invocation.
+  let t = skipTypes(n.typ, abstractInst)
+  assert t.kind == tyRef
+
+  r.kind = resVal
+  r.typ = mapType(t)
+  if r.typ == etyObject:
+    # the underlying type has reference semantics already, no
+    # boxing is needed
+    r.res = createVar(p, t.base, false)
   else:
-    lineF(p, "$1 = [[$2], 0];$n", [a.rdLoc, createVar(p, t, false)])
+    r.address = "[$1]" % createVar(p, t.base, false)
+    r.res = "0"
 
 proc genNewSeq(p: PProc, n: CgNode) =
   var x, y: TCompRes
@@ -1963,7 +1967,7 @@ proc genMagic(p: PProc, n: CgNode, r: var TCompRes) =
       gen(p, n[1], x)
       r.res = "($# == null && $# === 0)" % [x.address, x.res]
   of mEnumToStr: genRepr(p, n, r)
-  of mNew: genNew(p, n)
+  of mNew: genNew(p, n, r)
   of mChr: gen(p, n[1], r)
   of mArrToSeq:
     # only array literals doesn't need copy

compiler/front/condsyms.nim

@@ -71,4 +71,5 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasNkBreakStateNodeRemoved")
   defineSymbol("nimHasTyOwnedRemoved")
 
-  defineSymbol("nimskullReworkStaticExec")
+  defineSymbol("nimskullReworkStaticExec")
+  defineSymbol("nimskullNoMagicNewAssign")

compiler/mir/mirgen.nim

@@ -754,16 +754,13 @@ proc genMagic(c: var TCtx, n: PNode; m: TMagic): EValue =
     # use the canonical form:
     genDefault(c, n.typ)
   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
+    # ``new`` has 2 variants. The standard one with zero arguments, and the
+    # unsafe version that takes a ``size`` argument
+    assert n.len == 1 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(c): genArgExpression(c, n[0].typ[1], n[1]) => outOp() => name()
-      if n.len == 3:
+      if n.len == 2:
         # the size argument
-        chain(c): genArgExpression(c, n[0].typ[2], n[2]) => arg()
+        chain(c): genArgExpression(c, n[0].typ[1], n[1]) => arg()
 
     magicCall(c, m, typeOrVoid(c, n.typ))
   of mWasMoved:

compiler/sem/dfa.nim

@@ -764,9 +764,6 @@ 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:
-    genDef(c, n[1])
-    for i in 2..<n.len: gen(c, n[i])
   else:
     genCall(c, n)
 

compiler/sem/injectdestructors.nim

@@ -1196,31 +1196,6 @@ proc lowerBranchSwitch(buf: var MirNodeSeq, body: MirTree, graph: ModuleGraph,
 
   buf.add endNode(mnkRegion)
 
-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 == mNew:
-      c.seek(i)
-      c.replaceMulti(buf):
-        buf.subTree MirNode(kind: mnkRegion):
-          let typ = skipTypes(tree[operand(tree, Operation(i), 0)].typ,
-                              skipAliases + {tyVar})
-          # first destroy the previous value
-          genDestroy(buf, g, typ, opParamNode(0, typ))
-
-          # re-insert the call to ``new``
-          argBlock(buf):
-            chain(buf): opParam(0, typ) => tag(ekReassign) => name()
-
-            # add the remaining arguments (if any)
-            for j in 1..<numArgs(tree, Operation i):
-              let typ = tree[operand(tree, Operation i, j)].typ
-              chain(buf): opParam(buf, j.uint32, typ) => arg()
-
-          chain(buf): emit(n) => voidOut() # use the original 'new' operator
-
-      c.remove() # remove the ``mnkVoid`` node
-
 proc reportDiagnostics(g: ModuleGraph, tree: MirTree, sourceMap: SourceMap,
                        owner: PSym, diags: var seq[LocalDiag]) =
   ## Reports all diagnostics in `diags` as ``SemReport``s and clear the list
@@ -1301,11 +1276,6 @@ proc injectDestructorCalls*(g: ModuleGraph; idgen: IdGenerator; owner: PSym;
 
     let destructors = computeDestructors(tree, actx.cfg, values, entities)
 
-    # only inject destructors for calls to ``new`` if destructor-based
-    # ref-counting is used
-    if g.config.selectedGC in {gcArc, gcOrc}:
-      lowerNew(tree, g, changes)
-
     rewriteAssignments(
       tree, actx,
       AnalysisResults(v: cursor(values),

compiler/sem/nilcheck.nim

@@ -551,10 +551,8 @@ proc checkCall(n, ctx, map): Check =
             storeDependants(ctx, result.map, child, MaybeNil)
 
   if n[0].kind == nkSym and n[0].sym.magic == mNew:
-    # new hidden deref?
-    var value = if n[1].kind == nkHiddenDeref: n[1][0] else: n[1]
-    let b = ctx.index(value)
-    result.map.store(ctx, b, Safe, TAssign, value.info, value)
+    # special case for ``new``: the result is always non-nil regardless of
+    # what the type says
     result.nilability = Safe
   else:
     # echo "n ", n, " ", n.typ.isNil

compiler/sem/semexprs.nim

@@ -1019,7 +1019,7 @@ proc fixVarArgumentsAndAnalyse(c: PContext, n: PNode): PNode =
 
   var hasError = false
 
-  if magic in {mNew, mNewSeq}:
+  if magic == 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/sempass2.nim

@@ -975,7 +975,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, mNewSeq}:
+    if a.kind == nkSym and a.sym.magic == mNewSeq:
       # may not look like an assignment, but it is:
       let arg = n[1]
       initVarViaNew(tracked, arg)

compiler/vm/vmgen.nim

@@ -1099,10 +1099,10 @@ func usesRegister(p: BProc, s: LocalId): bool =
   ## (that is, whether the value is stored in a register directly)
   fitsRegister(p.body[s].typ) and not p[s].isIndirect
 
-proc genNew(c: var TCtx; n: CgNode) =
-  let dest = c.genLvalue(n[1])
+proc genNew(c: var TCtx; n: CgNode, dest: var TDest) =
+  prepare(c, dest, n, n.typ)
   c.gABx(n, opcNew, dest,
-         c.genType(n[1].typ.skipTypes(abstractVar-{tyTypeDesc})))
+         c.genType(n.typ.skipTypes(abstractInst-{tyTypeDesc})))
   c.freeTemp(dest)
 
 proc genNewSeq(c: var TCtx; n: CgNode) =
@@ -1682,8 +1682,7 @@ proc genMagic(c: var TCtx; n: CgNode; dest: var TDest; m: TMagic) =
   of mIsolate:
     genCall(c, n, dest)
   of mNew:
-    unused(c, n, dest)
-    c.genNew(n)
+    c.genNew(n, dest)
   of mNewSeq:
     unused(c, n, dest)
     c.genNewSeq(n)

lib/system.nim

@@ -265,14 +265,6 @@ proc typeof*(x: untyped; mode = typeOfIter): typedesc {.
 
 const ThisIsSystem = true
 
-when compileOption("gc", "refc"):
-  # TODO: obsolete magic definition; remove after the next csource update.
-  #       The magic is unrelated to refc, but since only the csource
-  #       compiler still supports and uses refc, we can use its presense as
-  #       a way it to identify the csources compiler
-  proc internalNew*[T](a: var ref T) {.magic: "New", noSideEffect.}
-    ## Leaked implementation detail. Do not use.
-
 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
@@ -566,16 +558,29 @@ when defined(js) or defined(nimdoc):
     JsRoot* = ref object of RootObj
       ## Root type of the JavaScript object hierarchy
 
-proc unsafeNew*[T](a: var ref T, size: Natural) {.magic: "New", noSideEffect.}
-  ## Creates a new object of type `T` and returns a safe (traced)
-  ## reference to it in `a`.
-  ##
-  ## This is **unsafe** as it allocates an object of the passed `size`.
-  ## This should only be used for optimization purposes when you know
-  ## what you're doing!
-  ##
-  ## See also:
-  ## * `new <#new,ref.T,proc(ref.T)>`_
+when defined(nimskullNoMagicNewAssign):
+  proc unsafeNew[T: ref](size: Natural): T {.magic: "New", noSideEffect.}
+
+  template unsafeNew*[T: ref](a: var T, size: Natural) =
+    ## Creates a new object of `T`'s underlying object type and returns a
+    ## safe (traced) reference to it in `a`.
+    ##
+    ## This is **unsafe** as it allocates an object of the passed `size`.
+    ## This should only be used for optimization purposes when you know
+    ## what you're doing!
+    a = unsafeNew[T](size)
+
+else:
+  proc unsafeNew*[T](a: var ref T, size: Natural) {.magic: "New", noSideEffect.}
+    ## Creates a new object of type `T` and returns a safe (traced)
+    ## reference to it in `a`.
+    ##
+    ## This is **unsafe** as it allocates an object of the passed `size`.
+    ## This should only be used for optimization purposes when you know
+    ## what you're doing!
+    ##
+    ## See also:
+    ## * `new <#new,ref.T,proc(ref.T)>`_
 
 proc sizeof*[T](x: T): int {.magic: "SizeOf", noSideEffect.}
   ## Returns the size of `x` in bytes.
@@ -830,9 +835,28 @@ template `isnot`*(x, y: untyped): untyped = not (x is y)
 
 template owned*(t: typedesc): typedesc {.deprecated.} = t
 
-when true:
-  template unown*(x: typed): untyped {.deprecated.} = x
+template unown*(x: typed): untyped {.deprecated.} = x
+
+when defined(nimskullNoMagicNewAssign):
+  proc new[T: ref](): T {.magic: "New", noSideEffect.}
+
+  template new*[T: ref](a: var T) =
+    ## Creates a new object of `T`'s underlying object type and returns
+    ## a safe (traced) reference to it in `a`.
+    a = new[T]()
+
+  proc new*(t: typedesc): auto =
+    ## Creates a new object of type `T` and returns a safe (traced)
+    ## reference to it as result value.
+    ##
+    ## When `T` is a ref type then the resulting type will be `T`,
+    ## otherwise it will be `ref T`.
+    when t is ref:
+      result = new[t]()
+    else:
+      result = new[ref t]()
 
+else:
   proc new*[T](a: var ref T) {.magic: "New", noSideEffect.}
     ## Creates a new object of type `T` and returns a safe (traced)
     ## reference to it in `a`.

tests/arc/tmovebug.nim

@@ -557,15 +557,16 @@ type
 
 proc newWrapper(): ref Wrapper =
   new(result)
-  result
+  result = result
 
 
 proc newWrapper2(a: int): ref Wrapper =
   new(result)
-  if a > 0:
-    result
-  else:
-    new(Wrapper)
+  result =
+    if a > 0:
+      result
+    else:
+      new(Wrapper)
 
 
 let w1 = newWrapper()