Repository: bazelbuild/buildtools
Pull request #425 (2018-11-12 17:40:42)

Implement a warning and a fix for the old style args API usage


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 0eb550e..0e20995 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -465,3 +465,17 @@ Please use the [depset](https://docs.bazel.build/versions/master/skylark/lib/dep
 instead:
 
     depset(transitive = [depset1, depset2])
+
+--------------------------------------------------------------------------------
+
+## <a name="args-api"></a>`ctx.actions.args().add()` for multiple arguments is deprecated
+
+  * Category_name: `args-api`
+  * Flag in Bazel: [`--incompatible_disallow_old_style_args_add`](https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#new-args-api)
+  * Automatic fix: yes
+
+It's deprecated to use the [`add`](https://docs.bazel.build/versions/master/skylark/lib/Args.html#add)
+method of `ctx.actions.args()` to add a list (or a depset) of variables. Please use either
+[`add_all`](https://docs.bazel.build/versions/master/skylark/lib/Args.html#add_all) or
+[`add_joined`](https://docs.bazel.build/versions/master/skylark/lib/Args.html#add_joined),
+depending on the desired behavior.
diff --git a/warn/types.go b/warn/types.go
index f650d69..d686a80 100644
--- a/warn/types.go
+++ b/warn/types.go
@@ -12,6 +12,9 @@ type Type int
 const (
 	Unknown Type = iota
 	Bool
+	Ctx
+	CtxActions
+	CtxActionsArgs
 	Depset
 	Dict
 	Int
@@ -23,6 +26,9 @@ func (t Type) String() string {
 	return [...]string{
 		"unknown",
 		"bool",
+		"ctx",
+		"ctx.actions",
+		"ctx.actions.args",
 		"depset",
 		"dict",
 		"int",
@@ -66,6 +72,10 @@ func detectTypes(f *build.File) map[build.Expr]Type {
 				case "dict":
 					nodeType = Dict
 				}
+			} else if dot, ok := (node.X).(*build.DotExpr); ok {
+				if result[dot.X] == CtxActions && dot.Name == "args" {
+					nodeType = CtxActionsArgs
+				}
 			}
 		case *build.ParenExpr:
 			nodeType = result[node.X]
@@ -77,6 +87,12 @@ func detectTypes(f *build.File) map[build.Expr]Type {
 			case "None":
 				nodeType = None
 				return
+			case "ctx":
+				binding := env.Get(node.Name)
+				if binding != nil && binding.Kind == bzlenv.Parameter {
+					nodeType = Ctx
+					return
+				}
 			}
 			binding := env.Get(node.Name)
 			if binding != nil {
@@ -84,6 +100,10 @@ func detectTypes(f *build.File) map[build.Expr]Type {
 					nodeType = t
 				}
 			}
+		case *build.DotExpr:
+			if result[node.X] == Ctx && node.Name == "actions" {
+				nodeType = CtxActions
+			}
 		case *build.BinaryExpr:
 			switch node.Op {
 			case "=", "+=", "-=", "*=", "/=", "//=", "%=", "|=":
diff --git a/warn/warn.go b/warn/warn.go
index f0d1a20..8bec7fa 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -645,6 +645,7 @@ var RuleWarningMap = map[string]func(f *build.File, pkg string, expr build.Expr)
 
 // FileWarningMap lists the warnings that run on the whole file.
 var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
+	"args-api":           argsAPIWarning,
 	"attr-cfg":           attrConfigurationWarning,
 	"attr-non-empty":     attrNonEmptyWarning,
 	"attr-single-file":   attrSingleFileWarning,
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index 039cad1..cd2efbb 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -432,3 +432,58 @@ func nativeGitRepositoryWarning(f *build.File, fix bool) []*Finding {
 func nativeHTTPArchiveWarning(f *build.File, fix bool) []*Finding {
 	return notLoadedFunctionUsageCheck(f, "http-archive", []string{"http_archive"}, "@bazel_tools//tools/build_defs/repo:http.bzl", fix)
 }
+
+func argsAPIWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+	types := detectTypes(f)
+
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		// Search for `<ctx.actions.args>.add()` nodes
+		call, ok := expr.(*build.CallExpr)
+		if !ok {
+			return
+		}
+		dot, ok := call.X.(*build.DotExpr)
+		if !ok || dot.Name != "add" || types[dot.X] != CtxActionsArgs {
+			return
+		}
+
+		// If neither before_each nor join_with nor map_fn is specified, the node is ok.
+		// Otherwise if join_with is specified, use `.add_joined` instead.
+		// Otherwise use `.add_all` instead.
+
+		_, beforeEachKw, beforeEach := getParam(call.List, "before_each")
+		_, _, joinWith := getParam(call.List, "join_with")
+		_, mapFnKw, mapFn := getParam(call.List, "map_fn")
+		if beforeEach == nil && joinWith == nil && mapFn == nil {
+			// No deprecated API detected
+			return
+		}
+		if !fix {
+			start, end := call.Span()
+			findings = append(findings,
+				makeFinding(f, start, end, "args-api",
+					"\"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".", true, nil))
+			return
+		}
+
+		dot.Name = "add_all"
+		if joinWith != nil {
+			dot.Name = "add_joined"
+			if beforeEach != nil {
+				// `add_joined` doesn't have a `before_each` parameter, replace it with `format_each`:
+				// `before_each = foo` -> `format_each = foo + "%s"`
+				beforeEachKw.Name = "format_each"
+				beforeEach.Y = &build.BinaryExpr{
+					X:  beforeEach.Y,
+					Op: "+",
+					Y:  &build.StringExpr{Value: "%s"},
+				}
+			}
+		}
+		if mapFnKw != nil {
+			mapFnKw.Name = "map_each"
+		}
+	})
+	return findings
+}


Test patch:
diff --git a/warn/types_test.go b/warn/types_test.go
index 9d7000e..11509e2 100644
--- a/warn/types_test.go
+++ b/warn/types_test.go
@@ -188,3 +188,39 @@ boq %= int:<n>
 boq
 `)
 }
+
+func TestContext(t *testing.T) {
+	checkTypes(t, `
+def foobar(ctx, foo, bar):
+    ctx
+    ctx.actions
+    ctx.actions.args()
+
+    actions = ctx.actions
+    not_args = actions.args
+    args = actions.args()
+    args
+`, `
+def foobar(ctx:<ctx>, foo, bar):
+    ctx:<ctx>
+    ctx.actions:<ctx:<ctx>.actions>
+    ctx.actions.args:<ctx.actions:<ctx:<ctx>.actions>.args()>
+
+    actions = ctx.actions:<ctx:<ctx>.actions>
+    not_args = ctx.actions:<actions>.args
+    args = ctx.actions.args:<ctx.actions:<actions>.args()>
+    ctx.actions.args:<args>
+`)
+
+	checkTypes(t, `
+def foobar(foo, bar):
+    ctx
+    ctx.actions
+    ctx.actions.args()
+`, `
+def foobar(foo, bar):
+    ctx
+    ctx.actions
+    ctx.actions.args()
+`)
+}
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index 2f543c7..f8f176f 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -331,3 +331,39 @@ def macro():
 		},
 		false)
 }
+
+func TestArgsApiWarning(t *testing.T) {
+	checkFindingsAndFix(t, "args-api", `
+def impl(ctx):
+    args = ctx.actions.args()
+    args.add(foo, bar)
+    args.add(foo, bar, before_each = aaa)
+    args.add(foo, bar, join_with = bbb)
+    args.add(foo, bar, before_each = ccc, join_with = ddd)
+    args.add(foo, bar, map_fn = eee)
+    args.add(foo, bar, map_fn = fff, before_each = ggg)
+    args.add(foo, bar, map_fn = hhh, join_with = iii)
+    args.add(foo, bar, map_fn = jjj, before_each = kkk, join_with = lll)
+`, `
+def impl(ctx):
+    args = ctx.actions.args()
+    args.add(foo, bar)
+    args.add_all(foo, bar, before_each = aaa)
+    args.add_joined(foo, bar, join_with = bbb)
+    args.add_joined(foo, bar, format_each = ccc + "%s", join_with = ddd)
+    args.add_all(foo, bar, map_each = eee)
+    args.add_all(foo, bar, map_each = fff, before_each = ggg)
+    args.add_joined(foo, bar, map_each = hhh, join_with = iii)
+    args.add_joined(foo, bar, map_each = jjj, format_each = kkk + "%s", join_with = lll)
+`,
+		[]string{
+			":4: \"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".",
+			":5: \"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".",
+			":6: \"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".",
+			":7: \"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".",
+			":8: \"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".",
+			":9: \"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".",
+			":10: \"ctx.actions.args().add()\" for multiple arguments is deprecated in favor of \"add_all()\" or \"add_joined()\".",
+		},
+		false)
+}
\ No newline at end of file