Repository: bazelbuild/buildtools
Pull request #415 (2018-11-09 12:56:05)

Implement a warning for the deprecated dict concatenation


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index d34b713..89084d8 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -315,3 +315,27 @@ Native `http_archive` function are [being removed](https://docs.bazel.build/vers
 Please use the Starklark versions instead:
 
     load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+
+--------------------------------------------------------------------------------
+
+## <a name="dict-concatenation"></a>Dictionary concatenation is deprecated
+
+The `+` operator to concatenate dicts is deprecated. The operator used to create a new dict and
+copy the data to it. There are several ways to avoid it, for example, instead of `d = d1 + d2 + d3`
+you can use one of the following:
+
+  * Use [Skylib](https://github.com/bazelbuild/bazel-skylib):
+  
+    load("@bazel_skylib//lib/dicts.bzl", "dicts")
+    
+    d = dicts.add(d1, d2, d3)
+
+  * The same if you don't want to use Skylib:
+
+    d = dict(d1.items() + d2.items() + d3.items())
+
+  * The same in several steps:
+
+    d = dict(d1)  # If you don't want `d1` to be mutated
+    d.update(d2)
+    d.update(d3)
diff --git a/warn/warn.go b/warn/warn.go
index 30f2cb7..d4d9165 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -431,6 +431,27 @@ func noEffectWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
+func dictionaryConcatenationWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+	types := detectTypes(f)
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		binary, ok := expr.(*build.BinaryExpr)
+		if !ok {
+			return
+		}
+		if binary.Op != "+" && binary.Op != "+=" {
+			return
+		}
+		if types[binary.X] == Dict || types[binary.Y] == Dict {
+			start, end := binary.Span()
+			findings = append(findings,
+				makeFinding(f, start, end, "dict-concatenation",
+					"Dictionary concatenation is deprecated.", true, 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{
@@ -444,6 +465,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"attr-single-file":   attrSingleFileWarning,
 	"constant-glob":      constantGlobWarning,
 	"ctx-actions":        ctxActionsWarning,
+	"dict-concatenation": dictionaryConcatenationWarning,
 	"duplicated-name":    duplicatedNameWarning,
 	"filetype":           fileTypeWarning,
 	"git-repository":     nativeGitRepositoryWarning,


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index ce0ae4f..746d56b 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -392,3 +392,24 @@ d //= e
 		},
 		false)
 }
+
+func TestDictionaryConcatenation(t *testing.T) {
+	checkFindings(t, "dict-concatenation", `
+d = {}
+
+d + foo
+foo + d
+d + foo + bar  # Should trigger 2 warnings: (d + foo) is recognized as a dict
+foo + bar + d  # Should trigger 1 warning: (foo + bar) is unknown
+d += foo + bar
+`,
+		[]string{
+			":3: Dictionary concatenation is deprecated.",
+			":4: Dictionary concatenation is deprecated.",
+			":5: Dictionary concatenation is deprecated.",
+			":5: Dictionary concatenation is deprecated.",
+			":6: Dictionary concatenation is deprecated.",
+			":7: Dictionary concatenation is deprecated.",
+		},
+		false)
+}