vet: add suggestions for function documentation (#7890)

cmd/tools/check-md.v

@@ -30,12 +30,12 @@ c) `v run cmd/tools/check-md.v -hide-warnings file.md` - same, but will not prin
 
 NB: There are several special keywords, which you can put after the code fences for v.
 These are:
-   compile      - default, you do not need to specify it. cmd/tools/check-md.v compile the example.
+	compile      - default, you do not need to specify it. cmd/tools/check-md.v compile the example.
-   ignore       - ignore the example, useful for examples that just use the syntax highlighting
+	ignore       - ignore the example, useful for examples that just use the syntax highlighting
-   failcompile  - known failing compilation. Useful for examples demonstrating compiler errors.
+	failcompile  - known failing compilation. Useful for examples demonstrating compiler errors.
-   oksyntax     - it should parse, it may not compile. Useful for partial examples.
+	oksyntax     - it should parse, it may not compile. Useful for partial examples.
-   badsyntax    - known bad syntax, it should not even parse
+	badsyntax    - known bad syntax, it should not even parse
-   wip          - like ignore; a planned feature; easy to search.
+	wip          - like ignore; a planned feature; easy to search.
 ')
 		exit(0)
 	}

cmd/tools/vvet/tests/array_init_one_val.out

@@ -1,2 +1,2 @@
-cmd/tools/vvet/tests/array_init_one_val.vv:2: Use `var == value` instead of `var in [value]`
+cmd/tools/vvet/tests/array_init_one_val.vv:2: error: Use `var == value` instead of `var in [value]`
 NB: You can run `v fmt -w file.v` to fix these automatically

cmd/tools/vvet/tests/indent_with_space.out

@@ -1,2 +1,2 @@
-cmd/tools/vvet/tests/indent_with_space.vv:2: Looks like you are using spaces for indentation.
+cmd/tools/vvet/tests/indent_with_space.vv:2: error: Looks like you are using spaces for indentation.
 NB: You can run `v fmt -w file.v` to fix these automatically

cmd/tools/vvet/tests/module_file_test.out

@@ -0,0 +1,4 @@
+cmd/tools/vvet/tests/module_file_test.vv:7: warning: Function documentation seems to be missing for "pub fn foo() string".
+cmd/tools/vvet/tests/module_file_test.vv:13: warning: A function name is missing from the documentation of "pub fn bar() string".
+cmd/tools/vvet/tests/module_file_test.vv:35: warning: Function documentation seems to be missing for "pub fn (f Foo) foo() string".
+cmd/tools/vvet/tests/module_file_test.vv:46: warning: A function name is missing from the documentation of "pub fn (f Foo) fooo() string".

cmd/tools/vvet/tests/module_file_test.vv

@@ -0,0 +1,49 @@
+module foo
+
+struct Foo {
+	foo int
+}
+
+pub fn foo() string {
+	// Missing doc
+	return 'foo'
+}
+
+// foo does bar
+pub fn bar() string {
+	// not using convention style: '// <fn name>'
+	return 'bar'
+}
+
+// fooo does x
+pub fn fooo() string {
+	// Documented
+	return 'fooo'
+}
+
+// booo does x
+fn booo() string {
+	// Documented, but not pub
+	return 'booo'
+}
+
+fn boo() string {
+	// Missing doc
+	return 'boo'
+}
+
+pub fn (f Foo) foo() string {
+	// Missing doc
+	return f.fo()
+}
+
+fn (f Foo) fo() string {
+	// Missing doc, but not pub
+	return 'foo'
+}
+
+// wrong doc
+pub fn (f Foo) fooo() string {
+	// not using convention
+	return f.fo()
+}

cmd/tools/vvet/tests/parens_space_a.out

@@ -1,2 +1,2 @@
-cmd/tools/vvet/tests/parens_space_a.vv:1: Looks like you are adding a space after `(`
+cmd/tools/vvet/tests/parens_space_a.vv:1: error: Looks like you are adding a space after `(`
 NB: You can run `v fmt -w file.v` to fix these automatically

cmd/tools/vvet/tests/parens_space_b.out

@@ -1,2 +1,2 @@
-cmd/tools/vvet/tests/parens_space_b.vv:1: Looks like you are adding a space before `)`
+cmd/tools/vvet/tests/parens_space_b.vv:1: error: Looks like you are adding a space before `)`
 NB: You can run `v fmt -w file.v` to fix these automatically

cmd/tools/vvet/vet_test.v

@@ -1,6 +1,14 @@
 import os
 import term
 import v.util.vtest
+import v.util
+
+const diff_cmd = find_diff_cmd()
+
+fn find_diff_cmd() string {
+	res := util.find_working_diff_command() or { '' }
+	return res
+}
 
 fn test_vet() {
 	vexe := os.getenv('VEXE')
@@ -25,7 +33,8 @@ fn check_path(vexe string, dir string, tests []string) int {
 	for path in paths {
 		program := path
 		print(path + ' ')
-		res := os.exec('$vexe vet $program') or { panic(err) }
+		// -force is needed so that `v vet` would not skip the regression files
+		res := os.exec('$vexe vet -force $program') or { panic(err) }
 		mut expected := os.read_file(program.replace('.vv', '') + '.out') or { panic(err) }
 		expected = clean_line_endings(expected)
 		found := clean_line_endings(res.output)
@@ -38,6 +47,9 @@ fn check_path(vexe string, dir string, tests []string) int {
 			println('found:')
 			println(found)
 			println('============\n')
+			println('diff:')
+			println(util.color_compare_strings(diff_cmd, found, expected))
+			println('============\n')
 			nb_fail++
 		} else {
 			println(term.green('OK'))

cmd/tools/vvet/vvet.v

@@ -2,80 +2,248 @@
 // Use of this source code is governed by an MIT license that can be found in the LICENSE file.
 module main
 
+import os
+import os.cmdline
 import v.vet
 import v.pref
 import v.parser
-import v.util
 import v.table
-import os
+import v.token
-import os.cmdline
 
-struct VetOptions {
+struct Vet {
-	is_verbose bool
+	opt    Options
 mut:
-	errors     []vet.Error
+	errors []vet.Error
+	file   string
 }
 
-fn (vet_options &VetOptions) vprintln(s string) {
+struct Options {
-	if !vet_options.is_verbose {
+	is_verbose bool
+}
+
+fn (vet &Vet) vprintln(s string) {
+	if !vet.opt.is_verbose {
 		return
 	}
 	println(s)
 }
 
+const vet_options = cmdline.options_after(os.args, ['vet'])
+
+const is_force = '-force' in vet_options
+
+const is_verbose = '-verbose' in vet_options || '-v' in vet_options
+
+const show_warnings = '-hide-warnings' !in vet_options
+
 fn main() {
-	args := util.join_env_vflags_and_os_args()
+	opt := Options{
-	paths := cmdline.only_non_options(cmdline.options_after(args, ['vet']))
+		is_verbose: is_verbose
-	mut vet_options := VetOptions{
-		is_verbose: '-verbose' in args || '-v' in args
 	}
+	mut vet := Vet{
+		opt: opt
+	}
+	mut paths := cmdline.only_non_options(vet_options)
+	vtmp := os.getenv('VTMP')
+	if vtmp != '' {
+		// `v test-cleancode` passes also `-o tmpfolder` as well as all options in VFLAGS
+		paths = paths.filter(!it.starts_with(vtmp))
+	}
+	//
 	for path in paths {
 		if !os.exists(path) {
 			eprintln('File/folder $path does not exist')
 			continue
 		}
-		if path.ends_with('_test.v') ||
-			(path.contains('/tests/') && !path.contains('cmd/tools/vvet/tests/')) {
-			eprintln('skipping $path')
-			continue
-		}
 		if path.ends_with('.v') || path.ends_with('.vv') {
-			vet_options.vet_file(path)
+			if path.contains('cmd/tools/vvet/tests/') {
-		} else if os.is_dir(path) {
+				if is_force || paths.len == 1 {
-			vet_options.vprintln("vetting folder '$path'...")
+					vet.vet_file(path, true)
+					continue
+				} else {
+					// The .vv files in that folder, are regression tests
+					// for `v vet` itself and thus are known to fail in
+					// a predictable way. They are run 1 by 1 by vet_test.v.
+					// They *should be skipped*, when run by more general
+					// invocations like for example `v vet cmd/tools`
+					eprintln("skipping vvet regression file: '$path' ...")
+					continue
+				}
+			}
+		}
+		if os.is_dir(path) {
+			vet.vprintln("vetting folder: '$path' ...")
 			vfiles := os.walk_ext(path, '.v')
 			vvfiles := os.walk_ext(path, '.vv')
 			mut files := []string{}
 			files << vfiles
 			files << vvfiles
 			for file in files {
-				if file.ends_with('_test.v') || file.contains('/tests/') { // TODO copy pasta
+				if !is_force && file.ends_with('.vv') && file.contains('cmd/tools/vvet/tests/') {
 					continue
 				}
-				vet_options.vet_file(file)
+				vet.vet_file(file, false)
 			}
 		}
 	}
-	if vet_options.errors.len > 0 {
+	//
-		for err in vet_options.errors.filter(it.kind == .error) {
+	warnings := vet.errors.filter(it.kind == .warning)
-			eprintln('$err.file_path:$err.pos.line_nr: $err.message')
+	errors := vet.errors.filter(it.kind == .error)
+	errors_vfmt := vet.errors.filter(it.kind == .error && it.fix == .vfmt)
+	if show_warnings {
+		for err in warnings {
+			eprintln('$err.file_path:$err.pos.line_nr: warning: $err.message')
 		}
+	}
+	for err in errors {
+		eprintln('$err.file_path:$err.pos.line_nr: error: $err.message')
+	}
+	if errors_vfmt.len > 0 {
 		eprintln('NB: You can run `v fmt -w file.v` to fix these automatically')
-		/*
+	}
-		for err in vet_options.errors.filter(it.kind == .warning) {
+	if errors.len > 0 {
-			eprintln('$err.file_path:$err.pos.line_nr: err.message')
-		}
-		*/
 		exit(1)
 	}
 }
 
-fn (mut vet_options VetOptions) vet_file(path string) {
+fn (mut v Vet) error(msg string, line int, fix vet.FixKind) {
+	pos := token.Position{
+		line_nr: line + 1
+	}
+	v.errors << vet.Error{
+		message: msg
+		file_path: v.file
+		pos: pos
+		kind: .error
+		fix: fix
+	}
+}
+
+fn (mut v Vet) warn(msg string, line int, fix vet.FixKind) {
+	pos := token.Position{
+		line_nr: line + 1
+	}
+	v.errors << vet.Error{
+		message: msg
+		file_path: v.file
+		pos: pos
+		kind: .warning
+		fix: fix
+	}
+}
+
+// vet_file vets the file read from `path`.
+fn (mut vt Vet) vet_file(path string, is_regression_test bool) {
+	if path.contains('/tests/') && !is_regression_test {
+		// skip all /tests/ files, since usually their content is not
+		// important enough to be documented/vetted, and they may even
+		// contain intentionally invalid code.
+		eprintln("skipping test file: '$path' ...")
+		return
+	}
+	vt.file = path
 	mut prefs := pref.new_preferences()
 	prefs.is_vet = true
 	table := table.new_table()
-	vet_options.vprintln("vetting file '$path'...")
+	vt.vprintln("vetting file '$path'...")
 	_, errors := parser.parse_vet_file(path, table, prefs)
 	// Transfer errors from scanner and parser
-	vet_options.errors << errors
+	vt.errors << errors
+	// Scan each line in file for things to improve
+	source_lines := os.read_lines(vt.file) or { []string{} }
+	for lnumber, line in source_lines {
+		vt.vet_line(source_lines, line, lnumber)
+	}
+}
+
+// vet_line vets the contents of `line` from `vet.file`.
+fn (mut vet Vet) vet_line(lines []string, line string, lnumber int) {
+	// Vet public functions
+	if line.starts_with('pub fn') ||
+		(line.starts_with('fn ') && !(line.starts_with('fn C.') || line.starts_with('fn main'))) {
+		// Scan function declarations for missing documentation
+		is_pub_fn := line.starts_with('pub fn')
+		if lnumber > 0 {
+			collect_tags := fn (line string) []string {
+				mut cleaned := line.all_before('/')
+				cleaned = cleaned.replace_each(['[', '', ']', '', ' ', ''])
+				return cleaned.split(',')
+			}
+			ident_fn_name := fn (line string) string {
+				mut fn_idx := line.index(' fn ') or { return '' }
+				mut skip := false
+				mut p_count := 0
+				mut fn_name := ''
+				for i := fn_idx + 4; i < line.len; i++ {
+					char := line[i]
+					if !skip && char == `(` {
+						p_count++
+						skip = true
+						continue
+					} else if skip && char == `)` {
+						skip = false
+						continue
+					} else if char == ` ` {
+						continue
+					} else if char.is_letter() {
+						// fn_name += char.str()
+						fn_name = line[i..].all_before('(')
+						break
+					}
+					if p_count > 1 {
+						break
+					}
+				}
+				return fn_name
+			}
+			mut line_above := lines[lnumber - 1]
+			mut tags := []string{}
+			if !line_above.starts_with('//') {
+				mut grab := true
+				for j := lnumber - 1; j >= 0; j-- {
+					prev_line := lines[j]
+					if prev_line.contains('}') { // We've looked back to the above scope, stop here
+						break
+					} else if prev_line.starts_with('[') {
+						tags << collect_tags(prev_line)
+						continue
+					} else if prev_line.starts_with('//') { // Single-line comment
+						grab = false
+						break
+					}
+				}
+				if grab {
+					clean_line := line.all_before_last('{').trim(' ')
+					if is_pub_fn {
+						vet.warn('Function documentation seems to be missing for "$clean_line".',
+							lnumber, .doc)
+					}
+				}
+			} else {
+				fn_name := ident_fn_name(line)
+				mut grab := true
+				for j := lnumber - 1; j >= 0; j-- {
+					prev_line := lines[j]
+					if prev_line.contains('}') { // We've looked back to the above scope, stop here
+						break
+					} else if prev_line.starts_with('// $fn_name ') {
+						grab = false
+						break
+					} else if prev_line.starts_with('[') {
+						tags << collect_tags(prev_line)
+						continue
+					} else if prev_line.starts_with('//') { // Single-line comment
+						continue
+					}
+				}
+				if grab {
+					clean_line := line.all_before_last('{').trim(' ')
+					if is_pub_fn {
+						vet.warn('A function name is missing from the documentation of "$clean_line".',
+							lnumber, .doc)
+					}
+				}
+			}
+		}
+	}
 }

vlib/v/vet/vet.v

@@ -11,6 +11,7 @@ pub enum ErrorKind {
 
 pub enum FixKind {
 	unknown
+	doc
 	vfmt
 }