Repository: bazelbuild/buildtools
Pull request #536 (2019-02-06 12:58:48)

Implement docstring warnings
For modules, ensure that a docstring exists.

For public functions that are long enough (at least 5 statements) ensure that a docstring exists and documents input and output parameters.

googlebot: We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors.  If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA ([login here](https://cla.developers.google.com/) to double check)?  If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

<!-- need_author_cla -->
googlebot: We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors.  If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA ([login here](https://cla.developers.google.com/) to double check)?  If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

<!-- need_author_cla -->
googlebot: CLAs look good, thanks!

<!-- ok -->

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 618b82a..1ef67c4 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -15,11 +15,13 @@ Warning categories supported by buildifier's linter:
   * [dict-concatenation](#dict-concatenation)
   * [duplicated-name](#duplicated-name)
   * [filetype](#filetype)
+  * [function-docstring](#function-docstring)
   * [git-repository](#git-repository)
   * [http-archive](#http-archive)
   * [integer-division](#integer-division)
   * [load](#load)
   * [load-on-top](#load-on-top)
+  * [module-docstring](#module-docstring)
   * [native-build](#native-build)
   * [native-package](#native-package)
   * [no-effect](#no-effect)
@@ -280,6 +282,40 @@ just use a list of strings.
 
 --------------------------------------------------------------------------------
 
+## <a name="function-docstring"></a>Function docstring
+
+  * Category_name: `function-docstring`
+  * Automatic fix: no
+
+Public functions should have docstrings describing functions and their signatures.
+A docstring is a string statement which should be the first statement of the file
+(it may follow comment lines). Docstrings are expected to be formatted in the
+following way:
+
+    """One-line summary: must be followed and may be preceded by a blank line.
+    
+    Optional additional description like this.
+    
+    If it's a function docstring and the function has more than one argument, the docstring has
+    to document these parameters as follows:
+
+    Args:
+      parameter1: description of the first parameter. Each parameter line
+        should be indented by one, preferably two, spaces (as here).
+      parameter2: description of the second
+        parameter that spans two lines. Each additional line should have a
+        hanging indentation of at least one, preferably two, additional spaces (as here).
+      another_parameter (unused, mutable): a parameter may be followed
+        by additional attributes in parentheses
+
+    Returns:
+      Description of the return value.
+      Should be indented by at least one, preferably two spaces (as here)
+      Can span multiple lines.
+    """
+
+--------------------------------------------------------------------------------
+
 ## <a name="git-repository"></a>Function `git_repository` is not global anymore
 
   * Category_name: `git-repository`
@@ -365,6 +401,16 @@ they can follow only comments and docstrings.
 
 --------------------------------------------------------------------------------
 
+## <a name="module-docstring"></a>The file has no module docstring.
+
+  * Category_name: `module-docstring`
+  * Automatic fix: no
+
+`.bzl` files should have docstrings on top of them. A docstring is a string statement
+which should be the first statement of the file (it may follow comment lines). 
+
+--------------------------------------------------------------------------------
+
 ## <a name="native-build"></a>The `native` module shouldn't be used in BUILD files
 
   * Category_name: `native-build`
diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel
index c261863..d36f031 100644
--- a/warn/BUILD.bazel
+++ b/warn/BUILD.bazel
@@ -10,6 +10,7 @@ go_library(
         "warn_bazel_operation.go",
         "warn_control_flow.go",
         "warn_cosmetic.go",
+        "warn_docstring.go",
         "warn_operation.go",
     ],
     importpath = "github.com/bazelbuild/buildtools/warn",
@@ -32,6 +33,7 @@ go_test(
         "warn_bazel_test.go",
         "warn_control_flow_test.go",
         "warn_cosmetic_test.go",
+        "warn_docstring_test.go",
         "warn_operation_test.go",
         "warn_test.go",
     ],
diff --git a/warn/warn.go b/warn/warn.go
index 7a431a2..54eb3a9 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -80,12 +80,14 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"dict-concatenation":  dictionaryConcatenationWarning,
 	"duplicated-name":     duplicatedNameWarning,
 	"filetype":            fileTypeWarning,
+	"function-docstring":  functionDocstringWarning,
 	"git-repository":      nativeGitRepositoryWarning,
 	"http-archive":        nativeHTTPArchiveWarning,
 	"integer-division":    integerDivisionWarning,
 	"load":                unusedLoadWarning,
 	"load-on-top":         loadOnTopWarning,
 	"return-value":        missingReturnValueWarning,
+	"module-docstring":    moduleDocstringWarning,
 	"native-build":        nativeInBuildFilesWarning,
 	"native-package":      nativePackageWarning,
 	"no-effect":           noEffectWarning,
diff --git a/warn/warn_docstring.go b/warn/warn_docstring.go
new file mode 100644
index 0000000..eb80b99
--- /dev/null
+++ b/warn/warn_docstring.go
@@ -0,0 +1,263 @@
+package warn
+
+import (
+	"fmt"
+	"github.com/bazelbuild/buildtools/build"
+	"regexp"
+	"strings"
+)
+
+const FunctionLengthDocstringThreshold = 5
+
+// getDocstrings returns a docstring of the statemenets and true if it exists.
+// Otherwise it returns the first non-comment statement and false.
+func getDocstring(stmts []build.Expr) (build.Expr, bool) {
+	for _, stmt := range stmts {
+		switch stmt := stmt.(type) {
+		case *build.CommentBlock:
+			continue
+		case *build.StringExpr:
+			return stmt, true
+		default:
+			return stmt, false
+		}
+	}
+	return nil, false
+}
+
+func moduleDocstringWarning(f *build.File, fix bool) []*Finding {
+	if f.Type != build.TypeDefault {
+		return []*Finding{}
+	}
+	if stmt, ok := getDocstring(f.Stmt); stmt != nil && !ok {
+		start, end := stmt.Span()
+		return []*Finding{
+			makeFinding(f, start, end, "module-docstring",
+				`The file has no module docstring.`, true, nil),
+		}
+	}
+	return []*Finding{}
+}
+
+func stmtsCount(stmts []build.Expr) int {
+	result := 0
+	for _, stmt := range stmts {
+		result++
+		switch stmt := stmt.(type) {
+		case *build.IfStmt:
+			result += stmtsCount(stmt.True)
+			result += stmtsCount(stmt.False)
+		case *build.ForStmt:
+			result += stmtsCount(stmt.Body)
+		}
+	}
+	return result
+}
+
+// docstringInfo contains information about a function docstring
+type docstringInfo struct {
+	hasHeader    bool                      // whether the docstring has a one-line header
+	args         map[string]build.Position // map of documented arguments, the values are line numbers
+	returns      bool                      // whether the return value is documented
+	argumentsPos build.Position            // line of the `Arguments:` block (not `Args:`), if it exists
+}
+
+// docstringBlock contains a block of a docstring (separated by empty lines)
+type docstringBlock struct {
+	startLineNo int      // line number of the first line of the block
+	lines       []string // lines
+}
+
+// countLeadingSpaces returns the number of leading spaces of a string.
+func countLeadingSpaces(s string) int {
+	spaces := 0
+	for _, c := range s {
+		if c == ' ' {
+			spaces++
+		} else {
+			break
+		}
+	}
+	return spaces
+}
+
+var argRegex = regexp.MustCompile(`^ *(\*?\*?\w+)( *\([\w\ ,]+\))?:`)
+
+// parseFunctionDocstring parses a function docstring and returns a docstringInfo object containing
+// the parsed information about the function, its arguments and its return value.
+func parseFunctionDocstring(doc *build.StringExpr) docstringInfo {
+	start, _ := doc.Span()
+	indent := start.LineRune - 1
+	lines := strings.Split(doc.Value, "\n")
+
+	// Dedent the lines
+	for i, line := range lines {
+		line = strings.TrimRight(line, " ")
+		if i != 0 {
+			for j, chr := range line {
+				if j >= indent || chr != ' ' {
+					line = line[j:]
+					break
+				}
+			}
+		}
+		lines[i] = line
+	}
+
+	// Split by empty lines
+	blocks := []docstringBlock{}
+	newBlock := true
+	for i, line := range lines {
+		if len(line) == 0 {
+			newBlock = true
+			continue
+		}
+		if newBlock {
+			newBlock = false
+			blocks = append(blocks, docstringBlock{
+				startLineNo: start.Line + i,
+				lines:       []string{},
+			})
+		}
+		blocks[len(blocks)-1].lines = append(blocks[len(blocks)-1].lines, line)
+	}
+
+	info := docstringInfo{}
+	info.args = make(map[string]build.Position)
+
+	if len(blocks) > 0 && len(blocks[0].lines) == 1 {
+		// Exactly one line in the first block
+		info.hasHeader = true
+	}
+
+	// Iterate over the blocks, extract data
+	for _, block := range blocks {
+		switch block.lines[0] {
+		case "Args:", "Arguments:":
+			if block.lines[0] == "Arguments:" {
+				// 'Args:' is preferred over 'Arguments:'
+				info.argumentsPos = build.Position{
+					Line:     block.startLineNo,
+					LineRune: indent,
+				}
+			}
+
+			argIndentation := 1000000 // Indentation at which previous arg documentation started
+			for i, line := range block.lines[1:] {
+				// Iterate over line and parse arguments. If the current indentation level is the same as
+				// the indentation level of the previous argument (or lower), assume that the new argument
+				// is being described on this line, otherwise it's a continued description of the previous
+				// argument.
+				newIndentation := countLeadingSpaces(line)
+				if newIndentation <= argIndentation {
+					// Extract the arg name from the first line of its description,
+					// e.g. "  my_arg (optional, deprecated): ..."
+					result := argRegex.FindStringSubmatch(line)
+					if len(result) > 1 {
+						argIndentation = newIndentation
+						info.args[result[1]] = build.Position{
+							Line:     block.startLineNo + i + 1, // the first line is skipped in the loop
+							LineRune: indent + argIndentation,
+						}
+					}
+				}
+			}
+		case "Returns:":
+			if len(block.lines) > 1 {
+				info.returns = true
+			}
+		}
+	}
+	return info
+}
+
+func getParamName(param build.Expr) string {
+	switch param := param.(type) {
+	case *build.Ident:
+		return param.Name
+	case *build.BinaryExpr:
+		// keyword parameter
+		if ident, ok := param.X.(*build.Ident); ok {
+			return ident.Name
+		}
+	case *build.UnaryExpr:
+		// *args or **kwargs
+		if ident, ok := param.X.(*build.Ident); ok {
+			return param.Op + ident.Name
+		}
+	}
+	return ""
+}
+
+func hasReturnValues(def *build.DefStmt) bool {
+	result := false
+	build.Walk(def, func(expr build.Expr, stack []build.Expr) {
+		ret, ok := expr.(*build.ReturnStmt)
+		if ok && ret.Result != nil {
+			result = true
+		}
+	})
+	return result
+}
+
+func functionDocstringWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+
+	for _, stmt := range f.Stmt {
+		def, ok := stmt.(*build.DefStmt)
+		if !ok {
+			continue
+		}
+
+		doc, ok := getDocstring(def.Body)
+		if !ok {
+			if !strings.HasPrefix(def.Name, "_") && stmtsCount(def.Body) >= FunctionLengthDocstringThreshold {
+				// Public functions that are not too short should have a docstring
+				start, end := stmt.Span()
+				findings = append(findings, makeFinding(f, start, end, "function-docstring",
+					fmt.Sprintf(`The function "%s" has no docstring.`, def.Name), true, nil))
+			}
+			continue
+		}
+
+		// Docstring exists, check for its contents
+		info := parseFunctionDocstring(doc.(*build.StringExpr))
+		start, end := doc.Span()
+
+		if !info.hasHeader {
+			findings = append(findings, makeFinding(f, start, end, "function-docstring",
+				fmt.Sprintf(`The docstring for the function "%s" should start with a one-line summary.`, def.Name), true, nil))
+		}
+		if info.argumentsPos.LineRune > 0 {
+			argumentsEnd := info.argumentsPos
+			argumentsEnd.LineRune += len("Arguments:")
+			findings = append(findings, makeFinding(f, info.argumentsPos, argumentsEnd, "function-docstring",
+				`Prefer 'Args:' to 'Arguments:' when documenting function arguments.`, true, nil))
+		}
+
+		paramNames := make(map[string]bool)
+		for _, param := range def.Params {
+			name := getParamName(param)
+			paramNames[name] = true
+			if _, ok := info.args[name]; !ok {
+				findings = append(findings, makeFinding(f, start, end, "function-docstring",
+					fmt.Sprintf(`Argument "%s" is not documented.`, name), true, nil))
+			}
+		}
+
+		for name, pos := range info.args {
+			if !paramNames[name] {
+				posEnd := pos
+				posEnd.LineRune += len(name)
+				findings = append(findings, makeFinding(f, pos, posEnd, "function-docstring",
+					fmt.Sprintf(`Argument "%s" is documented but doesn't exist in the function signature.`, name), true, nil))
+			}
+		}
+
+		if hasReturnValues(def) && !info.returns {
+			findings = append(findings, makeFinding(f, start, end, "function-docstring",
+				fmt.Sprintf(`Return value of "%s" is not documented.`, def.Name), true, nil))
+		}
+	}
+	return findings
+}


Test patch:
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 65564fb..f1ef945 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -76,10 +76,11 @@ attr.foo(bar)
 EOF
 
 cat > test/fix_report_golden <<EOF
-test/to_fix_tmp.bzl: applied fixes, 0 warnings left
+test/to_fix_tmp.bzl: applied fixes, 1 warnings left
 fixed test/to_fix_tmp.bzl
 EOF
 
+error_docstring="test/to_fix_tmp.bzl:1: module-docstring: The file has no module docstring. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#module-docstring)"
 error_integer="test/to_fix_tmp.bzl:1: integer-division: The \"/\" operator for integer division is deprecated in favor of \"//\". (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#integer-division)"
 error_dict="test/to_fix_tmp.bzl:2: unsorted-dict-items: Dictionary items are out of their lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#unsorted-dict-items)"
 error_cfg="test/to_fix_tmp.bzl:3: attr-cfg: cfg = \"data\" for attr definitions has no effect and should be removed. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#attr-cfg)"
@@ -89,6 +90,11 @@ test_lint () {
   cp test/to_fix.bzl test/to_fix_tmp.bzl
   echo "$4" > test/error_golden
 
+  cat > test/fix_report_golden <<EOF
+test/to_fix_tmp.bzl: applied fixes, $5 warnings left
+fixed test/to_fix_tmp.bzl
+EOF
+
   $buildifier --lint=warn $2 test/to_fix_tmp.bzl 2> test/error || ret=$?
   if [[ $ret -ne 4 ]]; then
     die "$1: warn: Expected buildifier to exit with 4, actual: $ret"
@@ -103,7 +109,7 @@ test_lint () {
   diff test/fix_report test/fix_report_golden || die "$1: wrong console output for --lint=fix"
 }
 
-test_lint "default" "" "test/fixed_golden.bzl" "$error_integer"$'\n'"$error_cfg"
-test_lint "all" "--warnings=all" "test/fixed_golden_all.bzl" "$error_integer"$'\n'"$error_dict"$'\n'"$error_cfg"
-test_lint "cfg" "--warnings=attr-cfg" "test/fixed_golden_cfg.bzl" "$error_cfg"
-test_lint "custom" "--warnings=-integer-division,+unsorted-dict-items" "test/fixed_golden_dict_cfg.bzl" "$error_dict"$'\n'"$error_cfg"
+test_lint "default" "" "test/fixed_golden.bzl" "$error_integer"$'\n'"$error_docstring"$'\n'"$error_cfg" 1
+test_lint "all" "--warnings=all" "test/fixed_golden_all.bzl" "$error_integer"$'\n'"$error_docstring"$'\n'"$error_dict"$'\n'"$error_cfg" 1
+test_lint "cfg" "--warnings=attr-cfg" "test/fixed_golden_cfg.bzl" "$error_cfg" 0
+test_lint "custom" "--warnings=-integer-division,+unsorted-dict-items" "test/fixed_golden_dict_cfg.bzl" "$error_docstring"$'\n'"$error_dict"$'\n'"$error_cfg" 1
diff --git a/warn/warn_docstring_test.go b/warn/warn_docstring_test.go
new file mode 100644
index 0000000..0423036
--- /dev/null
+++ b/warn/warn_docstring_test.go
@@ -0,0 +1,244 @@
+package warn
+
+import "testing"
+
+func TestModuleDocstring(t *testing.T) {
+	checkFindings(t, "module-docstring", ``,
+		[]string{},
+		scopeBzl)
+
+	checkFindings(t, "module-docstring", `
+# empty file`,
+		[]string{},
+		scopeBzl)
+
+	checkFindings(t, "module-docstring", `
+"""This is the module"""
+
+load("foo", "bar")
+
+bar()`,
+		[]string{},
+		scopeBzl)
+
+	checkFindings(t, "module-docstring", `
+load("foo", "bar")
+
+"""This is the module"""
+
+bar()`,
+		[]string{":1: The file has no module docstring."},
+		scopeBzl)
+
+	checkFindings(t, "module-docstring", `
+# comment
+
+# comment
+"""This is the module"""
+
+load("foo", "bar")
+
+bar()`,
+		[]string{},
+		scopeBzl)
+
+	checkFindings(t, "module-docstring", `
+# comment
+
+load("foo", "bar")
+
+# comment
+"""This is the module"""
+
+bar()`,
+		[]string{":3: The file has no module docstring."},
+		scopeBzl)
+}
+
+func TestFunctionDocstringExists(t *testing.T) {
+	checkFindings(t, "function-docstring", `
+def f(x):
+   # short function
+   return x
+`,
+		[]string{},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x):
+   # long function
+   x += 1
+   x *= 2
+   x /= 3
+   x -= 4
+   x %= 5
+   return x
+`,
+		[]string{":1: The function \"f\" has no docstring."},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def _f(x):
+   # long private function
+   x += 1
+   x *= 2
+   x /= 3
+   x -= 4
+   x %= 5
+   return x
+`,
+		[]string{},
+		scopeEverywhere)
+}
+
+func TestFunctionDocstringFormat(t *testing.T) {
+	checkFindings(t, "function-docstring", `
+def f(x):
+   """This is a function.
+
+   Args:
+     x: something
+
+   Returns:
+     something
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+   return x
+`,
+		[]string{},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x):
+   """This is a function.
+
+   Args:
+     x: something
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+`,
+		[]string{},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x):
+   """This is a function.
+
+   Args:
+     x: something
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+   return x
+`,
+		[]string{`2: Return value of "f" is not documented.`},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x):
+   """This is a function.
+
+   Args:
+     x: something
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+   if foo:
+     return
+   else:
+     return x
+`,
+		[]string{`2: Return value of "f" is not documented.`},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x, y):
+   """This is a function.
+
+   Arguments:
+     x: something
+        y: something (this is in fact the description of x continued)
+     z: something else
+
+   Returns:
+     None
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+`,
+		[]string{
+			`2: Argument "y" is not documented.`,
+			`4: Prefer 'Args:' to 'Arguments:' when documenting function arguments.`,
+			`7: Argument "z" is documented but doesn't exist in the function signature.`,
+		}, scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x, y, z = None, *args, **kwargs):
+   """This is a function.
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+`,
+		[]string{
+			`2: Argument "x" is not documented.`,
+			`2: Argument "y" is not documented.`,
+			`2: Argument "z" is not documented.`,
+			`2: Argument "*args" is not documented.`,
+			`2: Argument "**kwargs" is not documented.`,
+		},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f(x, y, z = None, *args, **kwargs):
+   """This is a function.
+
+   Args:
+    x: x
+    y (deprecated, mutable): y
+    z: z
+    *args: the args
+    **kwargs: the kwargs
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+`,
+		[]string{},
+		scopeEverywhere)
+
+	checkFindings(t, "function-docstring", `
+def f():
+   """This is a function.
+   this is the description
+   """
+   pass
+   pass
+   pass
+   pass
+   pass
+`,
+		[]string{`2: The docstring for the function "f" should start with a one-line summary.`},
+		scopeEverywhere)
+}