Repository: bazelbuild/buildtools
Pull request #538 (2019-02-07 18:37:17)

Warnings for variable names
All variables should be lower_snake_case, constant should be UPPER_SNAKE_CASE, and providers should be UpperCamelCase ending with `Info`.


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 35d8aa6..fe7da31 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -23,6 +23,7 @@ Warning categories supported by buildifier's linter:
   * [load](#load)
   * [load-on-top](#load-on-top)
   * [module-docstring](#module-docstring)
+  * [name-conventions](#name-conventions)
   * [native-build](#native-build)
   * [native-package](#native-package)
   * [no-effect](#no-effect)
@@ -421,6 +422,16 @@ which should be the first statement of the file (it may follow comment lines).
 
 --------------------------------------------------------------------------------
 
+## <a name="name-conventions"></a>Name conventions
+
+  * Category_name: `name-conventions`
+  * Automatic fix: yes
+
+By convention, all variables should be lower_snake_case, constant should be
+UPPER_SNAKE_CASE, and providers should be UpperCamelCase ending with `Info`.
+
+--------------------------------------------------------------------------------
+
 ## <a name="native-build"></a>The `native` module shouldn't be used in BUILD files
 
   * Category_name: `native-build`
diff --git a/build/walk.go b/build/walk.go
index 495ffac..c547197 100644
--- a/build/walk.go
+++ b/build/walk.go
@@ -184,3 +184,47 @@ func WalkOnce(v Expr, f func(x *Expr)) {
 		}
 	}
 }
+
+// walkStatements is a helper function for WalkStatements
+func walkStatements(v Expr, stack *[]Expr, f func(x Expr, stk []Expr)) {
+	if v == nil {
+		return
+	}
+
+	f(v, *stack)
+	*stack = append(*stack, v)
+
+	traverse := func(x Expr) {
+		walkStatements(x, stack, f)
+	}
+
+	switch expr := v.(type) {
+	case *File:
+		for _, s := range expr.Stmt {
+			traverse(s)
+		}
+	case *DefStmt:
+		for _, s := range expr.Body {
+			traverse(s)
+		}
+	case *IfStmt:
+		for _, s := range expr.True {
+			traverse(s)
+		}
+		for _, s := range expr.False {
+			traverse(s)
+		}
+	case *ForStmt:
+		for _, s := range expr.Body {
+			traverse(s)
+		}
+	}
+
+	*stack = (*stack)[:len(*stack)-1]
+}
+
+// WalkStatements traverses sub statements (not all nodes)
+func WalkStatements(v Expr, f func(x Expr, stk []Expr)) {
+	var stack []Expr
+	walkStatements(v, &stack, f)
+}
diff --git a/warn/warn.go b/warn/warn.go
index 102643a..118e298 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -89,6 +89,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"load-on-top":         loadOnTopWarning,
 	"return-value":        missingReturnValueWarning,
 	"module-docstring":    moduleDocstringWarning,
+	"name-conventions":    nameConventionsWarning,
 	"native-build":        nativeInBuildFilesWarning,
 	"native-package":      nativePackageWarning,
 	"no-effect":           noEffectWarning,
diff --git a/warn/warn_naming.go b/warn/warn_naming.go
index 3bb5215..20dbb1b 100644
--- a/warn/warn_naming.go
+++ b/warn/warn_naming.go
@@ -1,8 +1,10 @@
 package warn
 
 import (
+	"fmt"
 	"github.com/bazelbuild/buildtools/build"
 	"github.com/bazelbuild/buildtools/bzlenv"
+	"strings"
 )
 
 var ambiguousNames = map[string]bool{
@@ -65,3 +67,46 @@ func confusingNameWarning(f *build.File, fix bool) []*Finding {
 
 	return findings
 }
+
+func isUpperCamelCase(name string) bool {
+	if strings.HasPrefix(name, "_") {
+		// Private providers are allowed
+		name = name[1:]
+	}
+	return !strings.ContainsRune(name, '_') && name == strings.Title(name)
+}
+
+func isLowerSnakeCase(name string) bool {
+	return name == strings.ToLower(name)
+}
+
+func isUpperSnakeCase(name string) bool {
+	return name == strings.ToUpper(name)
+}
+
+func nameConventionsWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+
+	build.WalkStatements(f, func(stmt build.Expr, stack []build.Expr) {
+		// looking for provider declaration statements: `xxx = provider()`
+		// note that the code won't trigger on complex assignments, such as `x, y = foo, provider()`
+		binary, ok := stmt.(*build.BinaryExpr)
+		if !ok || binary.Op != "=" {
+			return
+		}
+		for _, ident := range bzlenv.CollectLValues(binary.X) {
+			if isLowerSnakeCase(ident.Name) || isUpperSnakeCase(ident.Name) {
+				continue
+			}
+			if isUpperCamelCase(ident.Name) && strings.HasSuffix(ident.Name, "Info") {
+				continue
+			}
+			start, end := ident.Span()
+			findings = append(findings,
+				makeFinding(f, start, end, "name-conventions",
+					fmt.Sprintf(`Variable name "%s" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`, ident.Name), true, nil))
+		}
+	})
+
+	return findings
+}


Test patch:
diff --git a/warn/warn_naming_test.go b/warn/warn_naming_test.go
index 449c16a..b6d2827 100644
--- a/warn/warn_naming_test.go
+++ b/warn/warn_naming_test.go
@@ -48,7 +48,7 @@ def l():  # here
 [l for l in s]
 
 cc_library(
-  name = "name",
+  name = "name-conventions",
   tags = [I for I in tags if I],
 )
 
@@ -78,3 +78,61 @@ def f(x):
 			":5: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
 		}, scopeEverywhere)
 }
+
+func TestVariableNames(t *testing.T) {
+	checkFindings(t, "name-conventions", `
+_ = 0
+foo = 1
+FOO = 2
+Foo = 3
+foo, Bar, BazInfo = 4, 5, 6
+foo, (BAR, _) = 7, (8, 9)
+fOO = 10
+FooInfo = 11
+_FooInfo = 12
+_BAR = 13
+_baz = 14
+_Foo_bar, foo_Baz = 15, 16
+`,
+		[]string{
+			`:4: Variable name "Foo" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:5: Variable name "Bar" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:7: Variable name "fOO" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:12: Variable name "_Foo_bar" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:12: Variable name "foo_Baz" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+		}, scopeEverywhere)
+
+	checkFindings(t, "name-conventions", `
+def f(x, _, Arg = None):
+  _ = 0
+  foo = 1
+  FOO = 2
+  Foo = 3
+  foo, Bar, BazInfo = 4, 5, 6
+  foo, (BAR, _) = 7, (8, 9)
+  fOO = 10
+  FooInfo = 11
+  _FooInfo = 12
+  _BAR = 13
+  _baz = 14
+  _Foo_bar, foo_Baz = 15, 16
+`,
+		[]string{
+			`:5: Variable name "Foo" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:6: Variable name "Bar" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:8: Variable name "fOO" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:13: Variable name "_Foo_bar" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+			`:13: Variable name "foo_Baz" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`,
+		}, scopeEverywhere)
+
+	checkFindings(t, "name-conventions", `
+foo(
+  Bar = 1,
+  ___ = 2,
+  BazInfo = 3,
+	baz = 4,
+  FOO = 5,
+)
+`,
+		[]string{}, scopeEverywhere)
+}