Repository: bazelbuild/buildtools
Pull request #416 (2018-11-09 14:05:32)

Implement a warning for the deprecated string iteration

vladmos: @laurentlb Please approve again, approvals on Github expire after every additional commit.

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 89084d8..534f91d 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -339,3 +339,16 @@ you can use one of the following:
     d = dict(d1)  # If you don't want `d1` to be mutated
     d.update(d2)
     d.update(d3)
+
+--------------------------------------------------------------------------------
+
+## <a name="string-iteration"></a>String iteration is deprecated
+
+Iteration over strings often leads to confusion with iteration over a sequence of strings,
+therefore strings won't be recognized as sequences of 1-element strings (like in Python).
+Use string indexing and `len` instead:
+
+    my_string = "hello world"
+    for i in range(len(my_string)):
+        char = my_string[i]
+        # do something with char
diff --git a/warn/warn.go b/warn/warn.go
index d4d9165..22f57f3 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -452,6 +452,52 @@ func dictionaryConcatenationWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
+func stringIterationWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+
+	addWarning := func(expr build.Expr) {
+		start, end := expr.Span()
+		findings = append(findings,
+			makeFinding(f, start, end, "string-iteration",
+				"String iteration is deprecated.", true, nil))
+	}
+
+	types := detectTypes(f)
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		switch expr := expr.(type) {
+		case *build.ForStmt:
+			if types[expr.X] == String {
+				addWarning(expr.X)
+			}
+		case *build.ForClause:
+			if types[expr.X] == String {
+				addWarning(expr.X)
+			}
+		case *build.CallExpr:
+			ident, ok := expr.X.(*build.Ident)
+			if !ok {
+				return
+			}
+			switch ident.Name {
+			case "all", "any", "reversed", "max", "min":
+				if len(expr.List) != 1 {
+					return
+				}
+				if types[expr.List[0]] == String {
+					addWarning(expr.List[0])
+				}
+			case "zip":
+				for _, arg := range expr.List {
+					if types[arg] == String {
+						addWarning(arg)
+					}
+				}
+			}
+		}
+	})
+	return findings
+}
+
 // RuleWarningMap lists the warnings that run on a single rule.
 // These warnings run only on BUILD files (not bzl files).
 var RuleWarningMap = map[string]func(f *build.File, pkg string, expr build.Expr) *Finding{
@@ -479,6 +525,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"package-on-top":     packageOnTopWarning,
 	"redefined-variable": redefinedVariableWarning,
 	"repository-name":    repositoryNameWarning,
+	"string-iteration":   stringIterationWarning,
 	"unused-variable":    unusedVariableWarning,
 }
 


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 746d56b..67bbd65 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -413,3 +413,45 @@ d += foo + bar
 		},
 		false)
 }
+
+func TestStringIteration(t *testing.T) {
+	checkFindings(t, "string-iteration", `
+s = "foo" + bar
+
+max(s)
+min(s)
+all(s)
+any(s)
+reversed(s)
+zip(s, a, b)
+zip(a, s)
+
+[foo(x) for x in s]
+
+for x in s:
+    pass
+
+# The following iterations over a list don't trigger warnings
+
+l = list()
+
+max(l)
+zip(l, foo)
+[foo(x) for x in l]
+
+for x in l:
+    pass
+`,
+		[]string{
+			":3: String iteration is deprecated.",
+			":4: String iteration is deprecated.",
+			":5: String iteration is deprecated.",
+			":6: String iteration is deprecated.",
+			":7: String iteration is deprecated.",
+			":8: String iteration is deprecated.",
+			":9: String iteration is deprecated.",
+			":11: String iteration is deprecated.",
+			":13: String iteration is deprecated.",
+		},
+		false)
+}