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.

6 files changed, 195 insertions, 74 deletions

diff --git a/compiler/sem.nim b/compiler/sem.nim
index a4552beee..760b83941 100644
--- a/compiler/sem.nim
+++ b/compiler/sem.nim
@@ -222,66 +222,7 @@ proc shouldCheckCaseCovered(caseTyp: PType): bool =
   else:
     discard
 
-proc endsInNoReturn(n: PNode): bool =
-  ## check if expr ends the block like raising or call of noreturn procs do
-  result = false # assume it does return
-
-  template checkBranch(branch) =
-    if not endsInNoReturn(branch):
-      # proved a branch returns
-      return false
-
-  var it = n
-  # skip these beforehand, no special handling needed
-  while it.kind in {nkStmtList, nkStmtListExpr} and it.len > 0:
-    it = it.lastSon
-
-  case it.kind
-  of nkIfStmt:
-    var hasElse = false
-    for branch in it:
-      checkBranch:
-        if branch.len == 2:
-          branch[1]
-        elif branch.len == 1:
-          hasElse = true
-          branch[0]
-        else:
-          raiseAssert "Malformed `if` statement during endsInNoReturn"
-    # none of the branches returned
-    result = hasElse # Only truly a no-return when it's exhaustive
-  of nkCaseStmt:
-    let caseTyp = skipTypes(it[0].typ, abstractVar-{tyTypeDesc})
-    # semCase should already have checked for exhaustiveness in this case
-    # effectively the same as having an else
-    var hasElse = caseTyp.shouldCheckCaseCovered()
-
-    # actual noreturn checks
-    for i in 1 ..< it.len:
-      let branch = it[i]
-      checkBranch:
-        case branch.kind
-        of nkOfBranch:
-          branch[^1]
-        of nkElifBranch:
-          branch[1]
-        of nkElse:
-          hasElse = true
-          branch[0]
-        else:
-          raiseAssert "Malformed `case` statement in endsInNoReturn"
-    # Can only guarantee a noreturn if there is an else or it's exhaustive
-    result = hasElse
-  of nkTryStmt:
-    checkBranch(it[0])
-    for i in 1 ..< it.len:
-      let branch = it[i]
-      checkBranch(branch[^1])
-    # none of the branches returned
-    result = true
-  else:
-    result = it.kind in nkLastBlockStmts or
-      it.kind in nkCallKinds and it[0].kind == nkSym and sfNoReturn in it[0].sym.flags
+proc endsInNoReturn(n: PNode): bool
 
 proc commonType*(c: PContext; x: PType, y: PNode): PType =
   # ignore exception raising branches in case/if expressions
diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim
index a29b07d92..8c7dda4e7 100644
--- a/compiler/semstmts.nim
+++ b/compiler/semstmts.nim
@@ -132,17 +132,140 @@ proc semExprBranchScope(c: PContext, n: PNode; expectedType: PType = nil): PNode
   closeScope(c)
 
 const
-  skipForDiscardable = {nkIfStmt, nkIfExpr, nkCaseStmt, nkOfBranch,
-    nkElse, nkStmtListExpr, nkTryStmt, nkFinally, nkExceptBranch,
+  skipForDiscardable = {nkStmtList, nkStmtListExpr,
+    nkOfBranch, nkElse, nkFinally, nkExceptBranch,
     nkElifBranch, nkElifExpr, nkElseExpr, nkBlockStmt, nkBlockExpr,
     nkHiddenStdConv, nkHiddenDeref}
 
 proc implicitlyDiscardable(n: PNode): bool =
-  var n = n
-  while n.kind in skipForDiscardable: n = n.lastSon
-  result = n.kind in nkLastBlockStmts or
-           (isCallExpr(n) and n[0].kind == nkSym and
-           sfDiscardable in n[0].sym.flags)
+  # same traversal as endsInNoReturn
+  template checkBranch(branch) =
+    if not implicitlyDiscardable(branch):
+      return false
+
+  var it = n
+  # skip these beforehand, no special handling needed
+  while it.kind in skipForDiscardable and it.len > 0:
+    it = it.lastSon
+
+  case it.kind
+  of nkIfExpr, nkIfStmt:
+    for branch in it:
+      checkBranch:
+        if branch.len == 2:
+          branch[1]
+        elif branch.len == 1:
+          branch[0]
+        else:
+          raiseAssert "Malformed `if` statement during implicitlyDiscardable"
+    # all branches are discardable
+    result = true
+  of nkCaseStmt:
+    for i in 1 ..< it.len:
+      let branch = it[i]
+      checkBranch:
+        case branch.kind
+        of nkOfBranch:
+          branch[^1]
+        of nkElifBranch:
+          branch[1]
+        of nkElse:
+          branch[0]
+        else:
+          raiseAssert "Malformed `case` statement in endsInNoReturn"
+    # all branches are discardable
+    result = true
+  of nkTryStmt:
+    checkBranch(it[0])
+    for i in 1 ..< it.len:
+      let branch = it[i]
+      if branch.kind != nkFinally:
+        checkBranch(branch[^1])
+    # all branches are discardable
+    result = true
+  of nkCallKinds:
+    result = it[0].kind == nkSym and {sfDiscardable, sfNoReturn} * it[0].sym.flags != {}
+  of nkLastBlockStmts:
+    result = true
+  else:
+    result = false
+
+proc endsInNoReturn(n: PNode, returningNode: var PNode): bool =
+  ## check if expr ends the block like raising or call of noreturn procs do
+  result = false # assume it does return
+
+  template checkBranch(branch) =
+    if not endsInNoReturn(branch, returningNode):
+      # proved a branch returns
+      return false
+
+  var it = n
+  # skip these beforehand, no special handling needed
+  while it.kind in skipForDiscardable and it.len > 0:
+    it = it.lastSon
+
+  case it.kind
+  of nkIfExpr, nkIfStmt:
+    var hasElse = false
+    for branch in it:
+      checkBranch:
+        if branch.len == 2:
+          branch[1]
+        elif branch.len == 1:
+          hasElse = true
+          branch[0]
+        else:
+          raiseAssert "Malformed `if` statement during endsInNoReturn"
+    # none of the branches returned
+    result = hasElse # Only truly a no-return when it's exhaustive
+  of nkCaseStmt:
+    let caseTyp = skipTypes(it[0].typ, abstractVar-{tyTypeDesc})
+    # semCase should already have checked for exhaustiveness in this case
+    # effectively the same as having an else
+    var hasElse = caseTyp.shouldCheckCaseCovered()
+
+    # actual noreturn checks
+    for i in 1 ..< it.len:
+      let branch = it[i]
+      checkBranch:
+        case branch.kind
+        of nkOfBranch:
+          branch[^1]
+        of nkElifBranch:
+          branch[1]
+        of nkElse:
+          hasElse = true
+          branch[0]
+        else:
+          raiseAssert "Malformed `case` statement in endsInNoReturn"
+    # Can only guarantee a noreturn if there is an else or it's exhaustive
+    result = hasElse
+  of nkTryStmt:
+    checkBranch(it[0])
+    var lastIndex = it.len - 1
+    if it[lastIndex].kind == nkFinally:
+      # if finally is noreturn, then the entire statement is noreturn
+      if endsInNoReturn(it[lastIndex][^1], returningNode):
+        return true
+      dec lastIndex
+    for i in 1 .. lastIndex:
+      let branch = it[i]
+      checkBranch(branch[^1])
+    # none of the branches returned
+    result = true
+  of nkLastBlockStmts:
+    result = true
+  of nkCallKinds:
+    result = it[0].kind == nkSym and sfNoReturn in it[0].sym.flags
+    if not result:
+      returningNode = it
+  else:
+    result = false
+    returningNode = it
+
+proc endsInNoReturn(n: PNode): bool =
+  var dummy: PNode = nil
+  result = endsInNoReturn(n, dummy)
 
 proc fixNilType(c: PContext; n: PNode) =
   if isAtom(n):
@@ -165,13 +288,9 @@ proc discardCheck(c: PContext, result: PNode, flags: TExprFlags) =
         localError(c.config, result.info, "expression has no type: " &
                renderTree(result, {renderNoComments}))
       else:
-        var n = result
-        while n.kind in skipForDiscardable:
-          if n.kind == nkTryStmt: n = n[0]
-          else: n = n.lastSon
-
         # Ignore noreturn procs since they don't have a type
-        if n.endsInNoReturn:
+        var n = result
+        if result.endsInNoReturn(n):
           return
 
         var s = "expression '" & $n & "' is of type '" &
diff --git a/lib/std/syncio.nim b/lib/std/syncio.nim
index 2b39375ea..c34a025af 100644
--- a/lib/std/syncio.nim
+++ b/lib/std/syncio.nim
@@ -874,7 +874,7 @@ proc writeFile*(filename: string, content: openArray[byte]) {.since: (1, 1).} =
   var f: File = nil
   if open(f, filename, fmWrite):
     try:
-      f.writeBuffer(unsafeAddr content[0], content.len)
+      discard f.writeBuffer(unsafeAddr content[0], content.len)
     finally:
       close(f)
   else:
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)]#