diff --git a/WARNINGS.md b/WARNINGS.md
index b01963ec2..a9f4326c1 100644
--- a/WARNINGS.md
+++ b/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-positional-params](#keyword-positional-params)
* [load](#load)
* [load-on-top](#load-on-top)
* [module-docstring](#module-docstring)
@@ -400,6 +401,17 @@ d //= e
--------------------------------------------------------------------------------
+## Keyword parameter should be positional
+
+ * Category_name: `keyword-positional-params`
+ * 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.
+
+ --------------------------------------------------------------------------------
+
## Loaded symbol is unused
* Category name: `load`
diff --git a/warn/warn.go b/warn/warn.go
index bcb0f3eb6..3d0418dc0 100644
--- a/warn/warn.go
+++ b/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-positional-params": keywordPositionalParametersWarning,
"load": unusedLoadWarning,
"load-on-top": loadOnTopWarning,
"module-docstring": moduleDocstringWarning,
diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go
index 7694811f1..29be0d907 100644
--- a/warn/warn_bazel_api.go
+++ b/warn/warn_bazel_api.go
@@ -832,3 +832,155 @@ func ruleImplReturnWarning(f *build.File) []*LinterFinding {
return findings
}
+
+type signature struct {
+ Positional []string // These parameters are typePositional-only
+ Keyword []string // These parameters are typeKeyword-only
+}
+
+var signatures = map[string]signature{
+ "all": {[]string{"elements"}, []string{}},
+ "any": {[]string{"elements"}, []string{}},
+ "tuple": {[]string{"x"}, []string{}},
+ "list": {[]string{"x"}, []string{}},
+ "len": {[]string{"x"}, []string{}},
+ "str": {[]string{"x"}, []string{}},
+ "repr": {[]string{"x"}, []string{}},
+ "bool": {[]string{"x"}, []string{}},
+ "int": {[]string{"x"}, []string{}},
+ "dir": {[]string{"x"}, []string{}},
+ "type": {[]string{"x"}, []string{}},
+ "hasattr": {[]string{"x", "name"}, []string{}},
+ "getattr": {[]string{"x", "name", "default"}, []string{}},
+ "select": {[]string{"x"}, []string{}},
+ "glob": {[]string{"include"}, []string{"exclude", "exclude_directories"}},
+}
+
+// functionName returns the name of the given function if it's a direct function call (e.g.
+// `foo(...)` or `native.foo(...)`, but not `foo.bar(...)` or `x[3](...)`
+func functionName(call *build.CallExpr) (string, bool) {
+ if ident, ok := call.X.(*build.Ident); ok {
+ return ident.Name, true
+ }
+ // Also check for `native.`
+ dot, ok := call.X.(*build.DotExpr)
+ if !ok {
+ return "", false
+ }
+ if ident, ok := dot.X.(*build.Ident); !ok || ident.Name != "native" {
+ return "", false
+ }
+ return dot.Name, true
+}
+
+const (
+ typePositional int = iota
+ typeKeyword
+ typeArgs
+ typeKwargs
+)
+
+// paramType returns the type of the param. If it's a typeKeyword param, also returns its name
+func paramType(param build.Expr) (int, string) {
+ switch param := param.(type) {
+ case *build.AssignExpr:
+ if param.Op == "=" {
+ ident, ok := param.LHS.(*build.Ident)
+ if ok {
+ return typeKeyword, ident.Name
+ }
+ return typeKeyword, ""
+ }
+ case *build.UnaryExpr:
+ switch param.Op {
+ case "*":
+ return typeArgs, ""
+ case "**":
+ return typeKwargs, ""
+ }
+ }
+ return typePositional, ""
+}
+
+// keywordPositionalParametersWarning checks for deprecated typeKeyword parameters of builtins
+func keywordPositionalParametersWarning(f *build.File) []*LinterFinding {
+ var findings []*LinterFinding
+
+ // Check for legacy typeKeyword parameters
+ build.WalkPointers(f, func(expr *build.Expr, stack []build.Expr) {
+ call, ok := (*expr).(*build.CallExpr)
+ if !ok || len(call.List) == 0 {
+ return
+ }
+ function, ok := functionName(call)
+ if !ok {
+ return
+ }
+
+ // Findings and replacements for the current call expression
+ var callFindings []*LinterFinding
+ var callReplacements []LinterReplacement
+
+ signature, ok := signatures[function]
+ if !ok {
+ return
+ }
+
+ var paramTypes []int // types of the parameters (typeKeyword or not) after the replacements has been applied.
+ for i, parameter := range call.List {
+ pType, name := paramType(parameter)
+ paramTypes = append(paramTypes, pType)
+
+ if pType == typeKeyword && i < len(signature.Positional) && signature.Positional[i] == name {
+ // The parameter should be typePositional
+ callFindings = append(callFindings, makeLinterFinding(
+ parameter,
+ fmt.Sprintf(`Keyword parameter %q for %q should be positional.`, signature.Positional[i], function),
+ ))
+ callReplacements = append(callReplacements, LinterReplacement{&call.List[i], makePositional(parameter)})
+ paramTypes[i] = typePositional
+ }
+
+ if pType == typePositional && i >= len(signature.Positional) && i < len(signature.Positional)+len(signature.Keyword) {
+ // The parameter should be typeKeyword
+ keyword := signature.Keyword[i-len(signature.Positional)]
+ callFindings = append(callFindings, makeLinterFinding(
+ parameter,
+ fmt.Sprintf(`Parameter at the position %d for %q should be keyword (%s = ...).`, i+1, function, keyword),
+ ))
+ callReplacements = append(callReplacements, LinterReplacement{&call.List[i], makeKeyword(parameter, keyword)})
+ paramTypes[i] = typeKeyword
+ }
+ }
+
+ if len(callFindings) == 0 {
+ return
+ }
+
+ // Only apply the replacements if the signature is correct after they have been applied
+ // (i.e. the order of the parameters is typePositional, typeKeyword, typeArgs, typeKwargs)
+ // Otherwise the signature will be not correct, probably it was incorrect initially.
+ // All the replacements should be applied to the first finding for the current node.
+
+ if sort.IntsAreSorted(paramTypes) {
+ // It's possible that the parameter list had `ForceCompact` set to true because it only contained
+ // positional arguments, and now it has keyword arguments as well. Reset the flag to let the
+ // printer decide how the function call should be formatted.
+ for _, t := range paramTypes {
+ if t == typeKeyword {
+ // There's at least one keyword argument
+ newCall := *call
+ newCall.ForceCompact = false
+ callFindings[0].Replacement = append(callFindings[0].Replacement, LinterReplacement{expr, &newCall})
+ break
+ }
+ }
+ // Attach all the parameter replacements to the first finding
+ callFindings[0].Replacement = append(callFindings[0].Replacement, callReplacements...)
+ }
+
+ findings = append(findings, callFindings...)
+ })
+
+ return findings
+}
diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go
index bc770812a..181a83f7b 100644
--- a/warn/warn_bazel_api_test.go
+++ b/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,141 @@ def macro():
},
scopeBzl|scopeBuild)
}
+
+func TestKeywordParameters(t *testing.T) {
+ checkFindingsAndFix(t, "keyword-positional-params", `
+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)
+select(x = {})
+`, `
+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)
+select({})
+`, []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 "select" should be positional.`,
+ }, scopeEverywhere)
+
+ checkFindingsAndFix(t, "keyword-positional-params", `
+hasattr(
+ x = foo,
+ name = "bar",
+)
+getattr(
+ x = foo,
+ name = "bar",
+)
+getattr(
+ x = foo,
+ name = "bar",
+ default = "baz",
+)
+`, `
+hasattr(
+ foo,
+ "bar",
+)
+getattr(
+ foo,
+ "bar",
+)
+getattr(
+ foo,
+ "bar",
+ "baz",
+)
+`, []string{
+ `:2: Keyword parameter "x" for "hasattr" should be positional.`,
+ `:3: Keyword parameter "name" for "hasattr" should be positional.`,
+ `:6: Keyword parameter "x" for "getattr" should be positional.`,
+ `:7: Keyword parameter "name" for "getattr" should be positional.`,
+ `:10: Keyword parameter "x" for "getattr" should be positional.`,
+ `:11: Keyword parameter "name" for "getattr" should be positional.`,
+ `:12: Keyword parameter "default" for "getattr" should be positional.`,
+ }, scopeEverywhere)
+
+ checkFindingsAndFix(t, "keyword-positional-params", `
+glob(["*.cc"], ["test*"])
+glob(["*.cc"])
+glob(include = [], exclude = [])
+glob([], exclude = [])
+glob([], [], 1)
+glob([], [], 1, 2)
+glob(*args, [])
+`, `
+glob(["*.cc"], exclude = ["test*"])
+glob(["*.cc"])
+glob([], exclude = [])
+glob([], exclude = [])
+glob([], exclude = [], exclude_directories = 1)
+glob([], [], 1, 2)
+glob(*args, [])
+`, []string{
+ `:1: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`,
+ `:3: Keyword parameter "include" for "glob" should be positional.`,
+ `:5: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
+ `:5: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
+ `:6: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
+ `:6: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
+ `:7: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
+ }, scopeEverywhere)
+
+ checkFindingsAndFix(t, "keyword-positional-params", `
+native.glob(["*.cc"], ["test*"])
+native.glob(["*.cc"])
+native.glob(include = [], exclude = [])
+native.glob([], exclude = [])
+native.glob([], [], 1)
+native.glob([], [], 1, 2)
+native.glob(*args, [])
+`, `
+native.glob(["*.cc"], exclude = ["test*"])
+native.glob(["*.cc"])
+native.glob([], exclude = [])
+native.glob([], exclude = [])
+native.glob([], exclude = [], exclude_directories = 1)
+native.glob([], [], 1, 2)
+native.glob(*args, [])
+`, []string{
+ `:1: Parameter at the position 2 for "glob" should be keyword (exclude = ...).`,
+ `:3: Keyword parameter "include" for "glob" should be positional.`,
+ `:5: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
+ `:5: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
+ `:6: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
+ `:6: Parameter at the position 3 for "glob" should be keyword (exclude_directories = ...)`,
+ `:7: Parameter at the position 2 for "glob" should be keyword (exclude = ...)`,
+ }, scopeEverywhere)
+}