From e83f27d1142fd02a89560733f99959819b6e1ec6 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Tue, 22 Aug 2023 21:28:12 +0200
Subject: [PATCH] sem: use `nkError` in `break`/`continue` analysis (#851)

## Summary

Split semantic analysis of 'break' and 'continue' statements into two
procedures and use `nkError` propagation instead of `localReport`. The
quality of errors reported for invalid 'break' statements is also
improved.

## Details

* split `semBreakOrContinue` into two procedures, as the handling of
  both had only very little in common
* refactor the analysis of 'break' statements. `ElaborateAst` is used
  for easier error propagation. In addition, the input AST is no longer
  modified
* report errors through `nkError` nodes -- `AstDiagKind` counterparts
  for the previously used reports are added

**Changed errors:**

* don't report an "invalid control flow" error when the provided
  identifier is erroneous. The identifier error is forwarded instead
* introduce the new "label expected" error and use it to replace
  "ill-formed AST" and "invalid control flow" errors when the AST in
  the label slot is not the symbol of a label

Two new error message tests are added for the changed errors.
---
 compiler/ast/ast_types.nim          |  9 ++++-
 compiler/ast/report_enums.nim       |  1 +
 compiler/front/cli_reporter.nim     | 14 +++++++-
 compiler/front/msgs.nim             |  3 ++
 compiler/sem/semexprs.nim           |  3 +-
 compiler/sem/semstmts.nim           | 53 +++++++++++++++++------------
 tests/errmsgs/texpected_label_1.nim |  7 ++++
 tests/errmsgs/texpected_label_2.nim |  8 +++++
 8 files changed, 73 insertions(+), 25 deletions(-)
 create mode 100644 tests/errmsgs/texpected_label_1.nim
 create mode 100644 tests/errmsgs/texpected_label_2.nim

diff --git a/compiler/ast/ast_types.nim b/compiler/ast/ast_types.nim
index a8605e1c7a0..db07fbf0b0a 100644
--- a/compiler/ast/ast_types.nim
+++ b/compiler/ast/ast_types.nim
@@ -1138,6 +1138,9 @@ type
     adSemInvalidExpression
     adSemExpectedNonemptyPattern
     # semstmts
+    adSemInvalidControlFlow
+    adSemExpectedLabel
+    adSemContinueCannotHaveLabel
     adSemUseOrDiscardExpr
     adSemAmbiguousIdent
     adSemCannotConvertToRange  # TODO: this should probably mention float?
@@ -1330,7 +1333,9 @@ type
         adSemInvalidRangeConversion,
         adSemFoldCannotComputeOffset,
         adSemExpectedIdentifierQuoteLimit,
-        adSemExpectedRangeType:
+        adSemExpectedRangeType,
+        adSemExpectedLabel,
+        adSemContinueCannotHaveLabel:
       discard
     of adSemExpectedIdentifierInExpr:
       notIdent*: PNode
@@ -1498,6 +1503,8 @@ type
     of adSemDefNameSym:
       defNameSym*: PSym
       defNameSymData*: AdSemDefNameSym
+    of adSemInvalidControlFlow:
+      label*: PSym
 
   AdSemDefNameSymKind* = enum
     adSemDefNameSymExpectedKindMismatch
diff --git a/compiler/ast/report_enums.nim b/compiler/ast/report_enums.nim
index 96071df72d7..85a20b9f9a8 100644
--- a/compiler/ast/report_enums.nim
+++ b/compiler/ast/report_enums.nim
@@ -466,6 +466,7 @@ type
     rsemDiscardingVoid
     rsemDiscardingProc
     rsemInvalidControlFlow
+    rsemExpectedLabel
     rsemContinueCannotHaveLabel
     rsemUseOrDiscard
     rsemUseOrDiscardExpr
diff --git a/compiler/front/cli_reporter.nim b/compiler/front/cli_reporter.nim
index 43094293ae2..6646efbdcaf 100644
--- a/compiler/front/cli_reporter.nim
+++ b/compiler/front/cli_reporter.nim
@@ -1190,6 +1190,9 @@ proc reportBody*(conf: ConfigRef, r: SemReport): string =
         if r.sym.isNil: r.ast.render else: r.symstr
       ]
 
+    of rsemExpectedLabel:
+      result = "expected the name of a label, but got: " & r.ast.render
+
     of rsemContinueCannotHaveLabel:
       result = "'continue' cannot have a label"
 
@@ -3279,7 +3282,9 @@ func astDiagToLegacyReport(conf: ConfigRef, diag: PAstDiag): Report {.inline.} =
       adSemFoldDivByZero,
       adSemFoldCannotComputeOffset,
       adSemExpectedIdentifierQuoteLimit,
-      adSemExpectedRangeType:
+      adSemExpectedRangeType,
+      adSemExpectedLabel,
+      adSemContinueCannotHaveLabel:
     semRep = SemReport(
         location: some diag.location,
         reportInst: diag.instLoc.toReportLineInfo,
@@ -3305,6 +3310,13 @@ func astDiagToLegacyReport(conf: ConfigRef, diag: PAstDiag): Report {.inline.} =
         ast: diag.wrongNode,
         typeMismatch: @[typeMismatch(diag.wrongNode[0].typ,
                                      diag.wrongNode.typ)])
+  of adSemInvalidControlFlow:
+    semRep = SemReport(
+        location: some diag.location,
+        reportInst: diag.instLoc.toReportLineInfo,
+        kind: rsemInvalidControlFlow,
+        ast: diag.wrongNode,
+        sym: diag.label)
   of adSemUseOrDiscardExpr:
     semRep = SemReport(
         location: some diag.undiscarded.info, # xxx: location override
diff --git a/compiler/front/msgs.nim b/compiler/front/msgs.nim
index d3851af384b..0d89a3d1e6b 100644
--- a/compiler/front/msgs.nim
+++ b/compiler/front/msgs.nim
@@ -521,6 +521,9 @@ func astDiagToLegacyReportKind*(
   of adSemImplementationNotAllowed: rsemImplementationNotAllowed
   of adSemInvalidExpression: rsemInvalidExpression
   of adSemExpectedNonemptyPattern: rsemExpectedNonemptyPattern
+  of adSemInvalidControlFlow: rsemInvalidControlFlow
+  of adSemExpectedLabel: rsemExpectedLabel
+  of adSemContinueCannotHaveLabel: rsemContinueCannotHaveLabel
   of adSemUseOrDiscardExpr: rsemUseOrDiscardExpr
   of adSemCannotConvertToRange: rsemCannotConvertToRange
   of adSemProveInit: rsemProveInit
diff --git a/compiler/sem/semexprs.nim b/compiler/sem/semexprs.nim
index 174caf63c84..fa8355576ae 100644
--- a/compiler/sem/semexprs.nim
+++ b/compiler/sem/semexprs.nim
@@ -3839,7 +3839,8 @@ proc semExpr(c: PContext, n: PNode, flags: TExprFlags = {}): PNode =
   of nkDiscardStmt: result = semDiscard(c, n)
   of nkWhileStmt: result = semWhile(c, n, flags)
   of nkTryStmt, nkHiddenTryStmt: result = semTry(c, n, flags)
-  of nkBreakStmt, nkContinueStmt: result = semBreakOrContinue(c, n)
+  of nkBreakStmt: result = c.config.extract(semBreakStmt(c, n))
+  of nkContinueStmt: result = semContinueStmt(c, n)
   of nkForStmt: result = semFor(c, n, flags)
   of nkCaseStmt: result = semCase(c, n, flags)
   of nkReturnStmt: result = semReturn(c, n)
diff --git a/compiler/sem/semstmts.nim b/compiler/sem/semstmts.nim
index 78ba6646695..213499ccda4 100644
--- a/compiler/sem/semstmts.nim
+++ b/compiler/sem/semstmts.nim
@@ -26,34 +26,43 @@ proc semDiscard(c: PContext, n: PNode): PNode =
       # tyProc is disallowed to prevent ``discard foo`` to be valid, when ``discard foo()`` is meant.
       localReport(c.config, n, reportSem rsemDiscardingProc)
 
-proc semBreakOrContinue(c: PContext, n: PNode): PNode =
-  result = n
+proc semBreakStmt(c: PContext, n: PNode): ElaborateAst =
+  result.initWith(n)
   checkSonsLen(n, 1, c.config)
-  if n[0].kind != nkEmpty:
-    if n.kind != nkContinueStmt:
-      var s: PSym
-      case n[0].kind:
-        of nkIdent: s = lookUp(c, n[0])
-        of nkSym: s = n[0].sym
-        else:
-          semReportIllformedAst(c.config, n, {nkIdent, nkSym})
-
-      s = getGenSym(c, s)
+  case n[0].kind
+  of nkIdent, nkSym:
+    let s = getGenSym(c, lookUp(c, n[0]))
+    result[0] = newSymNode(s, n[0].info)
+
+    case s.kind
+    of skError:
+      result[0] = s.ast
+    of skLabel:
+      # make sure the label is okay to use:
       if s.kind == skLabel and s.owner.id == c.p.owner.id:
-        var x = newSymNode(s)
-        x.info = n.info
         incl(s.flags, sfUsed)
-        n[0] = x
-        suggestSym(c.graph, x.info, s, c.graph.usageSym)
+        suggestSym(c.graph, n.info, s, c.graph.usageSym)
       else:
-        localReport(c.config, n.info, reportSym(rsemInvalidControlFlow, s))
-
+        # a label not part of the current context
+        result.diag = PAstDiag(kind: adSemInvalidControlFlow, label: s)
     else:
-      localReport(c.config, n, reportSem rsemContinueCannotHaveLabel)
-  elif (c.p.nestedLoopCounter <= 0) and
-       ((c.p.nestedBlockCounter <= 0) or n.kind == nkContinueStmt):
+      result[0] = c.config.newError(result[0]):
+        PAstDiag(kind: adSemExpectedLabel)
+  of nkEmpty:
+    result[0] = n[0]
+    if c.p.nestedLoopCounter <= 0 and c.p.nestedBlockCounter <= 0:
+      # nothing to break out of
+      result.diag = PAstDiag(kind: adSemInvalidControlFlow)
+  else:
+    result[0] = c.config.newError(n[0], PAstDiag(kind: adSemExpectedLabel))
 
-    localReport(c.config, n, reportSem rsemInvalidControlFlow)
+proc semContinueStmt(c: PContext, n: PNode): PNode =
+  if n[0].kind != nkEmpty:
+    c.config.newError(n, PAstDiag(kind: adSemContinueCannotHaveLabel))
+  elif c.p.nestedLoopCounter <= 0:
+    c.config.newError(n, PAstDiag(kind: adSemInvalidControlFlow))
+  else:
+    n
 
 proc semAsm(c: PContext, n: PNode): PNode =
   checkSonsLen(n, 2, c.config)
diff --git a/tests/errmsgs/texpected_label_1.nim b/tests/errmsgs/texpected_label_1.nim
new file mode 100644
index 00000000000..0988245aa60
--- /dev/null
+++ b/tests/errmsgs/texpected_label_1.nim
@@ -0,0 +1,7 @@
+discard """
+  errormsg: "expected the name of a label, but got: 1 + 1"
+  line: 7
+"""
+
+block:
+  break 1 + 1
\ No newline at end of file
diff --git a/tests/errmsgs/texpected_label_2.nim b/tests/errmsgs/texpected_label_2.nim
new file mode 100644
index 00000000000..8cd8fd24b07
--- /dev/null
+++ b/tests/errmsgs/texpected_label_2.nim
@@ -0,0 +1,8 @@
+discard """
+  errormsg: "expected the name of a label, but got: a"
+  line: 8
+"""
+
+block:
+  var a: int
+  break a
\ No newline at end of file