From adda4f985f765833b194f7bdc0f73db9fed56253 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Wed, 14 Dec 2022 22:43:58 +0800
Subject: [PATCH 1/8] fix #19580; add warning for bare except: clause

---
 changelog.md                       |  2 ++
 compiler/condsyms.nim              |  1 +
 compiler/docgen.nim                |  8 ++++----
 compiler/extccomp.nim              |  4 ++--
 compiler/lineinfos.nim             |  2 ++
 compiler/nim.cfg                   |  4 ++++
 compiler/semstmts.nim              |  5 ++++-
 compiler/types.nim                 | 12 ++++++++++++
 doc/manual.md                      | 19 +++++++++++--------
 doc/tut2.md                        |  4 ++--
 lib/std/assertions.nim             |  2 ++
 tests/nimdoc/trunnableexamples.nim |  2 +-
 12 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/changelog.md b/changelog.md
index 0181d03d2c87b..d312917bbeef2 100644
--- a/changelog.md
+++ b/changelog.md
@@ -133,6 +133,8 @@
   `foo` had type `proc ()` were assumed by the compiler to mean `foo(a, b, proc () = ...)`.
   This behavior is now deprecated. Use `foo(a, b) do (): ...` or `foo(a, b, proc () = ...)` instead.
 
+- If no exception or any exception deriving from Exception but not Defect or CatchableError given in except, a `warnBareExcept`warning will be triggered.
+
 ## Standard library additions and changes
 
 [//]: # "Changes:"
diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim
index 41b4c83aaa7e0..9d081ef0a9693 100644
--- a/compiler/condsyms.nim
+++ b/compiler/condsyms.nim
@@ -151,3 +151,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasWarnUnnamedBreak")
   defineSymbol("nimHasGenericDefine")
   defineSymbol("nimHasDefineAliases")
+  defineSymbol("nimHasWarnBareExcept")
diff --git a/compiler/docgen.nim b/compiler/docgen.nim
index fc37bd596e313..f8a804cec9cb3 100644
--- a/compiler/docgen.nim
+++ b/compiler/docgen.nim
@@ -1746,7 +1746,7 @@ proc commandJson*(cache: IdentCache, conf: ConfigRef) =
     let filename = getOutFile(conf, RelativeFile conf.projectName, JsonExt)
     try:
       writeFile(filename, content)
-    except:
+    except IOError:
       rawMessage(conf, errCannotOpenFile, filename.string)
 
 proc commandTags*(cache: IdentCache, conf: ConfigRef) =
@@ -1768,7 +1768,7 @@ proc commandTags*(cache: IdentCache, conf: ConfigRef) =
     let filename = getOutFile(conf, RelativeFile conf.projectName, TagsExt)
     try:
       writeFile(filename, content)
-    except:
+    except IOError:
       rawMessage(conf, errCannotOpenFile, filename.string)
 
 proc commandBuildIndex*(conf: ConfigRef, dir: string, outFile = RelativeFile"") =
@@ -1789,7 +1789,7 @@ proc commandBuildIndex*(conf: ConfigRef, dir: string, outFile = RelativeFile"")
 
   try:
     writeFile(filename, code)
-  except:
+  except IOError:
     rawMessage(conf, errCannotOpenFile, filename.string)
 
 proc commandBuildIndexJson*(conf: ConfigRef, dir: string, outFile = RelativeFile"") =
@@ -1803,5 +1803,5 @@ proc commandBuildIndexJson*(conf: ConfigRef, dir: string, outFile = RelativeFile
 
   try:
     writeFile(filename, $body)
-  except:
+  except IOError:
     rawMessage(conf, errCannotOpenFile, filename.string)
diff --git a/compiler/extccomp.nim b/compiler/extccomp.nim
index 256e02b223094..66de46556faa5 100644
--- a/compiler/extccomp.nim
+++ b/compiler/extccomp.nim
@@ -528,7 +528,7 @@ proc ccHasSaneOverflow*(conf: ConfigRef): bool =
     var exe = getConfigVar(conf, conf.cCompiler, ".exe")
     if exe.len == 0: exe = CC[conf.cCompiler].compilerExe
     # NOTE: should we need the full version, use -dumpfullversion
-    let (s, exitCode) = try: execCmdEx(exe & " -dumpversion") except: ("", 1)
+    let (s, exitCode) = try: execCmdEx(exe & " -dumpversion") except IOError, OSError, ValueError: ("", 1)
     if exitCode == 0:
       var major: int
       discard parseInt(s, major)
@@ -1018,7 +1018,7 @@ proc changeDetectedViaJsonBuildInstructions*(conf: ConfigRef; jsonFile: Absolute
 proc runJsonBuildInstructions*(conf: ConfigRef; jsonFile: AbsoluteFile) =
   var bcache: BuildCache
   try: bcache.fromJson(jsonFile.string.parseFile)
-  except:
+  except ValueError, KeyError, JsonKindError:
     let e = getCurrentException()
     conf.quitOrRaise "\ncaught exception:\n$#\nstacktrace:\n$#error evaluating JSON file: $#" %
       [e.msg, e.getStackTrace(), jsonFile.string]
diff --git a/compiler/lineinfos.nim b/compiler/lineinfos.nim
index ab9c902c3b652..cb60d8949ef15 100644
--- a/compiler/lineinfos.nim
+++ b/compiler/lineinfos.nim
@@ -86,6 +86,7 @@ type
     warnImplicitTemplateRedefinition = "ImplicitTemplateRedefinition",
     warnUnnamedBreak = "UnnamedBreak",
     warnStmtListLambda = "StmtListLambda",
+    warnBareExcept = "BareExcept",
     warnUser = "User",
     # hints
     hintSuccess = "Success", hintSuccessX = "SuccessX",
@@ -185,6 +186,7 @@ const
     warnImplicitTemplateRedefinition: "template '$1' is implicitly redefined; this is deprecated, add an explicit .redefine pragma",
     warnUnnamedBreak: "Using an unnamed break in a block is deprecated; Use a named block with a named break instead",
     warnStmtListLambda: "statement list expression assumed to be anonymous proc; this is deprecated, use `do (): ...` or `proc () = ...` instead",
+    warnBareExcept: "$1",
     warnUser: "$1",
     hintSuccess: "operation successful: $#",
     # keep in sync with `testament.isSuccess`
diff --git a/compiler/nim.cfg b/compiler/nim.cfg
index 20ce3810f2646..96b47b0e612a2 100644
--- a/compiler/nim.cfg
+++ b/compiler/nim.cfg
@@ -38,3 +38,7 @@ define:useStdoutAsStdmsg
 @if nimHasWarnUnnamedBreak:
   warningAserror[UnnamedBreak]:on
 @end
+
+@if nimHasWarnBareExcept:
+  warningAserror[BareExcept]:on
+@end
diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim
index d19d75611a015..d0843c417f87d 100644
--- a/compiler/semstmts.nim
+++ b/compiler/semstmts.nim
@@ -210,6 +210,8 @@ proc semTry(c: PContext, n: PNode; flags: TExprFlags; expectedType: PType = nil)
       isImported = true
     elif not isException(typ):
       localError(c.config, typeNode.info, errExprCannotBeRaised)
+    elif not isDefectOrCatchableError(typ):
+      message(c.config, a.info, warnBareExcept, "catch a more precise Exception deriving from CatchableError or Defect.")
 
     if containsOrIncl(check, typ.id):
       localError(c.config, typeNode.info, errExceptionAlreadyHandled)
@@ -251,7 +253,8 @@ proc semTry(c: PContext, n: PNode; flags: TExprFlags; expectedType: PType = nil)
       elif a.len == 1:
         # count number of ``except: body`` blocks
         inc catchAllExcepts
-
+        message(c.config, a.info, warnBareExcept,
+                  "The bare except clause is deprecated; use `except CatchableError, Defect:` instead")
       else:
         # support ``except KeyError, ValueError, ... : body``
         if catchAllExcepts > 0:
diff --git a/compiler/types.nim b/compiler/types.nim
index 4bbbaf13c941a..457568e327dd4 100644
--- a/compiler/types.nim
+++ b/compiler/types.nim
@@ -1721,6 +1721,18 @@ proc isDefectException*(t: PType): bool =
     t = skipTypes(t[0], abstractPtrs)
   return false
 
+proc isDefectOrCatchableError*(t: PType): bool =
+  var t = t.skipTypes(abstractPtrs)
+  while t.kind == tyObject:
+    if t.sym != nil and t.sym.owner != nil and
+        sfSystemModule in t.sym.owner.flags and
+        (t.sym.name.s == "Defect" or
+        t.sym.name.s == "CatchableError"):
+      return true
+    if t[0] == nil: break
+    t = skipTypes(t[0], abstractPtrs)
+  return false
+
 proc isSinkTypeForParam*(t: PType): bool =
   # a parameter like 'seq[owned T]' must not be used only once, but its
   # elements must, so we detect this case here:
diff --git a/doc/manual.md b/doc/manual.md
index 0a295fdb51310..04d4c4b2c853a 100644
--- a/doc/manual.md
+++ b/doc/manual.md
@@ -4773,8 +4773,10 @@ Example:
       echo "overflow!"
     except ValueError, IOError:
       echo "catch multiple exceptions!"
-    except:
-      echo "Unknown exception!"
+    except CatchableError:
+      echo "Catchable exception!"
+    except Defect:
+      echo "Defect!"
     finally:
       close(f)
   ```
@@ -4806,11 +4808,11 @@ Try can also be used as an expression; the type of the `try` branch then
 needs to fit the types of `except` branches, but the type of the `finally`
 branch always has to be `void`:
 
-  ```nim
+  ```nim test
   from std/strutils import parseInt
 
   let x = try: parseInt("133a")
-          except: -1
+          except ValueError: -1
           finally: echo "hi"
   ```
 
@@ -4818,8 +4820,9 @@ branch always has to be `void`:
 To prevent confusing code there is a parsing limitation; if the `try`
 follows a `(` it has to be written as a one liner:
 
-  ```nim
-  let x = (try: parseInt("133a") except: -1)
+  ```nim test
+  from std/strutils import parseInt
+  let x = (try: parseInt("133a") except ValueError: -1)
   ```
 
 
@@ -4867,7 +4870,7 @@ error message from `e`, and for such situations, it is enough to use
   ```nim
   try:
     # ...
-  except:
+  except CatchableError, Defect:
     echo getCurrentExceptionMsg()
   ```
 
@@ -5055,7 +5058,7 @@ An empty `raises` list (`raises: []`) means that no exception may be raised:
     try:
       unsafeCall()
       result = true
-    except:
+    except CatchableError, Defect:
       result = false
   ```
 
diff --git a/doc/tut2.md b/doc/tut2.md
index 3c858c64e1cc6..f15d009a1204c 100644
--- a/doc/tut2.md
+++ b/doc/tut2.md
@@ -393,7 +393,7 @@ The `try` statement handles exceptions:
       echo "could not convert string to integer"
     except IOError:
       echo "IO error!"
-    except:
+    except CatchableError, Defect:
       echo "Unknown exception!"
       # reraise the unknown exception:
       raise
@@ -425,7 +425,7 @@ module. Example:
   ```nim
   try:
     doSomethingHere()
-  except:
+  except CatchableError, Defect:
     let
       e = getCurrentException()
       msg = getCurrentExceptionMsg()
diff --git a/lib/std/assertions.nim b/lib/std/assertions.nim
index 03bab1b1b014d..5623ff8efc8c4 100644
--- a/lib/std/assertions.nim
+++ b/lib/std/assertions.nim
@@ -98,6 +98,7 @@ template doAssertRaises*(exception: typedesc, code: untyped) =
   const begin = "expected raising '" & astToStr(exception) & "', instead"
   const msgEnd = " by: " & astToStr(code)
   template raisedForeign {.gensym.} = raiseAssert(begin & " raised foreign exception" & msgEnd)
+  {.warning[BareExcept]:off.}
   when Exception is exception:
     try:
       if true:
@@ -116,5 +117,6 @@ template doAssertRaises*(exception: typedesc, code: untyped) =
       mixin `$` # alternatively, we could define $cstring in this module
       raiseAssert(begin & " raised '" & $e.name & "'" & msgEnd)
     except: raisedForeign()
+  {.warning[BareExcept]:on.}
   if wrong:
     raiseAssert(begin & " nothing was raised" & msgEnd)
diff --git a/tests/nimdoc/trunnableexamples.nim b/tests/nimdoc/trunnableexamples.nim
index ac7a0e26f191d..4997aff17a528 100644
--- a/tests/nimdoc/trunnableexamples.nim
+++ b/tests/nimdoc/trunnableexamples.nim
@@ -65,7 +65,7 @@ when true: # issue #12746
     runnableExamples:
       try:
         discard
-      except:
+      except CatchableError, Defect:
         # just the general except will work
         discard
 

From 8fd5474e7dd3d44a8f7bcc7edcf6559e5d48a37e Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Wed, 14 Dec 2022 22:51:20 +0800
Subject: [PATCH 2/8] fixes some easy ones

---
 lib/packages/docutils/rst.nim | 6 +++---
 lib/pure/htmlparser.nim       | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/packages/docutils/rst.nim b/lib/packages/docutils/rst.nim
index f81be7a50e613..e9f76a26fbc46 100644
--- a/lib/packages/docutils/rst.nim
+++ b/lib/packages/docutils/rst.nim
@@ -1610,7 +1610,7 @@ proc getFootnoteType(label: PRstNode): (FootnoteType, int) =
   elif label.len == 1 and label.sons[0].kind == rnLeaf:
     try:
       result = (fnManualNumber, parseInt(label.sons[0].text))
-    except:
+    except ValueError:
       result = (fnCitation, -1)
   else:
     result = (fnCitation, -1)
@@ -2899,13 +2899,13 @@ proc parseEnumList(p: var RstParser): PRstNode =
       let enumerator = p.tok[p.idx + 1 + wildIndex[w]].symbol
       # check that it's in sequence: enumerator == next(prevEnum)
       if "n" in wildcards[w]:  # arabic numeral
-        let prevEnumI = try: parseInt(prevEnum) except: 1
+        let prevEnumI = try: parseInt(prevEnum) except ValueError: 1
         if enumerator in autoEnums:
           if prevAE != "" and enumerator != prevAE:
             break
           prevAE = enumerator
           curEnum = prevEnumI + 1
-        else: curEnum = (try: parseInt(enumerator) except: 1)
+        else: curEnum = (try: parseInt(enumerator) except ValueError: 1)
         if curEnum - prevEnumI != 1:
           break
         prevEnum = enumerator
diff --git a/lib/pure/htmlparser.nim b/lib/pure/htmlparser.nim
index 6b1300f11bf17..24eab3abb9518 100644
--- a/lib/pure/htmlparser.nim
+++ b/lib/pure/htmlparser.nim
@@ -394,10 +394,10 @@ proc entityToRune*(entity: string): Rune =
     case entity[1]
     of '0'..'9':
       try: runeValue = parseInt(entity[1..^1])
-      except: discard
+      except ValueError: discard
     of 'x', 'X': # not case sensitive here
       try: runeValue = parseHexInt(entity[2..^1])
-      except: discard
+      except ValueError: discard
     else: discard # other entities are not defined with prefix `#`
     if runeValue notin 0..0x10FFFF: runeValue = 0 # only return legal values
     return Rune(runeValue)

From b1e2fee4bcd69478ca8421631f75e9d1de84dd7a Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Wed, 14 Dec 2022 22:52:38 +0800
Subject: [PATCH 3/8] Update doc/manual.md

---
 doc/manual.md | 2 --
 1 file changed, 2 deletions(-)

diff --git a/doc/manual.md b/doc/manual.md
index 04d4c4b2c853a..85b2d97220e2f 100644
--- a/doc/manual.md
+++ b/doc/manual.md
@@ -4775,8 +4775,6 @@ Example:
       echo "catch multiple exceptions!"
     except CatchableError:
       echo "Catchable exception!"
-    except Defect:
-      echo "Defect!"
     finally:
       close(f)
   ```

From eda7c431a1d4d14fe0859b23051f755c2ad554c6 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Wed, 14 Dec 2022 23:13:19 +0800
Subject: [PATCH 4/8] fixes docs

---
 doc/manual.md | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/doc/manual.md b/doc/manual.md
index 85b2d97220e2f..779e991866746 100644
--- a/doc/manual.md
+++ b/doc/manual.md
@@ -4786,9 +4786,6 @@ listed in an `except` clause, the corresponding statements are executed.
 The statements following the `except` clauses are called
 `exception handlers`:idx:.
 
-The empty `except`:idx: clause is executed if there is an exception that is
-not listed otherwise. It is similar to an `else` clause in `if` statements.
-
 If there is a `finally`:idx: clause, it is always executed after the
 exception handlers.
 
@@ -4868,7 +4865,7 @@ error message from `e`, and for such situations, it is enough to use
   ```nim
   try:
     # ...
-  except CatchableError, Defect:
+  except CatchableError:
     echo getCurrentExceptionMsg()
   ```
 
@@ -5056,7 +5053,7 @@ An empty `raises` list (`raises: []`) means that no exception may be raised:
     try:
       unsafeCall()
       result = true
-    except CatchableError, Defect:
+    except CatchableError:
       result = false
   ```
 

From 133ace8926960258cb1665b36dba2b02127b3328 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Wed, 14 Dec 2022 23:16:25 +0800
Subject: [PATCH 5/8] Update changelog.md

---
 changelog.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelog.md b/changelog.md
index d312917bbeef2..00bdb50c2e8a8 100644
--- a/changelog.md
+++ b/changelog.md
@@ -133,7 +133,7 @@
   `foo` had type `proc ()` were assumed by the compiler to mean `foo(a, b, proc () = ...)`.
   This behavior is now deprecated. Use `foo(a, b) do (): ...` or `foo(a, b, proc () = ...)` instead.
 
-- If no exception or any exception deriving from Exception but not Defect or CatchableError given in except, a `warnBareExcept`warning will be triggered.
+- If no exception or any exception deriving from Exception but not Defect or CatchableError given in except, a `warnBareExcept` warning will be triggered.
 
 ## Standard library additions and changes
 

From 8fc3a7b6b05b31dbba52f62f07ae44cce9f0b489 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Wed, 14 Dec 2022 23:20:05 +0800
Subject: [PATCH 6/8] addition

---
 doc/manual.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/manual.md b/doc/manual.md
index 779e991866746..f92ec67e2ed00 100644
--- a/doc/manual.md
+++ b/doc/manual.md
@@ -4773,7 +4773,7 @@ Example:
       echo "overflow!"
     except ValueError, IOError:
       echo "catch multiple exceptions!"
-    except CatchableError:
+    except CatchableError, Defect:
       echo "Catchable exception!"
     finally:
       close(f)

From 7159f22f8d590beebef84eb550f4c0a76e5cd457 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Thu, 15 Dec 2022 00:24:27 +0800
Subject: [PATCH 7/8] Apply suggestions from code review

Co-authored-by: Jacek Sieka <arnetheduck@gmail.com>
---
 compiler/semstmts.nim              | 2 +-
 doc/manual.md                      | 2 +-
 doc/tut2.md                        | 2 +-
 tests/nimdoc/trunnableexamples.nim | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim
index d0843c417f87d..a7a02d7fa65ae 100644
--- a/compiler/semstmts.nim
+++ b/compiler/semstmts.nim
@@ -254,7 +254,7 @@ proc semTry(c: PContext, n: PNode; flags: TExprFlags; expectedType: PType = nil)
         # count number of ``except: body`` blocks
         inc catchAllExcepts
         message(c.config, a.info, warnBareExcept,
-                  "The bare except clause is deprecated; use `except CatchableError, Defect:` instead")
+                  "The bare except clause is deprecated; use `except CatchableError:` instead")
       else:
         # support ``except KeyError, ValueError, ... : body``
         if catchAllExcepts > 0:
diff --git a/doc/manual.md b/doc/manual.md
index f92ec67e2ed00..779e991866746 100644
--- a/doc/manual.md
+++ b/doc/manual.md
@@ -4773,7 +4773,7 @@ Example:
       echo "overflow!"
     except ValueError, IOError:
       echo "catch multiple exceptions!"
-    except CatchableError, Defect:
+    except CatchableError:
       echo "Catchable exception!"
     finally:
       close(f)
diff --git a/doc/tut2.md b/doc/tut2.md
index f15d009a1204c..016dc60f6416e 100644
--- a/doc/tut2.md
+++ b/doc/tut2.md
@@ -393,7 +393,7 @@ The `try` statement handles exceptions:
       echo "could not convert string to integer"
     except IOError:
       echo "IO error!"
-    except CatchableError, Defect:
+    except CatchableError:
       echo "Unknown exception!"
       # reraise the unknown exception:
       raise
diff --git a/tests/nimdoc/trunnableexamples.nim b/tests/nimdoc/trunnableexamples.nim
index 4997aff17a528..c88d8fa3f0b9c 100644
--- a/tests/nimdoc/trunnableexamples.nim
+++ b/tests/nimdoc/trunnableexamples.nim
@@ -65,7 +65,7 @@ when true: # issue #12746
     runnableExamples:
       try:
         discard
-      except CatchableError, Defect:
+      except CatchableError:
         # just the general except will work
         discard
 

From 5e538b5d0488b2b5ec9f74b5ba601dd7d57bc293 Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Thu, 15 Dec 2022 10:15:04 +0800
Subject: [PATCH 8/8] Update doc/tut2.md

---
 doc/tut2.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/tut2.md b/doc/tut2.md
index 016dc60f6416e..4b90490826f24 100644
--- a/doc/tut2.md
+++ b/doc/tut2.md
@@ -425,7 +425,7 @@ module. Example:
   ```nim
   try:
     doSomethingHere()
-  except CatchableError, Defect:
+  except CatchableError:
     let
       e = getCurrentException()
       msg = getCurrentExceptionMsg()