Repository: bazelbuild/buildtools
Pull request #537 (2019-02-07 14:53:36)

Warn about confusing short names
Names `l`, `I`, and `O` can be confused with `I`, `l`, and `0`.


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 1ef67c4..35d8aa6 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -7,6 +7,7 @@ Warning categories supported by buildifier's linter:
   * [attr-non-empty](#attr-non-empty)
   * [attr-output-default](#attr-output-default)
   * [attr-single-file](#attr-single-file)
+  * [confusing-name](#confusing-name)
   * [constant-glob](#constant-glob)
   * [ctx-actions](#ctx-actions)
   * [ctx-args](#ctx-args)
@@ -98,6 +99,15 @@ is deprecated, please use `allow_single_file` instead.
 
 --------------------------------------------------------------------------------
 
+## <a name="confusing-name"></a>Never use `l`, `I`, or `O` as names
+
+  * Category_name: `confusing-name`
+  * Automatic fix: no
+
+The names `l`, `I`, or `O` can be easily confused with `I`, `l`, or `0` correspondingly.
+
+--------------------------------------------------------------------------------
+
 ## <a name="constant-glob"></a>Glob pattern has no wildcard ('*')
 
   * Category name: `constant-glob`
diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel
index d36f031..f1b7553 100644
--- a/warn/BUILD.bazel
+++ b/warn/BUILD.bazel
@@ -11,6 +11,7 @@ go_library(
         "warn_control_flow.go",
         "warn_cosmetic.go",
         "warn_docstring.go",
+        "warn_naming.go",
         "warn_operation.go",
     ],
     importpath = "github.com/bazelbuild/buildtools/warn",
@@ -34,6 +35,7 @@ go_test(
         "warn_control_flow_test.go",
         "warn_cosmetic_test.go",
         "warn_docstring_test.go",
+        "warn_naming_test.go",
         "warn_operation_test.go",
         "warn_test.go",
     ],
diff --git a/warn/warn.go b/warn/warn.go
index 54eb3a9..102643a 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -72,6 +72,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"attr-non-empty":      attrNonEmptyWarning,
 	"attr-output-default": attrOutputDefaultWarning,
 	"attr-single-file":    attrSingleFileWarning,
+	"confusing-name":      confusingNameWarning,
 	"constant-glob":       constantGlobWarning,
 	"ctx-actions":         ctxActionsWarning,
 	"ctx-args":            contextArgsAPIWarning,
diff --git a/warn/warn_control_flow.go b/warn/warn_control_flow.go
index d5e0d33..fbd768c 100644
--- a/warn/warn_control_flow.go
+++ b/warn/warn_control_flow.go
@@ -334,34 +334,24 @@ func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
-// collectLocalVariables traverses statements (e.g. of a function definition) and returns a map of
-// variables defined anywhere inside the function.
-func collectLocalVariables(stmts []build.Expr) map[string]bool {
-	variables := make(map[string]bool)
+// collectLocalVariables traverses statements (e.g. of a function definition) and returns a list
+// of idents for variables defined anywhere inside the function.
+func collectLocalVariables(stmts []build.Expr) []*build.Ident {
+	variables := []*build.Ident{}
 
 	for _, stmt := range stmts {
 		switch stmt := stmt.(type) {
 		case *build.DefStmt:
 			// Don't traverse nested functions
 		case *build.ForStmt:
-			for _, key := range bzlenv.CollectLValues(stmt.Vars) {
-				variables[key.Name] = true
-			}
-			for key := range collectLocalVariables(stmt.Body) {
-				variables[key] = true
-			}
+			variables = append(variables, bzlenv.CollectLValues(stmt.Vars)...)
+			variables = append(variables, collectLocalVariables(stmt.Body)...)
 		case *build.IfStmt:
-			for key := range collectLocalVariables(stmt.True) {
-				variables[key] = true
-			}
-			for key := range collectLocalVariables(stmt.False) {
-				variables[key] = true
-			}
+			variables = append(variables, collectLocalVariables(stmt.True)...)
+			variables = append(variables, collectLocalVariables(stmt.False)...)
 		case *build.BinaryExpr:
 			if stmt.Op == "=" {
-				for _, key := range bzlenv.CollectLValues(stmt.X) {
-					variables[key.Name] = true
-				}
+				variables = append(variables, bzlenv.CollectLValues(stmt.X)...)
 			}
 		}
 	}
@@ -469,6 +459,27 @@ func findUninitializedVariables(stmts []build.Expr, previouslyInitialized map[st
 	return false, locallyInitialized
 }
 
+func getFunctionParams(def *build.DefStmt) []*build.Ident {
+	params := []*build.Ident{}
+	for _, node := range def.Params {
+		switch node := node.(type) {
+		case *build.Ident:
+			params = append(params, node)
+		case *build.UnaryExpr:
+			// either *args or **kwargs
+			if ident, ok := node.X.(*build.Ident); ok {
+				params = append(params, ident)
+			}
+		case *build.BinaryExpr:
+			// x = value
+			if ident, ok := node.X.(*build.Ident); ok {
+				params = append(params, ident)
+			}
+		}
+	}
+	return params
+}
+
 // uninitializedVariableWarning warns about usages of values that may not have been initialized.
 func uninitializedVariableWarning(f *build.File, _ bool) []*Finding {
 	findings := []*Finding{}
@@ -480,25 +491,15 @@ func uninitializedVariableWarning(f *build.File, _ bool) []*Finding {
 
 		// Get all variables defined in the function body.
 		// If a variable is not defined there, it can be builtin, global, or loaded.
-		localVars := collectLocalVariables(def.Body)
+		localVars := make(map[string]bool)
+		for _, ident := range collectLocalVariables(def.Body) {
+			localVars[ident.Name] = true
+		}
 
 		// Function parameters are guaranteed to be defined everywhere in the function, even if they
 		// are redefined inside the function body. They shouldn't be taken into consideration.
-		for _, node := range def.Params {
-			switch node := node.(type) {
-			case *build.Ident:
-				delete(localVars, node.Name)
-			case *build.UnaryExpr:
-				// either *args or **kwargs
-				if ident, ok := node.X.(*build.Ident); ok {
-					delete(localVars, ident.Name)
-				}
-			case *build.BinaryExpr:
-				// x = value
-				if ident, ok := node.X.(*build.Ident); ok {
-					delete(localVars, ident.Name)
-				}
-			}
+		for _, ident := range getFunctionParams(def) {
+			delete(localVars, ident.Name)
 		}
 
 		// Search for all potentially initialized variables in the function body
diff --git a/warn/warn_naming.go b/warn/warn_naming.go
new file mode 100644
index 0000000..3bb5215
--- /dev/null
+++ b/warn/warn_naming.go
@@ -0,0 +1,67 @@
+package warn
+
+import (
+	"github.com/bazelbuild/buildtools/build"
+	"github.com/bazelbuild/buildtools/bzlenv"
+)
+
+var ambiguousNames = map[string]bool{
+	"I": true,
+	"l": true,
+	"O": true,
+}
+
+// ambiguousNameCheck checks for the names of idents and functions
+func ambiguousNameCheck(f *build.File, expr build.Expr, findings []*Finding) []*Finding {
+	var name string
+	switch expr := expr.(type) {
+	case *build.Ident:
+		name = expr.Name
+	case *build.DefStmt:
+		name = expr.Name
+	default:
+		return findings
+	}
+
+	if ambiguousNames[name] {
+		start, end := expr.Span()
+		findings = append(findings,
+			makeFinding(f, start, end, "confusing-name",
+				`Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').`, true, nil))
+	}
+	return findings
+}
+
+func confusingNameWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+
+	// check for global variable names
+	for _, ident := range collectLocalVariables(f.Stmt) {
+		findings = ambiguousNameCheck(f, ident, findings)
+	}
+
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		switch expr := expr.(type) {
+		case *build.DefStmt:
+			findings = ambiguousNameCheck(f, expr, findings)
+			for _, param := range getFunctionParams(expr) {
+				findings = ambiguousNameCheck(f, param, findings)
+			}
+			for _, ident := range collectLocalVariables(expr.Body) {
+				findings = ambiguousNameCheck(f, ident, findings)
+			}
+		case *build.Comprehension:
+			for _, clause := range expr.Clauses {
+				forClause, ok := clause.(*build.ForClause)
+				if !ok {
+					continue
+				}
+				for _, ident := range bzlenv.CollectLValues(forClause.Vars) {
+					findings = ambiguousNameCheck(f, ident, findings)
+				}
+			}
+		}
+	})
+
+	return findings
+}


Test patch:
diff --git a/warn/warn_naming_test.go b/warn/warn_naming_test.go
new file mode 100644
index 0000000..449c16a
--- /dev/null
+++ b/warn/warn_naming_test.go
@@ -0,0 +1,80 @@
+package warn
+
+import "testing"
+
+func TestAmbiguousNames(t *testing.T) {
+	checkFindings(t, "confusing-name", `
+i = 0
+I, x = 1, 2  # here
+l = 3  # here
+L = []
+L[l] = 4
+O = 5  # here
+
+x, l = a, b  # here
+a, b = x, l
+`,
+		[]string{
+			":2: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":3: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":6: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":8: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+		}, scopeEverywhere)
+
+	checkFindings(t, "confusing-name", `
+def l():  # here
+  if True:
+    i = 1
+    I = 2  # here
+  else:
+    l = 3  # here
+    L = 4
+  
+  for O in Os:  # here
+    pass
+
+  x, l = a, b  # here
+  a, b = x, l
+`,
+		[]string{
+			":1: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":4: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":6: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":9: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":12: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+		}, scopeEverywhere)
+
+	checkFindings(t, "confusing-name", `
+[l for l in s]
+
+cc_library(
+  name = "name",
+  tags = [I for I in tags if I],
+)
+
+def f(x):
+  return [O for O in x]
+`,
+		[]string{
+			":1: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":5: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":9: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+		}, scopeEverywhere)
+
+	checkFindings(t, "confusing-name", `
+[
+  foo(l, I, O)
+  for l in ls  # here
+  if l < m
+  for I, (x, O) in bar # here 2 times
+  for L in Ls
+  if I > l and O == 0
+  if x == L
+]
+`,
+		[]string{
+			":3: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":5: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+			":5: Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').",
+		}, scopeEverywhere)
+}