Warning against overriding some global attributes in rules. (#1176)

* 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.

* these tests seem brittle

* remove ddbug

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`

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",

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",

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"

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,

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

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