Repository: bazelbuild/buildtools
Pull request #1104 (2022-11-25 16:49:14)

Add a new warning for named args in dict methods
For reference: https://github.com/bazelbuild/starlark/blob/master/spec.md#dict%C2%B7get



Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 53eeb07..56ce695 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -18,6 +18,7 @@ Warning categories supported by buildifier's linter:
   * [`depset-iteration`](#depset-iteration)
   * [`depset-union`](#depset-union)
   * [`dict-concatenation`](#dict-concatenation)
+  * [`dict-method-named-arg`](#dict-method-named-arg)
   * [`duplicated-name`](#duplicated-name)
   * [`filetype`](#filetype)
   * [`function-docstring`](#function-docstring)
@@ -368,6 +369,22 @@ d.update(d2)
 d.update(d3)
 ```
 
+--------------------------------------------------------------------------------
+
+## <a name="dict-method-named-arg"></a>Dict methods do not have a named argument `default`
+
+  * Category name: `dict-method-named-arg`
+  * Automatic fix: no
+  * [Suppress the warning](#suppress): `# buildifier: disable=dict-method-named-arg`
+
+Dict methods `get`, `pop` and `setdefault` do not accept a named argument
+called `default`. Due to a bug, Bazel currently accepts that named argument.
+It is better to use a positional argument instead:```diff
+- mydict.get(5, default = 0)
++ mydict.get(5, 0)
+```
+
+
 --------------------------------------------------------------------------------
 
 ## <a name="duplicated-name"></a>A rule with name `foo` was already found on line
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index 06d8c68..21ad198 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -1,6 +1,6 @@
 # proto-file: warn/docs/docs.proto
 # proto-message: Warnings
-# After modifying this file, run `bazel build //warn/docs:warn_docs && cp bazel-bin/warn/docs/WARNINGS.md .`
+# After modifying this file, run `bazel build //warn/docs:warnings_docs && cp bazel-bin/warn/docs/WARNINGS.md .`
 
 warnings: {
   name: "attr-cfg"
@@ -233,6 +233,19 @@ warnings: {
   bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/6461"
 }
 
+warnings: {
+  name: "dict-method-named-arg"
+  header: "Dict methods do not have a named argument `default`"
+  description:
+    "Dict methods `get`, `pop` and `setdefault` do not accept a named argument\n"
+    "called `default`. Due to a bug, Bazel currently accepts that named argument.\n"
+    "It is better to use a positional argument instead:"
+    "```diff\n"
+    "- mydict.get(5, default = 0)\n"
+    "+ mydict.get(5, 0)\n"
+    "```\n"
+}
+
 warnings: {
   name: "duplicated-name"
   header: "A rule with name `foo` was already found on line"
diff --git a/warn/warn.go b/warn/warn.go
index b2703c0..fda95e2 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -130,6 +130,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"depset-items":              depsetItemsWarning,
 	"depset-iteration":          depsetIterationWarning,
 	"depset-union":              depsetUnionWarning,
+	"dict-method-named-arg":     dictMethodNamedArgWarning,
 	"dict-concatenation":        dictionaryConcatenationWarning,
 	"duplicated-name":           duplicatedNameWarning,
 	"filetype":                  fileTypeWarning,
diff --git a/warn/warn_operation.go b/warn/warn_operation.go
index 61446cc..b81529d 100644
--- a/warn/warn_operation.go
+++ b/warn/warn_operation.go
@@ -52,6 +52,48 @@ func dictionaryConcatenationWarning(f *build.File) []*LinterFinding {
 	return findings
 }
 
+// Detect the patterns:
+//
+//	dict.pop(..., default = ...)
+//	dict.pop(..., default = ...)
+//	dict.setdefault(..., default = ...)
+//
+// And recommend removing the `default =` part. These functions take positional argument instead.
+func dictMethodNamedArgWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
+
+	types := DetectTypes(f)
+	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
+
+		var callexpr *build.CallExpr
+		var ok bool
+		if callexpr, ok = expr.(*build.CallExpr); !ok {
+			return
+		}
+		var dotexpr *build.DotExpr
+		if dotexpr, ok = callexpr.X.(*build.DotExpr); !ok || types[dotexpr.X] != Dict {
+			return
+		}
+
+		if dotexpr.Name != "pop" && dotexpr.Name != "get" && dotexpr.Name != "setdefault" {
+			return
+		}
+
+		for _, expr := range callexpr.List {
+			assignExpr, ok := expr.(*build.AssignExpr)
+			if !ok {
+				continue
+			}
+			if left, ok := assignExpr.LHS.(*build.Ident); !ok || left.Name != "default" {
+				continue
+			}
+			findings = append(findings,
+				makeLinterFinding(expr, "Named argument \"default\" not allowed, use a positional (unnamed) argument"))
+		}
+	})
+	return findings
+}
+
 func stringIterationWarning(f *build.File) []*LinterFinding {
 	var findings []*LinterFinding
 


Test patch:
diff --git a/warn/warn_operation_test.go b/warn/warn_operation_test.go
index 57ea58f..dc2e9bf 100644
--- a/warn/warn_operation_test.go
+++ b/warn/warn_operation_test.go
@@ -150,3 +150,21 @@ x.append(foo(
 		},
 		scopeEverywhere)
 }
+
+func TestDictMethodNamedArg(t *testing.T) {
+	checkFindings(t, "dict-method-named-arg", `
+d = dict()
+d.get("a", "b")
+[].get("a", default = "b")
+
+d.get("a", default = "b") # warning
+d.pop("a", default = "b") # warning
+{}.setdefault("a", default = "b") # warning
+`,
+		[]string{
+			`:5: Named argument "default" not allowed`,
+			`:6: Named argument "default" not allowed`,
+			`:7: Named argument "default" not allowed`,
+		},
+		scopeEverywhere)
+}