fix #19580; add warning for bare except: clause (#21099)

* fix #19580; add warning for bare except: clause

* fixes some easy ones

* Update doc/manual.md

* fixes docs

* Update changelog.md

* addition

* Apply suggestions from code review

Co-authored-by: Jacek Sieka <arnetheduck@gmail.com>

* Update doc/tut2.md

Co-authored-by: Jacek Sieka <arnetheduck@gmail.com>

7 files changed, 29 insertions, 7 deletions

diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim
index 41b4c83aa..9d081ef0a 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 fc37bd596..f8a804cec 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 256e02b22..66de46556 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 ab9c902c3..cb60d8949 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 20ce3810f..96b47b0e6 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 d19d75611..a7a02d7fa 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:` instead")
       else:
         # support ``except KeyError, ValueError, ... : body``
         if catchAllExcepts > 0:
diff --git a/compiler/types.nim b/compiler/types.nim
index 4bbbaf13c..457568e32 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: