An unnamed break in a block now gives an UnnamedBreak warning (#20901)

* unnamed break in the block now gives an error

* bootstrap

* fixes

* more fixes

* break with label

* label again

* one moee

* Delete test5.txt

* it now gives a UnnamedBreak warning

* change the URL of bump back to the original one

changelog.md

@@ -105,6 +105,8 @@
 
 - Object fields now support default values, see https://nim-lang.github.io/Nim/manual.html#types-default-values-for-object-fields for details.
 
+- 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.
+
 ## Standard library additions and changes
 
 [//]: # "Changes:"

compiler/condsyms.nim

@@ -148,3 +148,4 @@ proc initDefines*(symbols: StringTableRef) =
 
   defineSymbol("nimHasOutParams")
   defineSymbol("nimHasSystemRaisesDefect")
+  defineSymbol("nimHasWarnUnnamedBreak")

compiler/lineinfos.nim

@@ -84,6 +84,7 @@ type
     warnEffect = "Effect",
     warnCastSizes = "CastSizes"
     warnTemplateRedefinition = "TemplateRedefinition",
+    warnUnnamedBreak = "UnnamedBreak",
     warnUser = "User",
     # hints
     hintSuccess = "Success", hintSuccessX = "SuccessX",
@@ -181,6 +182,7 @@ const
     warnEffect: "$1",
     warnCastSizes: "$1",
     warnTemplateRedefinition: "template '$1' is implicitly redefined, consider adding an explicit .redefine pragma",
+    warnUnnamedBreak: "Using an unnamed break in a block is deprecated; Use a named block with a named break instead",
     warnUser: "$1",
     hintSuccess: "operation successful: $#",
     # keep in sync with `testament.isSuccess`

compiler/nim.cfg

@@ -27,9 +27,13 @@ define:useStdoutAsStdmsg
 #gc:markAndSweep
 
 @if nimHasWarningObservableStores:
-  warning[ObservableStores]: off
+  warning[ObservableStores]:off
 @end
 
 @if nimHasWarningAsError:
-  warningAsError:GcUnsafe2:on
+  warningAsError[GcUnsafe2]:on
+@end
+
+@if nimHasWarnUnnamedBreak:
+  warningAserror[UnnamedBreak]:on
 @end

compiler/semdata.nim

@@ -38,6 +38,7 @@ type
     resultSym*: PSym          # the result symbol (if we are in a proc)
     nestedLoopCounter*: int   # whether we are in a loop or not
     nestedBlockCounter*: int  # whether we are in a block or not
+    breakInLoop*: bool        # whether we are in a loop without block
     next*: PProcCon           # used for stacking procedure contexts
     mappingExists*: bool
     mapping*: TIdTable

compiler/semexprs.nim

@@ -2640,6 +2640,8 @@ include semobjconstr
 proc semBlock(c: PContext, n: PNode; flags: TExprFlags; expectedType: PType = nil): PNode =
   result = n
   inc(c.p.nestedBlockCounter)
+  let oldBreakInLoop = c.p.breakInLoop
+  c.p.breakInLoop = false
   checkSonsLen(n, 2, c.config)
   openScope(c) # BUGFIX: label is in the scope of block!
   if n[0].kind != nkEmpty:
@@ -2657,6 +2659,7 @@ proc semBlock(c: PContext, n: PNode; flags: TExprFlags; expectedType: PType = ni
   if isEmptyType(n.typ): n.transitionSonsKind(nkBlockStmt)
   else: n.transitionSonsKind(nkBlockExpr)
   closeScope(c)
+  c.p.breakInLoop = oldBreakInLoop
   dec(c.p.nestedBlockCounter)
 
 proc semExportExcept(c: PContext, n: PNode): PNode =

compiler/semfields.nim

@@ -133,6 +133,8 @@ proc semForFields(c: PContext, n: PNode, m: TMagic): PNode =
       typeMismatch(c.config, calli.info, tupleTypeA, tupleTypeB, calli)
 
   inc(c.p.nestedLoopCounter)
+  let oldBreakInLoop = c.p.breakInLoop
+  c.p.breakInLoop = true
   if tupleTypeA.kind == tyTuple:
     var loopBody = n[^1]
     for i in 0..<tupleTypeA.len:
@@ -156,6 +158,7 @@ proc semForFields(c: PContext, n: PNode, m: TMagic): PNode =
       semForObjectFields(fc, t.n, n, stmts)
       if t[0] == nil: break
       t = skipTypes(t[0], skipPtrs)
+  c.p.breakInLoop = oldBreakInLoop
   dec(c.p.nestedLoopCounter)
   # for TR macros this 'while true: ...; break' loop is pretty bad, so
   # we avoid it now if we can:

compiler/semstmts.nim

@@ -73,6 +73,8 @@ proc semBreakOrContinue(c: PContext, n: PNode): PNode =
         localError(c.config, n.info, errInvalidControlFlowX % s.name.s)
     else:
       localError(c.config, n.info, errGenerated, "'continue' cannot have a label")
+  elif c.p.nestedBlockCounter > 0 and n.kind == nkBreakStmt and not c.p.breakInLoop:
+    localError(c.config, n.info, warnUnnamedBreak)
   elif (c.p.nestedLoopCounter <= 0) and ((c.p.nestedBlockCounter <= 0) or n.kind == nkContinueStmt):
     localError(c.config, n.info, errInvalidControlFlowX %
                renderTree(n, {renderNoComments}))
@@ -89,7 +91,10 @@ proc semWhile(c: PContext, n: PNode; flags: TExprFlags): PNode =
   openScope(c)
   n[0] = forceBool(c, semExprWithType(c, n[0], expectedType = getSysType(c.graph, n.info, tyBool)))
   inc(c.p.nestedLoopCounter)
+  let oldBreakInLoop = c.p.breakInLoop
+  c.p.breakInLoop = true
   n[1] = semStmt(c, n[1], flags)
+  c.p.breakInLoop = oldBreakInLoop
   dec(c.p.nestedLoopCounter)
   closeScope(c)
   if n[1].typ == c.enforceVoidContext:
@@ -959,11 +964,14 @@ proc semForVars(c: PContext, n: PNode; flags: TExprFlags): PNode =
           if not isDiscardUnderscore(v): addDecl(c, v)
         elif v.owner == nil: v.owner = getCurrOwner(c)
   inc(c.p.nestedLoopCounter)
+  let oldBreakInLoop = c.p.breakInLoop
+  c.p.breakInLoop = true
   openScope(c)
   n[^1] = semExprBranch(c, n[^1], flags)
   if efInTypeof notin flags:
     discardCheck(c, n[^1], flags)
   closeScope(c)
+  c.p.breakInLoop = oldBreakInLoop
   dec(c.p.nestedLoopCounter)
 
 proc implicitIterator(c: PContext, it: string, arg: PNode): PNode =

compiler/semtypes.nim

@@ -1457,6 +1457,8 @@ proc semStmtListType(c: PContext, n: PNode, prev: PType): PType =
 
 proc semBlockType(c: PContext, n: PNode, prev: PType): PType =
   inc(c.p.nestedBlockCounter)
+  let oldBreakInLoop = c.p.breakInLoop
+  c.p.breakInLoop = false
   checkSonsLen(n, 2, c.config)
   openScope(c)
   if n[0].kind notin {nkEmpty, nkSym}:
@@ -1465,6 +1467,7 @@ proc semBlockType(c: PContext, n: PNode, prev: PType): PType =
   n[1].typ = result
   n.typ = result
   closeScope(c)
+  c.p.breakInLoop = oldBreakInLoop
   dec(c.p.nestedBlockCounter)
 
 proc semGenericParamInInvocation(c: PContext, n: PNode): PType =

lib/std/envvars.nim

@@ -173,11 +173,11 @@ when not defined(nimscript):
 
     iterator envPairsImpl(): tuple[key, value: string] {.tags: [ReadEnvEffect].} =
       when defined(windows):
-        block:
+        block implBlock:
           template impl(get_fun, typ, size, zero, free_fun) =
             let env = get_fun()
             var e = env
-            if e == nil: break
+            if e == nil: break implBlock
             while true:
               let eend = strEnd(e)
               let kv = $e

lib/std/wrapnils.nim

@@ -60,7 +60,7 @@ proc finalize(n: NimNode, lhs: NimNode, level: int): NimNode =
   else:
     result = quote: (let `lhs` = `n`)
 
-proc process(n: NimNode, lhs: NimNode, level: int): NimNode =
+proc process(n: NimNode, lhs: NimNode, label: NimNode, level: int): NimNode =
   var n = n.copyNimTree
   var it = n
   let addr2 = bindSym"addr"
@@ -78,7 +78,7 @@ proc process(n: NimNode, lhs: NimNode, level: int): NimNode =
       let okSet = check[1]
       let kind1 = check[2]
       let tmp = genSym(nskLet, "tmpCase")
-      let body = process(objRef, tmp, level + 1)
+      let body = process(objRef, tmp, label, level + 1)
       let tmp3 = nnkDerefExpr.newTree(tmp)
       it[0][0] = tmp3
       let dot2 = nnkDotExpr.newTree(@[tmp, dot[1]])
@@ -87,17 +87,17 @@ proc process(n: NimNode, lhs: NimNode, level: int): NimNode =
       let assgn = finalize(n, lhs, level)
       result = quote do:
         `body`
-        if `tmp3`.`kind1` notin `okSet`: break
+        if `tmp3`.`kind1` notin `okSet`: break `label`
         `assgn`
       break
     elif it.kind in {nnkHiddenDeref, nnkDerefExpr}:
       let tmp = genSym(nskLet, "tmp")
-      let body = process(it[0], tmp, level + 1)
+      let body = process(it[0], tmp, label, level + 1)
       it[0] = tmp
       let assgn = finalize(n, lhs, level)
       result = quote do:
         `body`
-        if `tmp` == nil: break
+        if `tmp` == nil: break `label`
         `assgn`
       break
     elif it.kind == nnkCall: # consider extending to `nnkCallKinds`
@@ -113,10 +113,11 @@ macro `?.`*(a: typed): auto =
   ## presence of intermediate nil pointers/references, in which case a default
   ## value is produced.
   let lhs = genSym(nskVar, "lhs")
-  let body = process(a, lhs, 0)
+  let label = genSym(nskLabel, "label")
+  let body = process(a, lhs, label, 0)
   result = quote do:
     var `lhs`: type(`a`)
-    block:
+    block `label`:
       `body`
     `lhs`
 
@@ -144,10 +145,11 @@ macro `??.`*(a: typed): Option =
 
   let lhs = genSym(nskVar, "lhs")
   let lhs2 = genSym(nskVar, "lhs")
-  let body = process(a, lhs2, 0)
+  let label = genSym(nskLabel, "label")
+  let body = process(a, lhs2, label, 0)
   result = quote do:
     var `lhs`: Option[type(`a`)]
-    block:
+    block `label`:
       var `lhs2`: type(`a`)
       `body`
       `lhs` = option(`lhs2`)

tests/config.nims

@@ -42,3 +42,5 @@ switch("define", "nimPreviewHashRef")
 switch("define", "nimPreviewRangeDefault")
 when defined(windows):
   switch("tlsEmulation", "off")
+
+switch("warningAserror", "UnnamedBreak")

tests/controlflow/tcontrolflow.nim

@@ -19,7 +19,7 @@ block tbreak:
     run = false
     block myblock:
       if true:
-        break
+        break myblock
       echo "leaving myblock"
     x = true
   doAssert(x)
@@ -95,3 +95,22 @@ block tnestif:
   else:
       writeLine(stdout, "looks like Python")
   #OUT i == 2
+
+# bug https://github.com/nim-lang/RFCs/issues/451
+for i in 1..2: # works
+  break
+
+block: # works
+  for i in 1..2:
+    break
+
+block: # works
+  block:
+    discard 12 + 3
+  for i in 1..2:
+    break
+
+block named: # works
+  if true:
+    break named
+  doAssert false, "not reached"

tests/controlflow/tunamedbreak.nim

@@ -0,0 +1,15 @@
+
+discard """
+  cmd: "nim check $file"
+  action: "reject"
+  nimout: '''
+tunamedbreak.nim(12, 5) Error: Using an unnamed break in a block is deprecated; Use a named block with a named break instead [UnnamedBreak]
+tunamedbreak.nim(15, 3) Error: Using an unnamed break in a block is deprecated; Use a named block with a named break instead [UnnamedBreak]
+  '''
+"""
+for i in 1..2: # errors
+  block:
+    break
+
+block: # errors
+  break

tests/exception/texceptionbreak.nim

@@ -29,16 +29,16 @@ echo "2"
 try:
   raise newException(OSError, "Problem")
 except OSError:
-  block:
-    break
+  block label:
+    break label
 
 echo "3"
 
 # Fourth Variety
-block:
+block label:
   try:
     raise newException(OSError, "Problem")
   except OSError:
-    break
+    break label
 
 echo "4"

tests/iter/titer_issues.nim

@@ -369,9 +369,9 @@ block:
 
 # bug #18824
 iterator poc_iterator: int {.closure.}  =
-  block:
+  block bug18824:
     try:
-      break
+      break bug18824
     finally:
       echo "In defer"