Repository: bazelbuild/buildtools
Pull request #427 (2018-11-13 16:20:07)

Fix for the order of function arguments


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 0e20995..d5fa9ad 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -468,9 +468,9 @@ instead:
 
 --------------------------------------------------------------------------------
 
-## <a name="args-api"></a>`ctx.actions.args().add()` for multiple arguments is deprecated
+## <a name="ctx-args"></a>`ctx.actions.args().add()` for multiple arguments is deprecated
 
-  * Category_name: `args-api`
+  * Category_name: `ctx-args`
   * 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
 
@@ -479,3 +479,18 @@ method of `ctx.actions.args()` to add a list (or a depset) of variables. Please
 [`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.
+
+--------------------------------------------------------------------------------
+
+## <a name="args-order"></a>Function call arguments should be in the following order
+
+  * Category_name: `args-order`
+  * Flag in Bazel: `--incompatible_strict_argument_ordering`
+  * Automatic fix: yes
+
+Function call arguments should be in the following order:
+
+  * Positional arguments
+  * Keyword arguments
+  * Optional `*arg`
+  * Optional `**kwarg`
diff --git a/warn/warn.go b/warn/warn.go
index c8e04ab..e035455 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -640,6 +640,52 @@ func depsetIterationWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
+func argumentsOrderWarning(f *build.File, fix bool) []*Finding {
+	argumentType := func(expr build.Expr) int {
+		switch expr := expr.(type) {
+		case *build.UnaryExpr:
+			switch expr.Op {
+			case "**":
+				return 4
+			case "*":
+				return 3
+			}
+		case *build.BinaryExpr:
+			if expr.Op == "=" {
+				return 2
+			}
+		}
+		return 1
+	}
+
+	getComparator := func(args []build.Expr) func(i, j int) bool {
+		return func(i, j int) bool {
+			return argumentType(args[i]) < argumentType(args[j])
+		}
+	}
+
+	findings := []*Finding{}
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		call, ok := expr.(*build.CallExpr)
+		if !ok {
+			return
+		}
+		comparator := getComparator(call.List)
+		if fix {
+			sort.SliceStable(call.List, comparator)
+			return
+		}
+		if sort.SliceIsSorted(call.List, comparator) {
+			return
+		}
+		start, end := expr.Span()
+		findings = append(findings,
+			makeFinding(f, start, end, "args-order",
+				"Function call arguments should be in the following order: positional, keyword, *args, **kwargs.", 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{
@@ -648,12 +694,13 @@ 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,
+	"args-order":         argumentsOrderWarning,
 	"attr-cfg":           attrConfigurationWarning,
 	"attr-non-empty":     attrNonEmptyWarning,
 	"attr-single-file":   attrSingleFileWarning,
 	"constant-glob":      constantGlobWarning,
 	"ctx-actions":        ctxActionsWarning,
+	"ctx-args":           contextArgsAPIWarning,
 	"depset-iteration":   depsetIterationWarning,
 	"depset-union":       depsetUnionWarning,
 	"dict-concatenation": dictionaryConcatenationWarning,
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index 85b2515..f2db9be 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -434,7 +434,7 @@ 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 {
+func contextArgsAPIWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 	types := detectTypes(f)
 
@@ -463,7 +463,7 @@ func argsAPIWarning(f *build.File, fix bool) []*Finding {
 		if !fix {
 			start, end := call.Span()
 			findings = append(findings,
-				makeFinding(f, start, end, "args-api",
+				makeFinding(f, start, end, "ctx-args",
 					`"ctx.actions.args().add()" for multiple arguments is deprecated in favor of "add_all()" or "add_joined()".`, true, nil))
 			return
 		}


Test patch:
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index ef0061f..eba0193 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -332,8 +332,8 @@ def macro():
 		false)
 }
 
-func TestArgsApiWarning(t *testing.T) {
-	checkFindingsAndFix(t, "args-api", `
+func TestContextArgsAPIWarning(t *testing.T) {
+	checkFindingsAndFix(t, "ctx-args", `
 def impl(ctx):
     args = ctx.actions.args()
     args.add(foo, bar)
@@ -366,4 +366,4 @@ def impl(ctx):
 			`: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
+}
diff --git a/warn/warn_test.go b/warn/warn_test.go
index bbb3157..7524186 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -615,3 +615,40 @@ eee + fff | ggg
 		},
 		false)
 }
+
+func TestArgumentsOrder(t *testing.T) {
+	checkFindingsAndFix(t, "args-order", `
+foo(1, a = b, c + d, **e, *f)
+foo(b = c, a)
+foo(*d, a)
+foo(**e, a)
+foo(*d, b = c)
+foo(**e, b = c)
+foo(**e, *d)
+foo(**e, *d, b = c, b2 = c2, a, a2)
+foo(bar = bar(x = y, z), baz * 2)
+`, `
+foo(1, c + d, a = b, *f, **e)
+foo(a, b = c)
+foo(a, *d)
+foo(a, **e)
+foo(b = c, *d)
+foo(b = c, **e)
+foo(*d, **e)
+foo(a, a2, b = c, b2 = c2, *d, **e)
+foo(baz * 2, bar = bar(z, x = y))
+`,
+		[]string{
+			":1: Function call arguments should be in the following order",
+			":2: Function call arguments should be in the following order",
+			":3: Function call arguments should be in the following order",
+			":4: Function call arguments should be in the following order",
+			":5: Function call arguments should be in the following order",
+			":6: Function call arguments should be in the following order",
+			":7: Function call arguments should be in the following order",
+			":8: Function call arguments should be in the following order",
+			":9: Function call arguments should be in the following order",
+			":9: Function call arguments should be in the following order",
+		},
+		false)
+}