From da4aa2e1fb8b0084fcf03b85edf4a5c1d0170856 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Thu, 15 Oct 2020 12:52:30 +0200 Subject: [PATCH] renamed '=' to '=copy' [backport:1.2] (#15585) * Assign hook name changed to `=copy` * Adapt destructors.rst * [nobackport] Duplicate tests for =copy hook * Fix tests * added a changelog entry Co-authored-by: Clyybber --- changelog.md | 4 + compiler/ast.nim | 2 +- compiler/injectdestructors.nim | 4 +- compiler/sempass2.nim | 3 +- compiler/semstmts.nim | 4 +- doc/destructors.rst | 22 +- tests/arc/tcaseobjcopy.nim | 246 ++++++++ tests/arc/tcomputedgotocopy.nim | 41 ++ tests/arc/tmovebugcopy.nim | 526 ++++++++++++++++++ tests/arc/topt_no_cursor.nim | 12 +- tests/arc/topt_wasmoved_destroy_pairs.nim | 2 +- tests/arc/tweavecopy.nim | 154 +++++ tests/destructor/tconsume_twice.nim | 2 +- tests/destructor/tprevent_assign.nim | 2 +- tests/destructor/tprevent_assign2.nim | 2 +- tests/destructor/tprevent_assign3.nim | 2 +- tests/destructor/tuse_ownedref_after_move.nim | 2 +- 17 files changed, 1001 insertions(+), 29 deletions(-) create mode 100644 tests/arc/tcaseobjcopy.nim create mode 100644 tests/arc/tcomputedgotocopy.nim create mode 100644 tests/arc/tmovebugcopy.nim create mode 100644 tests/arc/tweavecopy.nim diff --git a/changelog.md b/changelog.md index 2c8f9b5e2f0a0..14bcbe6084f49 100644 --- a/changelog.md +++ b/changelog.md @@ -215,6 +215,10 @@ - The `=destroy` hook no longer has to reset its target, as the compiler now automatically inserts `wasMoved` calls where needed. +- The `=` hook is now called `=copy` for clarity. The old name `=` is still available so there + is no need to update your code. This change was backported to 1.2 too so you can use the + more readability `=copy` without loss of compatibility. + - In the newruntime it is now allowed to assign to the discriminator field without restrictions as long as case object doesn't have custom destructor. The discriminator value doesn't have to be a constant either. If you have a diff --git a/compiler/ast.nim b/compiler/ast.nim index 8adbfa15cb47e..f796e64c2d880 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -1331,7 +1331,7 @@ const MaxLockLevel* = 1000'i16 UnknownLockLevel* = TLockLevel(1001'i16) AttachedOpToStr*: array[TTypeAttachedOp, string] = [ - "=destroy", "=", "=sink", "=trace", "=dispose", "=deepcopy"] + "=destroy", "=copy", "=sink", "=trace", "=dispose", "=deepcopy"] proc `$`*(x: TLockLevel): string = if x.ord == UnspecifiedLockLevel.ord: result = "" diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index b18bcf34e1318..ea90299e2da93 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -235,7 +235,7 @@ template isUnpackedTuple(n: PNode): bool = proc checkForErrorPragma(c: Con; t: PType; ri: PNode; opname: string) = var m = "'" & opname & "' is not available for type <" & typeToString(t) & ">" - if opname == "=" and ri != nil: + if (opname == "=" or opname == "=copy") and ri != nil: m.add "; requires a copy because it's not the last read of '" m.add renderTree(ri) m.add '\'' @@ -319,7 +319,7 @@ proc genCopy(c: var Con; dest, ri: PNode): PNode = if tfHasOwned in t.flags and ri.kind != nkNilLit: # try to improve the error message here: if c.otherRead == nil: discard isLastRead(ri, c) - c.checkForErrorPragma(t, ri, "=") + c.checkForErrorPragma(t, ri, "=copy") result = c.genCopyNoCheck(dest, ri) proc genDiscriminantAsgn(c: var Con; s: var Scope; n: PNode): PNode = diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim index 5eb464bb80255..aa04f7451e78b 100644 --- a/compiler/sempass2.nim +++ b/compiler/sempass2.nim @@ -809,7 +809,8 @@ proc trackCall(tracked: PEffects; n: PNode) = if a.kind == nkSym and a.sym.name.s.len > 0 and a.sym.name.s[0] == '=' and tracked.owner.kind != skMacro: - let opKind = find(AttachedOpToStr, a.sym.name.s.normalize) + var opKind = find(AttachedOpToStr, a.sym.name.s.normalize) + if a.sym.name.s.normalize == "=": opKind = attachedAsgn.int if opKind != -1: # rebind type bounds operations after createTypeBoundOps call let t = n[1].typ.skipTypes({tyAlias, tyVar}) diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim index 27cd9019fafe1..b5e69c135447c 100644 --- a/compiler/semstmts.nim +++ b/compiler/semstmts.nim @@ -1732,7 +1732,7 @@ proc semOverride(c: PContext, s: PSym, n: PNode) = "signature for 'deepCopy' must be proc[T: ptr|ref](x: T): T") incl(s.flags, sfUsed) incl(s.flags, sfOverriden) - of "=", "=sink": + of "=", "=copy", "=sink": if s.magic == mAsgn: return incl(s.flags, sfUsed) incl(s.flags, sfOverriden) @@ -1754,7 +1754,7 @@ proc semOverride(c: PContext, s: PSym, n: PNode) = # attach these ops to the canonical tySequence obj = canonType(c, obj) #echo "ATTACHING TO ", obj.id, " ", s.name.s, " ", cast[int](obj) - let k = if name == "=": attachedAsgn else: attachedSink + let k = if name == "=" or name == "=copy": attachedAsgn else: attachedSink if obj.attachedOps[k] == s: discard "forward declared op" elif obj.attachedOps[k].isNil and tfCheckedForDestructor notin obj.flags: diff --git a/doc/destructors.rst b/doc/destructors.rst index b433c38d7a133..b581fce3e25de 100644 --- a/doc/destructors.rst +++ b/doc/destructors.rst @@ -40,7 +40,7 @@ written as: for i in 0..= 0 and k <= n + var pos = newSeq[int](k) + var current = newSeq[T](k) + for i in 0..k-1: + pos[k-i-1] = i + var done = false + while not done: + for i in 0..k-1: + current[i] = s[pos[k-i-1]] + yield current + var i = 0 + while i < k: + pos[i] += 1 + if pos[i] < n-i: + for j in 0..i-1: + pos[j] = pos[i] + i - j + break + i += 1 + if i >= k: + break + +type + UndefEx = object of ValueError + +proc main2 = + var delayedSyms = @[1, 2, 3] + var unp: seq[int] + block myb: + for a in 1 .. 2: + if delayedSyms.len > a: + unp = delayedSyms + for t in unp.combinations(a + 1): + try: + var h = false + for k in t: + echo "fff" + if h: continue + if true: + raise newException(UndefEx, "forward declaration") + break myb + except UndefEx: + echo t.len + echo "mmm" + +main2() + + + +type ME = object + who: string + +proc `=copy`(x: var ME, y: ME) = + if y.who.len > 0: echo "assign ",y.who + +proc `=sink`(x: var ME, y: ME) = + if y.who.len > 0: echo "sink ",y.who + +var dump: ME +template use(x) = dump = x +template def(x) = x = dump + +var c = true + +proc shouldSink() = + var x = ME(who: "me (sink)") + use(x) # we analyse this + if c: def(x) + else: def(x) + use(x) # ok, with the [else] part. + +shouldSink() + +dump = ME() + +proc shouldNotSink() = + var x = ME(who: "me (not sink)") + use(x) # we analyse this + if c: def(x) + use(x) # Not ok without the '[else]' + +shouldNotSink() + +# bug #14568 +import os + +type O2 = object + s: seq[int] + +proc `=sink`(dest: var O2, src: O2) = + echo "sinked and not optimized to a bitcopy" + +var testSeq: O2 + +proc update() = + # testSeq.add(0) # uncommenting this line fixes the leak + testSeq = O2(s: @[]) + testSeq.s.add(0) + +for i in 1..3: + update() + + +# bug #14961 +type + Foo = object + data: seq[int] + +proc initFoo(len: int): Foo = + result = (let s = newSeq[int](len); Foo(data: s) ) + +var f = initFoo(2) +echo initFoo(2) + +proc initFoo2(len: int) = + echo if true: + let s = newSeq[int](len); Foo(data: s) + else: + let s = newSeq[int](len); Foo(data: s) + +initFoo2(2) + +proc initFoo3(len: int) = + echo (block: + let s = newSeq[int](len); Foo(data: s)) + +initFoo3(2) + +proc initFoo4(len: int) = + echo (let s = newSeq[int](len); Foo(data: s)) + +initFoo4(2) + +proc initFoo5(len: int) = + echo (case true + of true: + let s = newSeq[int](len); Foo(data: s) + of false: + let s = newSeq[int](len); Foo(data: s)) + +initFoo5(2) + +proc initFoo6(len: int) = + echo (block: + try: + let s = newSeq[int](len); Foo(data: s) + finally: discard) + +initFoo6(2) + +proc initFoo7(len: int) = + echo (block: + try: + raise newException(CatchableError, "sup") + let s = newSeq[int](len); Foo(data: s) + except CatchableError: + let s = newSeq[int](len); Foo(data: s) ) + +initFoo7(2) + + +# bug #14902 +iterator zip[T](s: openarray[T]): (T, T) = + var i = 0 + while i < 10: + yield (s[i mod 2], s[i mod 2 + 1]) + inc i + +var lastMem = int.high + +proc leak = + const len = 10 + var x = @[newString(len), newString(len), newString(len)] + + var c = 0 + for (a, b) in zip(x): + let newMem = getOccupiedMem() + assert newMem <= lastMem + lastMem = newMem + c += a.len + echo c + +leak() + + +proc consume(a: sink string) = echo a + +proc weirdScopes = + if (let a = "hey"; a.len > 0): + echo a + + while (let a = "hey"; a.len > 0): + echo a + break + + var a = block: (a: "a", b: 2) + echo a + (discard; a) = (echo "ho"; (a: "b", b: 3)) + echo a + + var b = try: (b: "b", a: 2) + except: raise + echo b + (discard; b) = (echo "ho"; (b: "a", a: 3)) + echo b + + var s = "break" + consume((echo "hey"; s)) + echo s + + echo (block: + var a = "hey" + (echo "hey"; "ho")) + + var b2 = "ho" + echo (block: + var a = "hey" + (echo "hey"; b2)) + echo b2 + + type status = enum + alive + + var king = "king" + echo (block: + var a = "a" + when true: + var b = "b" + case alive + of alive: + try: + var c = "c" + if true: + king + else: + "the abyss" + except: + echo "he ded" + "dead king") + echo "live long; long live" + echo king + +weirdScopes() + + +# bug #14985 +proc getScope(): string = + if true: + "hi" + else: + "else" + +echo getScope() + +proc getScope3(): string = + try: + "try" + except: + "except" + +echo getScope3() + +proc getScope2(): string = + case true + of true: + "bye" + else: + "else" + +echo getScope2() diff --git a/tests/arc/topt_no_cursor.nim b/tests/arc/topt_no_cursor.nim index 5fd21439aac98..d5811e91e5fe9 100644 --- a/tests/arc/topt_no_cursor.nim +++ b/tests/arc/topt_no_cursor.nim @@ -33,9 +33,9 @@ result = ( var sibling saved -`=`(sibling, target.parent.left) -`=`(saved, sibling.right) -`=`(sibling.right, saved.left) +`=copy`(sibling, target.parent.left) +`=copy`(saved, sibling.right) +`=copy`(sibling.right, saved.left) `=sink`(sibling.parent, saved) `=destroy`(sibling) -- end of expandArc ------------------------ @@ -46,7 +46,7 @@ var lvalue lnext _ -`=`(lresult, [123]) +`=copy`(lresult, [123]) _ = ( let blitTmp = lresult blitTmp, ";") @@ -67,10 +67,10 @@ try: var it_cursor = x a = ( wasMoved(:tmpD) - `=`(:tmpD, it_cursor.key) + `=copy`(:tmpD, it_cursor.key) :tmpD, wasMoved(:tmpD_1) - `=`(:tmpD_1, it_cursor.val) + `=copy`(:tmpD_1, it_cursor.val) :tmpD_1) echo [ :tmpD_2 = `$`(a) diff --git a/tests/arc/topt_wasmoved_destroy_pairs.nim b/tests/arc/topt_wasmoved_destroy_pairs.nim index 2a6391dd212ee..2f971f1122700 100644 --- a/tests/arc/topt_wasmoved_destroy_pairs.nim +++ b/tests/arc/topt_wasmoved_destroy_pairs.nim @@ -38,7 +38,7 @@ try: return add(a): wasMoved(:tmpD) - `=`(:tmpD, x) + `=copy`(:tmpD, x) :tmpD inc i_1, 1 if cond: diff --git a/tests/arc/tweavecopy.nim b/tests/arc/tweavecopy.nim new file mode 100644 index 0000000000000..fc796b3525b24 --- /dev/null +++ b/tests/arc/tweavecopy.nim @@ -0,0 +1,154 @@ +discard """ + outputsub: '''Success''' + cmd: '''nim c --gc:arc --threads:on $file''' + disabled: "bsd" +""" + +# bug #13936 + +import std/atomics + +const MemBlockSize = 256 + +type + ChannelSPSCSingle* = object + full{.align: 128.}: Atomic[bool] + itemSize*: uint8 + buffer*{.align: 8.}: UncheckedArray[byte] + +proc `=copy`( + dest: var ChannelSPSCSingle, + source: ChannelSPSCSingle + ) {.error: "A channel cannot be copied".} + +proc initialize*(chan: var ChannelSPSCSingle, itemsize: SomeInteger) {.inline.} = + ## If ChannelSPSCSingle is used intrusive another data structure + ## be aware that it should be the last part due to ending by UncheckedArray + ## Also due to 128 bytes padding, it automatically takes half + ## of the default MemBlockSize + assert itemsize.int in 0 .. int high(uint8) + assert itemSize.int + + sizeof(chan.itemsize) + + sizeof(chan.full) < MemBlockSize + + chan.itemSize = uint8 itemsize + chan.full.store(false, moRelaxed) + +func isEmpty*(chan: var ChannelSPSCSingle): bool {.inline.} = + not chan.full.load(moAcquire) + +func tryRecv*[T](chan: var ChannelSPSCSingle, dst: var T): bool {.inline.} = + ## Try receiving the item buffered in the channel + ## Returns true if successful (channel was not empty) + ## + ## ⚠ Use only in the consumer thread that reads from the channel. + assert (sizeof(T) == chan.itemsize.int) or + # Support dummy object + (sizeof(T) == 0 and chan.itemsize == 1) + + let full = chan.full.load(moAcquire) + if not full: + return false + dst = cast[ptr T](chan.buffer.addr)[] + chan.full.store(false, moRelease) + return true + +func trySend*[T](chan: var ChannelSPSCSingle, src: sink T): bool {.inline.} = + ## Try sending an item into the channel + ## Reurns true if successful (channel was empty) + ## + ## ⚠ Use only in the producer thread that writes from the channel. + assert (sizeof(T) == chan.itemsize.int) or + # Support dummy object + (sizeof(T) == 0 and chan.itemsize == 1) + + let full = chan.full.load(moAcquire) + if full: + return false + cast[ptr T](chan.buffer.addr)[] = src + chan.full.store(true, moRelease) + return true + +# Sanity checks +# ------------------------------------------------------------------------------ +when isMainModule: + + when not compileOption("threads"): + {.error: "This requires --threads:on compilation flag".} + + template sendLoop[T](chan: var ChannelSPSCSingle, + data: sink T, + body: untyped): untyped = + while not chan.trySend(data): + body + + template recvLoop[T](chan: var ChannelSPSCSingle, + data: var T, + body: untyped): untyped = + while not chan.tryRecv(data): + body + + type + ThreadArgs = object + ID: WorkerKind + chan: ptr ChannelSPSCSingle + + WorkerKind = enum + Sender + Receiver + + template Worker(id: WorkerKind, body: untyped): untyped {.dirty.} = + if args.ID == id: + body + + proc thread_func(args: ThreadArgs) = + + # Worker RECEIVER: + # --------- + # <- chan + # <- chan + # <- chan + # + # Worker SENDER: + # --------- + # chan <- 42 + # chan <- 53 + # chan <- 64 + Worker(Receiver): + var val: int + for j in 0 ..< 10: + args.chan[].recvLoop(val): + # Busy loop, in prod we might want to yield the core/thread timeslice + discard + echo " Receiver got: ", val + doAssert val == 42 + j*11 + + Worker(Sender): + doAssert args.chan.full.load(moRelaxed) == false + for j in 0 ..< 10: + let val = 42 + j*11 + args.chan[].sendLoop(val): + # Busy loop, in prod we might want to yield the core/thread timeslice + discard + echo "Sender sent: ", val + + proc main() = + echo "Testing if 2 threads can send data" + echo "-----------------------------------" + var threads: array[2, Thread[ThreadArgs]] + + var chan = cast[ptr ChannelSPSCSingle](allocShared(MemBlockSize)) + chan[].initialize(itemSize = sizeof(int)) + + createThread(threads[0], thread_func, ThreadArgs(ID: Receiver, chan: chan)) + createThread(threads[1], thread_func, ThreadArgs(ID: Sender, chan: chan)) + + joinThread(threads[0]) + joinThread(threads[1]) + + freeShared(chan) + + echo "-----------------------------------" + echo "Success" + + main() diff --git a/tests/destructor/tconsume_twice.nim b/tests/destructor/tconsume_twice.nim index 0030267f8dd16..b0a039e9b79a1 100644 --- a/tests/destructor/tconsume_twice.nim +++ b/tests/destructor/tconsume_twice.nim @@ -1,6 +1,6 @@ discard """ cmd: "nim c --newruntime $file" - errormsg: "'=' is not available for type ; requires a copy because it's not the last read of 'a'; another read is done here: tconsume_twice.nim(13, 10); routine: consumeTwice" + errormsg: "'=copy' is not available for type ; requires a copy because it's not the last read of 'a'; another read is done here: tconsume_twice.nim(13, 10); routine: consumeTwice" line: 11 """ type diff --git a/tests/destructor/tprevent_assign.nim b/tests/destructor/tprevent_assign.nim index 108ccc37158aa..4c484ebc1de7d 100644 --- a/tests/destructor/tprevent_assign.nim +++ b/tests/destructor/tprevent_assign.nim @@ -1,5 +1,5 @@ discard """ - errormsg: "'=' is not available for type ; requires a copy because it's not the last read of 'otherTree'" + errormsg: "'=copy' is not available for type ; requires a copy because it's not the last read of 'otherTree'" line: 29 """ diff --git a/tests/destructor/tprevent_assign2.nim b/tests/destructor/tprevent_assign2.nim index 0e44817100db6..ef20672d597ba 100644 --- a/tests/destructor/tprevent_assign2.nim +++ b/tests/destructor/tprevent_assign2.nim @@ -1,5 +1,5 @@ discard """ - errormsg: "'=' is not available for type ; requires a copy because it's not the last read of 'otherTree'" + errormsg: "'=copy' is not available for type ; requires a copy because it's not the last read of 'otherTree'" file: "tprevent_assign2.nim" line: 48 """ diff --git a/tests/destructor/tprevent_assign3.nim b/tests/destructor/tprevent_assign3.nim index a8a35ea5e5193..0577aa5ff0743 100644 --- a/tests/destructor/tprevent_assign3.nim +++ b/tests/destructor/tprevent_assign3.nim @@ -1,5 +1,5 @@ discard """ - errormsg: "'=' is not available for type ; requires a copy because it's not the last read of 'otherTree'" + errormsg: "'=copy' is not available for type ; requires a copy because it's not the last read of 'otherTree'" file: "tprevent_assign3.nim" line: 46 """ diff --git a/tests/destructor/tuse_ownedref_after_move.nim b/tests/destructor/tuse_ownedref_after_move.nim index 46540837c3d9c..ce96b741edc5a 100644 --- a/tests/destructor/tuse_ownedref_after_move.nim +++ b/tests/destructor/tuse_ownedref_after_move.nim @@ -1,6 +1,6 @@ discard """ cmd: '''nim c --newruntime $file''' - errormsg: "'=' is not available for type ; requires a copy because it's not the last read of ':envAlt.b1'; another read is done here: tuse_ownedref_after_move.nim(52, 4)" + errormsg: "'=copy' is not available for type ; requires a copy because it's not the last read of ':envAlt.b1'; another read is done here: tuse_ownedref_after_move.nim(52, 4)" line: 48 """