From a21c4db569067f4d9a5f8feee553af1af582f5d1 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Thu, 27 Jun 2024 07:54:56 +0200
Subject: [PATCH] fix incorrect removal of self-assignments (#1357)

## Summary

Fix a bug with the MIR-based alias analysis that caused assignments
like `arr[x][1] = arr[y][1]` to be considered self-assignments and
therefore removed.

## Details

Change `comparePaths` to not regress the 'overlap' state from `maybe`
back to `yes` when the compared array indices are the same, which led
to expressions of the same shape where the outermost array access uses
the same static index to being treated as referring to the same
location (even when they don't).

`injectdestructors` uses the `aliasanalysis` results to remove self-
assignments, causing the erroneous removal of assignments. All other
uses of `aliasanalysis` are for disabling optimizations, so
observable behaviour was not affected there.

---------

Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
---
 compiler/sem/aliasanalysis.nim          |  9 ++++++--
 tests/assign/tarray_expr_assignment.nim | 28 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 tests/assign/tarray_expr_assignment.nim

diff --git a/compiler/sem/aliasanalysis.nim b/compiler/sem/aliasanalysis.nim
index 8906d6bc5b4..efab582f858 100644
--- a/compiler/sem/aliasanalysis.nim
+++ b/compiler/sem/aliasanalysis.nim
@@ -205,9 +205,14 @@ proc compare*(body: MirTree, a, b: Path): CmpLocsResult =
         break
 
     of pikIndex:
-      overlaps = sameIndex(na, nb)
-      if overlaps == no:
+      case sameIndex(na, nb)
+      of no:
+        overlaps = no
         break
+      of yes:
+        discard "don't change back to 'yes'"
+      of maybe:
+        overlaps = maybe
 
     inc i
 
diff --git a/tests/assign/tarray_expr_assignment.nim b/tests/assign/tarray_expr_assignment.nim
new file mode 100644
index 00000000000..43a7b7dcfdc
--- /dev/null
+++ b/tests/assign/tarray_expr_assignment.nim
@@ -0,0 +1,28 @@
+discard """
+  description: '''
+    Regression test for an alias-analysis bug that caused expressions involving
+    array-subscript expressions to erroneously be eliminated in some cases
+  '''
+  targets: c js vm
+"""
+
+type
+  Object = object
+    val: int
+  Type = array[2, array[2, Object]]
+
+# important: the bug only affected code in procedures that work with destructor-
+# having types
+proc `=destroy`(x: var Object) =
+  discard
+
+proc test(x, y: int) {.noinline.} =
+  var a = [[Object(val: 1), Object(val: 2)], [Object(val: 3), Object(val: 4)]]
+  # whether the two expressions refer to the same location is unknown; the
+  # assignment must not be removed
+  a[x][1] = a[y][1]
+  # note: the common trailing [1] sub-expressions are important for this test
+
+  doAssert a[0][1].val == 4
+
+test(0, 1)