Repository: bazelbuild/buildtools
Pull request #423 (2018-11-12 15:43:31)

Implement a warning for the deprecated depset concatenation syntax


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 49ba4d2..daed0f0 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -364,3 +364,18 @@ 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
+
+--------------------------------------------------------------------------------
+
+## <a name="depset-union"></a>Depsets should be joined using the depset constructor
+
+The following ways to merge two depsets are deprecated:
+
+    depset1 + depset2
+    depset1 | depset2
+    depset1.union(depset2)
+
+Please use the [depset](https://docs.bazel.build/versions/master/skylark/lib/depset.html) constructor
+instead:
+
+    depset(transitive = [depset1, depset2])
diff --git a/warn/warn.go b/warn/warn.go
index e45cceb..f0d1a20 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -452,6 +452,48 @@ func dictionaryConcatenationWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
+func depsetUnionWarning(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-union",
+				"Depsets should be joined using the depset constructor.", true, nil))
+	}
+
+	types := detectTypes(f)
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		switch expr := expr.(type) {
+		case *build.BinaryExpr:
+			// `depset1 + depset2` or `depset1 | depset2`
+			if types[expr.X] != Depset && types[expr.Y] != Depset {
+				return
+			}
+			switch expr.Op {
+			case "+", "|", "+=", "|=":
+				addWarning(expr)
+			}
+		case *build.CallExpr:
+			// `depset1.union(depset2)`
+			if len(expr.List) == 0 {
+				return
+			}
+			dot, ok := expr.X.(*build.DotExpr)
+			if !ok {
+				return
+			}
+			if dot.Name != "union" {
+				return
+			}
+			if types[dot.X] != Depset && types[expr.List[0]] != Depset {
+				return
+			}
+			addWarning(expr)
+		}
+	})
+	return findings
+}
+
 func stringIterationWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 
@@ -609,6 +651,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"constant-glob":      constantGlobWarning,
 	"ctx-actions":        ctxActionsWarning,
 	"depset-iteration":   depsetIterationWarning,
+	"depset-union":       depsetUnionWarning,
 	"dict-concatenation": dictionaryConcatenationWarning,
 	"duplicated-name":    duplicatedNameWarning,
 	"filetype":           fileTypeWarning,


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index a59dfa3..bbb3157 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -32,7 +32,9 @@ func compareFinding(t *testing.T, input string, expected []string, findings []*F
 	if len(expected) != len(findings) {
 		t.Errorf("Input: %s", input)
 		t.Errorf("number of matches: %d, want %d", len(findings), len(expected))
-		t.Errorf("expected findings: %v", expected)
+		for _, e := range expected {
+			t.Errorf("expected: %s", e)
+		}
 		for _, f := range findings {
 			t.Errorf("got: %d: %s", f.Start.Line, f.Message)
 		}
@@ -567,3 +569,49 @@ for x in l:
 		},
 		false)
 }
+
+func TestDepsetUnion(t *testing.T) {
+	checkFindings(t, "depset-union", `
+d = depset([1, 2, 3])
+
+d + foo
+foo + d
+d + foo + bar
+foo + bar + d
+
+d | foo
+foo | d
+d | foo | bar
+foo | bar | d
+
+d += foo
+d |= bar
+foo += d
+bar |= d
+
+d.union(aaa)
+bbb.union(d)
+
+ccc.union(ddd)
+eee + fff | ggg
+`,
+		[]string{
+			":3: Depsets should be joined using the depset constructor",
+			":4: Depsets should be joined using the depset constructor",
+			":5: Depsets should be joined using the depset constructor",
+			":5: Depsets should be joined using the depset constructor",
+			":6: Depsets should be joined using the depset constructor",
+			":8: Depsets should be joined using the depset constructor",
+			":9: Depsets should be joined using the depset constructor",
+			":10: Depsets should be joined using the depset constructor",
+			":10: Depsets should be joined using the depset constructor",
+			":11: Depsets should be joined using the depset constructor",
+			":13: Depsets should be joined using the depset constructor",
+			":14: Depsets should be joined using the depset constructor",
+			":15: Depsets should be joined using the depset constructor",
+			":16: Depsets should be joined using the depset constructor",
+			":18: Depsets should be joined using the depset constructor",
+			":19: Depsets should be joined using the depset constructor",
+		},
+		false)
+}