fix wrong cursor inference in the context of loops (#1388)

## Summary

* fix cursor inference bug where variables were erroneously inferred as
  cursors, leading to use-after-free issues
* improve cursor inference. `var`/`let` bindings are now inferred to be
  cursors more reliably

Fixes #1385

## Details

For `var`/`let` bindings defined in loops, the `aliveEnd` is now only
updated during the first analysis of the loop. This ensures that
variables defined outside the loop, but only assigned to within the
loop, always have longer alive times than variables defined within
loops, thus preventing outer variables borrowing from inner variables
(cursorfication cannot happen when a variable outlives its assignment
source).

This fix also renders the `isConditionallyReassigned` heuristic
obsolete, which was used to fix a more specific case of the same bug.
The heuristic disabled cursorfication for all variables of which
re-assignments are enclosed by a loop and `if`/`else`/`elif`/`of`,
which also applied to, e.g.:

```nim
while cond:
  if cond:
    var a = newString(...)
    var b = newString(...)
    var x: string
    x = a
    x = b # this assignment disabled `x` being turned into a cursor
    discard a
```

With the heuristic removed, in the above example, `x` is now turned
into a cursor, matching what would happen when there is no enclosing
loop or `if`.

compiler/sem/varpartitions.nim

@@ -81,7 +81,6 @@ type
     ownsData,
     preventCursor,
     isReassigned,
-    isConditionallyReassigned,
     viewDoesMutate,
     viewBorrowsFromConst
 
@@ -118,12 +117,13 @@ type
 
   Partitions* = object
     abstractTime: AbstractTime
+    loopStart: AbstractTime
     s: seq[VarIndex]
     graphs: seq[MutationInfo]
     goals: set[Goal]
     unanalysableMutation: bool
     inAsgnSource, inConstructor, inNoSideEffectSection: int
-    inConditional, inLoop: int
+    inLoop: int
     owner: PSym
     g: ModuleGraph
 
@@ -791,10 +791,6 @@ proc traverse(c: var Partitions; n: PNode) =
 
 proc markAsReassigned(c: var Partitions; vid: int) {.inline.} =
   c.s[vid].flags.incl isReassigned
-  if c.inConditional > 0 and c.inLoop > 0:
-    # bug #17033: live ranges with loops and conditionals are too
-    # complex for our current analysis, so we prevent the cursorfication.
-    c.s[vid].flags.incl isConditionallyReassigned
 
 proc computeLiveRanges(c: var Partitions; n: PNode) =
   # first pass: Compute live ranges for locals.
@@ -831,7 +827,11 @@ proc computeLiveRanges(c: var Partitions; n: PNode) =
     dec c.abstractTime
     if n.sym.kind in {skVar, skResult, skTemp, skLet, skForVar, skParam}:
       let id = variableId(c, n.sym)
-      if id >= 0:
+      # during the second iteration of loop analysis, only update the live
+      # ranges for variables that are not defined within the loop. The
+      # intention is to prevent outer variables from having the same (or
+      # shorter) alive ranges than inner variables
+      if id >= 0 and c.s[id].aliveStart < c.loopStart:
         c.s[id].aliveEnd = max(c.s[id].aliveEnd, c.abstractTime)
         if n.sym.kind == skResult:
           c.s[id].aliveStart = min(c.s[id].aliveStart, c.abstractTime)
@@ -864,19 +864,22 @@ proc computeLiveRanges(c: var Partitions; n: PNode) =
   of nkPragmaBlock:
     computeLiveRanges(c, n.lastSon)
   of nkWhileStmt, nkForStmt:
+    let start = c.abstractTime
     for child in n: computeLiveRanges(c, child)
     # analyse loops twice so that 'abstractTime' suffices to detect cases
     # like:
     #   while cond:
     #     mutate(graph)
     #     connect(graph, cursorVar)
+    if c.inLoop == 0:
+      # live ranges in nested loops are only computed once, during the first
+      # iteration of the outermost loop
+      c.loopStart = start
     inc c.inLoop
     for child in n: computeLiveRanges(c, child)
-    inc c.inLoop
-  of nkElifBranch, nkElifExpr, nkElse, nkOfBranch:
-    inc c.inConditional
-    for child in n: computeLiveRanges(c, child)
-    dec c.inConditional
+    dec c.inLoop
+    if c.inLoop == 0:
+      c.loopStart = MaxTime
   else:
     for child in n: computeLiveRanges(c, child)
 
@@ -889,6 +892,7 @@ proc computeGraphPartitions*(s: PSym; n: PNode; g: ModuleGraph; goals: set[Goal]
     if resultPos < s.ast.safeLen:
       registerResult(result, s.ast[resultPos])
 
+  result.loopStart = MaxTime
   computeLiveRanges(result, n)
   # restart the timer for the second pass:
   result.abstractTime = AbstractTime 0
@@ -949,7 +953,7 @@ proc computeCursors*(s: PSym; n: PNode; g: ModuleGraph) =
   var par = computeGraphPartitions(s, n, g, {cursorInference})
   for i in 0 ..< par.s.len:
     let v = addr(par.s[i])
-    if v.flags * {ownsData, preventCursor, isConditionallyReassigned} == {} and
+    if v.flags * {ownsData, preventCursor} == {} and
         v.sym.kind notin {skParam, skResult} and
         v.sym.flags * {sfThread, sfGlobal} == {} and hasDestructor(v.sym.typ):
       let rid = root(par, i)

tests/arc/topt_no_loop_cursor.nim

@@ -0,0 +1,58 @@
+discard """
+  description: '''
+    Ensure that outer variables don't borrow from locals within loops, when not
+    safe
+  '''
+  matrix: "--showir:mir_in:test --hints:off"
+  action: compile
+  nimoutFull: true
+  nimout: '''-- MIR: test
+scope:
+  def x: Object
+  scope:
+    while true:
+      scope:
+        scope:
+          def _3: bool = not(arg cond)
+          if _3:
+            scope:
+              goto [L2]
+        def y: Object = ()
+        def_cursor _5: Object = x
+        use(arg _5) -> [L3, L4, Resume]
+        x = sink y
+        goto [L3, L5]
+        finally (L3):
+          destroy y
+          continue {L4, L5}
+        L5:
+  L2:
+  goto [L4, L6]
+  finally (L4):
+    destroy x
+    continue {L6}
+  L6:
+
+-- end
+'''
+"""
+
+type Object = object
+
+# make Object a type that's eligible for cursor inference
+proc `=destroy`(x: var Object) =
+  discard
+
+proc use(x: Object) =
+  discard
+
+proc test(cond: bool) =
+  var x: Object
+  while cond:
+    var y = Object()
+    use x
+    # if `x` were a cursor, the above usage would observe a stale value,
+    # as the value assigned below went out of scope already
+    x = y
+
+test(false)

tests/arc/topt_refcursors.nim

@@ -23,7 +23,8 @@ scope:
         def_cursor _10: Node = it
         it = _10[].ri
   L2:
-  def_cursor jt: Node = root
+  def jt: Node
+  =copy(name jt, arg root)
   scope:
     while true:
       scope:
@@ -37,12 +38,25 @@ scope:
             scope:
               goto [L5]
         def_cursor _18: Node = jt
-        def_cursor ri: Node = _18[].ri
+        def ri: Node
+        =copy(name ri, arg _18[].ri)
         def_cursor _19: Node = jt
         def_cursor _20: string = _19[].s
-        echo(arg type(array[0..0, string]), arg _20) -> [Resume]
-        jt = ri
+        echo(arg type(array[0..0, string]), arg _20) -> [L6, L7, Resume]
+        =sink(name jt, arg ri)
+        wasMoved(name ri)
+        goto [L6, L8]
+        finally (L6):
+          =destroy(name ri)
+          continue {L7, L8}
+        L8:
   L5:
+  goto [L7, L9]
+  finally (L7):
+    =destroy(name jt)
+    continue {L9}
+  L9:
+
 -- end of expandArc ------------------------'''
 """
 
@@ -64,5 +78,3 @@ proc traverse(root: Node) =
     jt = ri
 
 traverse(nil)
-
-# XXX: This optimization is not sound