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>

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:"

compiler/condsyms.nim

@@ -151,3 +151,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasWarnUnnamedBreak")
   defineSymbol("nimHasGenericDefine")
   defineSymbol("nimHasDefineAliases")
+  defineSymbol("nimHasWarnBareExcept")

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)

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]

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`

compiler/nim.cfg

@@ -38,3 +38,7 @@ define:useStdoutAsStdmsg
 @if nimHasWarnUnnamedBreak:
   warningAserror[UnnamedBreak]:on
 @end
+
+@if nimHasWarnBareExcept:
+  warningAserror[BareExcept]:on
+@end

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:

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:

doc/manual.md

@@ -4773,8 +4773,8 @@ Example:
       echo "overflow!"
     except ValueError, IOError:
       echo "catch multiple exceptions!"
-    except:
+    except CatchableError:
-      echo "Unknown exception!"
+      echo "Catchable exception!"
     finally:
       close(f)
   ```
@@ -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.
 
@@ -4806,11 +4803,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 +4815,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
+  ```nim test
-  let x = (try: parseInt("133a") except: -1)
+  from std/strutils import parseInt
+  let x = (try: parseInt("133a") except ValueError: -1)
   ```
 
 
@@ -4867,7 +4865,7 @@ error message from `e`, and for such situations, it is enough to use
   ```nim
   try:
     # ...
-  except:
+  except CatchableError:
     echo getCurrentExceptionMsg()
   ```
 
@@ -5055,7 +5053,7 @@ An empty `raises` list (`raises: []`) means that no exception may be raised:
     try:
       unsafeCall()
       result = true
-    except:
+    except CatchableError:
       result = false
   ```
 

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:
       echo "Unknown exception!"
       # reraise the unknown exception:
       raise
@@ -425,7 +425,7 @@ module. Example:
   ```nim
   try:
     doSomethingHere()
-  except:
+  except CatchableError:
     let
       e = getCurrentException()
       msg = getCurrentExceptionMsg()

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

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)

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)

tests/nimdoc/trunnableexamples.nim

@@ -65,7 +65,7 @@ when true: # issue #12746
     runnableExamples:
       try:
         discard
-      except:
+      except CatchableError:
         # just the general except will work
         discard