From 13902a827b7cd8fcb62b5c8dec483a64b1c06bbc Mon Sep 17 00:00:00 2001
From: Nick Treleaven <n@trelsoft.com>
Date: Mon, 25 Apr 2022 10:09:25 +0100
Subject: [PATCH] checker: error if smaller signed == unsigned (#14078)

---
 vlib/builtin/js/map_test.js.v                 |  2 +-
 vlib/builtin/map_test.v                       |  2 +-
 vlib/net/websocket/message.v                  |  6 +--
 vlib/os/os_test.v                             | 12 +++---
 vlib/sync/stdatomic/atomic_test.v             |  2 +-
 vlib/v/checker/check_types.v                  | 35 +++++++++++++++++
 vlib/v/checker/checker.v                      | 12 ++++++
 .../checker/tests/compare_unsigned_signed.out | 39 ++++++++++++++++++-
 .../checker/tests/compare_unsigned_signed.vv  | 13 +++++++
 vlib/v/tests/infix_expr_test.v                |  8 ++--
 vlib/v/tests/shift_test.v                     |  6 +--
 11 files changed, 116 insertions(+), 21 deletions(-)

diff --git a/vlib/builtin/js/map_test.js.v b/vlib/builtin/js/map_test.js.v
index a71730e7cf..da6efd436f 100644
--- a/vlib/builtin/js/map_test.js.v
+++ b/vlib/builtin/js/map_test.js.v
@@ -931,7 +931,7 @@ fn test_u64_keys() {
 		m[i]++
 		assert m[i] == i + 1
 	}
-	assert m.len == end
+	assert u64(m.len) == end
 	keys := m.keys()
 	for i in u64(0) .. end {
 		assert keys[i] == i
diff --git a/vlib/builtin/map_test.v b/vlib/builtin/map_test.v
index 5fb57a1ccc..1ffdcd208e 100644
--- a/vlib/builtin/map_test.v
+++ b/vlib/builtin/map_test.v
@@ -919,7 +919,7 @@ fn test_u64_keys() {
 		m[i]++
 		assert m[i] == i + 1
 	}
-	assert m.len == end
+	assert u64(m.len) == end
 	keys := m.keys()
 	for i in u64(0) .. end {
 		assert keys[i] == i
diff --git a/vlib/net/websocket/message.v b/vlib/net/websocket/message.v
index d2c7e49e60..420effe2fe 100644
--- a/vlib/net/websocket/message.v
+++ b/vlib/net/websocket/message.v
@@ -223,7 +223,7 @@ pub fn (mut ws Client) parse_frame_header() ?Frame {
 		buffer[bytes_read] = rbuff[0]
 		bytes_read++
 		// parses the first two header bytes to get basic frame information
-		if bytes_read == u64(websocket.header_len_offset) {
+		if bytes_read == websocket.header_len_offset {
 			frame.fin = (buffer[0] & 0x80) == 0x80
 			frame.rsv1 = (buffer[0] & 0x40) == 0x40
 			frame.rsv2 = (buffer[0] & 0x20) == 0x20
@@ -249,7 +249,7 @@ pub fn (mut ws Client) parse_frame_header() ?Frame {
 				break
 			}
 		}
-		if frame.payload_len == 126 && bytes_read == u64(websocket.extended_payload16_end_byte) {
+		if frame.payload_len == 126 && bytes_read == websocket.extended_payload16_end_byte {
 			frame.header_len += 2
 			frame.payload_len = 0
 			frame.payload_len |= int(u32(buffer[2]) << 8)
@@ -259,7 +259,7 @@ pub fn (mut ws Client) parse_frame_header() ?Frame {
 				break
 			}
 		}
-		if frame.payload_len == 127 && bytes_read == u64(websocket.extended_payload64_end_byte) {
+		if frame.payload_len == 127 && bytes_read == websocket.extended_payload64_end_byte {
 			frame.header_len += 8
 			// these shift operators needs 64 bit on clang with -prod flag
 			mut payload_len := u64(0)
diff --git a/vlib/os/os_test.v b/vlib/os/os_test.v
index 3775dfcf80..3b523dd4f3 100644
--- a/vlib/os/os_test.v
+++ b/vlib/os/os_test.v
@@ -41,7 +41,7 @@ fn test_open_file() {
 	mut file := os.open_file(filename, 'w+', 0o666) or { panic(err) }
 	file.write_string(hello) or { panic(err) }
 	file.close()
-	assert hello.len == os.file_size(filename)
+	assert u64(hello.len) == os.file_size(filename)
 	read_hello := os.read_file(filename) or { panic('error reading file $filename') }
 	assert hello == read_hello
 	os.rm(filename) or { panic(err) }
@@ -58,7 +58,7 @@ fn test_open_file_binary() {
 	bytes := hello.bytes()
 	unsafe { file.write_ptr(bytes.data, bytes.len) }
 	file.close()
-	assert hello.len == os.file_size(filename)
+	assert u64(hello.len) == os.file_size(filename)
 	read_hello := os.read_bytes(filename) or { panic('error reading file $filename') }
 	assert bytes == read_hello
 	os.rm(filename) or { panic(err) }
@@ -100,7 +100,7 @@ fn test_create_file() ? {
 	filename := './test1.txt'
 	hello := 'hello world!'
 	create_and_write_to_file(filename, hello) ?
-	assert hello.len == os.file_size(filename)
+	assert u64(hello.len) == os.file_size(filename)
 	os.rm(filename) or { panic(err) }
 }
 
@@ -138,7 +138,7 @@ fn test_write_and_read_string_to_file() {
 	filename := './test1.txt'
 	hello := 'hello world!'
 	os.write_file(filename, hello) or { panic(err) }
-	assert hello.len == os.file_size(filename)
+	assert u64(hello.len) == os.file_size(filename)
 	read_hello := os.read_file(filename) or { panic('error reading file $filename') }
 	assert hello == read_hello
 	os.rm(filename) or { panic(err) }
@@ -157,7 +157,7 @@ fn test_write_and_read_bytes() {
 	// compare the length of the array with the file size (have to match).
 	unsafe { file_write.write_ptr(payload.data, 5) }
 	file_write.close()
-	assert payload.len == os.file_size(file_name)
+	assert u64(payload.len) == os.file_size(file_name)
 	mut file_read := os.open(os.real_path(file_name)) or {
 		eprintln('failed to open file $file_name')
 		return
@@ -792,7 +792,7 @@ fn test_truncate() ? {
 	mut f := os.create(filename) ?
 	f.write_string(hello) ?
 	f.close()
-	assert hello.len == os.file_size(filename)
+	assert u64(hello.len) == os.file_size(filename)
 	newlen := u64(40000)
 	os.truncate(filename, newlen) or { panic(err) }
 	assert newlen == os.file_size(filename)
diff --git a/vlib/sync/stdatomic/atomic_test.v b/vlib/sync/stdatomic/atomic_test.v
index 0d8b15a713..856fb12bf4 100644
--- a/vlib/sync/stdatomic/atomic_test.v
+++ b/vlib/sync/stdatomic/atomic_test.v
@@ -18,7 +18,7 @@ fn test_count_10_times_1_cycle_should_result_10_cycles_with_sync() {
 		go count_one_cycle(mut counter, mut wg)
 	}
 	wg.wait()
-	assert counter.counter == desired_iterations
+	assert counter.counter == u64(desired_iterations)
 	eprintln('   with synchronization the counter is: ${counter.counter:10} , expectedly == ${desired_iterations:10}')
 }
 
diff --git a/vlib/v/checker/check_types.v b/vlib/v/checker/check_types.v
index 9cd1ab98a5..ce15abbdf4 100644
--- a/vlib/v/checker/check_types.v
+++ b/vlib/v/checker/check_types.v
@@ -689,3 +689,38 @@ pub fn (mut c Checker) infer_fn_generic_types(func ast.Fn, mut node ast.CallExpr
 		c.need_recheck_generic_fns = true
 	}
 }
+
+pub fn (c &Checker) sizeof_integer(a ast.Type) int {
+	t := if a in ast.unsigned_integer_type_idxs { a.flip_signedness() } else { a }
+	r := match t {
+		ast.char_type_idx, ast.i8_type_idx {
+			1
+		}
+		ast.i16_type_idx {
+			2
+		}
+		ast.int_type_idx {
+			4
+		}
+		ast.rune_type_idx {
+			4
+		}
+		ast.i64_type_idx {
+			8
+		}
+		ast.isize_type_idx {
+			if c.pref.m64 { 8 } else { 4 }
+		}
+		ast.int_literal_type {
+			s := c.table.type_to_str(a)
+			panic('`$s` has unknown size')
+			0
+		}
+		else {
+			s := c.table.type_to_str(a)
+			panic('`$s` is not an integer')
+			0
+		}
+	}
+	return r
+}
diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v
index 8a572e375b..e1fc0502c1 100644
--- a/vlib/v/checker/checker.v
+++ b/vlib/v/checker/checker.v
@@ -640,6 +640,18 @@ pub fn (mut c Checker) infix_expr(mut node ast.InfixExpr) ast.Type {
 						rt := c.table.sym(right_type).name
 						c.error('negative value cannot be compared with `$rt`', node.left.pos)
 					}
+				} else if is_left_type_signed != is_right_type_signed
+					&& left_type != ast.int_literal_type_idx
+					&& right_type != ast.int_literal_type_idx {
+					ls := c.sizeof_integer(left_type)
+					rs := c.sizeof_integer(right_type)
+					// prevent e.g. `u32 == i16` but not `u16 == i32` as max_u16 fits in i32
+					// TODO u32 == i32, change < to <=
+					if (is_left_type_signed && ls < rs) || (is_right_type_signed && rs < ls) {
+						lt := c.table.sym(left_type).name
+						rt := c.table.sym(right_type).name
+						c.error('`$lt` cannot be compared with `$rt`', node.pos)
+					}
 				}
 			}
 		}
diff --git a/vlib/v/checker/tests/compare_unsigned_signed.out b/vlib/v/checker/tests/compare_unsigned_signed.out
index 23b98f95b1..29e68d85a1 100644
--- a/vlib/v/checker/tests/compare_unsigned_signed.out
+++ b/vlib/v/checker/tests/compare_unsigned_signed.out
@@ -17,10 +17,45 @@ vlib/v/checker/tests/compare_unsigned_signed.vv:10:16: error: `u8` cannot be com
    10 |     _ = u8(-1) == -1 // false!
       |                   ~~
    11 |     _ = -1 == u16(-1) // false!
-   12 | }
+   12 |
 vlib/v/checker/tests/compare_unsigned_signed.vv:11:6: error: negative value cannot be compared with `u16`
     9 |     // unsigned == literal
    10 |     _ = u8(-1) == -1 // false!
    11 |     _ = -1 == u16(-1) // false!
       |         ~~
-   12 | }
+   12 |     
+   13 |     // smaller unsigned == signed, OK
+vlib/v/checker/tests/compare_unsigned_signed.vv:18:12: error: `i8` cannot be compared with `u16`
+   16 |     
+   17 |     // smaller signed == unsigned, NG
+   18 |     _ = i8(0) == u16(0)
+      |               ~~
+   19 |     _ = i16(0) != u32(0)
+   20 |     _ = int(0) == u64(0)
+vlib/v/checker/tests/compare_unsigned_signed.vv:19:13: error: `i16` cannot be compared with `u32`
+   17 |     // smaller signed == unsigned, NG
+   18 |     _ = i8(0) == u16(0)
+   19 |     _ = i16(0) != u32(0)
+      |                ~~
+   20 |     _ = int(0) == u64(0)
+   21 |     _ = i32(0) == u64(0) // FIXME
+vlib/v/checker/tests/compare_unsigned_signed.vv:20:13: error: `int` cannot be compared with `u64`
+   18 |     _ = i8(0) == u16(0)
+   19 |     _ = i16(0) != u32(0)
+   20 |     _ = int(0) == u64(0)
+      |                ~~
+   21 |     _ = i32(0) == u64(0) // FIXME
+   22 |     // swap order
+vlib/v/checker/tests/compare_unsigned_signed.vv:23:13: error: `u16` cannot be compared with `i8`
+   21 |     _ = i32(0) == u64(0) // FIXME
+   22 |     // swap order
+   23 |     _ = u16(0) == i8(0)
+      |                ~~
+   24 |     _ = u64(0) == i16(0)
+   25 | }
+vlib/v/checker/tests/compare_unsigned_signed.vv:24:13: error: `u64` cannot be compared with `i16`
+   22 |     // swap order
+   23 |     _ = u16(0) == i8(0)
+   24 |     _ = u64(0) == i16(0)
+      |                ~~
+   25 | }
diff --git a/vlib/v/checker/tests/compare_unsigned_signed.vv b/vlib/v/checker/tests/compare_unsigned_signed.vv
index f79d9f4138..34a96acf48 100644
--- a/vlib/v/checker/tests/compare_unsigned_signed.vv
+++ b/vlib/v/checker/tests/compare_unsigned_signed.vv
@@ -9,4 +9,17 @@ fn main() {
 	// unsigned == literal
 	_ = u8(-1) == -1 // false!
 	_ = -1 == u16(-1) // false!
+	
+	// smaller unsigned == signed, OK
+	_ = u16(-1) == int(-1)
+	_ = int(-1) != u8(-1)
+	
+	// smaller signed == unsigned, NG
+	_ = i8(0) == u16(0)
+	_ = i16(0) != u32(0)
+	_ = int(0) == u64(0)
+	_ = i32(0) == u64(0) // FIXME
+	// swap order
+	_ = u16(0) == i8(0)
+	_ = u64(0) == i16(0)
 }
diff --git a/vlib/v/tests/infix_expr_test.v b/vlib/v/tests/infix_expr_test.v
index 38eeb44eda..aa099f9a67 100644
--- a/vlib/v/tests/infix_expr_test.v
+++ b/vlib/v/tests/infix_expr_test.v
@@ -42,9 +42,9 @@ fn test_cmp_u32_and_signed() {
 
 fn test_cmp_signed_and_u64() {
 	// ==
-	assert int(1) == u64(1)
+	// assert int(1) == u64(1)
 	// !=
-	assert int(1) != u64(2)
+	// assert int(1) != u64(2)
 	// >
 	assert !(int(1) > u64(1))
 	assert int(1) > u64(0)
@@ -63,9 +63,9 @@ fn test_cmp_signed_and_u64() {
 
 fn test_cmp_u64_and_signed() {
 	// ==
-	assert u64(1) == int(1)
+	// assert u64(1) == int(1)
 	// !=
-	assert u64(2) != int(1)
+	// assert u64(2) != int(1)
 	// >
 	assert !(u64(1) > int(1))
 	assert u64(1) > int(0)
diff --git a/vlib/v/tests/shift_test.v b/vlib/v/tests/shift_test.v
index e3480bd5fc..69e6694428 100644
--- a/vlib/v/tests/shift_test.v
+++ b/vlib/v/tests/shift_test.v
@@ -58,13 +58,13 @@ fn test_shift_operators() {
 	assert e == a
 	mut e3 := u64(1)
 	e3 <<= u32(i)
-	assert e3 == b
+	assert e3 == u64(b)
 	e3 >>= u32(i)
 	assert e == a
 	e3 <<= u64(i)
-	assert e3 == b
+	assert e3 == u64(b)
 	e3 >>= u64(i)
-	assert e3 == a
+	assert e3 == u64(a)
 	// Test shifts with custom int types
 	x := MyInt(2)
 	assert x << 2 == 8