fix noreturn/implicit discard check logic (#23681)

fixes #10440, fixes #13871, fixes #14665, fixes #19672, fixes #23677

The false positive in #23677 was caused by behavior in
`implicitlyDiscardable` where only the last node of `if`/`case`/`try`
etc expressions were considered, as in the final node of the final
branch (in this case `else`). To fix this we use the same iteration in
`implicitlyDiscardable` that we use in `endsInNoReturn`, with the
difference that for an `if`/`case`/`try` statement to be implicitly
discardable, all of its branches must be implicitly discardable.
`noreturn` calls are also considered implicitly discardable for this
reason, otherwise stuff like `if true: discardableCall() else: error()`
doesn't compile.

However `endsInNoReturn` also had bugs, one where `finally` was
considered in noreturn checking when it shouldn't, another where only
`nkIfStmt` was checked and not `nkIfExpr`, and the node given for the
error message was bad. So `endsInNoReturn` now skips over
`skipForDiscardable` which no longer contains
`nkIfStmt`/`nkCaseStmt`/`nkTryStmt`, stores the first encountered
returning node in a var parameter for the error message, and handles
`finally` and `nkIfExpr`.

Fixing #23677 already broke a line in `syncio` so some package code
might be affected.

3 files changed, 61 insertions, 0 deletions

diff --git a/tests/discard/t23677.nim b/tests/discard/t23677.nim
new file mode 100644
index 000000000..1ed7386bd
--- /dev/null
+++ b/tests/discard/t23677.nim
@@ -0,0 +1,12 @@
+discard """
+  errormsg: "expression '0' is of type 'int literal(0)' and has to be used (or discarded); start of expression here: t23677.nim(1, 1)"
+  line: 10
+  column: 3
+"""
+
+# issue #23677
+
+if true:
+  0
+else: 
+  raise newException(ValueError, "err") 
diff --git a/tests/discard/tdiscardable.nim b/tests/discard/tdiscardable.nim
index 69cb9f6a1..5988f5949 100644
--- a/tests/discard/tdiscardable.nim
+++ b/tests/discard/tdiscardable.nim
@@ -5,6 +5,7 @@ tdiscardable
 1
 something defered
 something defered
+hi
 '''
 """
 
@@ -110,3 +111,32 @@ block:
 
   doAssertRaises(ValueError):
     doAssert foo() == 12
+
+block: # issue #10440
+  proc x(): int {.discardable.} = discard
+  try:
+    x()
+  finally:
+    echo "hi"
+
+import macros
+
+block: # issue #14665
+  macro test(): untyped =   
+    let b = @[1, 2, 3, 4]
+
+    result = nnkStmtList.newTree()
+    var i = 0
+    while i < b.len:
+      if false:
+        # this quote do is mandatory, removing it fixes the problem
+        result.add quote do:
+          let testtest = 5
+      else:
+        result.add quote do:
+          let test = 6
+        inc i
+        # removing this continue fixes the problem too
+        continue
+      inc i
+  test()
diff --git a/tests/discard/tfinallyerrmsg.nim b/tests/discard/tfinallyerrmsg.nim
new file mode 100644
index 000000000..fbc8140aa
--- /dev/null
+++ b/tests/discard/tfinallyerrmsg.nim
@@ -0,0 +1,19 @@
+discard """
+  cmd: "nim check $file"
+"""
+
+block: # issue #19672
+  try:
+    10 #[tt.Error
+    ^ expression '10' is of type 'int literal(10)' and has to be used (or discarded); start of expression here: tfinallyerrmsg.nim(5, 1)]#
+  finally:
+    echo "Finally block"
+
+block: # issue #13871
+  template t(body: int) =
+    try:
+      body
+    finally:
+      echo "expression"
+  t: 2 #[tt.Error
+     ^ expression '2' is of type 'int literal(2)' and has to be used (or discarded)]#