fix: incorrect code generation for parameter borrowing (#1460)

## Summary

Fix parameters below the pass-by-reference size threshold not being
passed by reference when the procedure returns a non-direct view,
resulting in access violations at run-time when trying to access the
returned view.

## Details

* consider immutable non-direct views (e.g., `object` types with `lent`
  fields) when deciding whether pass-by-reference is used for the first
  parameter of a procedure
* move the `classifyViewType` procedures from `typeallowed` to `types`,
  as they're wholly unrelated to the "type allowed" checks

Fixes #1457.

compiler/ast/types.nim

@@ -60,6 +60,9 @@ type
     bvcSingle   ## single-location view
     bvcSequence ## view of contiguous locations
 
+  ViewTypeKind* = enum
+    noView, immutableView, mutableView
+
 proc base*(t: PType): PType =
   result = t[0]
 
@@ -1558,6 +1561,74 @@ proc classifyBackendView*(t: PType): BackendViewKind =
      tyAnd, tyOr, tyNot, tyAnything, tyFromExpr:
     unreachable()
 
+proc combine(dest: var ViewTypeKind, b: ViewTypeKind) {.inline.} =
+  case dest
+  of noView, mutableView:
+    dest = b
+  of immutableView:
+    if b == mutableView: dest = b
+
+proc classifyViewTypeAux(marker: var IntSet, t: PType): ViewTypeKind
+
+proc classifyViewTypeNode(marker: var IntSet, n: PNode): ViewTypeKind =
+  case n.kind
+  of nkSym:
+    result = classifyViewTypeAux(marker, n.typ)
+  of nkOfBranch:
+    result = classifyViewTypeNode(marker, n.lastSon)
+  else:
+    result = noView
+    for child in n:
+      result.combine classifyViewTypeNode(marker, child)
+      if result == mutableView: break
+
+proc classifyViewTypeAux(marker: var IntSet, t: PType): ViewTypeKind =
+  if containsOrIncl(marker, t.id): return noView
+  case t.kind
+  of tyVar:
+    result = mutableView
+  of tyLent, tyOpenArray, tyVarargs:
+    result = immutableView
+  of tyGenericInst, tyDistinct, tyAlias, tyInferred, tySink,
+     tyUncheckedArray, tySequence, tyArray, tyRef, tyStatic:
+    result = classifyViewTypeAux(marker, lastSon(t))
+  of tyFromExpr:
+    if t.len > 0:
+      result = classifyViewTypeAux(marker, lastSon(t))
+    else:
+      result = noView
+  of tyTuple:
+    result = noView
+    for i in 0..<t.len:
+      result.combine classifyViewTypeAux(marker, t[i])
+      if result == mutableView: break
+  of tyObject:
+    result = noView
+    if t.n != nil:
+      result = classifyViewTypeNode(marker, t.n)
+    if t[0] != nil:
+      result.combine classifyViewTypeAux(marker, t[0])
+  else:
+    # it doesn't matter what these types contain, 'ptr openArray' is not a
+    # view type!
+    result = noView
+
+proc classifyViewType*(t: PType): ViewTypeKind =
+  var marker = initIntSet()
+  result = classifyViewTypeAux(marker, t)
+
+proc directViewType*(t: PType): ViewTypeKind =
+  # does classify 't' without looking recursively into 't'.
+  case t.kind
+  of tyVar:
+    result = mutableView
+  of tyLent, tyOpenArray:
+    result = immutableView
+  of abstractInst-{tyTypeDesc}:
+    result = directViewType(t.lastSon)
+  else:
+    result = noView
+
 proc isPassByRef*(conf: ConfigRef; s: PSym, retType: PType): bool =
   var pt = skipTypes(s.typ, typedescInst)
   assert skResult != s.kind
@@ -1581,8 +1652,9 @@ proc isPassByRef*(conf: ConfigRef; s: PSym, retType: PType): bool =
   else:
     result = false
 
-  # first parameter and return type is 'lent T'? --> use pass by pointer if
-  # not already a pointer-like type
-  if s.position == 0 and retType != nil and retType.kind == tyLent:
+  # first parameter and return type is immutable view? --> use pass by pointer
+  # if not already a pointer-like type
+  if s.position == 0 and retType != nil and
+     classifyViewType(retType) == immutableView:
     result = pt.kind notin {tyVar, tyOpenArray, tyVarargs, tyRef, tyPtr,
                             tyPointer}

compiler/mir/mirgen.nim

@@ -766,7 +766,7 @@ proc genArgs(c: var TCtx, n: PNode) =
         c.add empty(c, n[i])
     elif i == 1 and not fntyp[0].isEmptyType() and
          not isHandleLike(t) and
-         classifyBackendView(fntyp[0]) != bvcNone:
+         classifyViewType(fntyp[0]) == immutableView:
       # the procedure returns a view, but the first parameter is not something
       # that resembles a handle. We need to make sure that the first argument
       # (which the view could be created from), is passed by reference

compiler/sem/typeallowed.nim

@@ -217,78 +217,6 @@ proc typeAllowedOrError*(t: PType, kind: TSymKind, c: PContext,
                                     kind: kind,
                                     allowedFlags: flags)))
 
-type
-  ViewTypeKind* = enum
-    noView, immutableView, mutableView
-
-proc combine(dest: var ViewTypeKind, b: ViewTypeKind) {.inline.} =
-  case dest
-  of noView, mutableView:
-    dest = b
-  of immutableView:
-    if b == mutableView: dest = b
-
-proc classifyViewTypeAux(marker: var IntSet, t: PType): ViewTypeKind
-
-proc classifyViewTypeNode(marker: var IntSet, n: PNode): ViewTypeKind =
-  case n.kind
-  of nkSym:
-    result = classifyViewTypeAux(marker, n.typ)
-  of nkOfBranch:
-    result = classifyViewTypeNode(marker, n.lastSon)
-  else:
-    result = noView
-    for child in n:
-      result.combine classifyViewTypeNode(marker, child)
-      if result == mutableView: break
-
-proc classifyViewTypeAux(marker: var IntSet, t: PType): ViewTypeKind =
-  if containsOrIncl(marker, t.id): return noView
-  case t.kind
-  of tyVar:
-    result = mutableView
-  of tyLent, tyOpenArray, tyVarargs:
-    result = immutableView
-  of tyGenericInst, tyDistinct, tyAlias, tyInferred, tySink,
-     tyUncheckedArray, tySequence, tyArray, tyRef, tyStatic:
-    result = classifyViewTypeAux(marker, lastSon(t))
-  of tyFromExpr:
-    if t.len > 0:
-      result = classifyViewTypeAux(marker, lastSon(t))
-    else:
-      result = noView
-  of tyTuple:
-    result = noView
-    for i in 0..<t.len:
-      result.combine classifyViewTypeAux(marker, t[i])
-      if result == mutableView: break
-  of tyObject:
-    result = noView
-    if t.n != nil:
-      result = classifyViewTypeNode(marker, t.n)
-    if t[0] != nil:
-      result.combine classifyViewTypeAux(marker, t[0])
-  else:
-    # it doesn't matter what these types contain, 'ptr openArray' is not a
-    # view type!
-    result = noView
-
-proc classifyViewType*(t: PType): ViewTypeKind =
-  var marker = initIntSet()
-  result = classifyViewTypeAux(marker, t)
-
-proc directViewType*(t: PType): ViewTypeKind =
-  # does classify 't' without looking recursively into 't'.
-  case t.kind
-  of tyVar:
-    result = mutableView
-  of tyLent, tyOpenArray:
-    result = immutableView
-  of abstractInst-{tyTypeDesc}:
-    result = directViewType(t.lastSon)
-  else:
-    result = noView
-
 proc requiresInit*(t: PType): bool =
   (t.flags * {tfRequiresInit, tfNeedsFullInit, tfNotNil} != {}) or
   classifyViewType(t) != noView

compiler/sem/varpartitions.nim

@@ -39,9 +39,6 @@ import
     options,
     msgs,
   ],
-  compiler/sem/[
-    typeallowed,
-  ],
   compiler/modules/[
     modulegraphs,
   ]

tests/lang_experimental/views/timmutable_borrow_from_first_parameter.nim

@@ -0,0 +1,27 @@
+discard """
+  description: '''
+    Ensure a view borrowing from the first parameter can be safely returned
+    from a procedure.
+  '''
+  targets: c js vm
+  knownIssue.js vm: "The first parameter isn't passed by reference"
+"""
+
+block direct_lent_view_primitive:
+  # test borrowing from primitive-type parameter with a direct view
+  proc test(x: int): lent int =
+    x
+
+  var x = 0
+  doAssert addr(test(x)) == addr(x)
+
+block object_lent_view_primitive:
+  # test borrowing from primitive-type parameter with an indirect view
+  type Object = object
+    x: lent int
+
+  proc test(x: int): Object =
+    Object(x: x)
+
+  var x = 0
+  doAssert addr(test(x).x) == addr(x)