Repository: bazelbuild/buildtools
Pull request #1176 (2023-07-27 20:20:47)

Warning against overriding some global attributes in rules.
Add lint warnings against naming attributes 'licenses', 'applicable_licenses', or 'package_metadata'.  Using any of those can cause unexpected behaviors when generating SBOMs.

aiuto: cc: @teeler  This would have caught the problem from yesterday.
aiuto: Friendly ping
aiuto: 
Friendly ping


aiuto: Another week, another ping.
vladmos: @aiuto Sorry for the delay, the PR was lost in my backlog.

@laurentlb it will be enabled internally by default (however that can be overridden). I'd keep them enabled unless there's a good reason for using these attributes internally.

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 8e7f732..e04a92c 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -2,10 +2,13 @@
 
 Warning categories supported by buildifier's linter:
 
+  * [`attr-applicable_licenses`](#attr-applicable_licenses)
   * [`attr-cfg`](#attr-cfg)
   * [`attr-license`](#attr-license)
+  * [`attr-licenses`](#attr-licenses)
   * [`attr-non-empty`](#attr-non-empty)
   * [`attr-output-default`](#attr-output-default)
+  * [`attr-package-metadata`](#attr-package-metadata)
   * [`attr-single-file`](#attr-single-file)
   * [`build-args-kwargs`](#build-args-kwargs)
   * [`bzl-visibility`](#bzl-visibility)
@@ -86,6 +89,16 @@ if debug:
 
 --------------------------------------------------------------------------------
 
+## <a name="attr-applicable_licenses"></a>Do not use `applicable_licenses` as an attribute name.
+
+  * Category name: `attr-applicable_licenses`
+  * Automatic fix: no
+  * [Suppress the warning](#suppress): `# buildifier: disable=attr-applicable_licenses`
+
+Using `applicable_licenses` as an attribute name may cause unexpected behavior. Its use may be prohibited in future Bazel releases.
+
+--------------------------------------------------------------------------------
+
 ## <a name="attr-cfg"></a>`cfg = "data"` for attr definitions has no effect
 
   * Category name: `attr-cfg`
@@ -111,6 +124,16 @@ The `attr.license()` method is almost never used and being deprecated.
 
 --------------------------------------------------------------------------------
 
+## <a name="attr-licenses"></a>Do not use `licenses` as an attribute name.
+
+  * Category name: `attr-licenses`
+  * Automatic fix: no
+  * [Suppress the warning](#suppress): `# buildifier: disable=attr-licenses`
+
+Using licenses as an attribute name may cause unexpected behavior.
+
+--------------------------------------------------------------------------------
+
 ## <a name="attr-non-empty"></a>`non_empty` attribute for attr definitions is deprecated
 
   * Category name: `attr-non-empty`
@@ -136,6 +159,17 @@ for these attributes instead.
 
 --------------------------------------------------------------------------------
 
+## <a name="attr-package-metadata"></a>Do not use `package_metadata` as an attribute name.
+
+  * Category name: `attr-package-metadata`
+  * Automatic fix: no
+  * Not supported by the latest version of Buildifier
+  * [Suppress the warning](#suppress): `# buildifier: disable=attr-package-metadata`
+
+Using `package_metadata` as an attribute name may cause unexpected behavior. Its use may be prohibited in future Bazel releases.
+
+--------------------------------------------------------------------------------
+
 ## <a name="attr-single-file"></a>`single_file` is deprecated
 
   * Category name: `attr-single-file`
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index af56fc8..e37df9c 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -23,6 +23,27 @@ warnings: {
   autofix: false
 }
 
+warnings: {
+  name: "attr-licenses"
+  header: "Do not use `licenses` as an attribute name."
+  description: "Using licenses as an attribute name may cause unexpected behavior."
+  autofix: false
+}
+
+warnings: {
+  name: "attr-applicable_licenses"
+  header: "Do not use `applicable_licenses` as an attribute name."
+  description: "Using `applicable_licenses` as an attribute name may cause unexpected behavior. Its use may be prohibited in future Bazel releases."
+  autofix: false
+}
+
+warnings: {
+  name: "attr-package-metadata"
+  header: "Do not use `package_metadata` as an attribute name."
+  description: "Using `package_metadata` as an attribute name may cause unexpected behavior. Its use may be prohibited in future Bazel releases."
+  autofix: false
+}
+
 warnings: {
   name: "attr-non-empty"
   header: "`non_empty` attribute for attr definitions is deprecated"
diff --git a/warn/warn.go b/warn/warn.go
index fda95e2..d60bfe6 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -116,8 +116,10 @@ var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFi
 
 // FileWarningMap lists the warnings that run on the whole file.
 var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
+	"attr-applicable_licenses":  attrApplicableLicensesWarning,
 	"attr-cfg":                  attrConfigurationWarning,
 	"attr-license":              attrLicenseWarning,
+	"attr-licenses":             attrLicensesWarning,
 	"attr-non-empty":            attrNonEmptyWarning,
 	"attr-output-default":       attrOutputDefaultWarning,
 	"attr-single-file":          attrSingleFileWarning,
diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go
index 0c23803..ecce12f 100644
--- a/warn/warn_bazel_api.go
+++ b/warn/warn_bazel_api.go
@@ -1084,3 +1084,43 @@ func providerParamsWarning(f *build.File) []*LinterFinding {
 	})
 	return findings
 }
+
+
+func attrNameWarning(f *build.File, names []string) []*LinterFinding {
+	if f.Type != build.TypeBzl {
+		return nil
+	}
+
+	var findings []*LinterFinding
+	build.WalkPointers(f, func(expr *build.Expr, stack []build.Expr) {
+		// Find nodes that match "attrs = {..., "license", ...}"
+		dict, ok := (*expr).(*build.DictExpr)
+		if !ok {
+			return
+		}
+		for _, item := range dict.List {
+			// include only string literal keys into consideration
+			value, ok := item.Key.(*build.StringExpr)
+			if !ok {
+				continue
+			}
+			for _, name := range names {
+				if value.Value == name {
+					findings = append(findings, makeLinterFinding(dict,
+						fmt.Sprintf(`Do not use '%s' as an attribute name.`+
+							` It may cause unexpected behavior.`, value.Value)))
+
+				}
+			}
+		}
+	})
+	return findings
+}
+
+func attrLicensesWarning(f *build.File) []*LinterFinding {
+	return attrNameWarning(f, []string{"licenses"})
+}
+
+func attrApplicableLicensesWarning(f *build.File) []*LinterFinding {
+	return attrNameWarning(f, []string{"applicable_licenses", "package_metadata"})
+}


Test patch:
diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go
index 226a256..d1ed03e 100644
--- a/buildifier/config/config_test.go
+++ b/buildifier/config/config_test.go
@@ -44,8 +44,10 @@ func ExampleExample() {
 	//   "mode": "fix",
 	//   "lint": "fix",
 	//   "warningsList": [
+	//     "attr-applicable_licenses",
 	//     "attr-cfg",
 	//     "attr-license",
+	//     "attr-licenses",
 	//     "attr-non-empty",
 	//     "attr-output-default",
 	//     "attr-single-file",
@@ -226,8 +228,10 @@ func TestValidate(t *testing.T) {
 		"type auto":             {options: "--type=auto"},
 		"type error":            {options: "--type=foo", wantErr: fmt.Errorf("unrecognized input type foo; valid types are build, bzl, workspace, default, module, auto")},
 		"warnings all": {options: "--warnings=all", wantWarnings: []string{
+			"attr-applicable_licenses",
 			"attr-cfg",
 			"attr-license",
+			"attr-licenses",
 			"attr-non-empty",
 			"attr-output-default",
 			"attr-single-file",
@@ -289,8 +293,10 @@ func TestValidate(t *testing.T) {
 			"unused-variable",
 		}},
 		"warnings default": {options: "--warnings=default", wantWarnings: []string{
+			"attr-applicable_licenses",
 			"attr-cfg",
 			"attr-license",
+			"attr-licenses",
 			"attr-non-empty",
 			"attr-output-default",
 			"attr-single-file",
@@ -352,8 +358,10 @@ func TestValidate(t *testing.T) {
 			"unused-variable",
 		}},
 		"warnings plus/minus": {options: "--warnings=+out-of-order-load,-print,-deprecated-function", wantWarnings: []string{
+			"attr-applicable_licenses",
 			"attr-cfg",
 			"attr-license",
+			"attr-licenses",
 			"attr-non-empty",
 			"attr-output-default",
 			"attr-single-file",
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 313c6cc..c6fafbe 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -248,8 +248,10 @@ cat > golden/.buildifier.example.json <<EOF
   "mode": "fix",
   "lint": "fix",
   "warningsList": [
+    "attr-applicable_licenses",
     "attr-cfg",
     "attr-license",
+    "attr-licenses",
     "attr-non-empty",
     "attr-output-default",
     "attr-single-file",
diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go
index 2ebee86..b23f970 100644
--- a/warn/warn_bazel_api_test.go
+++ b/warn/warn_bazel_api_test.go
@@ -799,3 +799,36 @@ func TestProvider(t *testing.T) {
 	checkFindings(t, "provider-params", `p = provider()`,
 		[]string{`1: Calls to 'provider' should provide a list of fields and a documentation:`}, scopeBzl)
 }
+
+func TestAttributeNameWarning(t *testing.T) {
+	checkFindings(t, "attr-licenses", `
+def _impl(ctx):
+    pass
+
+foo = rule(
+    implementation = _impl,
+    attrs = {
+        "license": attr.string(),
+        "licenses": attr.string(),
+    },
+)
+`, []string{
+		":6: Do not use 'licenses' as an attribute name. It may cause unexpected behavior.",
+	}, scopeBzl)
+
+	checkFindings(t, "attr-applicable_licenses", `
+def _impl(ctx):
+    pass
+
+foo = rule(
+    implementation = _impl,
+    attrs = {
+        "applicable_licenses": attr.string(),
+        "package_metadata": attr.string(),
+    },
+)
+`, []string{
+		":6: Do not use 'applicable_licenses' as an attribute name. It may cause unexpected behavior.",
+		":6: Do not use 'package_metadata' as an attribute name. It may cause unexpected behavior.",
+	}, scopeBzl)
+}