Fix for legacy named arguments

WARNINGS.md

@@ -24,6 +24,7 @@ Warning categories supported by buildifier's linter:
   * [git-repository](#git-repository)
   * [http-archive](#http-archive)
   * [integer-division](#integer-division)
+  * [keyword-parameters](#keyword-parameters)
   * [load](#load)
   * [load-on-top](#load-on-top)
   * [module-docstring](#module-docstring)
@@ -400,6 +401,17 @@ d //= e
 
 --------------------------------------------------------------------------------
 
+## <a name="keyword-parameters"></a>Keyword parameter should be positional
+
+  * Category_name: `keyword-parameters`
+  * Automatic fix: yes
+
+Some parameters for builtin functions in Starlark are keyword for legacy reasons;
+their names are not meaningful (e.g. `x`). Making them positional-only will improve
+the readability.
+
+ --------------------------------------------------------------------------------
+
 ## <a name="load"></a>Loaded symbol is unused
 
   * Category name: `load`

warn/warn.go

@@ -122,6 +122,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"git-repository":            nativeGitRepositoryWarning,
 	"http-archive":              nativeHTTPArchiveWarning,
 	"integer-division":          integerDivisionWarning,
+	"keyword-parameters":        keywordParametersWarning,
 	"load":                      unusedLoadWarning,
 	"load-on-top":               loadOnTopWarning,
 	"module-docstring":          moduleDocstringWarning,

warn/warn_bazel_api.go

@@ -832,3 +832,105 @@ func ruleImplReturnWarning(f *build.File) []*LinterFinding {
 
 	return findings
 }
+
+var legacyNamedParameters = map[string]string{
+	"all":     "elements",
+	"any":     "elements",
+	"tuple":   "x",
+	"list":    "x",
+	"len":     "x",
+	"str":     "x",
+	"repr":    "x",
+	"bool":    "x",
+	"int":     "x",
+	"dir":     "x",
+	"type":    "x",
+	"hasattr": "x",
+	"getattr": "x",
+	"select":  "x",
+}
+
+var legacyPositionalParameters = map[string]map[int]string{
+	"glob": {
+		1: "exclude",
+	},
+}
+
+// keywordParametersWarning checks for deprecated keyword parameters of builtins
+func keywordParametersWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
+
+	// Check for legacy keyword parameters
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		call, ok := expr.(*build.CallExpr)
+		if !ok || len(call.List) == 0 {
+			return
+		}
+		ident, ok := call.X.(*build.Ident)
+		if !ok {
+			return
+		}
+		parameter, ok := legacyNamedParameters[ident.Name]
+		if !ok {
+			return
+		}
+		assign, ok := call.List[0].(*build.AssignExpr)
+		if !ok || assign.Op != "=" {
+			return
+		}
+		key, ok := assign.LHS.(*build.Ident)
+		if !ok || key.Name != parameter {
+			return
+		}
+
+		findings = append(findings, makeLinterFinding(
+			call,
+			fmt.Sprintf(`Keyword parameter %q for %q should be positional.`, key.Name, ident.Name),
+			LinterReplacement{&call.List[0], makePositional(call.List[0])}))
+	})
+
+	// Check for legacy positional parameters
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+		call, ok := expr.(*build.CallExpr)
+		if !ok || len(call.List) == 0 {
+			return
+		}
+
+		var name string
+		ident, ok := call.X.(*build.Ident)
+		if ok {
+			name = ident.Name
+		} else {
+			// Also check for `native.`
+			dot, ok := call.X.(*build.DotExpr)
+			if !ok {
+				return
+			}
+			ident, ok := dot.X.(*build.Ident)
+			if !ok || ident.Name != "native" {
+				return
+			}
+			name = dot.Name
+		}
+
+		parameterInfo, ok := legacyPositionalParameters[name]
+		if !ok {
+			return
+		}
+
+		for index, value := range parameterInfo {
+			if index >= len(call.List) {
+				continue
+			}
+			if _, ok := call.List[index].(*build.AssignExpr); ok {
+				continue
+			}
+			findings = append(findings, makeLinterFinding(
+				call,
+				fmt.Sprintf(`Parameter at the position %d for %q should be keyword (%s = ...).`, index+1, name, value),
+				LinterReplacement{&call.List[index], makeKeyword(call.List[index], value)}))
+		}
+	})
+
+	return findings
+}

warn/warn_bazel_api_test.go

@@ -581,7 +581,7 @@ java_test()
 }
 
 func TestNativePyWarning(t *testing.T) {
-  checkFindingsAndFix(t, "native-py", `
+	checkFindingsAndFix(t, "native-py", `
 """My file"""
 
 def macro():
@@ -604,14 +604,14 @@ def macro():
 
 py_test()
 `, tables.PyLoadPath),
-    []string{
-      fmt.Sprintf(`:4: Function "py_library" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
-      fmt.Sprintf(`:5: Function "py_binary" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
-      fmt.Sprintf(`:6: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
-      fmt.Sprintf(`:7: Function "py_runtime" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
-      fmt.Sprintf(`:9: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
-    },
-    scopeBzl|scopeBuild)
+		[]string{
+			fmt.Sprintf(`:4: Function "py_library" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
+			fmt.Sprintf(`:5: Function "py_binary" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
+			fmt.Sprintf(`:6: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
+			fmt.Sprintf(`:7: Function "py_runtime" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
+			fmt.Sprintf(`:9: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath),
+		},
+		scopeBzl|scopeBuild)
 }
 
 func TestNativeProtoWarning(t *testing.T) {
@@ -650,3 +650,67 @@ def macro():
 		},
 		scopeBzl|scopeBuild)
 }
+
+func TestKeywordParameters(t *testing.T) {
+	checkFindingsAndFix(t, "keyword-parameters", `
+foo(key = value)
+all(elements = [True, False])
+any(elements = [True, False])
+tuple(x = [1, 2, 3])
+list(x = [1, 2, 3])
+len(x = [1, 2, 3])
+str(x = foo)
+repr(x = foo)
+bool(x = 3)
+int(x = "3")
+int(x = "13", base = 8)
+dir(x = foo)
+type(x = foo)
+hasattr(x = foo, name = "bar")
+getattr(x = foo, name = "bar", default = "baz")
+select(x = {})
+glob(["*.cc"], ["test*"])
+native.glob(["*.cc"], ["test*"])
+glob(["*.cc"])
+native.glob(["*.cc"])
+`, `
+foo(key = value)
+all([True, False])
+any([True, False])
+tuple([1, 2, 3])
+list([1, 2, 3])
+len([1, 2, 3])
+str(foo)
+repr(foo)
+bool(3)
+int("3")
+int("13", base = 8)
+dir(foo)
+type(foo)
+hasattr(foo, name = "bar")
+getattr(foo, name = "bar", default = "baz")
+select({})
+glob(["*.cc"], exclude = ["test*"])
+native.glob(["*.cc"], exclude = ["test*"])
+glob(["*.cc"])
+native.glob(["*.cc"])
+`, []string{
+		`:2: Keyword parameter "elements" for "all" should be positional.`,
+		`:3: Keyword parameter "elements" for "any" should be positional.`,
+		`:4: Keyword parameter "x" for "tuple" should be positional.`,
+		`:5: Keyword parameter "x" for "list" should be positional.`,
+		`:6: Keyword parameter "x" for "len" should be positional.`,
+		`:7: Keyword parameter "x" for "str" should be positional.`,
+		`:8: Keyword parameter "x" for "repr" should be positional.`,
+		`:9: Keyword parameter "x" for "bool" should be positional.`,
+		`:10: Keyword parameter "x" for "int" should be positional.`,
+		`:11: Keyword parameter "x" for "int" should be positional.`,
+		`:12: Keyword parameter "x" for "dir" should be positional.`,
+		`:13: Keyword parameter "x" for "type" should be positional.`,
+		`:14: Keyword parameter "x" for "hasattr" should be positional.`,
+		`:15: Keyword parameter "x" for "getattr" should be positional.`,
+		`:16: Keyword parameter "x" for "select" should be positional.`,
+		`:17: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`,
+		`:18: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`,
+	}, scopeEverywhere)
+}