fix: move analyzer regression with variants (#1311)

## Summary

Separate the `switch` lowering and destructor injection into two proper
passes and apply them concurrently, fixing a move analyzer regression.

## Details

* move the `switch` lowering out of `injectDestructorCalls`
* pass a `Changeset` instead of a mutable `MirBody` to the two passes
  in `injectdestructors`
* destructor injection and `switch` lowering are applied in a single
  batch by `backends.process` 
* the `switch` lowering is still disabled for the VM backend
* update the doc comment of `injectDestructorCalls` to reflect reality

### Move Analyzer

Applying both passes concurrently - instead of the destructor-
injection/move-analysis after `switch` lowering - fixes a regression
from ae54312

The move analyzer now sees a discriminator assignment as:
```nim
x.discr = y
```
instead of:
```nim
if x.discr != y:
  =destroy(x) # variant destructor
x.discr = y
```

allowing it to perform automatic moves from non-variant fields again
(refer to `tmove_from_non_variant_field.nim`).

### Misc

* fix MIR pretty-printing for object/ref constructors -- the colon was
  missing between the field and value

compiler/backend/backends.nim

@@ -26,6 +26,7 @@ import
     injecthooks,
     mirbodies,
     mirbridge,
+    mirchangesets,
     mirconstr,
     mirenv,
     mirgen,
@@ -347,7 +348,18 @@ proc process(body: var MirBody, prc: PSym, graph: ModuleGraph,
   ## Applies all applicable MIR passes to the `body`. `prc` is the enclosing
   ## procedure.
   if shouldInjectDestructorCalls(prc):
-    injectDestructorCalls(graph, idgen, env, prc, body)
+    block:
+      var c = initChangeset(body)
+      injectDestructorCalls(body.code, graph, env, c)
+      # XXX: ``vmgen`` doesn't support the code resulting from the switch
+      #      lowering, so branch destructors are disabled for the VM target
+      #      at the moment
+      if graph.config.backend != backendNimVm:
+        lowerBranchSwitch(body.code, graph, idgen, env, c)
+      body.apply(c)
+
+    # hook injection needs to happen *after* move analysis and destroy
+    # injection
     injectHooks(body, graph, env, prc)
 
     if graph.config.arcToExpand.hasKey(prc.name.s):

compiler/mir/utils.nim

@@ -418,6 +418,7 @@ proc exprToStr(nodes: MirTree, i: var int, result: var string, c: RenderCtx) =
     tree "(":
       commaSeparated:
         fieldToStr(next(nodes, i).field, typ, result, c)
+        result.add ": "
         argToStr()
       result.add ")"
   of mnkCall:

compiler/sem/injectdestructors.nim

@@ -826,56 +826,35 @@ func shouldInjectDestructorCalls*(owner: PSym): bool =
      {sfInjectDestructors, sfGeneratedOp} * owner.flags == {sfInjectDestructors} and
      (owner.kind != skIterator or not isInlineIterator(owner.typ))
 
-proc injectDestructorCalls*(g: ModuleGraph, idgen: IdGenerator,
-                            env: var MirEnv, owner: PSym,
-                            body: var MirBody) =
-  ## The ``injectdestructors`` pass entry point. The pass is made up of
-  ## multiple sub-passes, hence the mutable `body` (as opposed
-  ## to returning a ``Changeset``).
-  ##
-  ## For now, semantic errors and other diagnostics related to lifetime-hook
-  ## usage are also reported here.
-
-  # apply the first batch of passes:
-  block:
-    var changes = initChangeset(body)
-    # the VM implements branch switching itself - performing the lowering for
-    # code meant to run in it would be harmful
-    # FIXME: discriminant assignment lowering also needs to be disabled for
-    #        when generating code running at compile-time (e.g. inside a
-    #        macro)
-    # XXX: the lowering is *always* necessary, as the destructors for
-    #      fields inside switched-away-from branches won't be called
-    #      otherwise
-    # TODO: make the branch-switch lowering a separate and standalone pass --
-    #       it's not directly related to the rest of the processing here
-    if g.config.backend != backendNimVm:
-      for i, n in body.code.pairs:
-        if n.kind == mnkSwitch:
-          changes.replaceMulti(body.code, i, buf):
-            lowerBranchSwitch(buf, body.code, g, idgen, env, i)
-
-    body.apply(changes)
-
-  # apply the second batch of passes:
+proc lowerBranchSwitch*(tree: MirTree, g: ModuleGraph, idgen: IdGenerator,
+                        env: var MirEnv, changes: var Changeset) =
+  ## Lowers ``mnkSwitch`` operations into normal assignments, with a branch
+  ## destructor injected if the respective record-case requires it (i.e.,
+  ## because it contains fields requiring destruction).
+  for i, n in tree.pairs:
+    if n.kind == mnkSwitch:
+      changes.replaceMulti(tree, i, buf):
+        lowerBranchSwitch(buf, tree, g, idgen, env, i)
+
+proc injectDestructorCalls*(tree: MirTree, g: ModuleGraph, env: var MirEnv,
+                            changes: var Changeset) =
+  ## Collapses sink assignments into either copy or move assignments, and
+  ## injects the destroy operations for all entities requiring destruction.
   block:
     var
-      changes = initChangeset(body)
-      actx = AnalyseCtx(graph: g, cfg: computeDfg(body.code))
+      actx = AnalyseCtx(graph: g, cfg: computeDfg(tree))
 
     let
-      entities = initEntityDict(body.code, actx.cfg, env)
-      moves = collapseSink(body.code, actx.cfg, entities, env.types)
+      entities = initEntityDict(tree, actx.cfg, env)
+      moves = collapseSink(tree, actx.cfg, entities, env.types)
 
-    let destructors = computeDestructors(body.code, actx.cfg, entities)
+    let destructors = computeDestructors(tree, actx.cfg, entities)
 
     rewriteAssignments(
-      body.code, actx,
+      tree, actx,
       AnalysisResults(moves: cursor(moves),
                       entities: cursor(entities),
                       destroy: cursor(destructors)),
       env.types, changes)
 
-    injectDestructors(body.code, g, destructors, env, changes)
-
-    body.apply(changes)
+    injectDestructors(tree, g, destructors, env, changes)

tests/optimization/tmove_from_non_variant_field.nim

@@ -0,0 +1,43 @@
+discard """
+  description: '''
+    Ensure that a discriminator assignment doesn't prevent automatic moves
+    from fields outside the record-case
+  '''
+  action: compile
+  matrix: "--expandArc:test --checks:off"
+  nimout: '''--expandArc: test
+scope:
+  def _3: WithHooks = ()
+  def x: Object = (a: consume _3, kind: consume true)
+  bind_mut _4: WithHooks = x.a
+  result := move _4
+  wasMoved(name _4)
+  bind_mut _5: bool = x.kind
+  def _6: bool = copy kind
+  def _7: bool = eqB(arg _5, arg _6)
+  def _8: bool = not(arg _7)
+  if _8:
+    =destroy(name x)
+  _5 = _6
+  =destroy(name x)
+
+-- end of expandArc ------------------------'''
+"""
+
+type
+  WithHooks = object
+  Object = object
+    a: WithHooks
+    case kind: bool
+    of true, false:
+      b: WithHooks
+
+proc `=destroy`(x: var WithHooks) =
+  discard
+
+proc test(kind: bool): WithHooks {.exportc.} =
+  var x = Object(a: WithHooks(), kind: true)
+  result = x.a # can be moved
+  # the discriminator assignment must not prevent `x.a` from being moved out
+  # of
+  x.kind = kind