From 3942e80d9b67f39e0411cdf0471b97b2fe3988d9 Mon Sep 17 00:00:00 2001
From: Felix Krause <contact@flyx.org>
Date: Sat, 24 Sep 2016 16:45:49 +0200
Subject: [PATCH] Improved error messages and test them.

---
 test/tserialization.nim | 22 ++++++++++++----------
 yaml/serialization.nim  | 37 +++++++++++++++++++++----------------
 2 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/test/tserialization.nim b/test/tserialization.nim
index 0f4812a..861dbc0 100644
--- a/test/tserialization.nim
+++ b/test/tserialization.nim
@@ -69,15 +69,17 @@ template assertStringEqual(expected, actual: string) =
       echo "expected:\n", expected, "\nactual:\n", actual
       assert(false)
 
-template expectConstructionError(l, c: int, body: typed) =
+template expectConstructionError(li, co: int, message: string, body: typed) =
   try:
     body
     echo "Expected YamlConstructionError, but none was raised!"
     fail()
   except YamlConstructionError:
     let e = (ref YamlConstructionError)(getCurrentException())
-    doAssert l == e.line, "Expected error line " & $l & ", was " & $e.line
-    doAssert c == e.column, "Expected error column " & $c & ", was " & $e.column
+    doAssert li == e.line, "Expected error line " & $li & ", was " & $e.line
+    doAssert co == e.column, "Expected error column " & $co & ", was " & $e.column
+    doAssert message == e.msg, "Expected error message " & escape(message) &
+        ", got " & escape(e.msg)
 
 proc newNode(v: string): ref Node =
   new(result)
@@ -315,19 +317,19 @@ suite "Serialization":
   test "Load Tuple - unknown field":
     let input = "str: value\nfoo: bar\ni: 42\nb: true"
     var result: MyTuple
-    expectConstructionError(2, 1):
+    expectConstructionError(2, 1, "While constructing MyTuple: Unknown field: \"foo\""):
       load(input, result)
 
   test "Load Tuple - missing field":
     let input = "str: value\nb: true"
     var result: MyTuple
-    expectConstructionError(2, 8):
+    expectConstructionError(2, 8, "While constructing MyTuple: Missing field: \"i\""):
       load(input, result)
 
   test "Load Tuple - duplicate field":
     let input = "str: value\ni: 42\nb: true\nb: true"
     var result: MyTuple
-    expectConstructionError(4, 1):
+    expectConstructionError(4, 1, "While constructing MyTuple: Duplicate field: \"b\""):
       load(input, result)
 
   test "Load Multiple Documents":
@@ -362,19 +364,19 @@ suite "Serialization":
   test "Load custom object - unknown field":
     let input = "  firstnamechar: P\n  surname: Pan\n  age: 12\n  occupation: free"
     var result: Person
-    expectConstructionError(4, 3):
+    expectConstructionError(4, 3, "While constructing Person: Unknown field: \"occupation\""):
       load(input, result)
 
   test "Load custom object - missing field":
     let input = "surname: Pan\nage: 12\n  "
     var result: Person
-    expectConstructionError(3, 3):
+    expectConstructionError(3, 3, "While constructing Person: Missing field: \"firstnamechar\""):
       load(input, result)
 
   test "Load custom object - duplicate field":
     let input = "firstnamechar: P\nsurname: Pan\nage: 12\nsurname: Pan"
     var result: Person
-    expectConstructionError(4, 1):
+    expectConstructionError(4, 1, "While constructing Person: Duplicate field: \"surname\""):
       load(input, result)
 
   test "Load sequence with explicit tags":
@@ -445,7 +447,7 @@ suite "Serialization":
   test "Load custom variant object - missing field":
     let input = "[{name: Bastet}, {kind: akCat}]"
     var result: Animal
-    expectConstructionError(1, 32):
+    expectConstructionError(1, 32, "While constructing Animal: Missing field: \"purringIntensity\""):
       load(input, result)
 
   test "Dump cyclic data structure":
diff --git a/yaml/serialization.nim b/yaml/serialization.nim
index 9834e30..0f384cd 100644
--- a/yaml/serialization.nim
+++ b/yaml/serialization.nim
@@ -510,19 +510,19 @@ macro matchMatrix(t: typedesc): untyped =
   for i in 0..<numFields:
     result.add(newLit(false))
 
-proc checkDuplicate(s: NimNode, t: typedesc, name: string, i: int,
+proc checkDuplicate(s: NimNode, tName: string, name: string, i: int,
                     matched: NimNode): NimNode {.compileTime.} =
   result = newIfStmt((newNimNode(nnkBracketExpr).add(matched, newLit(i)),
       newNimNode(nnkRaiseStmt).add(newCall(bindSym("constructionError"), s,
-        newLit("While constructing " & typetraits.name(t) &
-               ": Duplicate field: " & escape(name))))))
+      newLit("While constructing " & tName & ": Duplicate field: " &
+      escape(name))))))
 
-proc checkMissing(s: NimNode, t: typedesc, name: string, i: int,
+proc checkMissing(s: NimNode, tName: string, name: string, i: int,
                   matched: NimNode): NimNode {.compileTime.} =
   result = newIfStmt((newCall("not", newNimNode(nnkBracketExpr).add(matched,
       newLit(i))), newNimNode(nnkRaiseStmt).add(newCall(
       bindSym("constructionError"), s, newLit("While constructing " &
-      typetraits.name(t) & ": Missing field: " & escape(name))))))
+      tName & ": Missing field: " & escape(name))))))
 
 proc markAsFound(i: int, matched: NimNode): NimNode {.compileTime.} =
   newAssignment(newNimNode(nnkBracketExpr).add(matched, newLit(i)),
@@ -532,25 +532,30 @@ macro ensureAllFieldsPresent(s: YamlStream, t: typedesc, o: typed,
                              matched: typed): typed =
   result = newStmtList()
   let
-    tDesc = getType(getType(t)[1])
+    tDecl = getType(t)
+    tName = $tDecl[1]
+    tDesc = getType(tDecl[1])
   var field = 0
   for child in tDesc[2].children:
     if child.kind == nnkRecCase:
-      result.add(checkMissing(s, t, $child[0], field, matched))
+      result.add(checkMissing(s, tName, $child[0], field, matched))
       for bIndex in 1 .. len(child) - 1:
         let discChecks = newStmtList()
         for item in child[bIndex][1].children:
           inc(field)
-          discChecks.add(checkMissing(s, t, $item, field, matched))
+          discChecks.add(checkMissing(s, tName, $item, field, matched))
         result.add(newIfStmt((infix(newDotExpr(o, newIdentNode($child[0])),
             "==", child[bIndex][0]), discChecks)))
     else:
-      result.add(checkMissing(s, t, $child, field, matched))
+      result.add(checkMissing(s, tName, $child, field, matched))
     inc(field)
 
 macro constructFieldValue(t: typedesc, stream: untyped, context: untyped,
                            name: untyped, o: untyped, matched: untyped): typed =
-  let tDesc = getType(getType(t)[1])
+  let
+    tDecl = getType(t)
+    tName = $tDecl[1]
+    tDesc = getType(tDecl[1])
   result = newNimNode(nnkCaseStmt).add(name)
   var fieldIndex = 0
   for child in tDesc[2].children:
@@ -560,7 +565,7 @@ macro constructFieldValue(t: typedesc, stream: untyped, context: untyped,
         discType = newCall("type", discriminant)
       var disOb = newNimNode(nnkOfBranch).add(newStrLitNode($child[0]))
       disOb.add(newStmtList(
-          checkDuplicate(stream, t, $child[0], fieldIndex, matched),
+          checkDuplicate(stream, tName, $child[0], fieldIndex, matched),
           newNimNode(nnkVarSection).add(
               newNimNode(nnkIdentDefs).add(
                   newIdentNode("value"), discType, newEmptyNode())),
@@ -581,7 +586,7 @@ macro constructFieldValue(t: typedesc, stream: untyped, context: untyped,
               newCall(bindSym("constructionError"), stream,
               infix(newStrLitNode("Field " & $item & " not allowed for " &
               $child[0] & " == "), "&", prefix(discriminant, "$"))))))
-          ob.add(newStmtList(checkDuplicate(stream, t, $item, fieldIndex,
+          ob.add(newStmtList(checkDuplicate(stream, tName, $item, fieldIndex,
               matched), ifStmt, markAsFound(fieldIndex, matched)))
           result.add(ob)
     else:
@@ -589,15 +594,15 @@ macro constructFieldValue(t: typedesc, stream: untyped, context: untyped,
       var ob = newNimNode(nnkOfBranch).add(newStrLitNode($child))
       let field = newDotExpr(o, newIdentNode($child))
       ob.add(newStmtList(
-          checkDuplicate(stream, t, $child, fieldIndex, matched),
+          checkDuplicate(stream, tName, $child, fieldIndex, matched),
           newCall("constructChild", stream, context, field),
           markAsFound(fieldIndex, matched)))
       result.add(ob)
     inc(fieldIndex)
   result.add(newNimNode(nnkElse).add(newNimNode(nnkRaiseStmt).add(
       newCall(bindSym("constructionError"), stream,
-      infix(newLit("While constructing " & typetraits.name(t) &
-             ": Unknown field: "), "&", name)))))
+      infix(newLit("While constructing " & tName & ": Unknown field: "), "&",
+      newCall(bindSym("escape"), name))))))
 
 proc isVariantObject(t: typedesc): bool {.compileTime.} =
   let tDesc = getType(t)
@@ -659,7 +664,7 @@ proc constructObject*[O: object|tuple](
     for fname, value in fieldPairs(result):
       if not matched[i]:
         raise s.constructionError("While constructing " &
-            typetraits.name(O) & ": Field missing: " & escape(fname))
+            typetraits.name(O) & ": Missing field: " & escape(fname))
       inc(i)
   else: ensureAllFieldsPresent(s, O, result, matched)