From 4abbeebf49f4ea07e19a3423cde21397a10e0650 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sat, 23 Sep 2023 00:07:57 +0200 Subject: [PATCH] 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 | 2 +- compiler/backend/ccgexprs.nim | 10 ++--- compiler/backend/jsgen.nim | 24 ++++++----- compiler/front/condsyms.nim | 3 +- compiler/mir/mirgen.nim | 13 +++--- compiler/sem/dfa.nim | 3 -- compiler/sem/injectdestructors.nim | 30 -------------- compiler/sem/nilcheck.nim | 6 +-- compiler/sem/semexprs.nim | 2 +- compiler/sem/sempass2.nim | 2 +- compiler/vm/vmgen.nim | 9 ++--- lib/system.nim | 64 ++++++++++++++++++++---------- tests/arc/tmovebug.nim | 11 ++--- 13 files changed, 84 insertions(+), 95 deletions(-) diff --git a/compiler/ast/ast_query.nim b/compiler/ast/ast_query.nim index fbfa069cb78..6454a714430 100644 --- a/compiler/ast/ast_query.nim +++ b/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} diff --git a/compiler/backend/ccgexprs.nim b/compiler/backend/ccgexprs.nim index c3ce9981458..89a936bcdfe 100644 --- a/compiler/backend/ccgexprs.nim +++ b/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}) diff --git a/compiler/backend/jsgen.nim b/compiler/backend/jsgen.nim index 356033ee51b..e99554fb102 100644 --- a/compiler/backend/jsgen.nim +++ b/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 diff --git a/compiler/front/condsyms.nim b/compiler/front/condsyms.nim index fa0d90aedb1..e5c9cf80dc6 100644 --- a/compiler/front/condsyms.nim +++ b/compiler/front/condsyms.nim @@ -71,4 +71,5 @@ proc initDefines*(symbols: StringTableRef) = defineSymbol("nimHasNkBreakStateNodeRemoved") defineSymbol("nimHasTyOwnedRemoved") - defineSymbol("nimskullReworkStaticExec") \ No newline at end of file + defineSymbol("nimskullReworkStaticExec") + defineSymbol("nimskullNoMagicNewAssign") \ No newline at end of file diff --git a/compiler/mir/mirgen.nim b/compiler/mir/mirgen.nim index 85b7f5e7306..5972a44c3d6 100644 --- a/compiler/mir/mirgen.nim +++ b/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: diff --git a/compiler/sem/dfa.nim b/compiler/sem/dfa.nim index 41d86275987..69636f5ea6a 100644 --- a/compiler/sem/dfa.nim +++ b/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.. tag(ekReassign) => name() - - # add the remaining arguments (if any) - for j in 1.. 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), diff --git a/compiler/sem/nilcheck.nim b/compiler/sem/nilcheck.nim index 762815b0923..d65fdfc1a37 100644 --- a/compiler/sem/nilcheck.nim +++ b/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 diff --git a/compiler/sem/semexprs.nim b/compiler/sem/semexprs.nim index 6b4a020c0fd..fe22ae5b71a 100644 --- a/compiler/sem/semexprs.nim +++ b/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': diff --git a/compiler/sem/sempass2.nim b/compiler/sem/sempass2.nim index 4634ce831b4..6b2338f1599 100644 --- a/compiler/sem/sempass2.nim +++ b/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..`_ +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`. diff --git a/tests/arc/tmovebug.nim b/tests/arc/tmovebug.nim index ca4aa944b7d..ec1c154ce0e 100644 --- a/tests/arc/tmovebug.nim +++ b/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()