From a137e50150cdbc48fcfb02064aa0c064fec4c7e8 Mon Sep 17 00:00:00 2001 From: ringabout <43030857+ringabout@users.noreply.github.com> Date: Thu, 2 Mar 2023 12:29:40 +0800 Subject: [PATCH] fixes #19291; implements `wasMoved` hook (#21303) * fixes #19291; implements `wasMoved` hook * basics * checkpoint * finish `wasMoved` * add a test for #19291 * add documentation and changelog * work `attachedWasMoved` with generics * fixes optimizer * register `=wasMoved` * handle wasMoved magcis * check another round * some patches * try `op == nil` * nicer * generate `wasMoved` before `destroy` * try again * fixes tests * default wasMoved * Update tests/destructor/tv2_cast.nim * Update tests/destructor/tv2_cast.nim * Update tests/arc/topt_refcursors.nim --- changelogs/changelog_2_0_0.md | 2 ++ compiler/ast.nim | 3 +- compiler/injectdestructors.nim | 17 +++++++--- compiler/liftdestructors.nim | 41 ++++++++++++++++++++--- compiler/optimizer.nim | 9 +++-- compiler/semstmts.nim | 5 ++- doc/manual.md | 2 +- tests/arc/topt_no_cursor.nim | 19 ++++++----- tests/arc/topt_refcursors.nim | 8 +++-- tests/arc/topt_wasmoved_destroy_pairs.nim | 13 ++++--- tests/destructor/tv2_cast.nim | 14 ++++---- tests/destructor/twasmoved_error.nim | 37 ++++++++++++++++++++ 12 files changed, 131 insertions(+), 39 deletions(-) create mode 100644 tests/destructor/twasmoved_error.nim diff --git a/changelogs/changelog_2_0_0.md b/changelogs/changelog_2_0_0.md index e081865bed3c1..cbe7554785350 100644 --- a/changelogs/changelog_2_0_0.md +++ b/changelogs/changelog_2_0_0.md @@ -341,6 +341,8 @@ - IBM Z architecture and macOS m1 arm64 architecture are supported. +- `=wasMoved` can be overridden by users. + ## Compiler changes - The `gc` switch has been renamed to `mm` ("memory management") in order to reflect the diff --git a/compiler/ast.nim b/compiler/ast.nim index aa70560ed5ff5..f93c8d9101a4d 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -935,6 +935,7 @@ type TTypeSeq* = seq[PType] TTypeAttachedOp* = enum ## as usual, order is important here + attachedWasMoved, attachedDestructor, attachedAsgn, attachedSink, @@ -1502,7 +1503,7 @@ proc newProcNode*(kind: TNodeKind, info: TLineInfo, body: PNode, const AttachedOpToStr*: array[TTypeAttachedOp, string] = [ - "=destroy", "=copy", "=sink", "=trace", "=deepcopy"] + "=wasMoved", "=destroy", "=copy", "=sink", "=trace", "=deepcopy"] proc `$`*(s: PSym): string = if s != nil: diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 20e7902028158..13141b765d40f 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -354,11 +354,18 @@ It is best to factor out piece of object that needs custom destructor into separ result.add newTree(nkFastAsgn, le, tmp) proc genWasMoved(c: var Con, n: PNode): PNode = - result = newNodeI(nkCall, n.info) - result.add(newSymNode(createMagic(c.graph, c.idgen, "wasMoved", mWasMoved))) - result.add copyTree(n) #mWasMoved does not take the address - #if n.kind != nkSym: - # message(c.graph.config, n.info, warnUser, "wasMoved(" & $n & ")") + let typ = n.typ.skipTypes({tyGenericInst, tyAlias, tySink}) + let op = getAttachedOp(c.graph, n.typ, attachedWasMoved) + if op != nil: + if sfError in op.flags: + c.checkForErrorPragma(n.typ, n, "=wasMoved") + result = genOp(c, op, n) + else: + result = newNodeI(nkCall, n.info) + result.add(newSymNode(createMagic(c.graph, c.idgen, "wasMoved", mWasMoved))) + result.add copyTree(n) #mWasMoved does not take the address + #if n.kind != nkSym: + # message(c.graph.config, n.info, warnUser, "wasMoved(" & $n & ")") proc genDefaultCall(t: PType; c: Con; info: TLineInfo): PNode = result = newNodeI(nkCall, info) diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index 738f659b70650..c7464d39e109d 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -88,6 +88,8 @@ proc defaultOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = let call = genBuiltin(c, mDefault, "default", x) call.typ = t body.add newAsgnStmt(x, call) + elif c.kind == attachedWasMoved: + body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc genAddr(c: var TLiftCtx; x: PNode): PNode = if x.kind == nkHiddenDeref: @@ -145,6 +147,11 @@ proc destructorCall(c: var TLiftCtx; op: PSym; x: PNode): PNode = else: result = destroy +proc genWasMovedCall(c: var TLiftCtx; op: PSym; x: PNode): PNode = + result = newNodeIT(nkCall, x.info, op.typ[0]) + result.add(newSymNode(op)) + result.add genAddr(c, x) + proc fillBodyObj(c: var TLiftCtx; n, body, x, y: PNode; enforceDefaultOp: bool) = case n.kind of nkSym: @@ -442,6 +449,20 @@ proc considerUserDefinedOp(c: var TLiftCtx; t: PType; body, x, y: PNode): bool = body.add newDeepCopyCall(c, op, x, y) result = true + of attachedWasMoved: + var op = getAttachedOp(c.g, t, attachedWasMoved) + if op != nil and sfOverriden in op.flags: + + if op.ast.isGenericRoutine: + # patch generic destructor: + op = instantiateGeneric(c, op, t, t.typeInst) + setAttachedOp(c.g, c.idgen.module, t, attachedWasMoved, op) + + #markUsed(c.g.config, c.info, op, c.g.usageSym) + onUse(c.info, op) + body.add genWasMovedCall(c, op, x) + result = true + proc declareCounter(c: var TLiftCtx; body: PNode; first: BiggestInt): PNode = var temp = newSym(skTemp, getIdent(c.g.cache, lowerings.genPrefix), nextSymId(c.idgen), c.fn, c.info) temp.typ = getSysType(c.g, body.info, tyInt) @@ -524,6 +545,7 @@ proc fillSeqOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = if canFormAcycle(t.elemType): # follow all elements: forallElements(c, t, body, x, y) + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc useSeqOrStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = createTypeBoundOps(c.g, c.c, t, body.info, c.idgen) @@ -561,6 +583,7 @@ proc useSeqOrStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = if op == nil: return # protect from recursion body.add newHookCall(c, op, x, y) + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc fillStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = case c.kind @@ -576,6 +599,7 @@ proc fillStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genBuiltin(c, mDestroy, "destroy", x) of attachedTrace: discard "strings are atomic and have no inner elements that are to trace" + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc cyclicType*(t: PType): bool = case t.kind @@ -674,6 +698,8 @@ proc atomicRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = # If the ref is polymorphic we have to account for this body.add callCodegenProc(c.g, "nimTraceRefDyn", c.info, genAddrOf(x, c.idgen), y) #echo "can follow ", elemType, " static ", isFinal(elemType) + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) + proc atomicClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = ## Closures are really like refs except they always use a virtual destructor @@ -722,6 +748,7 @@ proc atomicClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace: body.add callCodegenProc(c.g, "nimTraceRefDyn", c.info, genAddrOf(xenv, c.idgen), y) + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc weakrefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = case c.kind @@ -746,6 +773,7 @@ proc weakrefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.sons.insert(des, 0) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace: discard + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc ownedRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = var actions = newNodeI(nkStmtList, c.info) @@ -771,6 +799,7 @@ proc ownedRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genIf(c, x, actions) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace: discard + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc closureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = if c.kind == attachedDeepCopy: @@ -805,6 +834,7 @@ proc closureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.sons.insert(des, 0) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace: discard + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = let xx = genBuiltin(c, mAccessEnv, "accessEnv", x) @@ -820,6 +850,7 @@ proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = body.add genIf(c, xx, actions) of attachedDeepCopy: assert(false, "cannot happen") of attachedTrace: discard + of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x) proc fillBody(c: var TLiftCtx; t: PType; body, x, y: PNode) = case t.kind @@ -935,7 +966,7 @@ proc symPrototype(g: ModuleGraph; typ: PType; owner: PSym; kind: TTypeAttachedOp result.typ = newProcType(info, nextTypeId(idgen), owner) result.typ.addParam dest - if kind != attachedDestructor: + if kind notin {attachedDestructor, attachedWasMoved}: result.typ.addParam src if kind == attachedAsgn and g.config.selectedGC == gcOrc and @@ -975,7 +1006,7 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp; let dest = result.typ.n[1].sym let d = newDeref(newSymNode(dest)) - let src = if kind == attachedDestructor: newNodeIT(nkSym, info, getSysType(g, info, tyPointer)) + let src = if kind in {attachedDestructor, attachedWasMoved}: newNodeIT(nkSym, info, getSysType(g, info, tyPointer)) else: newSymNode(result.typ.n[2].sym) # register this operation already: @@ -1103,15 +1134,15 @@ proc createTypeBoundOps(g: ModuleGraph; c: PContext; orig: PType; info: TLineInf # bug #15122: We need to produce all prototypes before entering the # mind boggling recursion. Hacks like these imply we should rewrite # this module. - var generics: array[attachedDestructor..attachedTrace, bool] - for k in attachedDestructor..lastAttached: + var generics: array[attachedWasMoved..attachedTrace, bool] + for k in attachedWasMoved..lastAttached: generics[k] = getAttachedOp(g, canon, k) != nil if not generics[k]: setAttachedOp(g, idgen.module, canon, k, symPrototype(g, canon, canon.owner, k, info, idgen)) # we generate the destructor first so that other operators can depend on it: - for k in attachedDestructor..lastAttached: + for k in attachedWasMoved..lastAttached: if not generics[k]: discard produceSym(g, c, canon, k, info, idgen) else: diff --git a/compiler/optimizer.nim b/compiler/optimizer.nim index 98939f80de2c8..0b26e8d34f176 100644 --- a/compiler/optimizer.nim +++ b/compiler/optimizer.nim @@ -16,6 +16,8 @@ import from trees import exprStructuralEquivalent +import std/strutils + const nfMarkForDeletion = nfNone # faster than a lookup table @@ -110,16 +112,17 @@ proc analyse(c: var Con; b: var BasicBlock; n: PNode) = var reverse = false if n[0].kind == nkSym: let s = n[0].sym - if s.magic == mWasMoved: + let name = s.name.s.normalize + if s.magic == mWasMoved or name == "=wasmoved": b.wasMovedLocs.add n special = true - elif s.name.s == "=destroy": + elif name == "=destroy": if c.inFinally > 0 and (b.hasReturn or b.hasBreak): discard "cannot optimize away the destructor" else: c.wasMovedDestroyPair b, n special = true - elif s.name.s == "=sink": + elif name == "=sink": reverse = true if not special: diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim index 7dc976c19aea4..6237e6eb06272 100644 --- a/compiler/semstmts.nim +++ b/compiler/semstmts.nim @@ -1779,7 +1779,7 @@ proc whereToBindTypeHook(c: PContext; t: PType): PType = proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) = let t = s.typ var noError = false - let cond = if op == attachedDestructor: + let cond = if op in {attachedDestructor, attachedWasMoved}: t.len == 2 and t[0] == nil and t[1].kind == tyVar elif op == attachedTrace: t.len == 3 and t[0] == nil and t[1].kind == tyVar and t[2].kind == tyPointer @@ -1894,6 +1894,9 @@ proc semOverride(c: PContext, s: PSym, n: PNode) = of "=trace": if s.magic != mTrace: bindTypeHook(c, s, n, attachedTrace) + of "=wasmoved": + if s.magic != mWasMoved: + bindTypeHook(c, s, n, attachedWasMoved) else: if sfOverriden in s.flags: localError(c.config, n.info, errGenerated, diff --git a/doc/manual.md b/doc/manual.md index ef790c36240bb..31c620ca73657 100644 --- a/doc/manual.md +++ b/doc/manual.md @@ -4122,7 +4122,7 @@ the operator is in scope (including if it is private). ``` Type bound operators are: -`=destroy`, `=copy`, `=sink`, `=trace`, `=deepcopy`. +`=destroy`, `=copy`, `=sink`, `=trace`, `=deepcopy`, `=wasMoved`. These operations can be *overridden* instead of *overloaded*. This means that the implementation is automatically lifted to structured types. For instance, diff --git a/tests/arc/topt_no_cursor.nim b/tests/arc/topt_no_cursor.nim index 30d4c316c55fa..50dfa26ac0b84 100644 --- a/tests/arc/topt_no_cursor.nim +++ b/tests/arc/topt_no_cursor.nim @@ -1,7 +1,8 @@ discard """ nimoutFull: true cmd: '''nim c -r --warnings:off --hints:off --gc:arc --expandArc:newTarget --expandArc:delete --expandArc:p1 --expandArc:tt --hint:Performance:off --assertions:off --expandArc:extractConfig --expandArc:mergeShadowScope --expandArc:check $file''' - nimout: '''--expandArc: newTarget + nimout: ''' +--expandArc: newTarget var splat @@ -11,9 +12,9 @@ splat = splitDrive do: let blitTmp = path blitTmp :tmp = splat.drive -wasMoved(splat.drive) +`=wasMoved`(splat.drive) :tmp_1 = splat.path_1 -wasMoved(splat.path_1) +`=wasMoved`(splat.path_1) result = ( let blitTmp_1 = :tmp blitTmp_1, @@ -60,10 +61,10 @@ var try: it_cursor = x a = ( - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy`(:tmpD, it_cursor.key) :tmpD, - wasMoved(:tmpD_1) + `=wasMoved`(:tmpD_1) `=copy`(:tmpD_1, it_cursor.val) :tmpD_1) echo [ @@ -112,7 +113,7 @@ block :tmp: var :tmpD sym = shadowScope.symbols[i] addInterfaceDecl(c): - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy_1`(:tmpD, sym) :tmpD inc(i, 1) @@ -125,7 +126,7 @@ this.isValid = fileExists(this.value) if dirExists(this.value): var :tmpD par = (dir: - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy`(:tmpD, this.value) :tmpD, front: "") else: var @@ -133,10 +134,10 @@ if dirExists(this.value): :tmpD_2 :tmpD_3 par = (dir_1: parentDir(this.value), front_1: - wasMoved(:tmpD_1) + `=wasMoved`(:tmpD_1) `=copy`(:tmpD_1, :tmpD_3 = splitDrive do: - wasMoved(:tmpD_2) + `=wasMoved`(:tmpD_2) `=copy`(:tmpD_2, this.value) :tmpD_2 :tmpD_3.path) diff --git a/tests/arc/topt_refcursors.nim b/tests/arc/topt_refcursors.nim index c13d81badc1e0..8c638a4a1e530 100644 --- a/tests/arc/topt_refcursors.nim +++ b/tests/arc/topt_refcursors.nim @@ -1,7 +1,8 @@ discard """ output: '''''' cmd: '''nim c --gc:arc --expandArc:traverse --hint:Performance:off $file''' - nimout: '''--expandArc: traverse + nimout: ''' +--expandArc: traverse var it_cursor @@ -22,12 +23,13 @@ try: `=copy`(ri_1, jt.ri) echo [jt.s] `=sink`(jt, ri_1) - wasMoved(ri_1) + `=wasMoved`(ri_1) finally: `=destroy`(ri_1) finally: `=destroy`(jt) --- end of expandArc ------------------------''' +-- end of expandArc ------------------------ +''' """ type diff --git a/tests/arc/topt_wasmoved_destroy_pairs.nim b/tests/arc/topt_wasmoved_destroy_pairs.nim index 2f971f1122700..6577d678739b4 100644 --- a/tests/arc/topt_wasmoved_destroy_pairs.nim +++ b/tests/arc/topt_wasmoved_destroy_pairs.nim @@ -1,7 +1,8 @@ discard """ output: '''''' cmd: '''nim c --gc:arc --expandArc:main --expandArc:tfor --hint:Performance:off $file''' - nimout: '''--expandArc: main + nimout: ''' +--expandArc: main var a @@ -29,6 +30,7 @@ try: x = f() block :tmp: var i_cursor + mixin inc var i_1 = 0 block :tmp_1: while i_1 < 4: @@ -37,25 +39,26 @@ try: if i_cursor == 2: return add(a): - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy`(:tmpD, x) :tmpD inc i_1, 1 if cond: add(a): let blitTmp = x - wasMoved(x) + `=wasMoved`(x) blitTmp else: add(b): let blitTmp_1 = x - wasMoved(x) + `=wasMoved`(x) blitTmp_1 finally: `=destroy`(x) `=destroy_1`(b) `=destroy_1`(a) --- end of expandArc ------------------------''' +-- end of expandArc ------------------------ +''' """ proc f(): seq[int] = diff --git a/tests/destructor/tv2_cast.nim b/tests/destructor/tv2_cast.nim index 917cf0eb3d521..6fa419996ceb7 100644 --- a/tests/destructor/tv2_cast.nim +++ b/tests/destructor/tv2_cast.nim @@ -4,7 +4,8 @@ discard """ @[1953719668, 875770417] destroying O1''' cmd: '''nim c --gc:arc --expandArc:main --expandArc:main1 --expandArc:main2 --expandArc:main3 --hints:off --assertions:off $file''' - nimout: '''--expandArc: main + nimout: ''' +--expandArc: main var data @@ -12,7 +13,7 @@ var :tmpD_1 :tmpD_2 data = - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy`(:tmpD, cast[string]( :tmpD_2 = encode(cast[seq[byte]]( :tmpD_1 = newString(100) @@ -32,7 +33,7 @@ var :tmpD_1 s = newString(100) data = - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy`(:tmpD, cast[string]( :tmpD_1 = encode(toOpenArrayByte(s, 0, len(s) - 1)) :tmpD_1)) @@ -50,7 +51,7 @@ var :tmpD_1 s = newSeq(100) data = - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy`(:tmpD, cast[string]( :tmpD_1 = encode(s) :tmpD_1)) @@ -67,7 +68,7 @@ var :tmpD_1 :tmpD_2 data = - wasMoved(:tmpD) + `=wasMoved`(:tmpD) `=copy`(:tmpD, cast[string]( :tmpD_2 = encode do: :tmpD_1 = newSeq(100) @@ -77,7 +78,8 @@ data = `=destroy`(:tmpD_2) `=destroy`(:tmpD_1) `=destroy_1`(data) --- end of expandArc ------------------------''' +-- end of expandArc ------------------------ +''' """ func encode*(src: openArray[byte]): seq[byte] = diff --git a/tests/destructor/twasmoved_error.nim b/tests/destructor/twasmoved_error.nim new file mode 100644 index 0000000000000..1cd57e3df1108 --- /dev/null +++ b/tests/destructor/twasmoved_error.nim @@ -0,0 +1,37 @@ +discard """ + cmd: '''nim c --mm:arc $file''' + errormsg: "'=wasMoved' is not available for type ; routine: main" +""" + +# bug #19291 + +const + screenWidth = 800 + screenHeight = 450 + +var + ready = false +type + Game = object + +proc `=destroy`(x: var Game) = + assert ready, "Window is already opened" + ready = false + +proc `=sink`(x: var Game; y: Game) {.error.} +proc `=copy`(x: var Game; y: Game) {.error.} +proc `=wasMoved`(x: var Game) {.error.} + +proc initGame(width, height: int32, title: string): Game = + assert not ready, "Window is already closed" + ready = true + +proc update(x: Game) = discard + +proc main = + var g = initGame(screenWidth, screenHeight, "Tetris raylib") + g.update() + var g2 = g + echo "hello" + +main()