Make procedure mismatch more informative with pragma/call convention mismatches (#18384)

* Added more concise calling convention/pragma mismatch messages
* Now only adds callConvMsg/lock message  when sensible
* Fixed message formatting
* Added tests, and fixed some bugs
* Tests joined, and always indenting
* More tests and more bug fixes
* Fixed first test in tprocmismatch
* Using var param for writting mismatches
* Better logic for handling proc type rel and conv/pragma mismatch
* Refactored getProcConvMismatch
* Fixed callConv message formatting
* Fixed test for proper message
* Cleanup to address issues
* getProcConvMismatch now returns tuple, and reformatted code

4 files changed, 170 insertions, 32 deletions

diff --git a/compiler/semcall.nim b/compiler/semcall.nim
index b56a1dbac..cec656fc2 100644
--- a/compiler/semcall.nim
+++ b/compiler/semcall.nim
@@ -247,7 +247,13 @@ proc presentFailedCandidates(c: PContext, n: PNode, errors: CandidateErrors):
           candidates.add typeToString(got)
           candidates.addDeclaredLocMaybe(c.config, got)
           doAssert wanted != nil
-          if got != nil: effectProblem(wanted, got, candidates, c)
+          if got != nil:
+            if got.kind == tyProc and wanted.kind == tyProc:
+              # These are proc mismatches so,
+              # add the extra explict detail of the mismatch
+              candidates.addPragmaAndCallConvMismatch(wanted, got, c.config)
+            effectProblem(wanted, got, candidates, c)
+
       of kUnknown: discard "do not break 'nim check'"
       candidates.add "\n"
       if err.firstMismatch.arg == 1 and nArg.kind == nkTupleConstr and
diff --git a/compiler/sigmatch.nim b/compiler/sigmatch.nim
index 071685057..73ab5e9a3 100644
--- a/compiler/sigmatch.nim
+++ b/compiler/sigmatch.nim
@@ -86,21 +86,6 @@ type
 
   TTypeRelFlags* = set[TTypeRelFlag]
 
-  TTypeRelation* = enum      # order is important!
-    isNone, isConvertible,
-    isIntConv,
-    isSubtype,
-    isSubrange,              # subrange of the wanted type; no type conversion
-                             # but apart from that counts as ``isSubtype``
-    isBothMetaConvertible    # generic proc parameter was matched against
-                             # generic type, e.g., map(mySeq, x=>x+1),
-                             # maybe recoverable by rerun if the parameter is
-                             # the proc's return value
-    isInferred,              # generic proc was matched against a concrete type
-    isInferredConvertible,   # same as above, but requiring proc CC conversion
-    isGeneric,
-    isFromIntLit,            # conversion *from* int literal; proven safe
-    isEqual
 
 const
   isNilConversion = isConvertible # maybe 'isIntConv' fits better?
@@ -640,23 +625,9 @@ proc procTypeRel(c: var TCandidate, f, a: PType): TTypeRelation =
         return isNone
     elif a[0] != nil:
       return isNone
+    
+    result = getProcConvMismatch(c.c.config, f, a, result)[1]
 
-    if tfNoSideEffect in f.flags and tfNoSideEffect notin a.flags:
-      return isNone
-    elif tfThread in f.flags and a.flags * {tfThread, tfNoSideEffect} == {} and
-        optThreadAnalysis in c.c.config.globalOptions:
-      # noSideEffect implies ``tfThread``!
-      return isNone
-    elif f.flags * {tfIterator} != a.flags * {tfIterator}:
-      return isNone
-    elif f.callConv != a.callConv:
-      # valid to pass a 'nimcall' thingie to 'closure':
-      if f.callConv == ccClosure and a.callConv == ccNimCall:
-        result = if result == isInferred: isInferredConvertible
-                 elif result == isBothMetaConvertible: isBothMetaConvertible
-                 else: isConvertible
-      else:
-        return isNone
     when useEffectSystem:
       if compatibleEffects(f, a) != efCompat: return isNone
     when defined(drnim):
diff --git a/compiler/types.nim b/compiler/types.nim
index bc970a6d2..bdf08ffe9 100644
--- a/compiler/types.nim
+++ b/compiler/types.nim
@@ -26,6 +26,29 @@ type
       # most useful, shows: symbol + resolved symbols if it differs, e.g.:
       # tuple[a: MyInt{int}, b: float]
 
+  TTypeRelation* = enum      # order is important!
+    isNone, isConvertible,
+    isIntConv,
+    isSubtype,
+    isSubrange,              # subrange of the wanted type; no type conversion
+                             # but apart from that counts as ``isSubtype``
+    isBothMetaConvertible    # generic proc parameter was matched against
+                             # generic type, e.g., map(mySeq, x=>x+1),
+                             # maybe recoverable by rerun if the parameter is
+                             # the proc's return value
+    isInferred,              # generic proc was matched against a concrete type
+    isInferredConvertible,   # same as above, but requiring proc CC conversion
+    isGeneric,
+    isFromIntLit,            # conversion *from* int literal; proven safe
+    isEqual
+
+  ProcConvMismatch* = enum
+    pcmNoSideEffect
+    pcmNotGcSafe
+    pcmLockDifference
+    pcmNotIterator
+    pcmDifferentCallConv
+
 proc typeToString*(typ: PType; prefer: TPreferedDesc = preferName): string
 template `$`*(typ: PType): string = typeToString(typ)
 
@@ -1471,6 +1494,68 @@ proc skipHiddenSubConv*(n: PNode; g: ModuleGraph; idgen: IdGenerator): PNode =
   else:
     result = n
 
+proc getProcConvMismatch*(c: ConfigRef, f, a: PType, rel = isNone): (set[ProcConvMismatch], TTypeRelation) =
+  ## Returns a set of the reason of mismatch, and the relation for conversion.
+  result[1] = rel
+  if tfNoSideEffect in f.flags and tfNoSideEffect notin a.flags:
+    # Formal is pure, but actual is not
+    result[0].incl pcmNoSideEffect
+    result[1] = isNone
+
+  if tfThread in f.flags and a.flags * {tfThread, tfNoSideEffect} == {} and
+    optThreadAnalysis in c.globalOptions:
+    # noSideEffect implies ``tfThread``!
+    result[0].incl pcmNotGcSafe
+    result[1] = isNone
+
+  if f.flags * {tfIterator} != a.flags * {tfIterator}:
+    # One of them is an iterator so not convertible
+    result[0].incl pcmNotIterator
+    result[1] = isNone
+
+  if f.callConv != a.callConv:
+    # valid to pass a 'nimcall' thingie to 'closure':
+    if f.callConv == ccClosure and a.callConv == ccNimCall:
+      case result[1]
+      of isInferred: result[1] = isInferredConvertible
+      of isBothMetaConvertible: result[1] = isBothMetaConvertible
+      elif result[1] != isNone: result[1] = isConvertible
+    else:
+      result[1] = isNone
+      result[0].incl pcmDifferentCallConv
+
+  if f.lockLevel.ord != UnspecifiedLockLevel.ord and
+     a.lockLevel.ord != UnspecifiedLockLevel.ord:
+       # proctypeRel has more logic to catch this difference,
+       # so dont need to do `rel = isNone`
+       # but it's a pragma mismatch reason which is why it's here
+       result[0].incl pcmLockDifference
+
+proc addPragmaAndCallConvMismatch*(message: var string, formal, actual: PType, conf: ConfigRef) =
+  assert formal.kind == tyProc and actual.kind == tyProc
+  let (convMismatch, _) = getProcConvMismatch(conf, formal, actual)
+  var
+    gotPragmas = ""
+    expectedPragmas = ""
+  for reason in convMismatch:
+    case reason
+    of pcmDifferentCallConv:
+      message.add "\n  Calling convention mismatch: got '{.$1.}', but expected '{.$2.}'." % [$actual.callConv, $formal.callConv]
+    of pcmNoSideEffect:
+      expectedPragmas.add "noSideEffect, "
+    of pcmNotGcSafe:
+      expectedPragmas.add "gcsafe, "
+    of pcmLockDifference:
+      gotPragmas.add("locks: " & $actual.lockLevel & ", ")
+      expectedPragmas.add("locks: " & $formal.lockLevel & ", ")
+    of pcmNotIterator: discard
+
+  if expectedPragmas.len > 0:
+    gotPragmas.setLen(max(0, gotPragmas.len - 2)) # Remove ", "
+    expectedPragmas.setLen(max(0, expectedPragmas.len - 2)) # Remove ", "
+    message.add "\n  Pragma mismatch: got '{.$1.}', but expected '{.$2.}'." % [gotPragmas, expectedPragmas]
+
+
 proc typeMismatch*(conf: ConfigRef; info: TLineInfo, formal, actual: PType, n: PNode) =
   if formal.kind != tyError and actual.kind != tyError:
     let actualStr = typeToString(actual)
@@ -1491,6 +1576,7 @@ proc typeMismatch*(conf: ConfigRef; info: TLineInfo, formal, actual: PType, n: P
     if verbose: msg.addDeclaredLoc(conf, formal)
 
     if formal.kind == tyProc and actual.kind == tyProc:
+      msg.addPragmaAndCallConvMismatch(formal, actual, conf)
       case compatibleEffects(formal, actual)
       of efCompat: discard
       of efRaisesDiffer:
@@ -1503,6 +1589,7 @@ proc typeMismatch*(conf: ConfigRef; info: TLineInfo, formal, actual: PType, n: P
         msg.add "\n.tag effect is 'any tag allowed'"
       of efLockLevelsDiffer:
         msg.add "\nlock levels differ"
+
     if formal.kind == tyEnum and actual.kind == tyEnum:
       msg.add "\nmaybe use `-d:nimLegacyConvEnumEnum` for a transition period"
     localError(conf, info, msg)
diff --git a/tests/errmsgs/tproc_mismatch.nim b/tests/errmsgs/tproc_mismatch.nim
new file mode 100644
index 000000000..400c3d441
--- /dev/null
+++ b/tests/errmsgs/tproc_mismatch.nim
@@ -0,0 +1,74 @@
+discard """
+  action: reject
+  cmd: '''nim check --hints:off $options $file'''
+  nimoutFull: true
+  nimout: '''
+tproc_mismatch.nim(35, 52) Error: type mismatch: got <proc (a: int, c: float){.cdecl, noSideEffect, gcsafe, locks: 0.}> but expected 'proc (a: int, c: float){.closure, noSideEffect.}'
+  Calling convention mismatch: got '{.cdecl.}', but expected '{.closure.}'.
+tproc_mismatch.nim(39, 6) Error: type mismatch: got <proc (){.inline, noSideEffect, gcsafe, locks: 0.}>
+but expected one of: 
+proc bar(a: proc ())
+  first type mismatch at position: 1
+  required type for a: proc (){.closure.}
+  but expression 'fn1' is of type: proc (){.inline, noSideEffect, gcsafe, locks: 0.}
+  Calling convention mismatch: got '{.inline.}', but expected '{.closure.}'.
+
+expression: bar(fn1)
+tproc_mismatch.nim(43, 8) Error: type mismatch: got <proc (){.inline, noSideEffect, gcsafe, locks: 0.}> but expected 'proc (){.closure.}'
+  Calling convention mismatch: got '{.inline.}', but expected '{.closure.}'.
+tproc_mismatch.nim(48, 8) Error: type mismatch: got <proc (){.locks: 0.}> but expected 'proc (){.closure, noSideEffect.}'
+  Pragma mismatch: got '{..}', but expected '{.noSideEffect.}'.
+tproc_mismatch.nim(52, 8) Error: type mismatch: got <proc (a: int){.noSideEffect, gcsafe, locks: 0.}> but expected 'proc (a: float){.closure.}'
+tproc_mismatch.nim(61, 9) Error: type mismatch: got <proc (a: int){.locks: 0.}> but expected 'proc (a: int){.closure, gcsafe.}'
+  Pragma mismatch: got '{..}', but expected '{.gcsafe.}'.
+tproc_mismatch.nim(69, 9) Error: type mismatch: got <proc (a: int): int{.nimcall.}> but expected 'proc (a: int): int{.cdecl.}'
+  Calling convention mismatch: got '{.nimcall.}', but expected '{.cdecl.}'.
+tproc_mismatch.nim(70, 9) Error: type mismatch: got <proc (a: int): int{.cdecl.}> but expected 'proc (a: int): int{.nimcall.}'
+  Calling convention mismatch: got '{.cdecl.}', but expected '{.nimcall.}'.
+tproc_mismatch.nim(74, 9) Error: type mismatch: got <proc (a: int){.closure, locks: 3.}> but expected 'proc (a: int){.closure, locks: 1.}'
+  Pragma mismatch: got '{.locks: 3.}', but expected '{.locks: 1.}'.
+lock levels differ
+'''
+"""
+block: # CallConv mismatch
+  func a(a: int, c: float) {.cdecl.} = discard
+  var b: proc(a: int, c: float) {.noSideEffect.} = a
+block: # Parameter CallConv mismatch
+  proc fn1() {.inline.} = discard
+  proc bar(a: proc()) = discard
+  bar(fn1)
+block: # CallConv mismatch
+  proc fn1() {.inline.} = discard
+  var fn: proc()
+  fn = fn1
+block: # Pragma mismatch
+  var a = ""
+  proc fn1() = a.add "b"
+  var fn: proc() {.noSideEffect.}
+  fn = fn1
+block: # Fail match not do to Pragma or CallConv
+  proc fn1(a: int) = discard
+  var fn: proc(a: float)
+  fn = fn1
+block: # Infered noSideEffect assign
+  type Foo = ref object
+    x0: int
+  var g0 = Foo(x0: 1)
+  proc fn1(a: int) = g0.x0 = a
+  var fn2: proc(a: int)
+  var fn3: proc(a: int) {.gcsafe.}
+  fn2 = fn1
+  fn3 = fn1
+block: # Indrection through pragmas
+  {.pragma: inl1, inline.}
+  {.pragma: inl2, inline.}
+  {.pragma: p1, nimcall.}
+  {.pragma: p2, cdecl.}
+  var fn1: proc(a: int): int {.inl1, p1.}
+  var fn2: proc(a: int): int {.inl2, p2.}
+  fn2 = fn1
+  fn1 = fn2
+block: # Lock levels differ
+  var fn1: proc(a: int){.locks: 3.}
+  var fn2: proc(a: int){.locks: 1.}
+  fn2 = fn1