From 5497f4c6354ad8c3d46444991cc4f3a39b0ce163 Mon Sep 17 00:00:00 2001 From: Turiiya Date: Sat, 4 May 2024 17:41:46 +0200 Subject: [PATCH 1/5] tools.vet: add notice for empty strings conditions --- cmd/tools/vvet/tests/empty_string.out | 8 +++++++ cmd/tools/vvet/tests/empty_string.vv | 13 +++++++++++ cmd/tools/vvet/vvet.v | 32 +++++++++++++++++++++++++++ 3 files changed, 53 insertions(+) create mode 100644 cmd/tools/vvet/tests/empty_string.out create mode 100644 cmd/tools/vvet/tests/empty_string.vv diff --git a/cmd/tools/vvet/tests/empty_string.out b/cmd/tools/vvet/tests/empty_string.out new file mode 100644 index 00000000000000..f1d183e27bc581 --- /dev/null +++ b/cmd/tools/vvet/tests/empty_string.out @@ -0,0 +1,8 @@ +cmd/tools/vvet/tests/empty_string.vv:3: notice: Use `foo == ''` instead of `foo.len == 0` +cmd/tools/vvet/tests/empty_string.vv:4: notice: Use `foo != ''` instead of `foo.len != 0` +cmd/tools/vvet/tests/empty_string.vv:5: notice: Use `foo == ''` instead of `foo.len < 1` +cmd/tools/vvet/tests/empty_string.vv:6: notice: Use `foo != ''` instead of `foo.len > 0` +cmd/tools/vvet/tests/empty_string.vv:9: notice: Use `'' == bar` instead of `0 == bar.len` +cmd/tools/vvet/tests/empty_string.vv:10: notice: Use `'' != bar` instead of `0 != bar.len` +cmd/tools/vvet/tests/empty_string.vv:11: notice: Use `'' == bar` instead of `1 > bar.len` +cmd/tools/vvet/tests/empty_string.vv:12: notice: Use `'' != bar` instead of `0 < bar.len` diff --git a/cmd/tools/vvet/tests/empty_string.vv b/cmd/tools/vvet/tests/empty_string.vv new file mode 100644 index 00000000000000..aaf9dff0b35748 --- /dev/null +++ b/cmd/tools/vvet/tests/empty_string.vv @@ -0,0 +1,13 @@ +fn main() { + foo := 'foo' + foo.len == 0 + foo.len != 0 + foo.len < 1 + foo.len > 0 + + bar := 'bar' + 0 == bar.len + 0 != bar.len + 1 > bar.len + 0 < bar.len +} diff --git a/cmd/tools/vvet/vvet.v b/cmd/tools/vvet/vvet.v index 3d263fbcc66ebb..a91af57b72c244 100644 --- a/cmd/tools/vvet/vvet.v +++ b/cmd/tools/vvet/vvet.v @@ -289,6 +289,7 @@ fn (mut vt Vet) expr(expr ast.Expr) { } ast.InfixExpr { vt.vet_in_condition(expr) + vt.vet_empty_str(expr) vt.expr(expr.right) } ast.CallExpr { @@ -320,6 +321,37 @@ fn (mut vt Vet) const_decl(stmt ast.ConstDecl) { } } +fn (mut vt Vet) vet_empty_str(expr ast.InfixExpr) { + if expr.left is ast.SelectorExpr && expr.right is ast.IntegerLiteral + && expr.left.field_name == 'len' { + if expr.right.val == '0' && expr.op != .lt { + // TODO: remove as when values can be smart casted. + expr_str := (expr.left as ast.SelectorExpr).expr.str() + op := if expr.op == .gt { '!=' } else { expr.op.str() } + vt.notice("Use `${expr_str} ${op} ''` instead of `${expr_str}.len ${expr.op} 0`", + expr.pos.line_nr, .unknown) + } else if expr.right.val == '1' && expr.op in [.lt, .gt] { + expr_str := (expr.left as ast.SelectorExpr).expr.str() + op := if expr.op == .lt { '==' } else { '!=' } + vt.notice("Use `${expr_str} ${op} ''` instead of `${expr_str}.len ${expr.op} 1`", + expr.pos.line_nr, .unknown) + } + } else if expr.left is ast.IntegerLiteral && expr.right is ast.SelectorExpr + && expr.right.field_name == 'len' { + if (expr.left as ast.IntegerLiteral).val == '0' && expr.op != .gt { + expr_str := expr.right.expr.str() + op := if expr.op == .lt { '!=' } else { expr.op.str() } + vt.notice("Use `'' ${op} ${expr_str}` instead of `0 ${expr.op} ${expr_str}.len`", + expr.pos.line_nr, .unknown) + } else if (expr.left as ast.IntegerLiteral).val == '1' && expr.op in [.lt, .gt] { + expr_str := expr.right.expr.str() + op := if expr.op == .lt { '!=' } else { '==' } + vt.notice("Use `'' ${op} ${expr_str}` instead of `1 ${expr.op} ${expr_str}.len`", + expr.pos.line_nr, .unknown) + } + } +} + fn (vt &Vet) vprintln(s string) { if !vt.opt.is_verbose { return From 13113a49ce95cdde99ca76dfb95be8eb6a1b3ec7 Mon Sep 17 00:00:00 2001 From: Turiiya Date: Tue, 7 May 2024 12:00:30 +0200 Subject: [PATCH 2/5] extend detected cases --- cmd/tools/vvet/tests/empty_string.out | 18 +++++++++++------- cmd/tools/vvet/tests/empty_string.vv | 15 +++++++-------- cmd/tools/vvet/vvet.v | 11 +++++++++-- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/cmd/tools/vvet/tests/empty_string.out b/cmd/tools/vvet/tests/empty_string.out index f1d183e27bc581..94a0449f9aecbf 100644 --- a/cmd/tools/vvet/tests/empty_string.out +++ b/cmd/tools/vvet/tests/empty_string.out @@ -1,8 +1,12 @@ -cmd/tools/vvet/tests/empty_string.vv:3: notice: Use `foo == ''` instead of `foo.len == 0` +cmd/tools/vvet/tests/empty_string.vv:3: notice: Use `foo == ''` instead of `foo.len < 1` +cmd/tools/vvet/tests/empty_string.vv:4: notice: Use `foo != ''` instead of `foo.len > 0` +cmd/tools/vvet/tests/empty_string.vv:4: notice: Use `foo == ''` instead of `foo.len == 0` cmd/tools/vvet/tests/empty_string.vv:4: notice: Use `foo != ''` instead of `foo.len != 0` -cmd/tools/vvet/tests/empty_string.vv:5: notice: Use `foo == ''` instead of `foo.len < 1` -cmd/tools/vvet/tests/empty_string.vv:6: notice: Use `foo != ''` instead of `foo.len > 0` -cmd/tools/vvet/tests/empty_string.vv:9: notice: Use `'' == bar` instead of `0 == bar.len` -cmd/tools/vvet/tests/empty_string.vv:10: notice: Use `'' != bar` instead of `0 != bar.len` -cmd/tools/vvet/tests/empty_string.vv:11: notice: Use `'' == bar` instead of `1 > bar.len` -cmd/tools/vvet/tests/empty_string.vv:12: notice: Use `'' != bar` instead of `0 < bar.len` +cmd/tools/vvet/tests/empty_string.vv:7: notice: Use `'' == bar` instead of `1 > bar.len` +cmd/tools/vvet/tests/empty_string.vv:8: notice: Use `'' != bar` instead of `0 < bar.len` +cmd/tools/vvet/tests/empty_string.vv:8: notice: Use `'' == bar` instead of `0 == bar.len` +cmd/tools/vvet/tests/empty_string.vv:8: notice: Use `'' != bar` instead of `0 != bar.len` +cmd/tools/vvet/tests/empty_string.vv:10: notice: Use `foo == ''` instead of `foo.len < 1` +cmd/tools/vvet/tests/empty_string.vv:10: notice: Use `bar != ''` instead of `bar.len > 0` +cmd/tools/vvet/tests/empty_string.vv:10: notice: Use `foo == ''` instead of `foo.len == 0` +cmd/tools/vvet/tests/empty_string.vv:10: notice: Use `bar == ''` instead of `bar.len == 0` diff --git a/cmd/tools/vvet/tests/empty_string.vv b/cmd/tools/vvet/tests/empty_string.vv index aaf9dff0b35748..c40045168a5ce2 100644 --- a/cmd/tools/vvet/tests/empty_string.vv +++ b/cmd/tools/vvet/tests/empty_string.vv @@ -1,13 +1,12 @@ fn main() { foo := 'foo' - foo.len == 0 - foo.len != 0 - foo.len < 1 - foo.len > 0 + _ := foo.len < 1 + _ := foo.len > 0 || foo.len == 0 || foo.len != 0 bar := 'bar' - 0 == bar.len - 0 != bar.len - 1 > bar.len - 0 < bar.len + _ := 1 > bar.len + _ := 0 < bar.len || 0 == bar.len || 0 != bar.len + + if foo.len < 1 || bar.len > 0 || (foo.len == 0 && bar.len == 0) { + } } diff --git a/cmd/tools/vvet/vvet.v b/cmd/tools/vvet/vvet.v index a91af57b72c244..0997730e53b437 100644 --- a/cmd/tools/vvet/vvet.v +++ b/cmd/tools/vvet/vvet.v @@ -292,6 +292,9 @@ fn (mut vt Vet) expr(expr ast.Expr) { vt.vet_empty_str(expr) vt.expr(expr.right) } + ast.ParExpr { + vt.expr(expr.expr) + } ast.CallExpr { vt.expr(expr.left) vt.exprs(expr.args.map(it.expr)) @@ -322,10 +325,14 @@ fn (mut vt Vet) const_decl(stmt ast.ConstDecl) { } fn (mut vt Vet) vet_empty_str(expr ast.InfixExpr) { - if expr.left is ast.SelectorExpr && expr.right is ast.IntegerLiteral + if expr.left is ast.InfixExpr { + vt.expr(expr.left) + } else if expr.right is ast.InfixExpr { + vt.expr(expr.right) + } else if expr.left is ast.SelectorExpr && expr.right is ast.IntegerLiteral && expr.left.field_name == 'len' { if expr.right.val == '0' && expr.op != .lt { - // TODO: remove as when values can be smart casted. + // TODO: remove as-casts when values can be smart casted. expr_str := (expr.left as ast.SelectorExpr).expr.str() op := if expr.op == .gt { '!=' } else { expr.op.str() } vt.notice("Use `${expr_str} ${op} ''` instead of `${expr_str}.len ${expr.op} 0`", From 958fb8405de5c109122fbcad3f1857724abd96a2 Mon Sep 17 00:00:00 2001 From: Turiiya Date: Tue, 7 May 2024 13:08:12 +0200 Subject: [PATCH 3/5] update --- cmd/tools/vvet/vvet.v | 53 +++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/cmd/tools/vvet/vvet.v b/cmd/tools/vvet/vvet.v index 0997730e53b437..e6d6d108f0ce60 100644 --- a/cmd/tools/vvet/vvet.v +++ b/cmd/tools/vvet/vvet.v @@ -329,32 +329,35 @@ fn (mut vt Vet) vet_empty_str(expr ast.InfixExpr) { vt.expr(expr.left) } else if expr.right is ast.InfixExpr { vt.expr(expr.right) - } else if expr.left is ast.SelectorExpr && expr.right is ast.IntegerLiteral - && expr.left.field_name == 'len' { - if expr.right.val == '0' && expr.op != .lt { - // TODO: remove as-casts when values can be smart casted. - expr_str := (expr.left as ast.SelectorExpr).expr.str() - op := if expr.op == .gt { '!=' } else { expr.op.str() } - vt.notice("Use `${expr_str} ${op} ''` instead of `${expr_str}.len ${expr.op} 0`", - expr.pos.line_nr, .unknown) - } else if expr.right.val == '1' && expr.op in [.lt, .gt] { - expr_str := (expr.left as ast.SelectorExpr).expr.str() - op := if expr.op == .lt { '==' } else { '!=' } - vt.notice("Use `${expr_str} ${op} ''` instead of `${expr_str}.len ${expr.op} 1`", - expr.pos.line_nr, .unknown) + } else if expr.left is ast.SelectorExpr && expr.right is ast.IntegerLiteral { + operand := (expr.left as ast.SelectorExpr) // TODO: remove as-casts when multiple conds can be smart-casted. + if operand.expr is ast.Ident && operand.expr.info.typ == ast.string_type_idx + && operand.field_name == 'len' { + if expr.right.val == '0' && expr.op != .lt { + // Case: `var.len > 0`, `var.len == 0`, `var.len != 0` + op := if expr.op == .gt { '!=' } else { expr.op.str() } + vt.notice("Use `${operand.expr.name} ${op} ''` instead of `${operand.expr.name}.len ${expr.op} 0`", + expr.pos.line_nr, .unknown) + } else if expr.right.val == '1' && expr.op == .lt { + // Case: `var.len < 1` + vt.notice("Use `${operand.expr.name} == ''` instead of `${operand.expr.name}.len ${expr.op} 1`", + expr.pos.line_nr, .unknown) + } } - } else if expr.left is ast.IntegerLiteral && expr.right is ast.SelectorExpr - && expr.right.field_name == 'len' { - if (expr.left as ast.IntegerLiteral).val == '0' && expr.op != .gt { - expr_str := expr.right.expr.str() - op := if expr.op == .lt { '!=' } else { expr.op.str() } - vt.notice("Use `'' ${op} ${expr_str}` instead of `0 ${expr.op} ${expr_str}.len`", - expr.pos.line_nr, .unknown) - } else if (expr.left as ast.IntegerLiteral).val == '1' && expr.op in [.lt, .gt] { - expr_str := expr.right.expr.str() - op := if expr.op == .lt { '!=' } else { '==' } - vt.notice("Use `'' ${op} ${expr_str}` instead of `1 ${expr.op} ${expr_str}.len`", - expr.pos.line_nr, .unknown) + } else if expr.left is ast.IntegerLiteral && expr.right is ast.SelectorExpr { + operand := expr.right + if operand.expr is ast.Ident && operand.expr.info.typ == ast.string_type_idx + && operand.field_name == 'len' { + if (expr.left as ast.IntegerLiteral).val == '0' && expr.op != .gt { + // Case: `0 < var.len`, `0 == var.len`, `0 != var.len` + op := if expr.op == .lt { '!=' } else { expr.op.str() } + vt.notice("Use `'' ${op} ${operand.expr.name}` instead of `0 ${expr.op} ${operand.expr.name}.len`", + expr.pos.line_nr, .unknown) + } else if (expr.left as ast.IntegerLiteral).val == '1' && expr.op == .gt { + // Case: `1 > var.len` + vt.notice("Use `'' == ${operand.expr.name}` instead of `1 ${expr.op} ${operand.expr.name}.len`", + expr.pos.line_nr, .unknown) + } } } } From 08f73353e533736be76a01725d3fb1331c4ecc8c Mon Sep 17 00:00:00 2001 From: Turiiya Date: Sun, 19 May 2024 12:54:22 +0200 Subject: [PATCH 4/5] add tests when using `.len` on other types --- cmd/tools/vvet/tests/empty_string.vv | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/tools/vvet/tests/empty_string.vv b/cmd/tools/vvet/tests/empty_string.vv index c40045168a5ce2..64aa7b610bd2cd 100644 --- a/cmd/tools/vvet/tests/empty_string.vv +++ b/cmd/tools/vvet/tests/empty_string.vv @@ -9,4 +9,13 @@ fn main() { if foo.len < 1 || bar.len > 0 || (foo.len == 0 && bar.len == 0) { } + + // Should not notify when `.len` is used with other types. + baz := ['baz'] + _ := baz.len == 0 + + foobar := { + 'foo': 'bar' + } + _ := foobar.len < 1 } From 499bc433c4bb683e6c44d40ef852a6eece5b9730 Mon Sep 17 00:00:00 2001 From: Turiiya Date: Sun, 19 May 2024 13:26:34 +0200 Subject: [PATCH 5/5] sort operator token condition first --- cmd/tools/vvet/vvet.v | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/tools/vvet/vvet.v b/cmd/tools/vvet/vvet.v index e6d6d108f0ce60..bf7ddc5cb80995 100644 --- a/cmd/tools/vvet/vvet.v +++ b/cmd/tools/vvet/vvet.v @@ -333,12 +333,12 @@ fn (mut vt Vet) vet_empty_str(expr ast.InfixExpr) { operand := (expr.left as ast.SelectorExpr) // TODO: remove as-casts when multiple conds can be smart-casted. if operand.expr is ast.Ident && operand.expr.info.typ == ast.string_type_idx && operand.field_name == 'len' { - if expr.right.val == '0' && expr.op != .lt { + if expr.op != .lt && expr.right.val == '0' { // Case: `var.len > 0`, `var.len == 0`, `var.len != 0` op := if expr.op == .gt { '!=' } else { expr.op.str() } vt.notice("Use `${operand.expr.name} ${op} ''` instead of `${operand.expr.name}.len ${expr.op} 0`", expr.pos.line_nr, .unknown) - } else if expr.right.val == '1' && expr.op == .lt { + } else if expr.op == .lt && expr.right.val == '1' { // Case: `var.len < 1` vt.notice("Use `${operand.expr.name} == ''` instead of `${operand.expr.name}.len ${expr.op} 1`", expr.pos.line_nr, .unknown) @@ -348,12 +348,12 @@ fn (mut vt Vet) vet_empty_str(expr ast.InfixExpr) { operand := expr.right if operand.expr is ast.Ident && operand.expr.info.typ == ast.string_type_idx && operand.field_name == 'len' { - if (expr.left as ast.IntegerLiteral).val == '0' && expr.op != .gt { + if expr.op != .gt && (expr.left as ast.IntegerLiteral).val == '0' { // Case: `0 < var.len`, `0 == var.len`, `0 != var.len` op := if expr.op == .lt { '!=' } else { expr.op.str() } vt.notice("Use `'' ${op} ${operand.expr.name}` instead of `0 ${expr.op} ${operand.expr.name}.len`", expr.pos.line_nr, .unknown) - } else if (expr.left as ast.IntegerLiteral).val == '1' && expr.op == .gt { + } else if expr.op == .gt && (expr.left as ast.IntegerLiteral).val == '1' { // Case: `1 > var.len` vt.notice("Use `'' == ${operand.expr.name}` instead of `1 ${expr.op} ${operand.expr.name}.len`", expr.pos.line_nr, .unknown)