fix behaviour of closed-over locals in and/or expressions (#1278)

## Summary

Fix closed-over locals defined as part of `and`/`or` expressions not
having their default value when their defined-in sub-expression was
short-circuited.

## Details

Bindings part of `and`/`or` expressions were so far hoisted to the
start of their scope by the "unscoped def" handling in `cgirgen`, which
doesn't apply to lifted locals, since these don't have a `def` (they're
part of the environment object).

Performing the hoisting in `transf` makes sure the behaviour of closed-
over locals matches that of not closed-over ones, and it also removes
the reliance on `cgirgen`.

The hoisting works by scanning transformed `and`/`or` expressions for
bindings that are within the *same* scope as the `and`/`or` expression,
and moving them to the start of the `and`/`or` expression.

For example, `(let a = 0; x) or (let b = 0; y)` is transformed into
`(let a; let b; (a = 0; x) or (b = 0; y))`.

Lifted globals (`.global` and `.thread`) have to be accounted for by
the hoisting pass, since they weren't lifted at this point.

compiler/sem/transf.nim

@@ -1032,6 +1032,81 @@ proc flattenTree(root: PNode): PNode =
   else:
     result = root
 
+proc transformAndOr(c: PTransf, n: PNode): PNode =
+  ## Transforms both operands and hoists all locals within them that are in
+  ## the outermost scope to the start of the and/or expression:
+  ##
+  ##   (let x = 0; a) or (var y; y = 1; b)
+  ##   # ->
+  ##   (let x; var y; (x = 0; a) or (y = 1; b))
+  ##
+  ## This makes sure that the bindings' lifetime is delimited by the `and`/`or`
+  ## expression's enclosing scope, even after lowering the expression.
+  proc hoist(c: PTransf, n: sink PNode, target: PNode): PNode {.nimcall.} =
+    # traverse all statements/expressions within the *same* scope
+    case n.kind
+    of nkVarSection, nkLetSection:
+      result = newTreeI(nkStmtList, n.info)
+
+      for it in n.items:
+        case it.kind
+        of nkIdentDefs:
+          if (it[0].kind == nkSym and (sfGlobal notin it[0].sym.flags or
+              it[0].sym.owner.kind notin routineKinds)) or
+             it[0].kind == nkDotExpr:
+            # local or module-level global. The initializer expression must be
+            # processed first, since locations defined therein start their
+            # lifetime earlier. That is, for ``var x = (var y = 0; y)`` the
+            # ``var y`` must be hoisted first
+            let src = hoist(c, move it[2], target)
+            target.add newTreeI(n.kind, it.info,
+              newTreeI(nkIdentDefs, it.info, it[0], c.graph.emptyNode,
+                        c.graph.emptyNode))
+            if src.kind != nkEmpty:
+              result.add newTreeI(nkAsgn, it.info, it[0], src)
+          else:
+            # a global defined within a procedure, leave as is
+            result.add newTreeI(n.kind, n.info, it)
+        of nkVarTuple:
+          # lower into assignments first, then process the result
+          let x = lowerTupleUnpacking(c.graph, it, c.idgen, getCurrOwner(c))
+          result.add hoist(c, x, target)
+        else:
+          unreachable()
+
+      if result.len == 0:
+        # all definitions were hoisted
+        result = c.graph.emptyNode
+
+    of nkHiddenAddr, nkHiddenDeref, nkAddr, nkDerefExpr, nkObjDownConv,
+        nkObjUpConv, nkStringToCString, nkCStringToString, nkCheckedFieldExpr:
+      result = n
+      result[0] = hoist(c, move result[0], target)
+    of nkCast, nkConv, nkHiddenStdConv, nkHiddenSubConv, nkPragmaBlock:
+      result = n
+      result[1] = hoist(c, move result[1], target)
+    of nkDotExpr, nkBracketExpr, nkCallKinds, nkBracket, nkTupleConstr,
+        nkChckRangeF, nkChckRange64, nkChckRange, nkStmtListExpr, nkStmtList:
+      result = n
+      for i in 0..<result.len:
+        result[i] = hoist(c, move result[i], target)
+    else:
+      # other expressions or statements are either decalarative or open a
+      # new scope
+      result = n
+
+  if c.inlining > 0:
+    # hoisting already happened fro inlined and/or expressions
+    result = transformSons(c, n)
+  else:
+    var hoisted = newTreeIT(nkStmtListExpr, n.info, n.typ)
+    result = hoist(c, transformSons(c, n), hoisted)
+
+    if hoisted.len > 0:
+      # append the transformed expression to the statement list:
+      hoisted.add result
+      result = hoisted
+
 proc transformCall(c: PTransf, n: PNode): PNode =
   var n = flattenTree(n)
   let op = getMergeOp(n)
@@ -1066,6 +1141,8 @@ proc transformCall(c: PTransf, n: PNode): PNode =
     result = transform(c, n[1])
   elif magic == mExpandToAst:
     result = transformExpandToAst(c, n)
+  elif magic in {mAnd, mOr}:
+    result = transformAndOr(c, n)
   else:
     let s = transformSons(c, n)
     # bugfix: check after 'transformSons' if it's still a method call:

tests/lang_callable/closure/tclosure_issues.nim

@@ -121,4 +121,29 @@ block:
     doAssert typeof(inner) isnot "closure"
     inner()
 
-  test()
+  test()
+
+block closed_over_local_in_or_expression:
+  # regression test: closed-over locals defined in ``or`` expressions
+  # behaved differently compared to when not being closed over
+
+  proc test() =
+    var i = 0
+    var take = false
+    while i < 2: # run the body two times
+      if (let x = 1; take) or (let y = 2; not take):
+        proc inner() =
+          # close over `y`
+          discard y
+
+        inner()
+
+        if take:
+          doAssert y == 0
+        else:
+          doAssert y == 2
+
+      take = true # enable short-circuiting
+      inc i
+
+  test()