From c662431cfd668be6ba3b8df36ac8e46dd2237e5b Mon Sep 17 00:00:00 2001 From: yuyi Date: Thu, 25 Aug 2022 13:52:13 +0800 Subject: [PATCH] checker: check unsafe array assign (fix #9651) (#15515) --- cmd/tools/vgret.v | 4 ++-- cmd/tools/vpm.v | 2 +- cmd/tools/vrepl.v | 2 +- vlib/datatypes/linked_list.v | 4 ++-- vlib/math/big/special_array_ops.v | 6 ++--- vlib/mysql/orm.v | 2 +- vlib/net/html/data_structures.v | 4 ++-- vlib/semver/parse.v | 2 +- vlib/v/ast/table.v | 6 ++--- vlib/v/checker/assign.v | 22 ++++++++++++++++--- vlib/v/checker/check_types.v | 4 ++-- vlib/v/checker/checker.v | 2 +- vlib/v/checker/return.v | 2 +- vlib/v/checker/tests/array_assign_err.out | 7 ++++++ vlib/v/checker/tests/array_assign_err.vv | 13 +++++++++++ .../checker/tests/array_or_map_assign_err.out | 8 +++---- vlib/v/fmt/fmt.v | 6 ++--- vlib/v/gen/c/cgen.v | 4 ++-- vlib/v/gen/golang/golang.v | 6 ++--- vlib/v/gen/js/fn.v | 4 ++-- vlib/v/parser/assign.v | 2 +- vlib/v/parser/fn.v | 2 +- vlib/v/parser/parser.v | 12 +++++----- vlib/v/tests/shared_elem_test.v | 6 ++--- 24 files changed, 84 insertions(+), 48 deletions(-) create mode 100644 vlib/v/checker/tests/array_assign_err.out create mode 100644 vlib/v/checker/tests/array_assign_err.vv diff --git a/cmd/tools/vgret.v b/cmd/tools/vgret.v index f196ef1bf..410911feb 100644 --- a/cmd/tools/vgret.v +++ b/cmd/tools/vgret.v @@ -321,7 +321,7 @@ fn take_screenshots(opt Options, app AppConfig) ![]string { os.setenv('$k', rv, true) } - mut flags := app.capture.flags.join(' ') + flags := app.capture.flags.join(' ') result := opt.verbose_execute('${os.quoted_path(v_exe)} $flags -d gg_record run ${os.quoted_path(app.abs_path)}') if result.exit_code != 0 { return error('Failed taking screenshot of `$app.abs_path`:\n$result.output') @@ -336,7 +336,7 @@ fn take_screenshots(opt Options, app AppConfig) ![]string { existing_screenshots := get_app_screenshots(out_path, app)! - mut flags := app.capture.flags + flags := app.capture.flags mut p_app := os.new_process(app.abs_path) p_app.set_args(flags) diff --git a/cmd/tools/vpm.v b/cmd/tools/vpm.v index e68805b99..5fb5ea614 100644 --- a/cmd/tools/vpm.v +++ b/cmd/tools/vpm.v @@ -86,7 +86,7 @@ fn main() { if module_names.len == 0 && os.exists('./v.mod') { println('Detected v.mod file inside the project directory. Using it...') manifest := vmod.from_file('./v.mod') or { panic(err) } - module_names = manifest.dependencies + module_names = manifest.dependencies.clone() } mut source := Source.vpm if '--once' in options { diff --git a/cmd/tools/vrepl.v b/cmd/tools/vrepl.v index 0e9732e31..e4f5a195c 100644 --- a/cmd/tools/vrepl.v +++ b/cmd/tools/vrepl.v @@ -154,7 +154,7 @@ fn (r &Repl) current_source_code(should_add_temp_lines bool, not_add_print bool) if !not_add_print { lines = r.vstartup_lines.filter(!it.starts_with('print')) } else { - lines = r.vstartup_lines + lines = r.vstartup_lines.clone() } all_lines << lines } diff --git a/vlib/datatypes/linked_list.v b/vlib/datatypes/linked_list.v index d979cf8f3..8684406e1 100644 --- a/vlib/datatypes/linked_list.v +++ b/vlib/datatypes/linked_list.v @@ -83,7 +83,7 @@ pub fn (mut list LinkedList) pop() ?T { return error('Linked list is empty') } mut node := list.head - mut to_return := node.data + mut to_return := unsafe { node.data } if unsafe { node.next == 0 } { // first node case // set to null @@ -92,7 +92,7 @@ pub fn (mut list LinkedList) pop() ?T { for unsafe { node.next.next != 0 } { node = node.next } - to_return = node.next.data + to_return = unsafe { node.next.data } // set to null node.next = unsafe { nil } } diff --git a/vlib/math/big/special_array_ops.v b/vlib/math/big/special_array_ops.v index 7043a75f7..ef5e7e3a0 100644 --- a/vlib/math/big/special_array_ops.v +++ b/vlib/math/big/special_array_ops.v @@ -55,8 +55,8 @@ fn newton_divide_array_by_array(operand_a []u32, operand_b []u32, mut quotient [ q.inc() r -= b } - quotient = q.digits - remainder = r.digits + quotient = q.digits.clone() + remainder = r.digits.clone() shrink_tail_zeros(mut remainder) } @@ -241,7 +241,7 @@ fn toom3_multiply_digit_array(operand_a []u32, operand_b []u32, mut storage []u3 result := (((pinf.lshift(s) + t2).lshift(s) + t1).lshift(s) + tm1).lshift(s) + p0 - storage = result.digits + storage = result.digits.clone() } [inline] diff --git a/vlib/mysql/orm.v b/vlib/mysql/orm.v index c10bda762..3db7922fd 100644 --- a/vlib/mysql/orm.v +++ b/vlib/mysql/orm.v @@ -66,7 +66,7 @@ pub fn (db Connection) @select(config orm.SelectConfig, data orm.QueryData, wher stmt.bind_res(fields, dataptr, lens, num_fields) mut row := 0 - mut types := config.types + mut types := config.types.clone() mut field_types := []FieldType{} if config.is_count { types = [orm.type_idx['u64']] diff --git a/vlib/net/html/data_structures.v b/vlib/net/html/data_structures.v index 688b7561c..e10894ef4 100644 --- a/vlib/net/html/data_structures.v +++ b/vlib/net/html/data_structures.v @@ -54,13 +54,13 @@ fn (mut btree BTree) add_children(tag Tag) int { btree.all_tags << tag if btree.all_tags.len > 1 { for btree.childrens.len <= btree.node_pointer { - mut temp_array := btree.childrens + mut temp_array := btree.childrens.clone() temp_array << []int{} btree.childrens = temp_array } btree.childrens[btree.node_pointer] << btree.all_tags.len - 1 for btree.parents.len < btree.all_tags.len { - mut temp_array := btree.parents + mut temp_array := btree.parents.clone() temp_array << 0 btree.parents = temp_array } diff --git a/vlib/semver/parse.v b/vlib/semver/parse.v index 3443f5999..fc984cbce 100644 --- a/vlib/semver/parse.v +++ b/vlib/semver/parse.v @@ -62,7 +62,7 @@ fn (raw_ver RawVersion) coerce() ?Version { } fn (raw_ver RawVersion) complete() RawVersion { - mut raw_ints := raw_ver.raw_ints + mut raw_ints := raw_ver.raw_ints.clone() for raw_ints.len < 3 { raw_ints << '0' } diff --git a/vlib/v/ast/table.v b/vlib/v/ast/table.v index 46280b712..98009ea52 100644 --- a/vlib/v/ast/table.v +++ b/vlib/v/ast/table.v @@ -399,7 +399,7 @@ pub fn (t &Table) get_embeds(sym &TypeSymbol, options GetEmbedsOptions) [][]Type if unalias_sym.info is Struct { for embed in unalias_sym.info.embeds { embed_sym := t.sym(embed) - mut preceding := options.preceding + mut preceding := options.preceding.clone() preceding << embed embeds << t.get_embeds(embed_sym, preceding: preceding) } @@ -1932,7 +1932,7 @@ pub fn (mut t Table) unwrap_generic_type(typ Type, generic_names []string, concr } } } - mut all_methods := ts.methods + mut all_methods := unsafe { ts.methods } for imethod in imethods { for mut method in all_methods { if imethod.name == method.name { @@ -2097,7 +2097,7 @@ pub fn (mut t Table) generic_insts_to_concrete() { } sym.register_method(method) } - mut all_methods := parent.methods + mut all_methods := unsafe { parent.methods } for imethod in imethods { for mut method in all_methods { if imethod.name == method.name { diff --git a/vlib/v/checker/assign.v b/vlib/v/checker/assign.v index 9ab71ad62..ea399fb60 100644 --- a/vlib/v/checker/assign.v +++ b/vlib/v/checker/assign.v @@ -338,13 +338,29 @@ pub fn (mut c Checker) assign_stmt(mut node ast.AssignStmt) { } left_sym := c.table.sym(left_type_unwrapped) right_sym := c.table.sym(right_type_unwrapped) - if left_sym.kind == .array && !c.inside_unsafe && node.op in [.assign, .decl_assign] - && right_sym.kind == .array && left is ast.Ident && !left.is_blank_ident() - && right is ast.Ident { + + old_assign_error_condition := left_sym.kind == .array && !c.inside_unsafe + && node.op in [.assign, .decl_assign] && right_sym.kind == .array && left is ast.Ident + && !left.is_blank_ident() && right is ast.Ident + if old_assign_error_condition { // Do not allow `a = b`, only `a = b.clone()` c.error('use `array2 $node.op.str() array1.clone()` instead of `array2 $node.op.str() array1` (or use `unsafe`)', node.pos) } + // Do not allow `a = val.array_field`, only `a = val.array_field.clone()` + // TODO: turn this warning into an error after 2022/09/24 + // TODO: and remove the less strict check from above. + if left_sym.kind == .array && !c.inside_unsafe && right_sym.kind == .array + && left is ast.Ident && !left.is_blank_ident() && right in [ast.Ident, ast.SelectorExpr] + && ((node.op == .decl_assign && (left as ast.Ident).is_mut) + || node.op == .assign) { + // no point to show the notice, if the old error was already shown: + if !old_assign_error_condition { + mut_str := if node.op == .decl_assign { 'mut ' } else { '' } + c.note('use `${mut_str}array2 $node.op.str() array1.clone()` instead of `${mut_str}array2 $node.op.str() array1` (or use `unsafe`)', + node.pos) + } + } if left_sym.kind == .array && right_sym.kind == .array { // `mut arr := [u8(1),2,3]` // `arr = [byte(4),5,6]` diff --git a/vlib/v/checker/check_types.v b/vlib/v/checker/check_types.v index 722a05b4b..8a8ce530d 100644 --- a/vlib/v/checker/check_types.v +++ b/vlib/v/checker/check_types.v @@ -756,8 +756,8 @@ pub fn (mut c Checker) infer_fn_generic_types(func ast.Fn, mut node ast.CallExpr mut concrete_types := []ast.Type{} match arg_sym.info { ast.Struct, ast.Interface, ast.SumType { - generic_types = arg_sym.info.generic_types - concrete_types = arg_sym.info.concrete_types + generic_types = arg_sym.info.generic_types.clone() + concrete_types = arg_sym.info.concrete_types.clone() } else {} } diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v index 0642ed587..c7ea1eb75 100644 --- a/vlib/v/checker/checker.v +++ b/vlib/v/checker/checker.v @@ -542,7 +542,7 @@ pub fn (mut c Checker) expand_iface_embeds(idecl &ast.InterfaceDecl, level int, mut ares := []ast.InterfaceEmbedding{} for ie in iface_embeds { if iface_decl := c.table.interfaces[ie.typ] { - mut list := iface_decl.embeds + mut list := iface_decl.embeds.clone() if !iface_decl.are_embeds_expanded { list = c.expand_iface_embeds(idecl, level + 1, iface_decl.embeds) c.table.interfaces[ie.typ].embeds = list diff --git a/vlib/v/checker/return.v b/vlib/v/checker/return.v index f263ff5d1..d0e983cf3 100644 --- a/vlib/v/checker/return.v +++ b/vlib/v/checker/return.v @@ -30,7 +30,7 @@ pub fn (mut c Checker) return_stmt(mut node ast.Return) { exp_is_result := expected_type.has_flag(.result) mut expected_types := [expected_type] if expected_type_sym.info is ast.MultiReturn { - expected_types = expected_type_sym.info.types + expected_types = expected_type_sym.info.types.clone() if c.table.cur_concrete_types.len > 0 { expected_types = expected_types.map(c.unwrap_generic(it)) } diff --git a/vlib/v/checker/tests/array_assign_err.out b/vlib/v/checker/tests/array_assign_err.out new file mode 100644 index 000000000..e6a58febd --- /dev/null +++ b/vlib/v/checker/tests/array_assign_err.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/array_assign_err.vv:8:8: notice: use `mut array2 := array1.clone()` instead of `mut array2 := array1` (or use `unsafe`) + 6 | fn main() { + 7 | a := Dumb{[1, 2, 3]} + 8 | mut b := a.v // I expect a compiler error + | ~~ + 9 | for _ in 0 .. 100 { + 10 | b << [4, 5, 6] diff --git a/vlib/v/checker/tests/array_assign_err.vv b/vlib/v/checker/tests/array_assign_err.vv new file mode 100644 index 000000000..fb6f4eb60 --- /dev/null +++ b/vlib/v/checker/tests/array_assign_err.vv @@ -0,0 +1,13 @@ +struct Dumb { +mut: + v []int +} + +fn main() { + a := Dumb{[1, 2, 3]} + mut b := a.v // I expect a compiler error + for _ in 0 .. 100 { + b << [4, 5, 6] + } + println(a) // prints [4,5,6] in this case, but I've also seen crashes +} diff --git a/vlib/v/checker/tests/array_or_map_assign_err.out b/vlib/v/checker/tests/array_or_map_assign_err.out index 7931d926a..c1788de34 100644 --- a/vlib/v/checker/tests/array_or_map_assign_err.out +++ b/vlib/v/checker/tests/array_or_map_assign_err.out @@ -10,10 +10,10 @@ vlib/v/checker/tests/array_or_map_assign_err.vv:5:5: error: use `array2 = array1 4 | mut a3 := []int{} 5 | a3 = a1 | ^ - 6 | + 6 | 7 | m1 := {'one': 1} vlib/v/checker/tests/array_or_map_assign_err.vv:8:8: error: cannot copy map: call `move` or `clone` method (or use a reference) - 6 | + 6 | 7 | m1 := {'one': 1} 8 | m2 := m1 | ~~ @@ -24,10 +24,10 @@ vlib/v/checker/tests/array_or_map_assign_err.vv:10:7: error: cannot copy map: ca 9 | mut m3 := map[string]int{} 10 | m3 = m1 | ~~ - 11 | + 11 | 12 | _ = a2 vlib/v/checker/tests/array_or_map_assign_err.vv:25:8: error: cannot copy map: call `move` or `clone` method (or use a reference) - 23 | + 23 | 24 | fn foo(mut m map[string]int) { 25 | m2 := m | ^ diff --git a/vlib/v/fmt/fmt.v b/vlib/v/fmt/fmt.v index 43a889804..5192b36e7 100644 --- a/vlib/v/fmt/fmt.v +++ b/vlib/v/fmt/fmt.v @@ -1160,12 +1160,12 @@ pub fn (mut f Fmt) interface_decl(node ast.InterfaceDecl) { immut_fields := if node.mut_pos < 0 { node.fields } else { node.fields[..node.mut_pos] } mut_fields := if node.mut_pos < 0 { []ast.StructField{} } else { node.fields[node.mut_pos..] } - mut immut_methods := node.methods + mut immut_methods := node.methods.clone() mut mut_methods := []ast.FnDecl{} for i, method in node.methods { if method.params[0].is_mut { - immut_methods = node.methods[..i] - mut_methods = node.methods[i..] + immut_methods = node.methods[..i].clone() + mut_methods = node.methods[i..].clone() break } } diff --git a/vlib/v/gen/c/cgen.v b/vlib/v/gen/c/cgen.v index d932b23c4..6bbc32784 100644 --- a/vlib/v/gen/c/cgen.v +++ b/vlib/v/gen/c/cgen.v @@ -1030,7 +1030,7 @@ fn (mut g Gen) register_result(t ast.Type) string { fn (mut g Gen) write_optionals() { mut done := []string{} rlock g.done_optionals { - done = g.done_optionals + done = g.done_optionals.clone() } for base, styp in g.optionals { if base in done { @@ -5814,7 +5814,7 @@ static inline __shared__$interface_name ${shared_fn_name}(__shared__$cctype* x) } } } - mut methods := st_sym.methods + mut methods := st_sym.methods.clone() method_names := methods.map(it.name) match st_sym.info { ast.Struct, ast.Interface, ast.SumType { diff --git a/vlib/v/gen/golang/golang.v b/vlib/v/gen/golang/golang.v index 29fc8b23c..d0c2b7e19 100644 --- a/vlib/v/gen/golang/golang.v +++ b/vlib/v/gen/golang/golang.v @@ -1077,12 +1077,12 @@ pub fn (mut f Gen) interface_decl(node ast.InterfaceDecl) { immut_fields := if node.mut_pos < 0 { node.fields } else { node.fields[..node.mut_pos] } mut_fields := if node.mut_pos < 0 { []ast.StructField{} } else { node.fields[node.mut_pos..] } - mut immut_methods := node.methods + mut immut_methods := node.methods.clone() mut mut_methods := []ast.FnDecl{} for i, method in node.methods { if method.params[0].is_mut { - immut_methods = node.methods[..i] - mut_methods = node.methods[i..] + immut_methods = node.methods[..i].clone() + mut_methods = node.methods[i..].clone() break } } diff --git a/vlib/v/gen/js/fn.v b/vlib/v/gen/js/fn.v index 20d02cc95..0246bd613 100644 --- a/vlib/v/gen/js/fn.v +++ b/vlib/v/gen/js/fn.v @@ -675,7 +675,7 @@ fn (mut g JsGen) gen_method_decl(it ast.FnDecl, typ FnGenType) { g.push_pub_var(name) } } - mut args := it.params + args := it.params g.fn_args(args, it.is_variadic) g.writeln(') {') @@ -808,7 +808,7 @@ fn (mut g JsGen) gen_anon_fn(mut fun ast.AnonFn) { g.write('return function (') - mut args := it.params + args := it.params g.fn_args(args, it.is_variadic) g.writeln(') {') diff --git a/vlib/v/parser/assign.v b/vlib/v/parser/assign.v index 6951aa35e..bc1ddda20 100644 --- a/vlib/v/parser/assign.v +++ b/vlib/v/parser/assign.v @@ -6,7 +6,7 @@ module parser import v.ast fn (mut p Parser) assign_stmt() ast.Stmt { - mut defer_vars := p.defer_vars + mut defer_vars := p.defer_vars.clone() p.defer_vars = []ast.Ident{} exprs, comments := p.expr_list() diff --git a/vlib/v/parser/fn.v b/vlib/v/parser/fn.v index 63dbc1278..3c2b407e1 100644 --- a/vlib/v/parser/fn.v +++ b/vlib/v/parser/fn.v @@ -728,7 +728,7 @@ fn (mut p Parser) anon_fn() ast.AnonFn { tmp := p.label_names p.label_names = [] stmts = p.parse_block_no_scope(false) - label_names = p.label_names + label_names = p.label_names.clone() p.label_names = tmp } p.cur_fn_name = keep_fn_name diff --git a/vlib/v/parser/parser.v b/vlib/v/parser/parser.v index 2a905b768..9e9452c1e 100644 --- a/vlib/v/parser/parser.v +++ b/vlib/v/parser/parser.v @@ -327,9 +327,9 @@ pub fn (mut p Parser) parse() &ast.File { } p.scope.end_pos = p.tok.pos - mut errors := p.errors - mut warnings := p.warnings - mut notices := p.notices + mut errors := p.errors.clone() + mut warnings := p.warnings.clone() + mut notices := p.notices.clone() if p.pref.check_only { errors << p.scanner.errors @@ -2001,7 +2001,7 @@ fn (mut p Parser) parse_multi_expr(is_top_level bool) ast.Stmt { tok := p.tok mut pos := tok.pos() - mut defer_vars := p.defer_vars + mut defer_vars := p.defer_vars.clone() p.defer_vars = []ast.Ident{} left, left_comments := p.expr_list() @@ -3374,7 +3374,7 @@ fn (mut p Parser) const_decl() ast.ConstDecl { p.top_level_statement_start() mut attrs := []ast.Attr{} if p.attrs.len > 0 { - attrs = p.attrs + attrs = p.attrs.clone() p.attrs = [] } mut is_markused := false @@ -3491,7 +3491,7 @@ fn (mut p Parser) return_stmt() ast.Return { fn (mut p Parser) global_decl() ast.GlobalDecl { mut attrs := []ast.Attr{} if p.attrs.len > 0 { - attrs = p.attrs + attrs = p.attrs.clone() p.attrs = [] } diff --git a/vlib/v/tests/shared_elem_test.v b/vlib/v/tests/shared_elem_test.v index b6a1512e6..19c504af2 100644 --- a/vlib/v/tests/shared_elem_test.v +++ b/vlib/v/tests/shared_elem_test.v @@ -64,7 +64,7 @@ struct Efg { fn test_shared_auto_init_array() { e := Efg{} - shared a := e.a + shared a := unsafe { e.a } lock a { a << 23.0625 a << -133.25 @@ -81,11 +81,11 @@ fn test_shared_array_in_struct() { a: [1.25, 2.75, 7, 13.0625] i: 12 } - shared t := x.a + shared t := unsafe { x.a } lock t { t[2] = -1.125 } - shared tt := x.a + shared tt := unsafe { x.a } v := rlock tt { tt[3] } -- 2.30.2