msgs: prevent downgrading of error handling (#941)

## Summary

Fix the structured report hook being able to downgrade or disable the
error handling as decided by the compiler. This improves stability of
`nimsuggest` when encountering certain errors, as reporting internal
errors or other errors where execution of caller has to stop now behave
as the callsites expect.

## Details

Except for the `doDefault` action, all other actions returned by the
structured report hook overrode the compiler-decided error handling in
`handleReport`. Since `errorActions` wasn't called in those cases, this
also meant that the error counter wasn't incremented properly.

As a consequence of the downgrading, `internalError`, `globalReport`,
and other error reporting that relied on execution of either the
compiler or current procedure to abort could be effectively disabled
through the report hook, which is what `nimsuggest` did, resulting in
execution to continue where it shouldn't.

For the fix, `handleReport` now always computes the error handling
based on the `eh` parameter (via `errorActions`) itself, and then picks
the more severe handling. This means that the structured report hook
can
still "upgrade" error handling to something more severe.

Some follow-up cleanup and refactoring is performed:
* `reportAndFail` is removed, and `checkSonsLen`/`checkMinSonsLen` use
  `globalReport` again
* `internalError`/`internalAssert` now always exit the currently
  executing routine, which is additionally verified by a call to
  `unreachable` after the `handleReport` call, and their
  documentation is updated to reflect that
* `handleReport` is documented

Note that `errorActions` can still downgrade the error handling as
specified by `handleReport`'s parameter, based on the active
configuration. However, fatal errors (see `isCompilerFatal`, which
includes  `rintUnreachable`  and  `rintAssert` ) still always results in
the
compiler/tool aborting.

compiler/front/msgs.nim

@@ -621,24 +621,38 @@ proc handleReport*(
     r: Report,
     reportFrom: InstantiationInfo,
     eh: TErrorHandling = doNothing) {.noinline.} =
+  ## Prepares the report `r` and passes it to the active report hook.
+  ## `eh` is currently only a suggestion, and it is sometimes ignored depending
+  ## on the currently active configuration.
   var rep = r
   fillReportAndHandleVmTrace(conf, rep, reportFrom)
 
-  let
-    userAction = conf.report(rep)
-    (action, trace) =
-      case userAction
-      of doDefault:
-        errorActions(conf, rep, eh)
-      else:
-        (userAction, false)
+  let userAction = conf.report(rep)
+  # ``errorActions`` also increments the error counter, so make sure to always
+  # call it
+  var (action, trace) = errorActions(conf, rep, eh)
 
+  # decide what to do, based on the hook-provided action and the computed
+  # action. The more severe handling out of the two wins
+  case userAction
+  of doAbort:
+    # a hook-requested abort always overrides the computed handling
+    (action, trace) = (doAbort, false)
+  of doRaise:
+    case action
+    of doRaise, doAbort:
+      discard "a hook-requested raise doesn't override an abort"
+    of doNothing, doDefault:
+      (action, trace) = (doRaise, false)
+  of doNothing, doDefault:
+    discard "use the computed strategy"
+
+  # now perform the selected action:
   case action
   of doAbort:   quit(conf, trace)
   of doRaise:   raiseRecoverableError("report")
   of doNothing: discard
-  of doDefault: assert(
-    false,
+  of doDefault: unreachable(
     "Default error handing action must be turned into ignore/raise/abort")
 
 template globalAssert*(
@@ -665,34 +679,6 @@ template globalReport*(conf: ConfigRef, report: ReportTypes) =
   handleReport(
     conf, wrap(report, instLoc()), instLoc(), doRaise)
 
-proc reportAndFail*(
-  conf: ConfigRef, r: Report, reportFrom: InstantiationInfo) =
-  ## Similar to `handleReport`, but, unless overridden with aborting
-  ## (`doAbort`) by the structured report hook, always raises a recoverable
-  ## error.
-  var rep = r
-  fillReportAndHandleVmTrace(conf, rep, reportFrom)
-
-  case conf.report(rep)
-  of doAbort:
-    quit(conf, false)
-  of doDefault:
-    let (action, trace) = errorActions(conf, rep, doRaise)
-    case action
-    of doAbort:
-      quit(conf, trace)
-    of doRaise, doNothing:
-      raiseRecoverableError("report")
-    of doDefault:
-      unreachable()
-  of doRaise, doNothing:
-    raiseRecoverableError("report")
-
-template reportAndFail*(
-  conf: ConfigRef; info: TLineInfo, report: ReportTypes) =
-  reportAndFail(
-    conf, wrap(report, instLoc(), info), instLoc())
-
 template localReport*(conf: ConfigRef; info: TLineInfo, report: ReportTypes) =
   {.line.}:
     handleReport(
@@ -752,19 +738,18 @@ proc doInternalUnreachable*(conf: ConfigRef, info: TLineInfo, msg: string,
         wrap(intRep, instLoc, info)
 
   conf.handleReport(rep, instLoc, doAbort)
+  unreachable("not aborted")
 
 template internalError*(
     conf: ConfigRef,
     info: TLineInfo,
     fail: string,
   ): untyped =
-  ## Causes an internal error; but does not necessarily raise/end the currently
-  ## executing routine.
+  ## Causes an internal error. Always ends the currently executing routine.
   doInternalUnreachable(conf, info, fail, instLoc())
 
 template internalError*(conf: ConfigRef, fail: string): untyped =
-  ## Causes an internal error; but does not necessarily raise/end the currently
-  ## executing routine.
+  ## Causes an internal error. Always ends the currently executing routine.
   doInternalUnreachable(conf, unknownLineInfo, fail, instLoc())
 
 proc doInternalAssert*(conf: ConfigRef,
@@ -782,17 +767,18 @@ proc doInternalAssert*(conf: ConfigRef,
         wrap(intRep, instLoc, info)
 
   conf.handleReport(rep, instLoc, doAbort)
+  unreachable("not aborted")
 
 template internalAssert*(
     conf: ConfigRef, condition: bool, info: TLineInfo, failMsg: string = "") =
-  ## Causes an internal error if the provided condition evaluates to false; but
-  ## does not necessarily raise/end the currently executing routine.
+  ## Causes an internal error if the provided condition evaluates to false.
+  ## Always ends the currently executing routine.
   if not condition:
     doInternalAssert(conf, instLoc(), failMsg, info)
 
 template internalAssert*(conf: ConfigRef, condition: bool, failMsg = "") =
-  ## Causes an internal error if the provided condition evaluates to false; but
-  ## does not necessarily raise/end the currently executing routine.
+  ## Causes an internal error if the provided condition evaluates to false.
+  ## Always ends the currently executing routine.
   if not condition:
     doInternalAssert(conf, instLoc(), failMsg)
 

compiler/sem/semdata.nim

@@ -1065,13 +1065,13 @@ proc markIndirect*(c: PContext, s: PSym) {.inline.} =
 
 proc checkSonsLen*(n: PNode, length: int; conf: ConfigRef) =
   if n.len != length:
-    conf.reportAndFail(n.info, reportAst(
+    conf.globalReport(n.info, reportAst(
       rsemIllformedAst, n,
       str = "Expected $1 elements, but found $2" % [$length, $n.len]))
 
 proc checkMinSonsLen*(n: PNode, length: int; conf: ConfigRef) =
   if n.len < length:
-    conf.reportAndFail(n.info, reportAst(
+    conf.globalReport(n.info, reportAst(
       rsemIllformedAst, n,
       str = "Expected at least $1 elements, but found $2" % [$length, $n.len]))