From 02fc58d124dfeffa1ab18e1734dc938c056f4b82 Mon Sep 17 00:00:00 2001
From: walking devel <104449470+walkingdevel@users.noreply.github.com>
Date: Wed, 1 Feb 2023 08:53:34 +0000
Subject: [PATCH] orm: add type checker for `where` (#17179)

---
 vlib/v/ast/types.v                            |  6 +++
 vlib/v/checker/orm.v                          | 51 +++++++++++++++++--
 ...n_infix_expr_has_no_struct_field_error.out |  8 +++
 ...in_infix_expr_has_no_struct_field_error.vv | 22 ++++++++
 .../tests/orm_wrong_where_expr_error.out      |  7 +++
 .../tests/orm_wrong_where_expr_error.vv       | 19 +++++++
 6 files changed, 110 insertions(+), 3 deletions(-)
 create mode 100644 vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.out
 create mode 100644 vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.vv
 create mode 100644 vlib/v/checker/tests/orm_wrong_where_expr_error.out
 create mode 100644 vlib/v/checker/tests/orm_wrong_where_expr_error.vv

diff --git a/vlib/v/ast/types.v b/vlib/v/ast/types.v
index ce1979b13..bbbb6139b 100644
--- a/vlib/v/ast/types.v
+++ b/vlib/v/ast/types.v
@@ -1618,6 +1618,12 @@ pub fn (t &TypeSymbol) find_field(name string) ?StructField {
 	}
 }
 
+pub fn (t &TypeSymbol) has_field(name string) bool {
+	t.find_field(name) or { return false }
+
+	return true
+}
+
 fn (a &Aggregate) find_field(name string) ?StructField {
 	for mut field in unsafe { a.fields } {
 		if field.name == name {
diff --git a/vlib/v/checker/orm.v b/vlib/v/checker/orm.v
index fdf4a6943..f562f043a 100644
--- a/vlib/v/checker/orm.v
+++ b/vlib/v/checker/orm.v
@@ -42,9 +42,11 @@ fn (mut c Checker) sql_expr(mut node ast.SqlExpr) ast.Type {
 		} else {
 			ast.Type(0)
 		}
+
 		mut n := ast.SqlExpr{
 			pos: node.pos
 			has_where: true
+			where_expr: ast.None{}
 			typ: typ
 			db_expr: node.db_expr
 			table_expr: ast.TypeNode{
@@ -52,6 +54,7 @@ fn (mut c Checker) sql_expr(mut node ast.SqlExpr) ast.Type {
 				typ: typ
 			}
 		}
+
 		tmp_inside_sql := c.inside_sql
 		c.sql_expr(mut n)
 		c.inside_sql = tmp_inside_sql
@@ -100,19 +103,19 @@ fn (mut c Checker) sql_expr(mut node ast.SqlExpr) ast.Type {
 
 	node.fields = fields
 	node.sub_structs = sub_structs.move()
+	field_names := fields.map(it.name)
 
 	if node.has_where {
 		c.expr(node.where_expr)
 		c.check_expr_has_no_fn_calls_with_non_orm_return_type(&node.where_expr)
+		c.check_where_expr_has_no_pointless_exprs(sym, field_names, &node.where_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)
-
+			if !sym.has_field(order_ident_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
@@ -402,6 +405,48 @@ fn (mut c Checker) check_expr_has_no_fn_calls_with_non_orm_return_type(expr &ast
 	}
 }
 
+// check_where_expr_has_no_pointless_exprs checks that an expression has no pointless expressions
+// which don't affect the result. For example, `where 3` is pointless.
+// Also, it checks that the left side of the infix expression is always the structure field.
+fn (mut c Checker) check_where_expr_has_no_pointless_exprs(table_type_symbol &ast.TypeSymbol, field_names []string, expr &ast.Expr) {
+	// Skip type checking for generated subqueries
+	// that are not linked to scope and vars but only created for cgen.
+	if expr is ast.None {
+		return
+	}
+
+	if expr is ast.InfixExpr {
+		has_no_field_error := "left side of the `${expr.op}` expression must be one of the `${table_type_symbol.name}`'s fields"
+
+		if expr.left is ast.Ident {
+			left_ident_name := expr.left.name
+
+			if !table_type_symbol.has_field(left_ident_name) {
+				c.orm_error(util.new_suggestion(left_ident_name, field_names).say(has_no_field_error),
+					expr.left.pos)
+			}
+		} else if expr.left is ast.InfixExpr || expr.left is ast.ParExpr
+			|| expr.left is ast.PrefixExpr {
+			c.check_where_expr_has_no_pointless_exprs(table_type_symbol, field_names,
+				&expr.left)
+		} else {
+			c.orm_error(has_no_field_error, expr.left.pos())
+		}
+
+		if expr.right is ast.InfixExpr || expr.right is ast.ParExpr || expr.right is ast.PrefixExpr {
+			c.check_where_expr_has_no_pointless_exprs(table_type_symbol, field_names,
+				&expr.right)
+		}
+	} else if expr is ast.ParExpr {
+		c.check_where_expr_has_no_pointless_exprs(table_type_symbol, field_names, &expr.expr)
+	} else if expr is ast.PrefixExpr {
+		c.check_where_expr_has_no_pointless_exprs(table_type_symbol, field_names, &expr.right)
+	} else {
+		c.orm_error('`where` expression must have at least one comparison for filtering rows',
+			expr.pos())
+	}
+}
+
 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)
diff --git a/vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.out b/vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.out
new file mode 100644
index 000000000..91944f02d
--- /dev/null
+++ b/vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.out
@@ -0,0 +1,8 @@
+vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.vv:18:26: error: V ORM: left side of the `==` expression must be one of the `User`'s fields.
+1 possibility: `id`.
+   16 | 
+   17 |     users := sql db {
+   18 |         select from User where first == second
+      |                                ~~~~~
+   19 |     }
+   20 |
diff --git a/vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.vv b/vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.vv
new file mode 100644
index 000000000..851035566
--- /dev/null
+++ b/vlib/v/checker/tests/orm_left_side_expr_in_infix_expr_has_no_struct_field_error.vv
@@ -0,0 +1,22 @@
+import db.sqlite
+
+struct User {
+	id int [primary; sql: serial]
+}
+
+fn main() {
+	mut db := sqlite.connect(':memory:') or { panic(err) }
+
+	sql db {
+		create table User
+	}
+
+	first := 'first'
+	second := 'second'
+
+	users := sql db {
+		select from User where first == second
+	}
+
+	println(users)
+}
diff --git a/vlib/v/checker/tests/orm_wrong_where_expr_error.out b/vlib/v/checker/tests/orm_wrong_where_expr_error.out
new file mode 100644
index 000000000..5e5e897c3
--- /dev/null
+++ b/vlib/v/checker/tests/orm_wrong_where_expr_error.out
@@ -0,0 +1,7 @@
+vlib/v/checker/tests/orm_wrong_where_expr_error.vv:15:26: error: V ORM: `where` expression must have at least one comparison for filtering rows
+   13 | 
+   14 |     users := sql db {
+   15 |         select from User where 3
+      |                                ^
+   16 |     }
+   17 |
diff --git a/vlib/v/checker/tests/orm_wrong_where_expr_error.vv b/vlib/v/checker/tests/orm_wrong_where_expr_error.vv
new file mode 100644
index 000000000..c17e135ff
--- /dev/null
+++ b/vlib/v/checker/tests/orm_wrong_where_expr_error.vv
@@ -0,0 +1,19 @@
+import db.sqlite
+
+struct User {
+	id int [primary; sql: serial]
+}
+
+fn main() {
+	mut db := sqlite.connect(':memory:') or { panic(err) }
+
+	sql db {
+		create table User
+	}
+
+	users := sql db {
+		select from User where 3
+	}
+
+	println(users)
+}
-- 
2.30.2