Repository: bazelbuild/buildtools
Pull request #418 (2018-11-09 17:58:12)

Implement a fix for depset iterations

laurentlb: cc @meisterT 

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 534f91d..49ba4d2 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -352,3 +352,15 @@ Use string indexing and `len` instead:
     for i in range(len(my_string)):
         char = my_string[i]
         # do something with char
+
+--------------------------------------------------------------------------------
+
+## <a name="depset-iteration"></a>Depset iteration is deprecated
+
+Depsets are complex structures, iterations over them and lookups require flattening them to
+a list which may be a heavy operation. To make it more obvious it's now required to call
+the `.to_list()` method on them in order to be able to iterate their items:
+
+    deps = depset()
+    [x.path for x in deps]  # deprecated
+    [x.path for x in deps.to_list()]  # recommended
diff --git a/warn/types.go b/warn/types.go
index 77b8246..f650d69 100644
--- a/warn/types.go
+++ b/warn/types.go
@@ -144,7 +144,7 @@ func walkOnce(node build.Expr, env *bzlenv.Environment, fct func(e *build.Expr,
 	case *build.CallExpr:
 		fct(&expr.X, env)
 		for _, param := range expr.List {
-			if binary, ok := param.(*build.BinaryExpr); ok {
+			if binary, ok := param.(*build.BinaryExpr); ok && binary.Op == "=" {
 				fct(&binary.Y, env)
 			} else {
 				fct(&param, env)
diff --git a/warn/warn.go b/warn/warn.go
index 22f57f3..e45cceb 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -498,6 +498,103 @@ func stringIterationWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
+func depsetIterationWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+
+	addWarning := func(expr build.Expr) {
+		start, end := expr.Span()
+		findings = append(findings,
+			makeFinding(f, start, end, "depset-iteration",
+				"Depset iteration is deprecated.", true, nil))
+	}
+
+	// fixNode returns a call for .to_list() on the input node (assuming that it's a depset)
+	fixNode := func(expr build.Expr) build.Expr {
+		_, end := expr.Span()
+		return &build.CallExpr{
+			X: &build.DotExpr{
+				X:    expr,
+				Name: "to_list",
+			},
+			End: build.End{Pos: end},
+		}
+	}
+
+	types := detectTypes(f)
+	build.Edit(f, func(expr build.Expr, stack []build.Expr) build.Expr {
+		switch expr := expr.(type) {
+		case *build.ForStmt:
+			if types[expr.X] != Depset {
+				return nil
+			}
+			if !fix {
+				addWarning(expr.X)
+				return nil
+			}
+			expr.X = fixNode(expr.X)
+		case *build.ForClause:
+			if types[expr.X] != Depset {
+				return nil
+			}
+			if !fix {
+				addWarning(expr.X)
+				return nil
+			}
+			expr.X = fixNode(expr.X)
+		case *build.BinaryExpr:
+			if expr.Op != "in" && expr.Op != "not in" {
+				return nil
+			}
+			if types[expr.Y] != Depset {
+				return nil
+			}
+			if !fix {
+				addWarning(expr.Y)
+				return nil
+			}
+			expr.Y = fixNode(expr.Y)
+		case *build.CallExpr:
+			ident, ok := expr.X.(*build.Ident)
+			if !ok {
+				return nil
+			}
+			switch ident.Name {
+			case "all", "any", "depset", "len", "sorted", "max", "min", "list", "tuple":
+				if len(expr.List) != 1 {
+					return nil
+				}
+				if types[expr.List[0]] != Depset {
+					return nil
+				}
+				if !fix {
+					addWarning(expr.List[0])
+					return nil
+				}
+				newNode := fixNode(expr.List[0])
+				if ident.Name != "list" {
+					expr.List[0] = newNode
+					return nil
+				}
+				// `list(d.to_list())` can be simplified to just `d.to_list()`
+				return newNode
+			case "zip":
+				for i, arg := range expr.List {
+					if types[arg] != Depset {
+						continue
+					}
+					if !fix {
+						addWarning(arg)
+						return nil
+					}
+					expr.List[i] = fixNode(arg)
+				}
+			}
+		}
+		return nil
+	})
+	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{
@@ -511,6 +608,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"attr-single-file":   attrSingleFileWarning,
 	"constant-glob":      constantGlobWarning,
 	"ctx-actions":        ctxActionsWarning,
+	"depset-iteration":   depsetIterationWarning,
 	"dict-concatenation": dictionaryConcatenationWarning,
 	"duplicated-name":    duplicatedNameWarning,
 	"filetype":           fileTypeWarning,


Test patch:
diff --git a/warn/types_test.go b/warn/types_test.go
index 387cfb8..9d7000e 100644
--- a/warn/types_test.go
+++ b/warn/types_test.go
@@ -93,7 +93,7 @@ y = 4
 z = 5
 
 foo(y = "bar")
-foo(x, y = bar(z = z))
+foo(x, y = bar(z = z), t + z)
 
 
 def f(z = "bar"):
@@ -106,7 +106,7 @@ y = int:<4>
 z = int:<5>
 
 foo(y = string:<"bar">)
-foo(int:<x>, y = bar(z = int:<z>))
+foo(int:<x>, y = bar(z = int:<z>), int:<t + int:<z>>)
 
 def f(z = string:<"bar">):
     return string:<z>
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 67bbd65..a59dfa3 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -455,3 +455,115 @@ for x in l:
 		},
 		false)
 }
+
+func TestDepsetIteration(t *testing.T) {
+	checkFindingsAndFix(t, "depset-iteration", `
+d = depset([1, 2, 3]) + bar
+
+max(d + foo)
+min(d)
+all(d)
+any(d)
+sorted(d)
+zip(
+    d,
+    a,
+    b,
+)
+zip(
+     a,
+     d,
+)
+list(d)
+tuple(d)
+depset(d)
+len(d)
+1 in d
+2 not in d
+
+[foo(x) for x in d]
+
+for x in d:
+    pass
+
+# Non-iteration is ok
+
+foobar(d)
+d == b
+
+# The following iterations over a list don't trigger warnings
+
+l = list([1, 2, 3])
+
+max(l)
+zip(l, foo)
+[foo(x) for x in l]
+1 in l
+
+for x in l:
+    pass
+`, `
+d = depset([1, 2, 3]) + bar
+
+max((d + foo).to_list())
+min(d.to_list())
+all(d.to_list())
+any(d.to_list())
+sorted(d.to_list())
+zip(
+    d.to_list(),
+    a,
+    b,
+)
+zip(
+    a,
+    d.to_list(),
+)
+d.to_list()
+tuple(d.to_list())
+depset(d.to_list())
+len(d.to_list())
+1 in d.to_list()
+2 not in d.to_list()
+
+[foo(x) for x in d.to_list()]
+
+for x in d.to_list():
+    pass
+
+# Non-iteration is ok
+
+foobar(d)
+d == b
+
+# The following iterations over a list don't trigger warnings
+
+l = list([1, 2, 3])
+
+max(l)
+zip(l, foo)
+[foo(x) for x in l]
+1 in l
+
+for x in l:
+    pass
+`,
+		[]string{
+			":3: Depset iteration is deprecated.",
+			":4: Depset iteration is deprecated.",
+			":5: Depset iteration is deprecated.",
+			":6: Depset iteration is deprecated.",
+			":7: Depset iteration is deprecated.",
+			":9: Depset iteration is deprecated.",
+			":15: Depset iteration is deprecated.",
+			":17: Depset iteration is deprecated.",
+			":18: Depset iteration is deprecated.",
+			":19: Depset iteration is deprecated.",
+			":20: Depset iteration is deprecated.",
+			":21: Depset iteration is deprecated.",
+			":22: Depset iteration is deprecated.",
+			":24: Depset iteration is deprecated.",
+			":26: Depset iteration is deprecated.",
+		},
+		false)
+}