fix(sem): nimsuggest crashing for ill-formed AST

compiler/front/msgs.nim

@@ -41,6 +41,12 @@ from compiler/ast/reports_sem import SemReport
 export InstantiationInfo
 export TErrorHandling
 
+proc handleReport*(
+    conf: ConfigRef,
+    r: Report,
+    reportFrom: InstantiationInfo,
+    eh: TErrorHandling = doNothing) {.noinline.}
+
 template toStdOrrKind(stdOrr): untyped =
   if stdOrr == stdout: stdOrrStdout else: stdOrrStderr
 
@@ -600,18 +606,22 @@ proc report*(conf: ConfigRef, node: PNode): TErrorHandling =
   assert node.kind == nkError
   return conf.report(conf.astDiagToLegacyReport(conf, node.diag))
 
+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())
+
+  if r.category == repVM and r.vmReport.trace != nil:
+    handleReport(c, wrap(r.vmReport.trace[]), reportFrom)
+
 proc handleReport*(
     conf: ConfigRef,
     r: Report,
     reportFrom: InstantiationInfo,
     eh: TErrorHandling = doNothing) {.noinline.} =
   var rep = r
-  rep.reportFrom = toReportLineInfo(reportFrom)
-  if rep.category in { repSem, repVM } and rep.location.isSome():
-    rep.context = conf.getContext(rep.location.get())
-
-  if rep.category == repVM and rep.vmReport.trace != nil:
-    handleReport(conf, wrap(rep.vmReport.trace[]), reportFrom)
+  fillReportAndHandleVmTrace(conf, rep, reportFrom)
 
   let
     userAction = conf.report(rep)
@@ -654,6 +664,34 @@ 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(

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.globalReport(n.info, reportAst(
+    conf.reportAndFail(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.globalReport(n.info, reportAst(
+    conf.reportAndFail(n.info, reportAst(
       rsemIllformedAst, n,
       str = "Expected at least $1 elements, but found $2" % [$length, $n.len]))
 

nimsuggest/tests/terror_handling.nim

@@ -0,0 +1,17 @@
+# Ensure that AST not having the correct number of nodes doesn't cause
+# crashes. This a regression test for `checkSonsMinLen` and
+# `checkSonsLen` not aborting analysis.
+
+import std/macros
+
+macro t1(): untyped =
+  newNimNode(nnkCall) # an call AST with not enough sub-nodes
+
+t1()
+
+#[!]#
+discard """
+$nimsuggest --tester $file
+>chk $1
+chk;;skUnknown;;;;Error;;$file;;8;;12;;"illformed AST: ()";;0
+"""