From a1ee35e29b04e6d7d38eaf091848bfca54b096d5 Mon Sep 17 00:00:00 2001
From: Saem Ghani <saemghani+github@gmail.com>
Date: Thu, 12 Sep 2024 18:32:07 -0700
Subject: [PATCH] fix(sem): noreturn analysis for `void` exprs (#1454)

## Summary

`void`  branches preceeding with no-return terminal branches are checked
with no-return analysis to avoid false positives.

## Details

The analysis is updated to recursively check  `if/elif/of/except`
non-terminal branches as well as the terminal branch.

Fixes https://github.com/nim-works/nimskull/issues/1453
---
 compiler/ast/ast_query.nim                    | 53 +++++++++++--------
 tests/lang_exprs/tnoreturn_nested.nim         | 11 ++++
 ...eturn_nested_reject_nonexhaustive_case.nim | 16 ++++++
 ...oreturn_nested_reject_nonexhaustive_if.nim | 14 +++++
 .../tnoreturn_nested_reject_void.nim          | 16 ++++++
 5 files changed, 89 insertions(+), 21 deletions(-)
 create mode 100644 tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_case.nim
 create mode 100644 tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_if.nim
 create mode 100644 tests/lang_exprs/tnoreturn_nested_reject_void.nim

diff --git a/compiler/ast/ast_query.nim b/compiler/ast/ast_query.nim
index ab8cadf396..58fceac534 100644
--- a/compiler/ast/ast_query.nim
+++ b/compiler/ast/ast_query.nim
@@ -694,27 +694,38 @@ proc endsInNoReturn*(n: PNode): bool =
   ## Checks if expression `n` ends in an unstructured exit (raise, return,
   ## etc.) or a call of a noreturn proc. This is meant to be called on a
   ## semmed `n`.
-  var it = n
-  while it.kind in {nkStmtList, nkStmtListExpr} and it.len > 0 or
-        it.kind in {nkIfStmt, nkCaseStmt, nkBlockStmt, nkTryStmt} and it.typ.isEmptyType:
-    case it.kind
-    of nkStmtList, nkStmtListExpr, nkBlockStmt:
-      it = it.lastSon
-    of nkIfStmt, nkCaseStmt:
-      it = it.lastSon.lastSon
-    of nkTryStmt:
-      it =
-        case it[^1].kind
-        of nkFinally:
-          it[^2]
-        of nkExceptBranch:
-          it[^1]
-        of nkAllNodeKinds - {nkFinally, nkExceptBranch}:
-          unreachable()
-    else:
-      unreachable()
-  result = it.kind in nkLastBlockStmts or
-    it.kind in nkCallKinds and it[0].kind == nkSym and sfNoReturn in it[0].sym.flags
+  case n.kind
+  of nkStmtList, nkStmtListExpr:
+    result = n.len > 0 and endsInNoReturn(n[^1])
+  of nkBlockStmt, nkExceptBranch, nkElifBranch, nkElse, nkOfBranch:
+    result = endsInNoReturn(n[^1])
+  of nkIfStmt:
+    for it in n.items:
+      result = endsInNoReturn(it[^1])
+      if not result:
+        break
+    result = result and n[^1].kind == nkElse
+  of nkCaseStmt:
+    # skip the selector expression
+    for i in 1..<n.len:
+      result = endsInNoReturn(n[i])
+      if not result:
+        break
+    let requiresElse = n[0].typ.skipTypes(abstractRange).kind in
+                        {tyFloat..tyFloat64, tyString}
+    result = result and (n[^1].kind == nkElse or not requiresElse)
+  of nkTryStmt:
+    # ignore the 'finally' -- it doesn't contribute to the type
+    for i in 0..<(n.len - ord(n[^1].kind == nkFinally)):
+      result = endsInNoReturn(n[i])
+      if not result:
+        break
+  of nkLastBlockStmts:
+    result = true
+  of nkCallKinds:
+    result = n[0].kind == nkSym and sfNoReturn in n[0].sym.flags
+  else:
+    result = false
 
 type
   NodePosName* = enum
diff --git a/tests/lang_exprs/tnoreturn_nested.nim b/tests/lang_exprs/tnoreturn_nested.nim
index 786ee54d31..19481a0f1a 100644
--- a/tests/lang_exprs/tnoreturn_nested.nim
+++ b/tests/lang_exprs/tnoreturn_nested.nim
@@ -42,6 +42,17 @@ block nested_in_case:
 
   doAssert x is int
 
+  block nested_in_case_range:
+    type Foo = range[1..1]
+    let x =
+      if true:
+        0
+      else:
+        let y: Foo = 1
+        case y
+        of 1:
+          raise newException(CatchableError, "error")
+
 block nested_in_block:
   let x =
     if true:
diff --git a/tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_case.nim b/tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_case.nim
new file mode 100644
index 0000000000..f9565342df
--- /dev/null
+++ b/tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_case.nim
@@ -0,0 +1,16 @@
+discard """
+  description: '''
+    Ensure nested non-noreturn statements aren't treated as such
+  '''
+  errormsg: "expression '1' is of type 'int literal(1)' and has to be used (or discarded)"
+  line: 11
+"""
+
+let x =
+  if true:
+    1
+  else:
+    let y = "hi"
+    case y:
+    of "hi":
+      raise newException(Defect, "gone") # no return
\ No newline at end of file
diff --git a/tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_if.nim b/tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_if.nim
new file mode 100644
index 0000000000..a899fbbc6a
--- /dev/null
+++ b/tests/lang_exprs/tnoreturn_nested_reject_nonexhaustive_if.nim
@@ -0,0 +1,14 @@
+discard """
+  description: '''
+    Ensure nested non-noreturn statements aren't treated as such
+  '''
+  errormsg: "expression '1' is of type 'int literal(1)' and has to be used (or discarded)"
+  line: 11
+"""
+
+let x =
+  if true:
+    1
+  else:
+    if true:
+      raise newException(Defect, "gone") # no return
\ No newline at end of file
diff --git a/tests/lang_exprs/tnoreturn_nested_reject_void.nim b/tests/lang_exprs/tnoreturn_nested_reject_void.nim
new file mode 100644
index 0000000000..8ad6b6adcc
--- /dev/null
+++ b/tests/lang_exprs/tnoreturn_nested_reject_void.nim
@@ -0,0 +1,16 @@
+discard """
+  description: '''
+    Ensure nested non-noreturn statements aren't treated as such
+  '''
+  errormsg: "expression '1' is of type 'int literal(1)' and has to be used (or discarded)"
+  line: 11
+"""
+
+let x =
+  if true:
+    1
+  else:
+    if true:
+      echo "fun" # void
+    else:
+      raise newException(Defect, "gone") # no return
\ No newline at end of file