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 <darkmine956@gmail.com>

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

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 = "<unspecified>"

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 =

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})

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:

doc/destructors.rst

@@ -40,7 +40,7 @@ written as:
       for i in 0..<x.len: `=destroy`(x[i])
       dealloc(x.data)
 
-  proc `=`*[T](a: var myseq[T]; b: myseq[T]) =
+  proc `=copy`*[T](a: var myseq[T]; b: myseq[T]) =
     # do nothing for self-assignments:
     if a.data == b.data: return
     `=destroy`(a)
@@ -134,7 +134,7 @@ not free the resources afterwards by setting the object to its default value
 default value is written as ``wasMoved(x)``. When not provided the compiler
 is using a combination of `=destroy` and `copyMem` instead. This is efficient
 hence users rarely need to implement their own `=sink` operator, it is enough to
-provide `=destroy` and `=`, compiler will take care about the rest.
+provide `=destroy` and `=copy`, compiler will take care about the rest.
 
 The prototype of this hook for a type ``T`` needs to be:
 
@@ -157,33 +157,33 @@ The general pattern in ``=sink`` looks like:
 How self-assignments are handled is explained later in this document.
 
 
-`=` (copy) hook
+`=copy` hook
 ---------------
 
-The ordinary assignment in Nim conceptually copies the values. The ``=`` hook
+The ordinary assignment in Nim conceptually copies the values. The ``=copy`` hook
 is called for assignments that couldn't be transformed into ``=sink``
 operations.
 
 The prototype of this hook for a type ``T`` needs to be:
 
 .. code-block:: nim
 
-  proc `=`(dest: var T; source: T)
+  proc `=copy`(dest: var T; source: T)
 
 
-The general pattern in ``=`` looks like:
+The general pattern in ``=copy`` looks like:
 
 .. code-block:: nim
 
-  proc `=`(dest: var T; source: T) =
+  proc `=copy`(dest: var T; source: T) =
     # protect against self-assignments:
     if dest.field != source.field:
       `=destroy`(dest)
       wasMoved(dest)
       dest.field = duplicateResource(source.field)
 
 
-The ``=`` proc can be marked with the ``{.error.}`` pragma. Then any assignment
+The ``=copy`` proc can be marked with the ``{.error.}`` pragma. Then any assignment
 that otherwise would lead to a copy is prevented at compile-time.
 
 
@@ -201,7 +201,7 @@ Swap
 ====
 
 The need to check for self-assignments and also the need to destroy previous
-objects inside ``=`` and ``=sink`` is a strong indicator to treat
+objects inside ``=copy`` and ``=sink`` is a strong indicator to treat
 ``system.swap`` as a builtin primitive of its own that simply swaps every
 field in the involved objects via ``copyMem`` or a comparable mechanism.
 In other words, ``swap(a, b)`` is **not** implemented
@@ -326,7 +326,7 @@ destroyed at the scope exit.
 
   x = y
   ------------------          (copy)
-  `=`(x, y)
+  `=copy`(x, y)
 
 
   f_sink(g())
@@ -336,7 +336,7 @@ destroyed at the scope exit.
 
   f_sink(notLastReadOf y)
   --------------------------     (copy-to-sink)
-  (let tmp; `=`(tmp, y);
+  (let tmp; `=copy`(tmp, y);
   f_sink(tmp))
 
 

tests/arc/tcaseobjcopy.nim

@@ -0,0 +1,246 @@
+discard """
+  valgrind: true
+  cmd: "nim c --gc:arc -d:useMalloc $file"
+  output: '''myobj destroyed
+myobj destroyed
+myobj destroyed
+A
+B
+begin
+end
+prevented
+(ok: true, value: "ok")
+myobj destroyed
+'''
+"""
+
+# bug #13102
+
+type
+  D = ref object
+  R = object
+    case o: bool
+    of false:
+      discard
+    of true:
+      field: D
+
+iterator things(): R =
+  when true:
+    var
+      unit = D()
+    while true:
+      yield R(o: true, field: unit)
+  else:
+    while true:
+      var
+        unit = D()
+      yield R(o: true, field: unit)
+
+proc main =
+  var i = 0
+  for item in things():
+    discard item.field
+    inc i
+    if i == 2: break
+
+main()
+
+# bug #13149
+
+type
+  TMyObj = object
+    p: pointer
+    len: int
+
+proc `=destroy`(o: var TMyObj) =
+  if o.p != nil:
+    dealloc o.p
+    o.p = nil
+    echo "myobj destroyed"
+
+proc `=copy`(dst: var TMyObj, src: TMyObj) =
+  `=destroy`(dst)
+  dst.p = alloc(src.len)
+  dst.len = src.len
+
+proc `=sink`(dst: var TMyObj, src: TMyObj) =
+  `=destroy`(dst)
+  dst.p = src.p
+  dst.len = src.len
+
+type
+  TObjKind = enum Z, A, B
+  TCaseObj = object
+    case kind: TObjKind
+    of Z: discard
+    of A:
+      x1: int # this int plays important role
+      x2: TMyObj
+    of B:
+      y: TMyObj
+
+proc testSinks: TCaseObj =
+  result = TCaseObj(kind: A, x1: 5000, x2: TMyObj(len: 5, p: alloc(5)))
+  result = TCaseObj(kind: B, y: TMyObj(len: 3, p: alloc(3)))
+
+proc use(x: TCaseObj) = discard
+
+proc testCopies(i: int) =
+  var a: array[2, TCaseObj]
+  a[i] = TCaseObj(kind: A, x1: 5000, x2: TMyObj(len: 5, p: alloc(5)))
+  a[i+1] = a[i] # copy, cannot move
+  use(a[i])
+
+let x1 = testSinks()
+testCopies(0)
+
+# bug #12957
+
+type
+  PegKind* = enum
+    pkCharChoice,
+    pkSequence
+  Peg* = object ## type that represents a PEG
+    case kind: PegKind
+    of pkCharChoice: charChoice: ref set[char]
+    else: discard
+    sons: seq[Peg]
+
+proc charSet*(s: set[char]): Peg =
+  ## constructs a PEG from a character set `s`
+  result = Peg(kind: pkCharChoice)
+  new(result.charChoice)
+  result.charChoice[] = s
+
+proc len(a: Peg): int {.inline.} = return a.sons.len
+proc myadd(d: var Peg, s: Peg) {.inline.} = add(d.sons, s)
+
+proc sequence*(a: openArray[Peg]): Peg =
+  result = Peg(kind: pkSequence, sons: @[])
+  when false:
+    #works too:
+    result.myadd(a[0])
+    result.myadd(a[1])
+  for x in items(a):
+    # works:
+    #result.sons.add(x)
+    # fails:
+    result.myadd x
+  if result.len == 1:
+    result = result.sons[0] # this must not move!
+
+when true:
+  # bug #12957
+
+  proc p =
+    echo "A"
+    let x = sequence([charSet({'a'..'z', 'A'..'Z', '_'}),
+              charSet({'a'..'z', 'A'..'Z', '0'..'9', '_'})])
+    echo "B"
+  p()
+
+  proc testSubObjAssignment =
+    echo "begin"
+    # There must be extactly one element in the array constructor!
+    let x = sequence([charSet({'a'..'z', 'A'..'Z', '_'})])
+    echo "end"
+  testSubObjAssignment()
+
+
+#------------------------------------------------
+
+type
+  MyObject = object
+    x1: string
+    case kind1: bool
+      of false: y1: string
+      of true:
+          y2: seq[string]
+          case kind2: bool
+              of true: z1: string
+              of false:
+                z2: seq[string]
+                flag: bool
+    x2: string
+
+proc test_myobject =
+  var x: MyObject
+  x.x1 = "x1"
+  x.x2 = "x2"
+  x.y1 = "ljhkjhkjh"
+  x.kind1 = true
+  x.y2 = @["1", "2"]
+  x.kind2 = true
+  x.z1 = "yes"
+  x.kind2 = false
+  x.z2 = @["1", "2"]
+  x.kind2 = true
+  x.z1 = "yes"
+  x.kind2 = true # should be no effect
+  doAssert(x.z1 == "yes")
+  x.kind2 = false
+  x.kind1 = x.kind2 # support self assignment with effect
+
+  try:
+    x.kind1 = x.flag # flag is not accesible
+  except FieldDefect:
+    echo "prevented"
+
+  doAssert(x.x1 == "x1")
+  doAssert(x.x2 == "x2")
+
+
+test_myobject()
+
+
+#------------------------------------------------
+# bug #14244
+
+type
+  RocksDBResult*[T] = object
+    case ok*: bool
+    of true:
+      value*: T
+    else:
+      error*: string
+
+proc init(): RocksDBResult[string] =
+  result.ok = true
+  result.value = "ok"
+
+echo init()
+
+
+#------------------------------------------------
+# bug #14312
+
+type MyObj = object
+  case kind: bool
+    of false: x0: int # would work with a type like seq[int]; value would be reset
+    of true: x1: string
+
+var a = MyObj(kind: false, x0: 1234)
+a.kind = true
+doAssert(a.x1 == "")
+
+block:
+  # bug #15532
+  type Kind = enum
+    k0, k1
+
+  type Foo = object
+    y: int
+    case kind: Kind
+    of k0: x0: int
+    of k1: x1: int
+
+  const j0 = Foo(y: 1, kind: k0, x0: 2)
+  const j1 = Foo(y: 1, kind: k1, x1: 2)
+
+  doAssert j0.y == 1
+  doAssert j0.kind == k0
+  doAssert j1.kind == k1
+
+  doAssert j1.x1 == 2
+  doAssert j0.x0 == 2