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-positional-params](#keyword-positional-params)
   * [load](#load)
   * [load-on-top](#load-on-top)
   * [module-docstring](#module-docstring)
@@ -400,6 +401,17 @@ d //= e
 
 --------------------------------------------------------------------------------
 
+## <a name="keyword-positional-params"></a>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.
+
+ --------------------------------------------------------------------------------
+
 ## <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-positional-params": keywordPositionalParametersWarning,
 	"load":                      unusedLoadWarning,
 	"load-on-top":               loadOnTopWarning,
 	"module-docstring":          moduleDocstringWarning,

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.<name>`
+	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
+}

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)
+}