msgs: more decision making in handleReport

compiler/front/cli_reporter.nim

@@ -3981,17 +3981,8 @@ proc reportHook*(conf: ConfigRef, r: Report): TErrorHandling =
   ## `reportBody` for report, which then calls respective (for each report
   ## category) `reportBody` overloads defined above
   assertKind r
-  let wkind = conf.writabilityKind(r)
-  # debug reports can be both enabled and force enabled. So adding a case here
-  # is not really useful, since report writability kind does not necessarily
-  # dictate how it is written, just whether it can/must/cannot be written.
-  # xxx: the above convoluted comment brought to you by _glaring_ design flaws!
-  if wkind == writeDisabled:
-    return
-  else:
-    if wkind == writeForceEnabled:
-      echo conf.reportFull(r)
-    elif r.kind == rsemProcessing and conf.hintProcessingDots:
+  if true:
+    if r.kind == rsemProcessing and conf.hintProcessingDots:
       # xxx: the report hook is handling processing dots output, why? this whole
       #      infrastructure is overwrought. seriously, they're not hints, they're
       #      progress indicators.

compiler/front/msgs.nim

@@ -609,7 +609,6 @@ proc report*(conf: ConfigRef, node: PNode): TErrorHandling =
 
 proc fillReportAndHandleVmTrace(c: ConfigRef, r: var Report,
                                 reportFrom: InstantiationInfo) =
-  r.reportFrom = toReportLineInfo(reportFrom)
   if r.category in { repSem, repVM } and r.location.isSome():
     r.context = c.getContext(r.location.get())
 
@@ -621,13 +620,38 @@ proc handleReport*(
     r: Report,
     reportFrom: InstantiationInfo,
     eh: TErrorHandling = doNothing) {.noinline.} =
-  ## Prepares the report `r` and passes it to the active report hook.
+  ## Takes the report `r` and handles it. If the report is "enabled" according
+  ## to the active configuration, it is passed to the active report hook and,
+  ## if the report corresponds to an error, error handling is performed.
   ## `eh` is currently only a suggestion, and it is sometimes ignored depending
   ## on the currently active configuration.
   var rep = r
-  fillReportAndHandleVmTrace(conf, rep, reportFrom)
+  rep.reportFrom = toReportLineInfo(reportFrom)
+
+  if not conf.isEnabled(rep):
+    # the report is disabled -> neither invoke the report hook nor perform
+    # error handling
+    return
+
+  var userAction = doNothing
+  case writabilityKind(conf, rep)
+  of writeDisabled:
+    discard "don't invoke the hook"
+  of writeEnabled:
+    # go through the report hook
+    fillReportAndHandleVmTrace(conf, rep, reportFrom)
+    userAction = conf.report(rep)
+  of writeForceEnabled:
+    # also go through the report hook, but temporarily override ``writeln``
+    # with something that always echoes something
+    fillReportAndHandleVmTrace(conf, rep, reportFrom)
+    let oldHook = conf.writelnHook
+    conf.writelnHook = proc (conf: ConfigRef, msg: string, flags: MsgFlags) =
+      echo msg
+
+    userAction = conf.report(rep)
+    conf.writelnHook = oldHook
 
-  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)

compiler/front/options.nim

@@ -294,6 +294,9 @@ type
     ## textual output from the compiler goes through this callback.
     writeHook*: proc(conf: ConfigRef, output: string, flags: MsgFlags) {.closure.}
     structuredReportHook*: ReportHook
+      ## callback that is invoked when an enabled report is passed to report
+      ## handling. The callback is meant to handle rendering/displaying of
+      ## the report
     astDiagToLegacyReport*: proc(conf: ConfigRef, d: PAstDiag): Report
     setMsgFormat*: proc(config: ConfigRef, fmt: MsgFormatKind) {.closure.}
       ## callback that sets the message format for legacy reporting, needs to
@@ -706,26 +709,16 @@ func writabilityKind*(conf: ConfigRef, r: Report): ReportWritabilityKind =
     ## evaluation` context. `sem` and `semexprs` in particular will clear
     ## `conf.m.errorOutputs` as a signal for this. For more details see the
     ## comment for `MsgConfig.errorOutputs`.
-  if (
-    (conf.isEnabled(r) and r.category == repDebug and compTimeCtx)
+  if r.category == repDebug and compTimeCtx:
     # Force write of the report messages using regular stdout if compTimeCtx
     # is enabled
-  ):
-    return writeForceEnabled
-
-  elif (
-    # Not explicitly enabled
-    not conf.isEnabled(r)
-  ) or (
+    writeForceEnabled
+  elif compTimeCtx:
     # Or we are in the special hack mode for `compiles()` processing
-    compTimeCtx
-  ):
-
     # Return without writing
-    return writeDisabled
-
+    writeDisabled
   else:
-    return writeEnabled
+    writeEnabled
 
 const
   oldExperimentalFeatures* = {dotOperators, callOperator}

compiler/front/sexp_reporter.nim

@@ -138,11 +138,7 @@ proc sexp*(t: PSym): SexpNode =
 
 proc reportHook*(conf: ConfigRef, r: Report): TErrorHandling =
   writeConf = conf
-  let wkind = conf.writabilityKind(r)
-
-  if wkind == writeDisabled:
-    return
-  else:
+  if true:
     var s = newSList()
     s.add newSSymbol(multiReplace($r.kind, {
       "rsem": "Sem",
@@ -174,7 +170,4 @@ proc reportHook*(conf: ConfigRef, r: Report): TErrorHandling =
       of repBackend:  s.addFields(r.backendReport, f)
       of repExternal: s.addFields(r.externalReport, f)
 
-    if wkind == writeForceEnabled:
-      echo s.toLine().toString(conf.useColor)
-    else:
-      conf.writeln(s.toLine().toString(conf.useColor))
+    conf.writeln(s.toLine().toString(conf.useColor))

nimsuggest/tests/tchk1.nim

@@ -21,10 +21,6 @@ chk;;skUnknown;;;;Error;;$file;;12;;0;;"identifier expected, but found \'keyword
 chk;;skUnknown;;;;Error;;$file;;14;;0;;"nestable statement requires indentation";;0
 chk;;skUnknown;;;;Error;;$file;;12;;0;;"implementation of \'foo\' expected";;0
 chk;;skUnknown;;;;Error;;$file;;17;;0;;"invalid indentation";;0
-chk;;skUnknown;;;;Hint;;$file;;20;;95;;"Hint: line too long [LineTooLong]";;0
-chk;;skUnknown;;;;Hint;;$file;;21;;83;;"Hint: line too long [LineTooLong]";;0
-chk;;skUnknown;;;;Hint;;$file;;28;;97;;"Hint: line too long [LineTooLong]";;0
-chk;;skUnknown;;;;Hint;;$file;;29;;98;;"Hint: line too long [LineTooLong]";;0
 chk;;skUnknown;;;;Hint;;$file;;12;;9;;"\'foo\' is declared but not used [XDeclaredButNotUsed]";;0
 chk;;skUnknown;;;;Hint;;$file;;14;;5;;"\'main\' is declared but not used [XDeclaredButNotUsed]";;0
 """

tests/misc/twarning_as_error_regression.nim

@@ -0,0 +1,20 @@
+discard """
+  description: '''
+    Regression test for warnings and hints promoted to errors counting as
+    reported (and thus leading to compilation aborting) even if disabled
+  '''
+  matrix: "--warning:Deprecated:on --warningAsError:Deprecated --hint:XDeclaredButNotUsed:on --hintAsError:XDeclaredButNotUsed"
+  action: compile
+"""
+
+{.push hint[XDeclaredButNotUsed]:off.}
+block:
+  var x = 0 # must not quit the compiler
+{.pop.}
+
+proc p() {.deprecated, used.} =
+  discard
+
+{.push warning[Deprecated]:off.}
+p() # must not quit the compiler
+{.pop.}