cgen: make `os` less special, fix an -autofree leak on just `import os`

* Improve documentation of v.util.Surrounder

* Remove `os` from the list of "no auto free" `builtin` mods

* Fix -autofree freeing of `const x = []string{}`.

* Add a valgrind regression test.

* Implement os.getenv_opt in vlib/os/environment.js.v too.

vlib/builtin/array.v

@@ -554,7 +554,7 @@ pub fn (mut a []string) free() {
 	for s in a {
 		unsafe { s.free() }
 	}
-	unsafe { free(a.data) }
+	unsafe { (&array(&a)).free() }
 }
 
 // str returns a string representation of the array of strings

vlib/os/environment.js.v

@@ -20,6 +20,17 @@ pub fn getenv(key string) string {
 	return res
 }
 
+// `getenv_opt` returns the value of the environment variable named by the key.
+// If such an environment variable does not exist, then it returns `none`.
+pub fn getenv_opt(key string) ?string {
+	#if (!$ENV[key]) return none__;
+
+	mut res := ''
+	#if ($ENV[key]) res = new string($ENV[key]);
+
+	return res
+}
+
 // unsetenv clears an environment variable with `name`.
 pub fn unsetenv(name string) int {
 	#$ENV[name] = ""

vlib/os/os.v

@@ -3,17 +3,19 @@
 // that can be found in the LICENSE file.
 module os
 
-pub const (
+import strings
-	max_path_len  = 4096
-	wd_at_startup = getwd()
-)
 
-const (
+pub const max_path_len = 4096
-	f_ok = 0
+
-	x_ok = 1
+pub const wd_at_startup = getwd()
-	w_ok = 2
+
-	r_ok = 4
+const f_ok = 0
-)
+
+const x_ok = 1
+
+const w_ok = 2
+
+const r_ok = 4
 
 pub struct Result {
 pub:
@@ -390,6 +392,15 @@ fn executable_fallback() string {
 	return exepath
 }
 
+pub struct ErrExecutableNotFound {
+	msg  string = 'os: failed to find executable'
+	code int
+}
+
+fn error_failed_to_find_executable() IError {
+	return IError(&ErrExecutableNotFound{})
+}
+
 // find_exe_path walks the environment PATH, just like most shell do, it returns
 // the absolute path of the executable if found
 pub fn find_abs_path_of_executable(exepath string) ?string {
@@ -400,7 +411,8 @@ pub fn find_abs_path_of_executable(exepath string) ?string {
 		return real_path(exepath)
 	}
 	mut res := ''
-	paths := getenv('PATH').split(path_delimiter)
+	path := getenv('PATH')
+	paths := path.split(path_delimiter)
 	for p in paths {
 		found_abs_path := join_path(p, exepath)
 		if exists(found_abs_path) && is_executable(found_abs_path) {
@@ -411,7 +423,7 @@ pub fn find_abs_path_of_executable(exepath string) ?string {
 	if res.len > 0 {
 		return real_path(res)
 	}
-	return error('failed to find executable')
+	return error_failed_to_find_executable()
 }
 
 // exists_in_system_path returns `true` if `prog` exists in the system's PATH
@@ -440,14 +452,23 @@ pub fn is_abs_path(path string) bool {
 // join_path returns a path as string from input string parameter(s).
 [manualfree]
 pub fn join_path(base string, dirs ...string) string {
-	mut result := []string{cap: 256}
+	defer {
-	result << base.trim_right('\\/')
+		unsafe { dirs.free() }
-	for d in dirs {
-		result << d
 	}
-	res := result.join(path_separator)
+	mut sb := strings.new_builder(base.len + dirs.len * 50)
-	unsafe { result.free() }
+	defer {
-	return res
+		unsafe { sb.free() }
+	}
+	sbase := base.trim_right('\\/')
+	defer {
+		unsafe { sbase.free() }
+	}
+	sb.write_string(sbase)
+	for d in dirs {
+		sb.write_string(path_separator)
+		sb.write_string(d)
+	}
+	return sb.str()
 }
 
 // walk_ext returns a recursive list of all files in `path` ending with `ext`.
@@ -607,7 +628,9 @@ pub fn temp_dir() string {
 }
 
 fn default_vmodules_path() string {
-	return join_path(home_dir(), '.vmodules')
+	hdir := home_dir()
+	res := join_path(hdir, '.vmodules')
+	return res
 }
 
 // vmodules_dir returns the path to a folder, where v stores its global modules.
@@ -621,12 +644,28 @@ pub fn vmodules_dir() string {
 
 // vmodules_paths returns a list of paths, where v looks up for modules.
 // You can customize it through setting the environment variable VMODULES
+[manualfree]
 pub fn vmodules_paths() []string {
 	mut path := getenv('VMODULES')
 	if path == '' {
+		unsafe { path.free() }
 		path = default_vmodules_path()
 	}
-	list := path.split(path_delimiter).map(it.trim_right(path_separator))
+	defer {
+		unsafe { path.free() }
+	}
+	splitted := path.split(path_delimiter)
+	defer {
+		unsafe { splitted.free() }
+	}
+	mut list := []string{cap: splitted.len}
+	for i in 0 .. splitted.len {
+		si := splitted[i]
+		trimmed := si.trim_right(path_separator)
+		list << trimmed
+		unsafe { trimmed.free() }
+		unsafe { si.free() }
+	}
 	return list
 }
 

vlib/os/os_nix.c.v

@@ -243,12 +243,9 @@ pub fn loginname() string {
 }
 
 fn init_os_args(argc int, argv &&byte) []string {
-	mut args_ := []string{}
+	mut args_ := []string{len: argc}
-	// mut args := []string(make(0, argc, sizeof(string)))
-	// mut args := []string{len:argc}
 	for i in 0 .. argc {
-		// args [i] = argv[i].vstring()
+		args_[i] = unsafe { tos_clone(argv[i]) }
-		unsafe { args_ << (&byte(argv[i])).vstring_literal() }
 	}
 	return args_
 }

vlib/v/gen/c/cgen.v

@@ -1762,7 +1762,7 @@ fn (mut g Gen) stmt(node ast.Stmt) {
 		}
 		ast.Module {
 			// g.is_builtin_mod = node.name == 'builtin'
-			g.is_builtin_mod = node.name in ['builtin', 'os', 'strconv', 'strings', 'gg']
+			g.is_builtin_mod = node.name in ['builtin', 'strconv', 'strings']
 			// g.cur_mod = node.name
 			g.cur_mod = node
 		}
@@ -5843,7 +5843,11 @@ fn (mut g Gen) const_decl_init_later(mod string, name string, expr ast.Expr, typ
 	if g.is_autofree {
 		sym := g.table.get_type_symbol(typ)
 		if styp.starts_with('Array_') {
+			if sym.has_method_with_generic_parent('free') {
+				g.cleanup.writeln('\t${styp}_free(&$cname);')
+			} else {
 				g.cleanup.writeln('\tarray_free(&$cname);')
+			}
 		} else if styp == 'string' {
 			g.cleanup.writeln('\tstring_free(&$cname);')
 		} else if sym.kind == .map {

vlib/v/tests/multiple_paths_in_vmodules/vmodules_overrides_test.v

@@ -18,8 +18,11 @@ fn test_vexe_is_set() {
 fn test_compiling_without_vmodules_fails() {
 	os.chdir(vroot) or {}
 	os.setenv('VMODULES', '', true)
-	res := os.execute('"$vexe" run "$mainvv"')
+	cmd := '"$vexe" run "$mainvv"'
+	dump(cmd)
+	res := os.execute(cmd)
 	assert res.exit_code == 1
+	dump(res)
 	assert res.output.trim_space().contains('builder error: cannot import module "yyy" (not found)')
 }
 

vlib/v/tests/valgrind/import_os_and_use_its_constants.v

@@ -0,0 +1,20 @@
+import os
+
+const vpaths = os.vmodules_paths()
+
+fn main() {
+	println(vpaths)
+	println(os.args)
+	println(os.wd_at_startup)
+	//
+	fullpath := os.join_path('abc', 'xyz', 'def')
+	println(fullpath)
+	//
+	x := 'abc'
+	t := x.trim_right('/')
+	println(x)
+	println(t)
+
+	z := x + t
+	println(z)
+}

vlib/v/util/surrounder.v

@@ -2,13 +2,43 @@ module util
 
 import strings
 
+// Surrounder is an utility to help you manage a stack of additions, that
+// should be done *both* _before_ and _after_ a given piece of generated
+// code, in a synchronised way. It does so by forcing you to put the
+// creation/destruction samples next to each other, so that it is easier to
+// keep them in sync and spot errors.
+// NB: Surrounder writes the creation samples (`befores`) in the _same_ order
+// they were added, and the destruction samples (`afters`) in _reverse_ order.
+//
+// Usage:
+// ```v
+// mut sr := new_surrounder(2) // just a rough initial guess; it can grow
+// sr.add('string tmp1 = ...;', 'string_free(&tmp1);')
+// sr.add('string tmp2 = ...;', 'string_free(&tmp2);')
+// ..
+// sr.builder_write_befores(mut some_string_builder)
+// some_string_builder.writeln('MIDDLE_that_uses_tmp1_and_tmp2')
+// sr.builder_write_afters(mut some_string_builder)
+// ```
+// ... will produce this in `some_string_builder`:
+// ```
+// string tmp1 = ...;
+// string tmp2 = ...;
+// MIDDLE_that_uses_tmp1_and_tmp2
+// string_free(&tmp2);
+// string_free(&tmp1);
+// ```
+
 [noinit]
 pub struct Surrounder {
-pub mut:
+mut:
 	befores []string
 	afters  []string
 }
 
+// new_surrounder creates a new Surrounder instance.
+// The expected_length is a hint for the initial capacity for the
+// befores/afters stacks.
 pub fn new_surrounder(expected_length int) Surrounder {
 	return Surrounder{
 		befores: []string{cap: expected_length}
@@ -16,11 +46,16 @@ pub fn new_surrounder(expected_length int) Surrounder {
 	}
 }
 
+// add appends a `before`/`after` pair to the surrounder
+// When you call .after() or .builder_write_afters(),
+// all `before` parts will be in order, while all `after`
+// parts, will be in reverse order.
 pub fn (mut s Surrounder) add(before string, after string) {
 	s.befores << before
 	s.afters << after
 }
 
+// before returns all the `before` parts that were accumulated so far
 [manualfree]
 pub fn (s &Surrounder) before() string {
 	len := s.befores.len
@@ -30,7 +65,7 @@ pub fn (s &Surrounder) before() string {
 			unsafe { res.free() }
 		}
 		for i := 0; i < len; i++ {
-			x := &s.befores[i]
+			x := s.befores[i]
 			if x.len > 0 {
 				res.writeln(x)
 			}
@@ -41,6 +76,8 @@ pub fn (s &Surrounder) before() string {
 	return ''
 }
 
+// after returns all the `after` parts that were accumulated so far,
+// in reverse order of their addition.
 [manualfree]
 pub fn (s &Surrounder) after() string {
 	len := s.afters.len
@@ -50,7 +87,7 @@ pub fn (s &Surrounder) after() string {
 			unsafe { res.free() }
 		}
 		for i := len - 1; i >= 0; i-- {
-			x := &s.afters[i]
+			x := s.afters[i]
 			if x.len > 0 {
 				res.writeln(x)
 			}
@@ -61,11 +98,13 @@ pub fn (s &Surrounder) after() string {
 	return ''
 }
 
+// builder_write_befores writeln-es all the `before` parts into the given
+// string builder `sb`.
 pub fn (s &Surrounder) builder_write_befores(mut sb strings.Builder) {
 	len := s.befores.len
 	if len > 0 {
 		for i := 0; i < len; i++ {
-			x := &s.befores[i]
+			x := s.befores[i]
 			if x.len > 0 {
 				sb.writeln(x)
 			}
@@ -73,11 +112,14 @@ pub fn (s &Surrounder) builder_write_befores(mut sb strings.Builder) {
 	}
 }
 
+// builder_write_afters writeln-es all the `after` parts into the given
+// string builder `sb`. They will be written there in reverse order, compared
+// to how/when they were added.
 pub fn (s &Surrounder) builder_write_afters(mut sb strings.Builder) {
 	len := s.afters.len
 	if len > 0 {
 		for i := len - 1; i >= 0; i-- {
-			x := &s.afters[i]
+			x := s.afters[i]
 			if x.len > 0 {
 				sb.writeln(x)
 			}
@@ -85,6 +127,8 @@ pub fn (s &Surrounder) builder_write_afters(mut sb strings.Builder) {
 	}
 }
 
+// free frees the private resources associated with the surrounder instance
+// Called automatically by `-autofree`, or in `[manualfree]` tagged functions.
 [unsafe]
 pub fn (mut s Surrounder) free() {
 	unsafe {

vlib/v/util/surrounder_test.v

@@ -0,0 +1,36 @@
+module util
+
+import strings
+
+fn test_creation() {
+	mut sr0 := new_surrounder(0)
+	assert sr0.befores.cap == 0
+	assert sr0.afters.cap == 0
+	//
+	mut sr10 := new_surrounder(10)
+	assert sr10.befores.cap == 10
+	assert sr10.afters.cap == 10
+}
+
+fn test_before_and_after() {
+	mut sr := new_surrounder(0)
+	sr.add('string tmp1;', 'string_free(&tmp1);')
+	sr.add('string tmp2;', 'string_free(&tmp2);')
+	start := sr.before()
+	finish := sr.after()
+	assert start == 'string tmp1;\nstring tmp2;\n'
+	assert finish == 'string_free(&tmp2);\nstring_free(&tmp1);\n'
+}
+
+fn test_string_builder() {
+	mut sr := new_surrounder(0)
+	sr.add('x1', 'free x1')
+	sr.add('x2', 'free x2')
+	sr.add('x3', 'free x3')
+	mut sb := strings.new_builder(512)
+	sr.builder_write_befores(mut sb)
+	sb.writeln('middle')
+	sr.builder_write_afters(mut sb)
+	s := sb.str()
+	assert s == 'x1\nx2\nx3\nmiddle\nfree x3\nfree x2\nfree x1\n'
+}