From 629d43caf58dfcf9106f6db6bc1e885c621cfb70 Mon Sep 17 00:00:00 2001
From: Nick Treleaven <n@trelsoft.com>
Date: Mon, 15 Feb 2021 13:48:24 +0000
Subject: [PATCH] checker: warn when using `goto` outside of `unsafe` (#8741)

---
 doc/docs.md                         | 14 +++++++++-----
 vlib/flag/flag.v                    | 27 +++++++++++++--------------
 vlib/v/checker/checker.v            |  4 ++++
 vlib/v/checker/tests/goto_label.out |  2 +-
 vlib/v/checker/tests/goto_label.vv  | 17 +++++++++--------
 vlib/v/parser/lock.v                |  7 +++----
 6 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/doc/docs.md b/doc/docs.md
index 73e4fc6637..844b3e6ae8 100644
--- a/doc/docs.md
+++ b/doc/docs.md
@@ -3910,21 +3910,25 @@ fn C.DefWindowProc(hwnd int, msg int, lparam int, wparam int)
 
 V allows unconditionally jumping to a label with `goto`. The label name must be contained
 within the same function as the `goto` statement. A program may `goto` a label outside
-or deeper than the current scope, but it must not skip a variable initialization.
+or deeper than the current scope. `goto` allows jumping past variable initialization or 
+jumping back to code that accesses memory that has already been freed, so it requires
+`unsafe`.
 
 ```v ignore
 if x {
 	// ...
 	if y {
-		goto my_label
+		unsafe {
+			goto my_label
+		}
 	}
 	// ...
 }
 my_label:
 ```
-`goto` should be avoided when `for` can be used instead. In particular,
-[labelled break](#labelled-break--continue) can be used to break out of
-a nested loop.
+`goto` should be avoided, particularly when `for` can be used instead.
+[Labelled break/continue](#labelled-break--continue) can be used to break out of
+a nested loop, and those do not risk violating memory-safety.
 
 # Appendices
 
diff --git a/vlib/flag/flag.v b/vlib/flag/flag.v
index 7ddeb269d5..ef317f367f 100644
--- a/vlib/flag/flag.v
+++ b/vlib/flag/flag.v
@@ -362,22 +362,21 @@ pub fn (fs FlagParser) usage() string {
 	if positive_min_arg || positive_max_arg || no_arguments {
 		if no_arguments {
 			use += 'This application does not expect any arguments\n\n'
-			goto end_of_arguments_handling
+		} else {
+			mut s := []string{}
+			if positive_min_arg {
+				s << 'at least $fs.min_free_args'
+			}
+			if positive_max_arg {
+				s << 'at most $fs.max_free_args'
+			}
+			if positive_min_arg && positive_max_arg && fs.min_free_args == fs.max_free_args {
+				s = ['exactly $fs.min_free_args']
+			}
+			sargs := s.join(' and ')
+			use += 'The arguments should be $sargs in number.\n\n'
 		}
-		mut s := []string{}
-		if positive_min_arg {
-			s << 'at least $fs.min_free_args'
-		}
-		if positive_max_arg {
-			s << 'at most $fs.max_free_args'
-		}
-		if positive_min_arg && positive_max_arg && fs.min_free_args == fs.max_free_args {
-			s = ['exactly $fs.min_free_args']
-		}
-		sargs := s.join(' and ')
-		use += 'The arguments should be $sargs in number.\n\n'
 	}
-	end_of_arguments_handling:
 	if fs.flags.len > 0 {
 		use += 'Options:\n'
 		for f in fs.flags {
diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v
index 61bd07d1ad..9d4ea0b301 100644
--- a/vlib/v/checker/checker.v
+++ b/vlib/v/checker/checker.v
@@ -3178,6 +3178,10 @@ fn (mut c Checker) stmt(node ast.Stmt) {
 		}
 		ast.GotoLabel {}
 		ast.GotoStmt {
+			if !c.inside_unsafe {
+				c.warn('`goto` requires `unsafe` (consider using labelled break/continue)',
+					node.pos)
+			}
 			if node.name !in c.cur_fn.label_names {
 				c.error('unknown label `$node.name`', node.pos)
 			}
diff --git a/vlib/v/checker/tests/goto_label.out b/vlib/v/checker/tests/goto_label.out
index fd97bf7e8f..a2591ad85b 100644
--- a/vlib/v/checker/tests/goto_label.out
+++ b/vlib/v/checker/tests/goto_label.out
@@ -45,5 +45,5 @@ vlib/v/checker/tests/goto_label.vv:25:7: error: unknown label `undefined`
    24 |     goto g1 // back
    25 |     goto undefined
       |          ~~~~~~~~~
-   26 | }
+   26 | }}
    27 |
diff --git a/vlib/v/checker/tests/goto_label.vv b/vlib/v/checker/tests/goto_label.vv
index d25d5d97e8..23b2cd75a7 100644
--- a/vlib/v/checker/tests/goto_label.vv
+++ b/vlib/v/checker/tests/goto_label.vv
@@ -1,4 +1,4 @@
-fn f() {
+fn f() {unsafe {
 	goto f1 // forward
 	goto f2
 	f1:
@@ -14,19 +14,20 @@ fn f() {
 	goto a1
 	goto f1 // back
 	goto f2
-}
+}}
 
-fn g() {
+fn g() {unsafe {
 	goto g1 // forward
 	g1:
 	goto f1
 	goto a1
 	goto g1 // back
 	goto undefined
-}
+}}
 
 // implicit main
-goto m1
-m1:
-goto m1
-
+unsafe {
+	goto m1
+	m1:
+	goto m1
+}
diff --git a/vlib/v/parser/lock.v b/vlib/v/parser/lock.v
index 222951d1d3..0f703bbb49 100644
--- a/vlib/v/parser/lock.v
+++ b/vlib/v/parser/lock.v
@@ -9,9 +9,9 @@ fn (mut p Parser) lock_expr() ast.LockExpr {
 	mut pos := p.tok.position()
 	mut lockeds := []ast.Ident{}
 	mut is_rlocked := []bool{}
-	for {
+	outer: for {
 		if p.tok.kind == .lcbr {
-			goto start_stmts
+			break
 		}
 		is_rlock := p.tok.kind == .key_rlock
 		if !is_rlock && p.tok.kind != .key_lock {
@@ -31,7 +31,7 @@ fn (mut p Parser) lock_expr() ast.LockExpr {
 			is_rlocked << is_rlock
 			p.next()
 			if p.tok.kind == .lcbr {
-				goto start_stmts
+				break outer
 			}
 			if p.tok.kind == .semicolon {
 				p.next()
@@ -40,7 +40,6 @@ fn (mut p Parser) lock_expr() ast.LockExpr {
 			p.check(.comma)
 		}
 	}
-	start_stmts:
 	stmts := p.parse_block()
 	pos.update_last_line(p.prev_tok.line_nr)
 	return ast.LockExpr{