From 6d0853beb65ddb7f94f3b1ef376ba7a3490c2f5a Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 25 Apr 2023 22:37:16 +0100
Subject: [PATCH] sigmatch: simpler procedural type matching

Summary
=======

* simplify the logic for handling generic routines using `auto` as the
  return types
* reject un-instantiated routines earlier when used  in branch and array
  constructor expressions, resulting in clearer error messages
  (`has no concrete type` instead of `cannot instantiate`)
* support lambda expressions coming from template expansions

Details
=======

Introduced by https://github.com/nim-lang/Nim/pull/3234, when both the
formal and actual type are unresolved meta types, the relation is
considered to be `isBothMetaConvertible`. The intention seems to have
been supporting passing `auto`-returning routines or lambda-expressions
to generic procedure-type parameters, but the logic introduced by the
aforementioned PR is not necessary for achieving that.

`auto` return types now use dedicated handling in `procTypeRel`, both
the formal and actual type being unresolved generics results in a
mismatch again, and `typeRel` is no longer applied repeatedly for
`isBothMetaConvertible` matches.

There was also the issue of `generateTypeInstance` being misused, which
could result in spurious `cannot instantiate` or internal compiler
errors. Since meta types are possibly involved, the correct routine to
use is `prepareMetatypeForSigmatch`. Testing that the produced
type instance is not a meta type was also done incorrectly (`isMetaType`
was used instead of `containsGenericType`).

Finally, `nkStmtListExpr` nodes are now considered when instantiating a
generic routine as part of parameter matching, which allows for complex
expressions yielding un-instantiated generic routines (e.g.:
`call((discard x; genericRoutine))`).
---
 compiler/sem/sem.nim                          |  17 ++
 compiler/sem/semexprs.nim                     |   1 +
 compiler/sem/semstmts.nim                     |   1 +
 compiler/sem/semtypinst.nim                   |   4 +
 compiler/sem/sigmatch.nim                     | 165 +++++++++++-------
 .../tgeneric_intantiation_outside_call.nim    |   2 +-
 .../tgeneric_procedure_cannot_assigned.nim    |   2 +-
 .../overload/tproc_type_inference.nim         |  95 ++++++++++
 8 files changed, 223 insertions(+), 64 deletions(-)
 create mode 100644 tests/lang_callable/overload/tproc_type_inference.nim

diff --git a/compiler/sem/sem.nim b/compiler/sem/sem.nim
index 765b37f6b14..cc0af1410d6 100644
--- a/compiler/sem/sem.nim
+++ b/compiler/sem/sem.nim
@@ -243,6 +243,23 @@ proc fitNodeConsiderViewType(c: PContext, formal: PType, arg: PNode; info: TLine
   else:
    result = a
 
+proc genericProcCheck(c: PContext, n: PNode): PNode =
+  ## Checks that the analysed expression `n` is of generic procedural type.
+  ## Returns either `n` or an error.
+  if n.typ != nil and n.typ.kind == tyError:
+    return n
+
+  # skip all statement list wrappers:
+  var it {.cursor.} = n
+  while it.kind == nkStmtListExpr:
+    it = it.lastSon
+
+  if (it.kind == nkSym and it.sym.isGenericRoutineStrict) or
+     it.isGenericRoutine:
+    c.config.newError(n, PAstDiag(kind: adSemProcHasNoConcreteType))
+  else:
+    n
+
 proc inferWithMetatype(c: PContext, formal: PType,
                        arg: PNode, coerceDistincts = false): PNode
 
diff --git a/compiler/sem/semexprs.nim b/compiler/sem/semexprs.nim
index 80d7159af16..c136cdc0dfb 100644
--- a/compiler/sem/semexprs.nim
+++ b/compiler/sem/semexprs.nim
@@ -782,6 +782,7 @@ proc semArrayConstr(c: PContext, n: PNode, flags: TExprFlags): PNode =
 
       if e.kind != nkError:
         e = semExprWithType(c, e, {})
+        e = genericProcCheck(c, e)
 
       if typ.isNil:
         # must be the first item; initialize the common type:
diff --git a/compiler/sem/semstmts.nim b/compiler/sem/semstmts.nim
index 40a7a948803..873fe9cd050 100644
--- a/compiler/sem/semstmts.nim
+++ b/compiler/sem/semstmts.nim
@@ -80,6 +80,7 @@ proc semProc(c: PContext, n: PNode): PNode
 
 proc semExprBranch(c: PContext, n: PNode; flags: TExprFlags = {}): PNode =
   result = semExpr(c, n, flags)
+  result = genericProcCheck(c, result)
   if result.typ != nil:
     # XXX tyGenericInst here?
     if result.typ.kind in {tyVar, tyLent}: result = newDeref(result)
diff --git a/compiler/sem/semtypinst.nim b/compiler/sem/semtypinst.nim
index 96c9a3df7c4..ca92665d473 100644
--- a/compiler/sem/semtypinst.nim
+++ b/compiler/sem/semtypinst.nim
@@ -793,6 +793,10 @@ proc recomputeFieldPositions*(t: PType; obj: PNode; currPosition: var int) =
 
 proc generateTypeInstance*(p: PContext, pt: TIdTable, info: TLineInfo,
                            t: PType): PType =
+  ## Produces the instantiated type for the generic type `t`, using the
+  ## bindings provided by `pt`. All type variables used by `t` must have
+  ## *concrete* type bounds to them -- both meta types and missing bindings
+  ## are disallowed and will result in an instantiation failure.
   # Given `t` like Foo[T]
   # pt: Table with type mappings: T -> int
   # Desired result: Foo[int]
diff --git a/compiler/sem/sigmatch.nim b/compiler/sem/sigmatch.nim
index 6b9b2190ed5..679b9a02fae 100644
--- a/compiler/sem/sigmatch.nim
+++ b/compiler/sem/sigmatch.nim
@@ -41,6 +41,7 @@ import
   ],
   compiler/utils/[
     debugutils,
+    idioms
   ]
 
 # xxx: reports are a code smell meaning data types are misplaced, for example
@@ -509,20 +510,20 @@ proc procParamTypeRel(c: var TCandidate, f, a: PType): TTypeRelation =
   ## For example we have:
   ##
   ## .. code-block:: nim
-  ##   proc myMap[T,S](sIn: seq[T], f: proc(x: T): S): seq[S] = ...
-  ##   proc innerProc[Q,W](q: Q): W = ...
+  ##   proc myMap[T,U,S](sIn: seq[T], f: proc(x: T, y: U): S): seq[S] = ...
+  ##   proc innerProc[Q](q: Q, b: Q): auto = ...
   ##
   ## And we want to match: myMap(@[1,2,3], innerProc)
   ## This proc (procParamTypeRel) will do the following steps in
-  ## three different calls:
-  ## - matches f=T to a=Q. Since f is metatype, we resolve it
-  ##    to int (which is already known at this point). So in this case
-  ##    Q=int mapping will be saved to c.bindings.
-  ## - matches f=S to a=W. Both of these metatypes are unknown, so we
-  ##    return with isBothMetaConvertible to ask for rerun.
-  ## - matches f=S to a=W. At this point the return type of innerProc
-  ##    is known (we get it from c.bindings). We can use that value
-  ##    to match with f, and save back to c.bindings.
+  ## two different calls:
+  ## - matches `f`=T to `a`=Q. `f` is a resolved metatype ('int' is bound to
+  ##   it already), so `a` can be inferred; a binding for Q=int is saved
+  ## - matces `f`=U to `a`=Q. `f` is an unresolved metatype, but since Q was
+  ##   already inferred as 'int', U can be inferred from it; a binding for
+  ##   U=int is saved
+  ##
+  ## The 'auto' return type doesn't reach here, but is instead handled by
+  ## ``procTypeRel``.
   var
     f = f
     a = a
@@ -531,6 +532,7 @@ proc procParamTypeRel(c: var TCandidate, f, a: PType): TTypeRelation =
     let aResolved = PType(idTableGet(c.bindings, a))
     if aResolved != nil:
       a = aResolved
+
   if a.isMetaType:
     if f.isMetaType:
       # We are matching a generic proc (as proc param)
@@ -539,12 +541,14 @@ proc procParamTypeRel(c: var TCandidate, f, a: PType): TTypeRelation =
       # type is already fully-determined, so we are
       # going to try resolve it
       if c.call != nil:
-        f = generateTypeInstance(c.c, c.bindings, c.call.info, f)
+        f = prepareMetatypeForSigmatch(c.c, c.bindings, c.call.info, f)
       else:
         f = nil
-      if f.isNil() or f.isMetaType:
-        # no luck resolving the type, so the inference fails
-        return isBothMetaConvertible
+      if f.isNil() or containsGenericType(f):
+        # no luck resolving the type, so the inference fails. Note that using
+        # ``isMetaType`` wont work, as it doesn't check for nested
+        # unresolved type variables
+        return isNone
     # Note that this typeRel call will save a's resolved type into c.bindings
     let reverseRel = typeRel(c, a, f)
     if reverseRel >= isGeneric:
@@ -577,12 +581,16 @@ proc procTypeRel(c: var TCandidate, f, a: PType): TTypeRelation =
     for i in 1..<f.len:
       checkParam(f[i], a[i])
 
-    if f[0] != nil:
-      if a[0] != nil:
-        checkParam(f[0], a[0])
+    if f[0] != nil and a[0] != nil:
+      # both have return types
+      if a[0].kind == tyUntyped:
+        # special handling for the return type: if `a` is 'auto' we first
+        # instantiate the procedure passed as the argument
+        result = isBothMetaConvertible
       else:
-        return isNone
-    elif a[0] != nil:
+        checkParam(f[0], a[0])
+    elif a[0] != f[0]:
+      # one has a void return type while the other doesn't
       return isNone
 
     result = getProcConvMismatch(c.c.config, f, a, result)[1]
@@ -2078,6 +2086,59 @@ template matchesVoidProc(t: PType): bool =
   (t.kind == tyProc and t.len == 1 and t[0].isNil()) or
     (t.kind == tyBuiltInTypeClass and t[0].kind == tyProc)
 
+proc instantiateRoutineExpr(c: PContext, bindings: TIdTable, n: PNode): PNode =
+  ## Instantiates the generic routine that the expression `n` names. Returns
+  ## the updated expression, or an error.
+  let orig = n
+  var
+    n = n
+    depth = 0
+
+  # the symbol or lambda expression might be coming from a statement list
+  # expression, so we have to unwrap it first
+  while n.kind == nkStmtListExpr:
+    n = n.lastSon
+    inc depth
+
+  # instantiate the symbol
+  case n.kind
+  of nkProcDef, nkFuncDef, nkIteratorDef, nkLambdaKinds:
+    result = c.semInferredLambda(c, bindings, n)
+  of nkSym:
+    let inferred = c.semGenerateInstance(c, n.sym, bindings, n.info)
+    result =
+      if inferred.isError:
+        inferred.ast
+      else:
+        newSymNode(inferred, n.info)
+  else:
+    # only lambda and symbol expressions are able to provide uninstantiated
+    # generic routines
+    unreachable(n.kind)
+
+  if orig.kind == nkStmtListExpr:
+    # make a copy of the tree, update the types, and fill in the instantiated
+    # lambda/symbol expression
+    let updated = copyTreeWithoutNode(orig, n)
+
+    var it {.cursor.} = updated
+    # traverse all nested statement list expressions and update their type:
+    for _ in 0..<depth-1:
+      it.typ = result.typ
+      it = it.lastSon
+
+    # set the type for last statement list and add the instantiated
+    # expression:
+    it.typ = result.typ
+    it.add result
+
+    if result.isError:
+      result = c.config.wrapError(updated)
+    else:
+      result = updated
+  else:
+    discard "result is already set"
+
 proc paramTypesMatchAux(m: var TCandidate, f, a: PType,
                         argSemantized: PNode): PNode =
   if argSemantized.isError:
@@ -2154,42 +2215,29 @@ proc paramTypesMatchAux(m: var TCandidate, f, a: PType,
         argSemantized
     return
 
-  var
-    bothMetaCounter = 0
-      ## bothMetaCounter is for safety to avoid any infinite loop,
-      ## we don't have any example when it is needed.
-    lastBindingsLength = -1
-      ## lastBindingsLenth use to track whether m.bindings remains the same,
-      ## because in that case there is no point in continuing.
-  
-  # If r == isBothMetaConvertible then we rerun typeRel.
-  while r == isBothMetaConvertible and
-      lastBindingsLength != m.bindings.counter and
-      bothMetaCounter < 100:                        # ensure termination
-
-    lastBindingsLength = m.bindings.counter
-    inc(bothMetaCounter)
-
-    case arg.kind
-    of nkProcDef, nkFuncDef, nkIteratorDef, nkLambdaKinds:
-      result = c.semInferredLambda(c, m.bindings, arg)
-    of nkSym:
-      let inferred = c.semGenerateInstance(c, arg.sym, m.bindings, arg.info)
-      result =
-        if inferred.isError:
-          inferred.ast
-        else:
-          newSymNode(inferred, arg.info)
-    else:
-      result = nil
-      return
-
+  if r == isBothMetaConvertible:
+    result = instantiateRoutineExpr(c, m.bindings, arg)
     if result.isError:
       return
 
     inc(m.convMatches)
     arg = result
+    # now that we know the result type, re-run the match. We cannot simply
+    # match the result types unfortunately, as the formal type might be some
+    # complex generic type
     r = typeRel(m, f, arg.typ)
+    case r
+    of isConvertible, isNone:
+      discard "okay; these stay"
+    of isInferredConvertible, isInferred:
+      # there's nothing left to infer for the procedure type used as the
+      # argument, so these are not possible
+      unreachable()
+    else:
+      # some form of inference must have taken place (otherwise we wouldn't
+      # have reached here), so the match has to be counted as generic;
+      # ``isBothMetaConvertible`` is used to signal this.
+      r = isBothMetaConvertible
 
   # now check the relation result in `r`
   case r
@@ -2219,16 +2267,7 @@ proc paramTypesMatchAux(m: var TCandidate, f, a: PType,
       else:
         implicitConv(nkHiddenStdConv, f, arg, m, c)
   of isInferred, isInferredConvertible:
-    case arg.kind
-    of nkProcDef, nkFuncDef, nkIteratorDef, nkLambdaKinds:
-      result = c.semInferredLambda(c, m.bindings, arg)
-    of nkSym:
-      let inferred = c.semGenerateInstance(c, arg.sym, m.bindings, arg.info)
-      result = newSymNode(inferred, arg.info)
-    else:
-      result = nil
-      return
-
+    result = instantiateRoutineExpr(c, m.bindings, arg)
     if result.isError:
       return
 
@@ -2255,8 +2294,10 @@ proc paramTypesMatchAux(m: var TCandidate, f, a: PType,
     else:
       result = arg
   of isBothMetaConvertible:
-    # This is the result for the 101th time.
-    result = nil
+    # we reach here if a generic procedure with an 'auto' return type was
+    # instantiated. Regarding the counters, this is treated the same way
+    # as ``isInferred`` is
+    inc(m.genericMatches)
   of isFromIntLit:
     # too lazy to introduce another ``*matches`` field, so we conflate
     # ``isIntConv`` and ``isIntLit`` here:
diff --git a/tests/lang_callable/generics/tgeneric_intantiation_outside_call.nim b/tests/lang_callable/generics/tgeneric_intantiation_outside_call.nim
index 73a8558c3d3..07cfc0535bf 100644
--- a/tests/lang_callable/generics/tgeneric_intantiation_outside_call.nim
+++ b/tests/lang_callable/generics/tgeneric_intantiation_outside_call.nim
@@ -1,5 +1,5 @@
 discard """
-  errormsg: "cannot instantiate: \'T\'"
+  errormsg: "'m' doesn't have a concrete type, due to unspecified generic parameters"
   description: '''
     . From https://github.com/nim-lang/Nim/issues/8270
       SIGSEGV on pragma with array of generic proc
diff --git a/tests/lang_callable/generics/tgeneric_procedure_cannot_assigned.nim b/tests/lang_callable/generics/tgeneric_procedure_cannot_assigned.nim
index 6d114254fb4..64ee2d69025 100644
--- a/tests/lang_callable/generics/tgeneric_procedure_cannot_assigned.nim
+++ b/tests/lang_callable/generics/tgeneric_procedure_cannot_assigned.nim
@@ -1,5 +1,5 @@
 discard """
-  errormsg: "cannot instantiate: \'T\'"
+  errormsg: "'foo' doesn't have a concrete type, due to unspecified generic parameters"
   description: '''
     . From https://github.com/nim-lang/Nim/issues/7141
       3 lines to break the compiler: proc [T], assignment and SIGSEGV
diff --git a/tests/lang_callable/overload/tproc_type_inference.nim b/tests/lang_callable/overload/tproc_type_inference.nim
new file mode 100644
index 00000000000..39cb55d4eea
--- /dev/null
+++ b/tests/lang_callable/overload/tproc_type_inference.nim
@@ -0,0 +1,95 @@
+discard """
+  description: '''
+    Tests for inference of parameter types of procedural types during overload
+    resolution
+  '''
+"""
+
+block inferred_actual_type:
+  # passing a generic routine to a parameter of concrete procedural type
+  # works; the types are inferred and an instantiation created
+  proc call(x: proc(x: int): bool) =
+    doAssert x(1) == true
+
+  proc generic[A, B](x: A): B =
+    result = true
+
+  call(generic)
+  # also works for lambda expressions:
+  call(proc(x: auto): bool = true)
+
+block inferred_actual_type_late_bound:
+  # inference also works when the formal type is a type variable that has a
+  # type bound already
+  proc call[T](a: T, x: proc(x: T): bool) =
+    doAssert x(a) == true
+
+  proc generic[A, B](x: A): B =
+    result = true
+
+  call(1, generic)
+  # also works for lambda expressions:
+  call(2, proc(x: auto): bool = true)
+
+block cross_inference:
+  # the inferred type is bound to the type variable of the actual parameter,
+  # meaning that each further usage of the type variable acts like the type it
+  # was inferred as
+  proc call[A, B](val: A, x: proc(a: A, b: B): bool) =
+    doAssert x(val, val) == true
+
+  proc generic[A](a: A, b: A): bool =
+    result = true
+
+  call(1, generic)
+  # a lambda expression with two parameters using the same type variable
+  # (i.e., generic parameter) doesn't work
+
+block more_complex_inference:
+  # inference also works when both the formal and actual type are not directly
+  # generic parameters
+  type
+    Container[T] = object
+    Alias[U]     = Container[U]
+
+  proc call[T](val: T, x: proc(a: Container[T]): bool) =
+    doAssert x(Container[T]()) == true
+
+  proc generic[T](a: Alias[T]): bool =
+    result = true
+
+  call(1, generic)
+
+block auto_return_type:
+  # a generic routine using an 'auto' return type can also be passed to a
+  # parameter of procedural type. All type variables of the input procedural
+  # type are inferred first, and then the routine is instantiated. After
+  # that, the type of the instantiated routine is matched against the formal
+  # type
+  proc callSimple(val: int, x: proc(x: int): int) =
+    doAssert x(val) == val
+
+  proc callGenericRet[T](val: int, x: proc(x: int): T) =
+    # the return type of the procedural type is generic
+    doAssert x(val) == val
+
+  proc generic[T](x: T): auto =
+    result = x
+
+  callSimple(1, generic)
+  callGenericRet(2, generic)
+  # lambda expressions also support the auto return type
+  callSimple(3, proc(x: auto): auto = x)
+  callGenericRet(3, proc(x: auto): auto = x)
+
+block complex_argument_expression_with_auto_return_type:
+  # complex expressions yielding generic routines also work
+  proc call(val: int, x: proc(x: int): int) =
+    doAssert x(val) == val
+
+  proc generic[T](x: T): auto =
+    result = x
+
+  call(1, (discard ""; generic))
+  # also works with lambda expressions:
+  call(2, (discard ""; (proc(x: auto): auto = x)))
\ No newline at end of file