From d56373926460b6ff87b315a02746d9c2efa9c168 Mon Sep 17 00:00:00 2001
From: walking devel <104449470+walkingdevel@users.noreply.github.com>
Date: Tue, 31 Jan 2023 08:22:02 +0000
Subject: [PATCH] checker: add type checking for ORM `limit`, `offset`, and
 `order by`. (#17095)

---
 vlib/orm/orm_test.v                           |   7 +-
 vlib/v/checker/match.v                        |   6 +-
 vlib/v/checker/orm.v                          | 126 ++++++++++++++----
 vlib/v/checker/tests/orm_empty_struct.out     |   2 +-
 .../tests/orm_limit_less_than_zero_error.out  |   7 +
 .../tests/orm_limit_less_than_zero_error.vv   |  22 +++
 vlib/v/checker/tests/orm_no_default_value.out |   2 +-
 vlib/v/checker/tests/orm_not_a_struct.out     |   2 +-
 ...ing_non_struct_field_in_order_by_error.out |  14 ++
 ...sing_non_struct_field_in_order_by_error.vv |  18 +++
 vlib/v/gen/c/sql.v                            |   2 +-
 11 files changed, 176 insertions(+), 32 deletions(-)
 create mode 100644 vlib/v/checker/tests/orm_limit_less_than_zero_error.out
 create mode 100644 vlib/v/checker/tests/orm_limit_less_than_zero_error.vv
 create mode 100644 vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.out
 create mode 100644 vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.vv

diff --git a/vlib/orm/orm_test.v b/vlib/orm/orm_test.v
index 034e82111..91c080112 100644
--- a/vlib/orm/orm_test.v
+++ b/vlib/orm/orm_test.v
@@ -3,6 +3,10 @@
 import time
 import db.sqlite
 
+const (
+	offset_const = 2
+)
+
 struct Module {
 	id           int       [primary; sql: serial]
 	name         string
@@ -233,16 +237,17 @@ fn test_orm() {
 	assert y.len == 2
 	assert y[0].id == 2
 
-	offset_const := 2
 	z := sql db {
 		select from User order by id limit 2 offset offset_const
 	}
 	assert z.len == 2
 	assert z[0].id == 3
+
 	oldest := sql db {
 		select from User order by age desc limit 1
 	}
 	assert oldest.age == 34
+
 	offs := 1
 	second_oldest := sql db {
 		select from User order by age desc limit 1 offset offs
diff --git a/vlib/v/checker/match.v b/vlib/v/checker/match.v
index 266b1425e..45ebb56a2 100644
--- a/vlib/v/checker/match.v
+++ b/vlib/v/checker/match.v
@@ -1,7 +1,6 @@
 module checker
 
 import v.ast
-import v.pref
 import v.util
 import v.token
 import strings
@@ -147,7 +146,10 @@ fn (mut c Checker) get_comptime_number_value(mut expr ast.Expr) ?i64 {
 		return expr.val.i64()
 	}
 	if mut expr is ast.Ident {
-		if mut obj := c.table.global_scope.find_const(expr.name) {
+		has_expr_mod_in_name := expr.name.contains('.')
+		expr_name := if has_expr_mod_in_name { expr.name } else { '${expr.mod}.${expr.name}' }
+
+		if mut obj := c.table.global_scope.find_const(expr_name) {
 			if obj.typ == 0 {
 				obj.typ = c.expr(obj.expr)
 			}
diff --git a/vlib/v/checker/orm.v b/vlib/v/checker/orm.v
index 120e41a46..fdf4a6943 100644
--- a/vlib/v/checker/orm.v
+++ b/vlib/v/checker/orm.v
@@ -4,9 +4,11 @@ module checker
 
 import v.ast
 import v.token
+import v.util
 
 const (
 	fkey_attr_name = 'fkey'
+	v_orm_prefix   = 'V ORM'
 )
 
 fn (mut c Checker) sql_expr(mut node ast.SqlExpr) ast.Type {
@@ -22,7 +24,7 @@ fn (mut c Checker) sql_expr(mut node ast.SqlExpr) ast.Type {
 		c.cur_orm_ts = old_ts
 	}
 	if sym.info !is ast.Struct {
-		c.error('the table symbol `${sym.name}` has to be a struct', node.table_expr.pos)
+		c.orm_error('the table symbol `${sym.name}` has to be a struct', node.table_expr.pos)
 		return ast.void_type
 	}
 	info := sym.info as ast.Struct
@@ -103,20 +105,42 @@ fn (mut c Checker) sql_expr(mut node ast.SqlExpr) ast.Type {
 		c.expr(node.where_expr)
 		c.check_expr_has_no_fn_calls_with_non_orm_return_type(&node.where_expr)
 	}
-	if node.has_offset {
-		c.expr(node.offset_expr)
+
+	if node.has_order {
+		if mut node.order_expr is ast.Ident {
+			order_ident_name := node.order_expr.name
+
+			sym.find_field(order_ident_name) or {
+				field_names := fields.map(it.name)
+
+				c.orm_error(util.new_suggestion(order_ident_name, field_names).say('`${sym.name}` structure has no field with name `${order_ident_name}`'),
+					node.order_expr.pos)
+				return ast.void_type
+			}
+		} else {
+			c.orm_error("expected `${sym.name}` structure's field", node.order_expr.pos())
+			return ast.void_type
+		}
+
+		c.expr(node.order_expr)
 	}
+
 	if node.has_limit {
 		c.expr(node.limit_expr)
+		c.check_sql_value_expr_is_comptime_with_natural_number_or_expr_with_int_type(mut node.limit_expr,
+			'limit')
 	}
-	if node.has_order {
-		c.expr(node.order_expr)
+
+	if node.has_offset {
+		c.expr(node.offset_expr)
+		c.check_sql_value_expr_is_comptime_with_natural_number_or_expr_with_int_type(mut node.offset_expr,
+			'offset')
 	}
 	c.expr(node.db_expr)
 
 	if node.or_expr.kind == .block {
 		if node.or_expr.stmts.len == 0 {
-			c.error('or block needs to return a default value', node.or_expr.pos)
+			c.orm_error('or block needs to return a default value', node.or_expr.pos)
 		}
 		if node.or_expr.stmts.len > 0 && node.or_expr.stmts.last() is ast.ExprStmt {
 			c.expected_or_type = node.typ
@@ -176,6 +200,7 @@ fn (mut c Checker) sql_stmt_line(mut node ast.SqlStmtLine) ast.Type {
 		c.error('unknown type `${table_sym.name}`', node.pos)
 		return ast.void_type
 	}
+
 	info := table_sym.info as ast.Struct
 	fields := c.fetch_and_verify_orm_fields(info, node.table_expr.pos, table_sym.name)
 	mut sub_structs := map[int]ast.SqlStmtLine{}
@@ -216,7 +241,7 @@ fn (mut c Checker) sql_stmt_line(mut node ast.SqlStmtLine) ast.Type {
 	for i, column in node.updated_columns {
 		x := node.fields.filter(it.name == column)
 		if x.len == 0 {
-			c.error('type `${table_sym.name}` has no field named `${column}`', node.pos)
+			c.orm_error('type `${table_sym.name}` has no field named `${column}`', node.pos)
 			continue
 		}
 		field := x[0]
@@ -241,19 +266,19 @@ fn (mut c Checker) check_orm_struct_field_attributes(field ast.StructField) {
 	for attr in field.attrs {
 		if attr.name == checker.fkey_attr_name {
 			if field_type.kind != .array && field_type.kind != .struct_ {
-				c.error('The `${checker.fkey_attr_name}` attribute must be used only with arrays and structures',
+				c.orm_error('the `${checker.fkey_attr_name}` attribute must be used only with arrays and structures',
 					attr.pos)
 				return
 			}
 
 			if !attr.has_arg {
-				c.error('The `${checker.fkey_attr_name}` attribute must have an argument',
+				c.orm_error('the `${checker.fkey_attr_name}` attribute must have an argument',
 					attr.pos)
 				return
 			}
 
 			if attr.kind != .string {
-				c.error('`${checker.fkey_attr_name}` attribute must be string. Try [${checker.fkey_attr_name}: \'${attr.arg}\'] instead of [${checker.fkey_attr_name}: ${attr.arg}]',
+				c.orm_error("`${checker.fkey_attr_name}` attribute must be string. Try [${checker.fkey_attr_name}: '${attr.arg}'] instead of [${checker.fkey_attr_name}: ${attr.arg}]",
 					attr.pos)
 				return
 			}
@@ -265,7 +290,7 @@ fn (mut c Checker) check_orm_struct_field_attributes(field ast.StructField) {
 			}
 
 			field_struct_type.find_field(attr.arg) or {
-				c.error('`${field_struct_type.name}` struct has no field with name `${attr.arg}`',
+				c.orm_error('`${field_struct_type.name}` struct has no field with name `${attr.arg}`',
 					attr.pos)
 				return
 			}
@@ -275,7 +300,7 @@ fn (mut c Checker) check_orm_struct_field_attributes(field ast.StructField) {
 	}
 
 	if field_type.kind == .array && !has_fkey_attr {
-		c.error('A field that holds an array must be defined with the `${checker.fkey_attr_name}` attribute',
+		c.orm_error('a field that holds an array must be defined with the `${checker.fkey_attr_name}` attribute',
 			field.pos)
 	}
 }
@@ -288,15 +313,61 @@ fn (mut c Checker) fetch_and_verify_orm_fields(info ast.Struct, pos token.Pos, t
 		&& c.table.sym(c.table.sym(it.typ).array_info().elem_type).kind == .struct_))
 		&& !it.attrs.contains('skip'))
 	if fields.len == 0 {
-		c.error('V orm: select: empty fields in `${table_name}`', pos)
+		c.orm_error('select: empty fields in `${table_name}`', pos)
 		return []ast.StructField{}
 	}
 	if fields[0].name != 'id' {
-		c.error('V orm: `id int` must be the first field in `${table_name}`', pos)
+		c.orm_error('`id int` must be the first field in `${table_name}`', pos)
 	}
 	return fields
 }
 
+// check_sql_value_expr_is_comptime_with_natural_number_or_expr_with_int_type checks that an expression is compile-time
+// and contains an integer greater than or equal to zero or it is a runtime expression with an integer type.
+fn (mut c Checker) check_sql_value_expr_is_comptime_with_natural_number_or_expr_with_int_type(mut expr ast.Expr, sql_keyword string) {
+	comptime_number := c.get_comptime_number_value(mut expr) or {
+		c.check_sql_expr_type_is_int(expr, sql_keyword)
+		return
+	}
+
+	if comptime_number < 0 {
+		c.orm_error('`${sql_keyword}` must be greater than or equal to zero', expr.pos())
+	}
+}
+
+fn (mut c Checker) check_sql_expr_type_is_int(expr &ast.Expr, sql_keyword string) {
+	if expr is ast.Ident {
+		if expr.obj.typ.is_int() {
+			return
+		}
+	} else if expr is ast.CallExpr {
+		if expr.return_type == 0 {
+			return
+		}
+
+		type_symbol := c.table.sym(expr.return_type)
+		is_error_type := expr.return_type.has_flag(.result) || expr.return_type.has_flag(.option)
+		is_acceptable_type := type_symbol.is_int() && !is_error_type
+
+		if !is_acceptable_type {
+			error_type_symbol := c.fn_return_type_flag_to_string(expr.return_type)
+			c.orm_error('function calls in `${sql_keyword}` must return only an integer type, but `${expr.name}` returns `${error_type_symbol}${type_symbol.name}`',
+				expr.pos)
+		}
+
+		return
+	} else if expr is ast.ParExpr {
+		c.check_sql_expr_type_is_int(&expr.expr, sql_keyword)
+		return
+	}
+
+	c.orm_error('the type of `${sql_keyword}` must be an integer type', expr.pos())
+}
+
+fn (mut c Checker) orm_error(message string, pos token.Pos) {
+	c.error('${checker.v_orm_prefix}: ${message}', pos)
+}
+
 // check_expr_has_no_fn_calls_with_non_orm_return_type checks that an expression has no function calls
 // that return complex types which can't be transformed into SQL.
 fn (mut c Checker) check_expr_has_no_fn_calls_with_non_orm_return_type(expr &ast.Expr) {
@@ -312,23 +383,15 @@ fn (mut c Checker) check_expr_has_no_fn_calls_with_non_orm_return_type(expr &ast
 		}
 
 		type_symbol := c.table.sym(expr.return_type)
-		is_result_type := expr.return_type.has_flag(.result)
-		is_option_type := expr.return_type.has_flag(.option)
 		is_time := type_symbol.cname == 'time__Time'
 		is_not_pointer := !type_symbol.is_pointer()
-		is_error_type := is_result_type || is_option_type
+		is_error_type := expr.return_type.has_flag(.result) || expr.return_type.has_flag(.option)
 		is_acceptable_type := (type_symbol.is_primitive() || is_time) && is_not_pointer
 			&& !is_error_type
 
 		if !is_acceptable_type {
-			error_type_symbol := if is_result_type {
-				'!'
-			} else if is_option_type {
-				'?'
-			} else {
-				''
-			}
-			c.error('V ORM: function calls must return only primitive types and time.Time, but `${expr.name}` returns `${error_type_symbol}${type_symbol.name}`',
+			error_type_symbol := c.fn_return_type_flag_to_string(expr.return_type)
+			c.orm_error('function calls must return only primitive types and time.Time, but `${expr.name}` returns `${error_type_symbol}${type_symbol.name}`',
 				expr.pos)
 		}
 	} else if expr is ast.ParExpr {
@@ -338,3 +401,16 @@ fn (mut c Checker) check_expr_has_no_fn_calls_with_non_orm_return_type(expr &ast
 		c.check_expr_has_no_fn_calls_with_non_orm_return_type(&expr.right)
 	}
 }
+
+fn (_ &Checker) fn_return_type_flag_to_string(typ ast.Type) string {
+	is_result_type := typ.has_flag(.result)
+	is_option_type := typ.has_flag(.option)
+
+	return if is_result_type {
+		'!'
+	} else if is_option_type {
+		'?'
+	} else {
+		''
+	}
+}
diff --git a/vlib/v/checker/tests/orm_empty_struct.out b/vlib/v/checker/tests/orm_empty_struct.out
index 2006d8315..493799347 100644
--- a/vlib/v/checker/tests/orm_empty_struct.out
+++ b/vlib/v/checker/tests/orm_empty_struct.out
@@ -1,4 +1,4 @@
-vlib/v/checker/tests/orm_empty_struct.vv:9:15: error: V orm: select: empty fields in `Person`
+vlib/v/checker/tests/orm_empty_struct.vv:9:15: error: V ORM: select: empty fields in `Person`
     7 |     db := sqlite.connect(':memory:')!
     8 |     _ := sql db {
     9 |         select from Person
diff --git a/vlib/v/checker/tests/orm_limit_less_than_zero_error.out b/vlib/v/checker/tests/orm_limit_less_than_zero_error.out
new file mode 100644
index 000000000..ab0899e17
--- /dev/null
+++ b/vlib/v/checker/tests/orm_limit_less_than_zero_error.out
@@ -0,0 +1,7 @@
+vlib/v/checker/tests/orm_limit_less_than_zero_error.vv:18:26: error: V ORM: `limit` must be greater than or equal to zero
+   16 | 
+   17 |     users := sql db {
+   18 |         select from User limit user_limit
+      |                                ~~~~~~~~~~
+   19 |     }
+   20 |
diff --git a/vlib/v/checker/tests/orm_limit_less_than_zero_error.vv b/vlib/v/checker/tests/orm_limit_less_than_zero_error.vv
new file mode 100644
index 000000000..2b76c7017
--- /dev/null
+++ b/vlib/v/checker/tests/orm_limit_less_than_zero_error.vv
@@ -0,0 +1,22 @@
+module main
+
+import db.sqlite
+
+const (
+	user_limit = -10
+)
+
+struct User {
+	id       int    [primary; sql: serial]
+	username string
+}
+
+fn main() {
+	db := sqlite.connect(':memory:') or { panic(err) }
+
+	users := sql db {
+		select from User limit user_limit
+	}
+
+	println(users)
+}
diff --git a/vlib/v/checker/tests/orm_no_default_value.out b/vlib/v/checker/tests/orm_no_default_value.out
index 707e137b3..7df9362c1 100644
--- a/vlib/v/checker/tests/orm_no_default_value.out
+++ b/vlib/v/checker/tests/orm_no_default_value.out
@@ -1,4 +1,4 @@
-vlib/v/checker/tests/orm_no_default_value.vv:11:4: error: or block needs to return a default value
+vlib/v/checker/tests/orm_no_default_value.vv:11:4: error: V ORM: or block needs to return a default value
     9 |     _ := sql db {
    10 |         select from Person
    11 |     } or {
diff --git a/vlib/v/checker/tests/orm_not_a_struct.out b/vlib/v/checker/tests/orm_not_a_struct.out
index 3948231c3..66bf20aa8 100644
--- a/vlib/v/checker/tests/orm_not_a_struct.out
+++ b/vlib/v/checker/tests/orm_not_a_struct.out
@@ -1,4 +1,4 @@
-vlib/v/checker/tests/orm_not_a_struct.vv:10:15: error: the table symbol `Person` has to be a struct
+vlib/v/checker/tests/orm_not_a_struct.vv:10:15: error: V ORM: the table symbol `Person` has to be a struct
     8 |     db := sqlite.connect(':memory:')!
     9 |     _ := sql db {
    10 |         select from Person
diff --git a/vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.out b/vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.out
new file mode 100644
index 000000000..da507cf32
--- /dev/null
+++ b/vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.out
@@ -0,0 +1,14 @@
+vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.vv:14:29: error: V ORM: `User` structure has no field with name `database`.
+2 possibilities: `id`, `username`.
+   12 | 
+   13 |     users := sql db {
+   14 |         select from User order by database
+      |                                   ~~~~~~~~
+   15 |     }
+   16 |
+vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.vv:17:2: error: `println` can not print void expressions
+   15 |     }
+   16 | 
+   17 |     println(users)
+      |     ~~~~~~~~~~~~~~
+   18 | }
diff --git a/vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.vv b/vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.vv
new file mode 100644
index 000000000..a31a0e11d
--- /dev/null
+++ b/vlib/v/checker/tests/orm_using_non_struct_field_in_order_by_error.vv
@@ -0,0 +1,18 @@
+module main
+
+import db.sqlite
+
+struct User {
+	id       int    [primary; sql: serial]
+	username string
+}
+
+fn main() {
+	db := sqlite.connect(':memory:') or { panic(err) }
+
+	users := sql db {
+		select from User order by database
+	}
+
+	println(users)
+}
diff --git a/vlib/v/gen/c/sql.v b/vlib/v/gen/c/sql.v
index 90642b4a8..5e09d7d84 100644
--- a/vlib/v/gen/c/sql.v
+++ b/vlib/v/gen/c/sql.v
@@ -307,7 +307,7 @@ fn (mut g Gen) sql_expr_to_orm_primitive(expr ast.Expr) {
 		}
 		else {
 			eprintln(expr)
-			verror('Unknown expr')
+			verror('V ORM: ${expr.type_name()} is not supported')
 		}
 	}
 }
-- 
2.30.2