Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools.vet: add notice for empty strings conditions #21421

Merged
merged 5 commits into from
May 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions cmd/tools/vvet/tests/empty_string.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
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: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`
21 changes: 21 additions & 0 deletions cmd/tools/vvet/tests/empty_string.vv
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
fn main() {
foo := 'foo'
_ := foo.len < 1
_ := foo.len > 0 || foo.len == 0 || foo.len != 0

bar := 'bar'
_ := 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) {
}

// Should not notify when `.len` is used with other types.
baz := ['baz']
_ := baz.len == 0

foobar := {
'foo': 'bar'
}
_ := foobar.len < 1
}
42 changes: 42 additions & 0 deletions cmd/tools/vvet/vvet.v
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,12 @@ 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.ParExpr {
vt.expr(expr.expr)
}
ast.CallExpr {
vt.expr(expr.left)
vt.exprs(expr.args.map(it.expr))
Expand Down Expand Up @@ -320,6 +324,44 @@ fn (mut vt Vet) const_decl(stmt ast.ConstDecl) {
}
}

fn (mut vt Vet) vet_empty_str(expr ast.InfixExpr) {
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 {
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.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.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)
}
}
} 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.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.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)
}
}
}
}

fn (vt &Vet) vprintln(s string) {
if !vt.opt.is_verbose {
return
Expand Down