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>
(cherry picked from commit 91ce8c3)

Author: 2 people

changelog.md

@@ -24,6 +24,33 @@
   This warning will become an error in future versions! Use a `cast` operation
   like `cast[cstring](x)` instead.
 
+- `logging` will default to flushing all log level messages. To get the legacy behaviour of only flushing Error and Fatal messages, use `-d:nimV1LogFlushBehavior`.
+
+- Object fields now support default values, see https://nim-lang.github.io/Nim/manual.html#types-default-values-for-object-fields for details.
+
+- Redefining templates with the same signature was previously
+  allowed to support certain macro code. To do this explicitly, the
+  `{.redefine.}` pragma has been added. Note that this is only for templates.
+  Implicit redefinition of templates is now deprecated and will give an error in the future.
+
+- Using an unnamed break in a block is deprecated. This warning will become an error in future versions! Use a named block with a named break instead.
+
+- Several Standard libraries are moved to nimble packages, use `nimble` to install them:
+  - `std/punycode` => `punycode`
+  - `std/asyncftpclient` => `asyncftpclient`
+  - `std/smtp` => `smtp`
+  - `std/db_common` => `db_connector/db_common`
+  - `std/db_sqlite` => `db_connector/db_sqlite`
+  - `std/db_mysql` => `db_connector/db_mysql`
+  - `std/db_postgres` => `db_connector/db_postgres`
+  - `std/db_odbc` => `db_connector/db_odbc`
+
+- Previously, calls like `foo(a, b): ...` or `foo(a, b) do: ...` where the final argument of
+  `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
 
 - `macros.parseExpr` and `macros.parseStmt` now accept an optional

compiler/condsyms.nim

@@ -140,3 +140,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasEffectsOf")
 
   defineSymbol("nimHasEnforceNoRaises")
+  defineSymbol("nimHasWarnBareExcept")

compiler/docgen.nim

@@ -1536,7 +1536,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) =
@@ -1559,7 +1559,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"") =
@@ -1580,5 +1580,5 @@ proc commandBuildIndex*(conf: ConfigRef, dir: string, outFile = RelativeFile"")
 
   try:
     writeFile(filename, code)
-  except:
+  except IOError:
     rawMessage(conf, errCannotOpenFile, filename.string)

compiler/extccomp.nim

@@ -517,7 +517,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)
@@ -1006,7 +1006,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

@@ -78,6 +78,7 @@ type
     warnCstringConv = "CStringConv",
     warnPtrToCstringConv = "PtrToCstringConv",
     warnEffect = "Effect",
+    warnBareExcept = "BareExcept",
     warnUser = "User",
     # hints
     hintSuccess = "Success", hintSuccessX = "SuccessX",
@@ -169,6 +170,7 @@ const
     warnCstringConv: "$1",
     warnPtrToCstringConv: "unsafe conversion to 'cstring' from '$1'; this will become a compile time error in the future",
     warnEffect: "$1",
+    warnBareExcept: "$1",
     warnUser: "$1",
     hintSuccess: "operation successful: $#",
     # keep in sync with `testament.isSuccess`

compiler/nim.cfg

@@ -31,3 +31,7 @@ define:useStdoutAsStdmsg
   experimental:strictEffects
   warningAsError:Effect:on
 @end
+
+@if nimHasWarnBareExcept:
+  warningAserror[BareExcept]:on
+@end

compiler/semstmts.nim

@@ -191,6 +191,8 @@ proc semTry(c: PContext, n: PNode; flags: TExprFlags): PNode =
       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)
@@ -230,7 +232,8 @@ proc semTry(c: PContext, n: PNode; flags: TExprFlags): PNode =
       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

@@ -1666,6 +1666,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.rst

@@ -4506,8 +4506,8 @@ Example:
       echo "overflow!"
     except ValueError, IOError:
       echo "catch multiple exceptions!"
-    except:
-      echo "Unknown exception!"
+    except CatchableError:
+      echo "Catchable exception!"
     finally:
       close(f)
 
@@ -4518,9 +4518,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.
 
@@ -4538,19 +4535,21 @@ 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`:
 
-.. code-block:: nim
+  ```nim test
   from std/strutils import parseInt
 
   let x = try: parseInt("133a")
-          except: -1
+          except ValueError: -1
           finally: echo "hi"
 
 
 To prevent confusing code there is a parsing limitation; if the `try`
 follows a `(` it has to be written as a one liner:
 
-.. code-block:: nim
-  let x = (try: parseInt("133a") except: -1)
+  ```nim test
+  from std/strutils import parseInt
+  let x = (try: parseInt("133a") except ValueError: -1)
+  ```
 
 
 Except clauses
@@ -4594,7 +4593,7 @@ error message from `e`, and for such situations, it is enough to use
 .. code-block:: nim
   try:
     # ...
-  except:
+  except CatchableError:
     echo getCurrentExceptionMsg()
 
 Custom exceptions
@@ -4784,7 +4783,7 @@ An empty `raises` list (`raises: []`) means that no exception may be raised:
     try:
       unsafeCall()
       result = true
-    except:
+    except CatchableError:
       result = false
 
 

doc/tut2.rst

@@ -395,7 +395,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
@@ -426,7 +426,7 @@ module. Example:
 .. code-block:: nim
   try:
     doSomethingHere()
-  except:
+  except CatchableError:
     let
       e = getCurrentException()
       msg = getCurrentExceptionMsg()

lib/packages/docutils/rst.nim

@@ -1513,7 +1513,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)
@@ -2420,13 +2420,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

@@ -391,10 +391,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/system/assertions.nim

@@ -93,6 +93,7 @@ template doAssertRaises*(exception: typedesc, code: untyped) =
   const begin = "expected raising '" & astToStr(exception) & "', instead"
   const msgEnd = " by: " & astToStr(code)
   template raisedForeign = raiseAssert(begin & " raised foreign exception" & msgEnd)
+  {.warning[BareExcept]:off.}
   when Exception is exception:
     try:
       if true:
@@ -111,5 +112,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