Reboot of nim-lang#16195 (nim-lang#16746)

* fix nim-lang#16185

* fix test

* fix comment

* fix comment

* better approach

* Add more tests and move sameLocation to injectdestructors

* Better and more strict sameLocation

* Small cleanup and preliminary spec clarification

* Fix

* Fix doc

* Expand test

Co-authored-by: Andrey R (cooldome) <[email protected]>

compiler/injectdestructors.nim

@@ -968,68 +968,95 @@ proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode): PNode =
     else:
       internalError(c.graph.config, n.info, "cannot inject destructors to node kind: " & $n.kind)
 
+proc sameLocation*(a, b: PNode): bool =
+  proc sameConstant(a, b: PNode): bool =
+    a.kind in nkLiterals and exprStructuralEquivalent(a, b)
+
+  const nkEndPoint = {nkSym, nkDotExpr, nkCheckedFieldExpr, nkBracketExpr}
+  if a.kind in nkEndPoint and b.kind in nkEndPoint:
+    if a.kind == b.kind:
+      case a.kind
+      of nkSym: a.sym == b.sym
+      of nkDotExpr, nkCheckedFieldExpr: sameLocation(a[0], b[0]) and sameLocation(a[1], b[1])
+      of nkBracketExpr: sameLocation(a[0], b[0]) and sameConstant(a[1], b[1])
+      else: false
+    else: false
+  else:
+    case a.kind
+    of nkSym, nkDotExpr, nkCheckedFieldExpr, nkBracketExpr:
+      # Reached an endpoint, flip to recurse the other side.
+      sameLocation(b, a)
+    of nkAddr, nkHiddenAddr, nkDerefExpr, nkHiddenDeref:
+      # We don't need to check addr/deref levels or differentiate between the two,
+      # since pointers don't have hooks :) (e.g: var p: ptr pointer; p[] = addr p)
+      sameLocation(a[0], b)
+    of nkObjDownConv, nkObjUpConv: sameLocation(a[0], b)
+    of nkHiddenStdConv, nkHiddenSubConv: sameLocation(a[1], b)
+    else: false
+
 proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope, isDecl = false): PNode =
-  case ri.kind
-  of nkCallKinds:
-    result = c.genSink(dest, p(ri, c, s, consumed), isDecl)
-  of nkBracketExpr:
-    if isUnpackedTuple(ri[0]):
-      # unpacking of tuple: take over the elements
+  if sameLocation(dest, ri):
+    # rule (self-assignment-removal):
+    result = newNodeI(nkEmpty, dest.info)
+  else:
+    case ri.kind
+    of nkCallKinds:
       result = c.genSink(dest, p(ri, c, s, consumed), isDecl)
-    elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and
-        not aliases(dest, ri):
-      # Rule 3: `=sink`(x, z); wasMoved(z)
-      var snk = c.genSink(dest, ri, isDecl)
-      result = newTree(nkStmtList, snk, c.genWasMoved(ri))
-    else:
-      result = c.genCopy(dest, ri)
-      result.add p(ri, c, s, consumed)
-      c.finishCopy(result, dest, isFromSink = false)
-  of nkBracket:
-    # array constructor
-    if ri.len > 0 and isDangerousSeq(ri.typ):
-      result = c.genCopy(dest, ri)
-      result.add p(ri, c, s, consumed)
-      c.finishCopy(result, dest, isFromSink = false)
-    else:
+    of nkBracketExpr:
+      if isUnpackedTuple(ri[0]):
+        # unpacking of tuple: take over the elements
+        result = c.genSink(dest, p(ri, c, s, consumed), isDecl)
+      elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and
+          not aliases(dest, ri):
+        # Rule 3: `=sink`(x, z); wasMoved(z)
+        var snk = c.genSink(dest, ri, isDecl)
+        result = newTree(nkStmtList, snk, c.genWasMoved(ri))
+      else:
+        result = c.genCopy(dest, ri)
+        result.add p(ri, c, s, consumed)
+        c.finishCopy(result, dest, isFromSink = false)
+    of nkBracket:
+      # array constructor
+      if ri.len > 0 and isDangerousSeq(ri.typ):
+        result = c.genCopy(dest, ri)
+        result.add p(ri, c, s, consumed)
+        c.finishCopy(result, dest, isFromSink = false)
+      else:
+        result = c.genSink(dest, p(ri, c, s, consumed), isDecl)
+    of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit:
       result = c.genSink(dest, p(ri, c, s, consumed), isDecl)
-  of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit:
-    result = c.genSink(dest, p(ri, c, s, consumed), isDecl)
-  of nkSym:
-    if dest.kind == nkSym and dest.sym == ri.sym:
-      # rule (self-assignment-removal):
-      result = newNodeI(nkEmpty, dest.info)
-    elif isSinkParam(ri.sym) and isLastRead(ri, c):
-      # Rule 3: `=sink`(x, z); wasMoved(z)
-      let snk = c.genSink(dest, ri, isDecl)
-      result = newTree(nkStmtList, snk, c.genWasMoved(ri))
-    elif ri.sym.kind != skParam and ri.sym.owner == c.owner and
-        isLastRead(ri, c) and canBeMoved(c, dest.typ) and not isCursor(ri, c):
-      # Rule 3: `=sink`(x, z); wasMoved(z)
-      let snk = c.genSink(dest, ri, isDecl)
-      result = newTree(nkStmtList, snk, c.genWasMoved(ri))
-    else:
-      result = c.genCopy(dest, ri)
-      result.add p(ri, c, s, consumed)
-      c.finishCopy(result, dest, isFromSink = false)
-  of nkHiddenSubConv, nkHiddenStdConv, nkConv, nkObjDownConv, nkObjUpConv:
-    result = c.genSink(dest, p(ri, c, s, sinkArg), isDecl)
-  of nkStmtListExpr, nkBlockExpr, nkIfExpr, nkCaseStmt, nkTryStmt:
-    template process(child, s): untyped = moveOrCopy(dest, child, c, s, isDecl)
-    # We know the result will be a stmt so we use that fact to optimize
-    handleNestedTempl(ri, process, willProduceStmt = true)
-  of nkRaiseStmt:
-    result = pRaiseStmt(ri, c, s)
-  else:
-    if isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and
-        canBeMoved(c, dest.typ):
-      # Rule 3: `=sink`(x, z); wasMoved(z)
-      let snk = c.genSink(dest, ri, isDecl)
-      result = newTree(nkStmtList, snk, c.genWasMoved(ri))
+    of nkSym:
+      if isSinkParam(ri.sym) and isLastRead(ri, c):
+        # Rule 3: `=sink`(x, z); wasMoved(z)
+        let snk = c.genSink(dest, ri, isDecl)
+        result = newTree(nkStmtList, snk, c.genWasMoved(ri))
+      elif ri.sym.kind != skParam and ri.sym.owner == c.owner and
+          isLastRead(ri, c) and canBeMoved(c, dest.typ) and not isCursor(ri, c):
+        # Rule 3: `=sink`(x, z); wasMoved(z)
+        let snk = c.genSink(dest, ri, isDecl)
+        result = newTree(nkStmtList, snk, c.genWasMoved(ri))
+      else:
+        result = c.genCopy(dest, ri)
+        result.add p(ri, c, s, consumed)
+        c.finishCopy(result, dest, isFromSink = false)
+    of nkHiddenSubConv, nkHiddenStdConv, nkConv, nkObjDownConv, nkObjUpConv:
+      result = c.genSink(dest, p(ri, c, s, sinkArg), isDecl)
+    of nkStmtListExpr, nkBlockExpr, nkIfExpr, nkCaseStmt, nkTryStmt:
+      template process(child, s): untyped = moveOrCopy(dest, child, c, s, isDecl)
+      # We know the result will be a stmt so we use that fact to optimize
+      handleNestedTempl(ri, process, willProduceStmt = true)
+    of nkRaiseStmt:
+      result = pRaiseStmt(ri, c, s)
     else:
-      result = c.genCopy(dest, ri)
-      result.add p(ri, c, s, consumed)
-      c.finishCopy(result, dest, isFromSink = false)
+      if isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and
+          canBeMoved(c, dest.typ):
+        # Rule 3: `=sink`(x, z); wasMoved(z)
+        let snk = c.genSink(dest, ri, isDecl)
+        result = newTree(nkStmtList, snk, c.genWasMoved(ri))
+      else:
+        result = c.genCopy(dest, ri)
+        result.add p(ri, c, s, consumed)
+        c.finishCopy(result, dest, isFromSink = false)
 
 proc computeUninit(c: var Con) =
   if not c.uninitComputed:

doc/destructors.rst

@@ -376,8 +376,14 @@ it's subtle.
 
 The simple case of ``x = x`` cannot be turned
 into ``=sink(x, x); wasMoved(x)`` because that would lose ``x``'s value.
-The solution is that simple self-assignments are simply transformed into
-an empty statement that does nothing.
+The solution is that simple self-assignments that consist of
+
+- Symbols: ``x = x``
+- Field access: ``x.f = x.f``
+- Array, sequence or string access with indices known at compile-time: ``x[0] = x[0]``
+
+are transformed into an empty statement that does nothing.
+The compiler is free to optimize further cases.
 
 The complex case looks like a variant of ``x = f(x)``, we consider
 ``x = select(rand() < 0.5, x, y)`` here:

tests/arc/t14383.nim

@@ -168,3 +168,50 @@ proc testRefs() =
 testRefs()
 
 doAssert(dDestroyed == 2)
+
+
+#------------------------------------------------------------------------------
+# Issue #16185, complex self-assingment elimination
+#------------------------------------------------------------------------------
+
+type
+  CpuStorage*[T] = ref CpuStorageObj[T]
+  CpuStorageObj[T] = object
+    size*: int
+    raw_buffer*: ptr UncheckedArray[T]
+  Tensor[T] = object
+    buf*: CpuStorage[T]
+  TestObject = object
+    x: Tensor[float]
+
+proc `=destroy`[T](s: var CpuStorageObj[T]) =
+  if s.raw_buffer != nil:
+    s.raw_buffer.deallocShared()
+    s.size = 0
+    s.raw_buffer = nil
+
+proc `=`[T](a: var CpuStorageObj[T]; b: CpuStorageObj[T]) {.error.}
+
+proc allocCpuStorage[T](s: var CpuStorage[T], size: int) =
+  new(s)
+  s.raw_buffer = cast[ptr UncheckedArray[T]](allocShared0(sizeof(T) * size))
+  s.size = size
+
+proc newTensor[T](size: int): Tensor[T] =
+  allocCpuStorage(result.buf, size)
+
+proc `[]`[T](t: Tensor[T], idx: int): T = t.buf.raw_buffer[idx]
+proc `[]=`[T](t: Tensor[T], idx: int, val: T) = t.buf.raw_buffer[idx] = val
+
+proc toTensor[T](s: seq[T]): Tensor[T] =
+  result = newTensor[T](s.len)
+  for i, x in s:
+    result[i] = x
+
+proc main2() =
+  var t: TestObject
+  t.x = toTensor(@[1.0, 2, 3, 4])
+  t.x = t.x  
+  doAssert(t.x.buf != nil) # self-assignment above should be eliminated
+
+main2()

tests/arc/tmovebug.nim

@@ -73,6 +73,25 @@ bye
 ()
 ()
 ()
+1
+destroy
+1
+destroy
+1
+destroy
+copy (self-assign)
+1
+destroy
+1
+destroy
+1
+destroy
+destroy
+copy
+@[(f: 2), (f: 2), (f: 3)]
+destroy
+destroy
+destroy
 '''
 """
 
@@ -556,4 +575,96 @@ let w2 = newWrapper2(1)
 echo $w2[]
 
 let w3 = newWrapper2(-1)
-echo $w3[]
+echo $w3[]
+
+
+#---------------------------------------------------------------------
+#self-assignments
+
+# Self-assignments that are not statically determinable will get
+# turned into `=copy` calls as caseBracketExprCopy demonstrates.
+# (`=copy` handles self-assignments at runtime)
+
+type
+  OO = object
+    f: int
+  W = object
+    o: OO
+
+proc `=destroy`(x: var OO) =
+  if x.f != 0:
+    echo "destroy"
+    x.f = 0
+
+proc `=sink`(x: var OO, y: OO) =
+  `=destroy`(x)
+  echo "sink"
+  x.f = y.f
+
+proc `=copy`(x: var OO, y: OO) =
+  if x.f != y.f:
+    `=destroy`(x)
+    echo "copy"
+    x.f = y.f
+  else:
+    echo "copy (self-assign)"
+
+proc caseSym =
+  var o = OO(f: 1)
+  o = o # NOOP
+  echo o.f # "1"
+  # "destroy"
+
+caseSym()
+
+proc caseDotExpr =
+  var w = W(o: OO(f: 1))
+  w.o = w.o # NOOP
+  echo w.o.f # "1"
+  # "destroy"
+
+caseDotExpr()
+
+proc caseBracketExpr =
+  var w = [0: OO(f: 1)]
+  w[0] = w[0] # NOOP
+  echo w[0].f # "1"
+  # "destroy"
+
+caseBracketExpr()
+
+proc caseBracketExprCopy =
+  var w = [0: OO(f: 1)]
+  let i = 0
+  w[i] = w[0] # "copy (self-assign)"
+  echo w[0].f # "1"
+  # "destroy"
+
+caseBracketExprCopy()
+
+proc caseDotExprAddr =
+  var w = W(o: OO(f: 1))
+  w.o = addr(w.o)[] # NOOP
+  echo w.o.f # "1"
+  # "destroy"
+
+caseDotExprAddr()
+
+proc caseBracketExprAddr =
+  var w = [0: OO(f: 1)]
+  addr(w[0])[] = addr(addr(w[0])[])[] # NOOP
+  echo w[0].f # "1"
+  # "destroy"
+
+caseBracketExprAddr()
+
+proc caseNotAConstant =
+  var i = 0
+  proc rand: int =
+    result = i
+    inc i
+  var s = @[OO(f: 1), OO(f: 2), OO(f: 3)]
+  s[rand()] = s[rand()] # "destroy" "copy"
+  echo s # @[(f: 2), (f: 2), (f: 3)]
+
+caseNotAConstant()