From ef2566218e701e6294608c6c30a367624786dcd6 Mon Sep 17 00:00:00 2001
From: Araq <rumpf_a@web.de>
Date: Sun, 22 Mar 2020 19:32:37 +0100
Subject: [PATCH] better error messages for Nim's effect system

---
 changelog.md                |  1 +
 compiler/semcall.nim        | 17 ++++++++++++++
 compiler/sempass2.nim       | 46 +++++++++++++++++++++----------------
 tests/effects/teffects1.nim |  3 ++-
 4 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/changelog.md b/changelog.md
index d5d1be956..a471ffbb2 100644
--- a/changelog.md
+++ b/changelog.md
@@ -189,6 +189,7 @@ echo f
   this is **very bad** style. You should inherit from `ValueError`, `IOError`,
   `OSError` or from a different specific exception type that inherits from
   `CatchableError` and cannot be confused with a `Defect`.
+- The error reporting for Nim's effect system has been improved.
 
 
 ## Bugfixes
diff --git a/compiler/semcall.nim b/compiler/semcall.nim
index 55ff80b6c..d8834dc90 100644
--- a/compiler/semcall.nim
+++ b/compiler/semcall.nim
@@ -136,6 +136,23 @@ proc effectProblem(f, a: PType; result: var string) =
     elif tfNoSideEffect in f.flags and tfNoSideEffect notin a.flags:
       result.add "\n  This expression can have side effects. Annotate the " &
           "proc with {.noSideEffect.} to get extended error information."
+    else:
+      case compatibleEffects(f, a)
+      of efCompat: discard
+      of efRaisesDiffer:
+        result.add "\n  The `.raises` requirements differ."
+      of efRaisesUnknown:
+        result.add "\n  The `.raises` requirements differ. Annotate the " &
+            "proc with {.raises: [].} to get extended error information."
+      of efTagsDiffer:
+        result.add "\n  The `.tags` requirements differ."
+      of efTagsUnknown:
+        result.add "\n  The `.tags` requirements differ. Annotate the " &
+            "proc with {.tags: [].} to get extended error information."
+      of efLockLevelsDiffer:
+        result.add "\n  The `.locks` requirements differ. Annotate the " &
+            "proc with {.locks: 0.} to get extended error information."
+
 
 proc renderNotLValue(n: PNode): string =
   result = $n
diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim
index d5514d8d1..c8ad14c41 100644
--- a/compiler/sempass2.nim
+++ b/compiler/sempass2.nim
@@ -281,8 +281,14 @@ proc addToIntersection(inter: var TIntersection, s: int) =
       return
   inter.add((id: s, count: 1))
 
-proc throws(tracked, n: PNode) =
-  if n.typ == nil or n.typ.kind != tyError: tracked.add n
+proc throws(tracked, n, orig: PNode) =
+  if n.typ == nil or n.typ.kind != tyError:
+    if orig != nil:
+      let x = copyNode(n)
+      x.info = orig.info
+      tracked.add x
+    else:
+      tracked.add n
 
 proc getEbase(g: ModuleGraph; info: TLineInfo): PType =
   result = g.sysTypeFromName(info, "Exception")
@@ -302,34 +308,34 @@ proc createTag(g: ModuleGraph; n: PNode): PNode =
   result.typ = g.sysTypeFromName(n.info, "RootEffect")
   if not n.isNil: result.info = n.info
 
-proc addEffect(a: PEffects, e: PNode, useLineInfo=true) =
+proc addEffect(a: PEffects, e, comesFrom: PNode) =
   assert e.kind != nkRaiseStmt
   var aa = a.exc
   for i in a.bottom..<aa.len:
-    if sameType(a.graph.excType(aa[i]), a.graph.excType(e)):
-      if not useLineInfo or a.config.cmd == cmdDoc: return
-      elif aa[i].info == e.info: return
-  throws(a.exc, e)
+    # we only track the first node that can have the effect E in order
+    # to safe space and time.
+    if sameType(a.graph.excType(aa[i]), a.graph.excType(e)): return
+  throws(a.exc, e, comesFrom)
 
-proc addTag(a: PEffects, e: PNode, useLineInfo=true) =
+proc addTag(a: PEffects, e, comesFrom: PNode) =
   var aa = a.tags
   for i in 0..<aa.len:
-    if sameType(aa[i].typ.skipTypes(skipPtrs), e.typ.skipTypes(skipPtrs)):
-      if not useLineInfo or a.config.cmd == cmdDoc: return
-      elif aa[i].info == e.info: return
-  throws(a.tags, e)
+    # we only track the first node that can have the effect E in order
+    # to safe space and time.
+    if sameType(aa[i].typ.skipTypes(skipPtrs), e.typ.skipTypes(skipPtrs)): return
+  throws(a.tags, e, comesFrom)
 
 proc mergeEffects(a: PEffects, b, comesFrom: PNode) =
   if b.isNil:
-    addEffect(a, createRaise(a.graph, comesFrom))
+    addEffect(a, createRaise(a.graph, comesFrom), comesFrom)
   else:
-    for effect in items(b): addEffect(a, effect, useLineInfo=comesFrom != nil)
+    for effect in items(b): addEffect(a, effect, comesFrom)
 
 proc mergeTags(a: PEffects, b, comesFrom: PNode) =
   if b.isNil:
-    addTag(a, createTag(a.graph, comesFrom))
+    addTag(a, createTag(a.graph, comesFrom), comesFrom)
   else:
-    for effect in items(b): addTag(a, effect, useLineInfo=comesFrom != nil)
+    for effect in items(b): addTag(a, effect, comesFrom)
 
 proc listEffects(a: PEffects) =
   for e in items(a.exc):  message(a.config, e.info, hintUser, typeToString(e.typ))
@@ -505,8 +511,8 @@ proc notNilCheck(tracked: PEffects, n: PNode, paramType: PType) =
     of impYes: discard
 
 proc assumeTheWorst(tracked: PEffects; n: PNode; op: PType) =
-  addEffect(tracked, createRaise(tracked.graph, n))
-  addTag(tracked, createTag(tracked.graph, n))
+  addEffect(tracked, createRaise(tracked.graph, n), nil)
+  addTag(tracked, createTag(tracked.graph, n), nil)
   let lockLevel = if op.lockLevel == UnspecifiedLockLevel: UnknownLockLevel
                   else: op.lockLevel
   #if lockLevel == UnknownLockLevel:
@@ -730,7 +736,7 @@ proc track(tracked: PEffects, n: PNode) =
     if n[0].kind != nkEmpty:
       n[0].info = n.info
       #throws(tracked.exc, n[0])
-      addEffect(tracked, n[0], useLineInfo=false)
+      addEffect(tracked, n[0], nil)
       for i in 0..<n.safeLen:
         track(tracked, n[i])
       createTypeBoundOps(tracked, n[0].typ, n.info)
@@ -738,7 +744,7 @@ proc track(tracked: PEffects, n: PNode) =
       # A `raise` with no arguments means we're going to re-raise the exception
       # being handled or, if outside of an `except` block, a `ReraiseError`.
       # Here we add a `Exception` tag in order to cover both the cases.
-      addEffect(tracked, createRaise(tracked.graph, n))
+      addEffect(tracked, createRaise(tracked.graph, n), nil)
   of nkCallKinds:
     # p's effects are ours too:
     var a = n[0]
diff --git a/tests/effects/teffects1.nim b/tests/effects/teffects1.nim
index 8f827110c..6ca24d53e 100644
--- a/tests/effects/teffects1.nim
+++ b/tests/effects/teffects1.nim
@@ -1,6 +1,7 @@
 discard """
   errormsg: "can raise an unlisted exception: ref IOError"
-  file: "io.nim"
+  file: "teffects1.nim"
+  line: 17
 """
 
 type