From 4bc45bb6bc04d69cbbe2e2a93fb325fcd81a5310 Mon Sep 17 00:00:00 2001
From: SirOlaf <34164198+SirOlaf@users.noreply.github.com>
Date: Thu, 30 Nov 2023 11:01:42 +0100
Subject: [PATCH] Fix endsInNoReturn for case statements (#23009)

While looking at the CI I noticed that there's a couple false positives
for `case` statements that cannot be checked for exhaustiveness since my
changes, this should resolve them.

---------

Co-authored-by: SirOlaf <>
(cherry picked from commit 9140f8e2212c91347704cec0f98c0345ddf0ea1e)
---
 compiler/sem.nim                   | 23 +++++++++++++--
 compiler/semstmts.nim              | 10 ++-----
 tests/controlflow/tunreachable.nim | 45 +++++++++++++++++++++++++++---
 3 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/compiler/sem.nim b/compiler/sem.nim
index 794cf15dc4902..6d38c0b7b1f25 100644
--- a/compiler/sem.nim
+++ b/compiler/sem.nim
@@ -208,6 +208,18 @@ proc commonType*(c: PContext; x, y: PType): PType =
         result = newType(k, nextTypeId(c.idgen), r.owner)
         result.addSonSkipIntLit(r, c.idgen)
 
+const shouldChckCovered = {tyInt..tyInt64, tyChar, tyEnum, tyUInt..tyUInt64, tyBool}
+proc shouldCheckCaseCovered(caseTyp: PType): bool =
+  result = false
+  case caseTyp.kind
+  of shouldChckCovered:
+    result = true
+  of tyRange:
+    if skipTypes(caseTyp[0], abstractInst).kind in shouldChckCovered:
+      result = true
+  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
@@ -237,6 +249,12 @@ proc endsInNoReturn(n: PNode): bool =
     # 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:
@@ -246,11 +264,12 @@ proc endsInNoReturn(n: PNode): bool =
         of nkElifBranch:
           branch[1]
         of nkElse:
+          hasElse = true
           branch[0]
         else:
           raiseAssert "Malformed `case` statement in endsInNoReturn"
-    # none of the branches returned
-    result = true
+    # 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:
diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim
index 1dec24e5166f9..0783bdf052101 100644
--- a/compiler/semstmts.nim
+++ b/compiler/semstmts.nim
@@ -1135,20 +1135,14 @@ proc semCase(c: PContext, n: PNode; flags: TExprFlags; expectedType: PType = nil
   openScope(c)
   pushCaseContext(c, n)
   n[0] = semExprWithType(c, n[0])
-  var chckCovered = false
   var covered: Int128 = toInt128(0)
   var typ = commonTypeBegin
   var expectedType = expectedType
   var hasElse = false
   let caseTyp = skipTypes(n[0].typ, abstractVar-{tyTypeDesc})
-  const shouldChckCovered = {tyInt..tyInt64, tyChar, tyEnum, tyUInt..tyUInt64, tyBool}
+  var chckCovered = caseTyp.shouldCheckCaseCovered()
   case caseTyp.kind
-  of shouldChckCovered:
-    chckCovered = true
-  of tyRange:
-    if skipTypes(caseTyp[0], abstractInst).kind in shouldChckCovered:
-      chckCovered = true
-  of tyFloat..tyFloat128, tyString, tyCstring, tyError:
+  of tyFloat..tyFloat128, tyString, tyCstring, tyError, shouldChckCovered, tyRange:
     discard
   else:
     popCaseContext(c)
diff --git a/tests/controlflow/tunreachable.nim b/tests/controlflow/tunreachable.nim
index 64e199e17f010..06321ce8a9855 100644
--- a/tests/controlflow/tunreachable.nim
+++ b/tests/controlflow/tunreachable.nim
@@ -2,9 +2,11 @@ discard """
   cmd: "nim check --warningAsError:UnreachableCode $file"
   action: "reject"
   nimout: '''
-tunreachable.nim(24, 3) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
-tunreachable.nim(31, 3) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
-tunreachable.nim(40, 3) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
+tunreachable.nim(26, 3) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
+tunreachable.nim(33, 3) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
+tunreachable.nim(42, 3) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
+tunreachable.nim(65, 5) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
+tunreachable.nim(77, 5) Error: unreachable code after 'return' statement or '{.noReturn.}' proc [UnreachableCode]
 '''
 """
   
@@ -39,4 +41,39 @@ proc main3() =
     return
   echo "after"
 
-main3()
\ No newline at end of file
+main3()
+
+
+block:
+  # Cases like strings are not checked for exhaustiveness unless they have an else
+  proc main4(x: string) =
+    case x
+    of "a":
+      return
+    # reachable
+    echo "after"
+
+  main4("a")
+
+  proc main5(x: string) =
+    case x
+    of "a":
+      return
+    else:
+      return
+    # unreachable
+    echo "after"
+
+  main5("a")
+
+block:
+  # In this case no else is needed because it's exhaustive
+  proc exhaustive(x: bool) =
+    case x
+    of true:
+      return
+    of false:
+      return
+    echo "after"
+
+  exhaustive(true)