From 2eac7941ffff22bd1c38ebb44023be13ca402f3f Mon Sep 17 00:00:00 2001
From: metagn <metagngn@gmail.com>
Date: Mon, 11 Nov 2024 12:48:28 +0300
Subject: [PATCH] stricter skip for conversions in array indices in transf
 (#24424)

fixes #17958

In `transf`, conversions in subscript expressions are skipped (with
`skipConv`'s rules). This is because array indexing can produce
conversions to the range type that is the array's index type, which
causes a `RangeDefect` rather than an `IndexDefect` (and also
`--rangeChecks` and `--indexChecks` are both considered). However this
causes problems when explicit conversions are used, between types of
different bitsizes, because those also get skipped.

To fix this, we only skip the conversion if:

* it's a hidden (implicit) conversion
* it's a range check conversion (produces `nkChckRange`)
* the subscript is on an array type and the result type of the
conversion has the same bounds as the array index type

And `skipConv` rules also still apply (int/float classification).

Another idea would be to prevent the implicit conversion to the array
index type from being generated. But there is no good way to do this:
matching to the base type instead prevents types like `uint32` from
implicitly converting (i.e. it can convert to `range[0..3]` but not
`int`), and analyzing whether this is an array bound check is easier in
`transf`, since `sigmatch` just produces a type conversion.

The rules for skipping the conversion could also receive some other
tweaks: We could add a rule that changing bitsizes also doesn't skip the
conversion, but this breaks the `uint32` case. We could simplify it to
only removing implicit skips to specifically fix #17958, but this is
wrong in general.

We could also add something like `nkChckIndex` that generates index
errors instead but this is weird when it doesn't have access to the
collection type and it might be overkill.

(cherry picked from commit 76c5f16ac5e06cc4cdd24c41067ffcf35bdf77dc)
---
 compiler/transf.nim                  | 15 ++++++++++++---
 tests/array/tindexconv.nim           |  4 ++++
 tests/array/tinvalidarrayaccess.nim  |  2 +-
 tests/array/tinvalidarrayaccess2.nim |  2 +-
 tests/js/tindexdefect.nim            |  5 +++--
 5 files changed, 21 insertions(+), 7 deletions(-)
 create mode 100644 tests/array/tindexconv.nim

diff --git a/compiler/transf.nim b/compiler/transf.nim
index 6e235fe430..dd4f3679d8 100644
--- a/compiler/transf.nim
+++ b/compiler/transf.nim
@@ -835,9 +835,18 @@ proc transformArrayAccess(c: PTransf, n: PNode): PNode =
   if n[0].kind == nkSym and n[0].sym.kind == skType:
     result = n
   else:
-    result = newTransNode(n)
-    for i in 0..<n.len:
-      result[i] = transform(c, skipConv(n[i]))
+    result = transformSons(c, n)
+    if n.len >= 2 and result[1].kind in {nkChckRange, nkChckRange64} and
+        n[1].kind in {nkHiddenStdConv, nkHiddenSubConv}:
+      # implicit conversion, was transformed into range check
+      # remove in favor of index check if conversion to array index type
+      # has to be done here because the array index type needs to be relaxed
+      # i.e. a uint32 index can implicitly convert to range[0..3] but not int
+      let arr = skipTypes(n[0].typ, abstractVarRange)
+      if arr.kind == tyArray and
+          firstOrd(c.graph.config, arr) == getOrdValue(result[1][1]) and
+          lastOrd(c.graph.config, arr) == getOrdValue(result[1][2]):
+        result[1] = result[1].skipConv
 
 proc getMergeOp(n: PNode): PSym =
   case n.kind
diff --git a/tests/array/tindexconv.nim b/tests/array/tindexconv.nim
new file mode 100644
index 0000000000..aa95ed4b67
--- /dev/null
+++ b/tests/array/tindexconv.nim
@@ -0,0 +1,4 @@
+block: # issue #17958
+  var mem: array[uint8, uint8]
+  let val = 0xffff'u16
+  discard mem[uint8 val]   # Error: unhandled exception: index 65535 not in 0 .. 255 [IndexDefect]
diff --git a/tests/array/tinvalidarrayaccess.nim b/tests/array/tinvalidarrayaccess.nim
index f8bce45ef3..5656deb781 100644
--- a/tests/array/tinvalidarrayaccess.nim
+++ b/tests/array/tinvalidarrayaccess.nim
@@ -1,5 +1,5 @@
 discard """
-  errormsg: "index 2 not in 0 .. 1"
+  errormsg: "conversion from int literal(2) to range 0..1(int) is invalid"
   line: 18
 """
 block:
diff --git a/tests/array/tinvalidarrayaccess2.nim b/tests/array/tinvalidarrayaccess2.nim
index 0a07038344..1d8d0cfff5 100644
--- a/tests/array/tinvalidarrayaccess2.nim
+++ b/tests/array/tinvalidarrayaccess2.nim
@@ -1,5 +1,5 @@
 discard """
-  errormsg: "index 3 not in 0 .. 1"
+  errormsg: "conversion from int literal(3) to range 0..1(int) is invalid"
   line: 9
 """
 
diff --git a/tests/js/tindexdefect.nim b/tests/js/tindexdefect.nim
index 37994ec2e7..eea3768838 100644
--- a/tests/js/tindexdefect.nim
+++ b/tests/js/tindexdefect.nim
@@ -5,5 +5,6 @@ discard """
 """
 
 var s = ['a']
-let z = s[10000] == 'a'
-echo z
\ No newline at end of file
+let i = 10000
+let z = s[i] == 'a'
+echo z