Add a buildifier warning to enforce symbol load location (#1318)
* Add a buildifier warning to enforce rule load location
* Minor refactoring
* Rename to AllowedSymbolLoadLocations
diff --git a/WARNINGS.md b/WARNINGS.md
index 79b7531..2514b88 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -2,6 +2,7 @@
Warning categories supported by buildifier's linter:
+ * [`allowed-symbol-load-locations`](#allowed-symbol-load-locations)
* [`attr-applicable_licenses`](#attr-applicable_licenses)
* [`attr-cfg`](#attr-cfg)
* [`attr-license`](#attr-license)
@@ -128,6 +129,27 @@
--------------------------------------------------------------------------------
+## <a name="allowed-symbol-load-locations"></a>Symbol must be loaded from a specific location
+
+ * Category name: `allowed-symbol-load-locations`
+ * Automatic fix: no
+ * [Suppress the warning](#suppress): `# buildifier: disable=allowed-symbol-load-locations`
+
+Warns when a symbol is loaded from a location other than the expected ones.
+Expected locations are specified in the tables file:
+
+```json
+{
+ "AllowedSymbolLoadLocations": {
+ "genrule": [
+ "//tools/bazel:genrule.bzl"
+ ]
+ }
+}
+```
+
+--------------------------------------------------------------------------------
+
## <a name="attr-applicable_licenses"></a>Do not use `applicable_licenses` as an attribute name.
* Category name: `attr-applicable_licenses`
diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go
index 167a21a..c929176 100644
--- a/buildifier/config/config_test.go
+++ b/buildifier/config/config_test.go
@@ -43,6 +43,7 @@
// "mode": "fix",
// "lint": "fix",
// "warningsList": [
+ // "allowed-symbol-load-locations",
// "attr-applicable_licenses",
// "attr-cfg",
// "attr-license",
@@ -263,6 +264,7 @@
"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{
+ "allowed-symbol-load-locations",
"attr-applicable_licenses",
"attr-cfg",
"attr-license",
@@ -364,6 +366,7 @@
"unused-variable",
}},
"warnings default": {options: "--warnings=default", wantWarnings: []string{
+ "allowed-symbol-load-locations",
"attr-applicable_licenses",
"attr-cfg",
"attr-license",
@@ -465,6 +468,7 @@
"unused-variable",
}},
"warnings plus/minus": {options: "--warnings=+unsorted-dict-items,-print,-deprecated-function", wantWarnings: []string{
+ "allowed-symbol-load-locations",
"attr-applicable_licenses",
"attr-cfg",
"attr-license",
@@ -556,7 +560,6 @@
"repository-name",
"return-value",
"rule-impl-return",
-
"skylark-comment",
"skylark-docstring",
"string-iteration",
@@ -567,6 +570,7 @@
"unused-variable",
}},
"warnings no duplicates": {options: "--warnings=+unused-variable", wantWarnings: []string{
+ "allowed-symbol-load-locations",
"attr-applicable_licenses",
"attr-cfg",
"attr-license",
@@ -658,7 +662,6 @@
"repository-name",
"return-value",
"rule-impl-return",
-
"skylark-comment",
"skylark-docstring",
"string-iteration",
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 0b28a4d..b9a6389 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -258,6 +258,7 @@
"mode": "fix",
"lint": "fix",
"warningsList": [
+ "allowed-symbol-load-locations",
"attr-applicable_licenses",
"attr-cfg",
"attr-license",
@@ -691,3 +692,36 @@
$buildifier --lint=warn --warnings=deprecated-function BUILD 2> report || ret=$?
diff -u report_golden report || die "$1: wrong console output for multifile warnings (WORKSPACE exists)"
+
+cd ..
+
+# Test allowed symbol load locations
+
+mkdir test_dir/allowed_locations
+cd test_dir/allowed_locations
+
+cat > BUILD <<EOF
+load(":f.bzl", "s1", "s2")
+load(":a.bzl", "s3")
+load(":a.bzl", "s4")
+EOF
+
+cat > buildifier.tables <<EOF
+{
+ "AllowedSymbolLoadLocations": {
+ "s1": [":z.bzl"],
+ "s3": [":y.bzl", ":x.bzl"],
+ "s4": [":a.bzl"]
+ }
+}
+EOF
+
+cat > report_golden <<EOF
+BUILD:1: allowed-symbol-load-locations: Symbol "s1" must be loaded from :z.bzl. (https://github.com/bazelbuild/buildtools/blob/main/WARNINGS.md#allowed-symbol-load-locations)
+BUILD:2: allowed-symbol-load-locations: Symbol "s3" must be loaded from one of the allowed locations: :x.bzl, :y.bzl. (https://github.com/bazelbuild/buildtools/blob/main/WARNINGS.md#allowed-symbol-load-locations)
+EOF
+
+$buildifier --lint=warn --warnings=allowed-symbol-load-locations -tables=buildifier.tables BUILD 2> report || true
+diff -u report_golden report || die "$1: wrong console output for allowed symbol load locations"
+
+cd ../..
diff --git a/tables/jsonparser.go b/tables/jsonparser.go
index d2ae1dc..88e40eb 100644
--- a/tables/jsonparser.go
+++ b/tables/jsonparser.go
@@ -31,6 +31,7 @@
NamePriority map[string]int
StripLabelLeadingSlashes bool
ShortenAbsoluteLabelsToRelative bool
+ AllowedSymbolLoadLocations map[string][]string
}
// ParseJSONDefinitions reads and parses JSON table definitions from file.
@@ -55,9 +56,9 @@
}
if merge {
- MergeTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative)
+ MergeTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative, definitions.AllowedSymbolLoadLocations)
} else {
- OverrideTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative)
+ OverrideTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative, definitions.AllowedSymbolLoadLocations)
}
return nil
}
diff --git a/tables/jsonparser_test.go b/tables/jsonparser_test.go
index 800934d..e876570 100644
--- a/tables/jsonparser_test.go
+++ b/tables/jsonparser_test.go
@@ -30,13 +30,14 @@
}
expected := Definitions{
- IsLabelArg: map[string]bool{"srcs": true},
- LabelDenylist: map[string]bool{},
- IsSortableListArg: map[string]bool{"srcs": true, "visibility": true},
- SortableDenylist: map[string]bool{"genrule.srcs": true},
- SortableAllowlist: map[string]bool{},
- NamePriority: map[string]int{"name": -1},
- StripLabelLeadingSlashes: true,
+ IsLabelArg: map[string]bool{"srcs": true},
+ LabelDenylist: map[string]bool{},
+ IsSortableListArg: map[string]bool{"srcs": true, "visibility": true},
+ SortableDenylist: map[string]bool{"genrule.srcs": true},
+ SortableAllowlist: map[string]bool{},
+ NamePriority: map[string]int{"name": -1},
+ StripLabelLeadingSlashes: true,
+ AllowedSymbolLoadLocations: map[string][]string{"genrule": {"//tools/bazel:genrule.bzl"}},
}
if !reflect.DeepEqual(expected, definitions) {
t.Errorf("ParseJSONDefinitions(simple_tables.json) = %v; want %v", definitions, expected)
diff --git a/tables/tables.go b/tables/tables.go
index 7ff8b02..49584f4 100644
--- a/tables/tables.go
+++ b/tables/tables.go
@@ -265,8 +265,11 @@
"single_version_override": true,
}
+// AllowedSymbolLoadLocations contains locations for loading rules that are allowed to be used.
+var AllowedSymbolLoadLocations = map[string]map[string]bool{}
+
// OverrideTables allows a user of the build package to override the special-case rules. The user-provided tables replace the built-in tables.
-func OverrideTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool) {
+func OverrideTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool, symbolLoadLocation map[string][]string) {
IsLabelArg = labelArg
LabelDenylist = denylist
IsListArg = listArg
@@ -276,10 +279,19 @@
NamePriority = namePriority
StripLabelLeadingSlashes = stripLabelLeadingSlashes
ShortenAbsoluteLabelsToRelative = shortenAbsoluteLabelsToRelative
+
+ AllowedSymbolLoadLocations = map[string]map[string]bool{}
+ for k, v := range symbolLoadLocation {
+ locations := map[string]bool{}
+ for _, l := range v {
+ locations[l] = true
+ }
+ AllowedSymbolLoadLocations[k] = locations
+ }
}
// MergeTables allows a user of the build package to override the special-case rules. The user-provided tables are merged into the built-in tables.
-func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool) {
+func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool, symbolLoadLocation map[string][]string) {
for k, v := range labelArg {
IsLabelArg[k] = v
}
@@ -303,4 +315,12 @@
}
StripLabelLeadingSlashes = stripLabelLeadingSlashes || StripLabelLeadingSlashes
ShortenAbsoluteLabelsToRelative = shortenAbsoluteLabelsToRelative || ShortenAbsoluteLabelsToRelative
+
+ for k, v := range symbolLoadLocation {
+ locations := map[string]bool{}
+ for _, l := range v {
+ locations[l] = true
+ }
+ AllowedSymbolLoadLocations[k] = locations
+ }
}
diff --git a/tables/testdata/simple_tables.json b/tables/testdata/simple_tables.json
index 9243e2d..f28ff4a 100644
--- a/tables/testdata/simple_tables.json
+++ b/tables/testdata/simple_tables.json
@@ -19,5 +19,10 @@
"NamePriority": {
"name": -1
},
- "StripLabelLeadingSlashes": true
+ "StripLabelLeadingSlashes": true,
+ "AllowedSymbolLoadLocations": {
+ "genrule": [
+ "//tools/bazel:genrule.bzl"
+ ]
+ }
}
diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel
index 225b5fc..377a389 100644
--- a/warn/BUILD.bazel
+++ b/warn/BUILD.bazel
@@ -13,6 +13,7 @@
"warn_cosmetic.go",
"warn_deprecated.go",
"warn_docstring.go",
+ "warn_load.go",
"warn_macro.go",
"warn_naming.go",
"warn_operation.go",
@@ -42,6 +43,7 @@
"warn_cosmetic_test.go",
"warn_deprecated_test.go",
"warn_docstring_test.go",
+ "warn_load_test.go",
"warn_macro_test.go",
"warn_naming_test.go",
"warn_operation_test.go",
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index 9d3eb8e..8be362e 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -3,6 +3,23 @@
# After modifying this file, run `bazel build //warn/docs:warnings_docs && cp bazel-bin/warn/docs/WARNINGS.md .`
warnings: {
+ name: "allowed-symbol-load-locations"
+ header: "Symbol must be loaded from a specific location"
+ description:
+ "Warns when a symbol is loaded from a location other than the expected ones.\n"
+ "Expected locations are specified in the tables file:\n\n"
+ "```json\n"
+ "{\n"
+ " \"AllowedSymbolLoadLocations\": {\n"
+ " \"genrule\": [\n"
+ " \"//tools/bazel:genrule.bzl\"\n"
+ " ]\n"
+ " }\n"
+ "}\n"
+ "```"
+}
+
+warnings: {
name: "attr-cfg"
header: "`cfg = \"data\"` for attr definitions has no effect"
description:
diff --git a/warn/warn.go b/warn/warn.go
index 01bcbb5..2d5d71b 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -116,57 +116,58 @@
// 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,
- "build-args-kwargs": argsKwargsInBuildFilesWarning,
- "canonical-repository": canonicalRepositoryWarning,
- "confusing-name": confusingNameWarning,
- "constant-glob": constantGlobWarning,
- "ctx-actions": ctxActionsWarning,
- "ctx-args": contextArgsAPIWarning,
- "depset-items": depsetItemsWarning,
- "depset-iteration": depsetIterationWarning,
- "depset-union": depsetUnionWarning,
- "dict-method-named-arg": dictMethodNamedArgWarning,
- "dict-concatenation": dictionaryConcatenationWarning,
- "duplicated-name": duplicatedNameWarning,
- "external-path": externalPathWarning,
- "filetype": fileTypeWarning,
- "function-docstring": functionDocstringWarning,
- "function-docstring-header": functionDocstringHeaderWarning,
- "function-docstring-args": functionDocstringArgsWarning,
- "function-docstring-return": functionDocstringReturnWarning,
- "integer-division": integerDivisionWarning,
- "keyword-positional-params": keywordPositionalParametersWarning,
- "list-append": listAppendWarning,
- "load": unusedLoadWarning,
- "module-docstring": moduleDocstringWarning,
- "name-conventions": nameConventionsWarning,
- "native-build": nativeInBuildFilesWarning,
- "native-package": nativePackageWarning,
- "no-effect": noEffectWarning,
- "output-group": outputGroupWarning,
- "overly-nested-depset": overlyNestedDepsetWarning,
- "package-name": packageNameWarning,
- "package-on-top": packageOnTopWarning,
- "print": printWarning,
- "provider-params": providerParamsWarning,
- "redefined-variable": redefinedVariableWarning,
- "repository-name": repositoryNameWarning,
- "rule-impl-return": ruleImplReturnWarning,
- "return-value": missingReturnValueWarning,
- "skylark-comment": skylarkCommentWarning,
- "skylark-docstring": skylarkDocstringWarning,
- "string-iteration": stringIterationWarning,
- "uninitialized": uninitializedVariableWarning,
- "unreachable": unreachableStatementWarning,
- "unsorted-dict-items": unsortedDictItemsWarning,
- "unused-variable": unusedVariableWarning,
+ "allowed-symbol-load-locations": symbolLoadLocationWarning,
+ "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,
+ "build-args-kwargs": argsKwargsInBuildFilesWarning,
+ "canonical-repository": canonicalRepositoryWarning,
+ "confusing-name": confusingNameWarning,
+ "constant-glob": constantGlobWarning,
+ "ctx-actions": ctxActionsWarning,
+ "ctx-args": contextArgsAPIWarning,
+ "depset-items": depsetItemsWarning,
+ "depset-iteration": depsetIterationWarning,
+ "depset-union": depsetUnionWarning,
+ "dict-method-named-arg": dictMethodNamedArgWarning,
+ "dict-concatenation": dictionaryConcatenationWarning,
+ "duplicated-name": duplicatedNameWarning,
+ "external-path": externalPathWarning,
+ "filetype": fileTypeWarning,
+ "function-docstring": functionDocstringWarning,
+ "function-docstring-header": functionDocstringHeaderWarning,
+ "function-docstring-args": functionDocstringArgsWarning,
+ "function-docstring-return": functionDocstringReturnWarning,
+ "integer-division": integerDivisionWarning,
+ "keyword-positional-params": keywordPositionalParametersWarning,
+ "list-append": listAppendWarning,
+ "load": unusedLoadWarning,
+ "module-docstring": moduleDocstringWarning,
+ "name-conventions": nameConventionsWarning,
+ "native-build": nativeInBuildFilesWarning,
+ "native-package": nativePackageWarning,
+ "no-effect": noEffectWarning,
+ "output-group": outputGroupWarning,
+ "overly-nested-depset": overlyNestedDepsetWarning,
+ "package-name": packageNameWarning,
+ "package-on-top": packageOnTopWarning,
+ "print": printWarning,
+ "provider-params": providerParamsWarning,
+ "redefined-variable": redefinedVariableWarning,
+ "repository-name": repositoryNameWarning,
+ "rule-impl-return": ruleImplReturnWarning,
+ "return-value": missingReturnValueWarning,
+ "skylark-comment": skylarkCommentWarning,
+ "skylark-docstring": skylarkDocstringWarning,
+ "string-iteration": stringIterationWarning,
+ "uninitialized": uninitializedVariableWarning,
+ "unreachable": unreachableStatementWarning,
+ "unsorted-dict-items": unsortedDictItemsWarning,
+ "unused-variable": unusedVariableWarning,
}
// MultiFileWarningMap lists the warnings that run on the whole file, but may use other files.
diff --git a/warn/warn_load.go b/warn/warn_load.go
new file mode 100644
index 0000000..4f9c7fe
--- /dev/null
+++ b/warn/warn_load.go
@@ -0,0 +1,50 @@
+package warn
+
+import (
+ "fmt"
+ "slices"
+ "strings"
+
+ "github.com/bazelbuild/buildtools/build"
+ "github.com/bazelbuild/buildtools/tables"
+)
+
+func symbolLoadLocationWarning(f *build.File) []*LinterFinding {
+ var findings []*LinterFinding
+
+ for stmtIndex := 0; stmtIndex < len(f.Stmt); stmtIndex++ {
+ load, ok := f.Stmt[stmtIndex].(*build.LoadStmt)
+ if !ok {
+ continue
+ }
+
+ for i := 0; i < len(load.From); i++ {
+ from := load.From[i]
+
+ expected, ok := tables.AllowedSymbolLoadLocations[from.Name]
+ if !ok || expected[load.Module.Value] {
+ continue
+ }
+
+ var f *LinterFinding
+ if len(expected) == 1 {
+ var loc string
+ for l := range expected {
+ loc = l
+ break
+ }
+ f = makeLinterFinding(from, fmt.Sprintf("Symbol %q must be loaded from %s.", from.Name, loc))
+ } else {
+ locs := make([]string, 0, len(expected))
+ for l := range expected {
+ locs = append(locs, l)
+ }
+ slices.Sort(locs)
+ f = makeLinterFinding(from, fmt.Sprintf("Symbol %q must be loaded from one of the allowed locations: %s.", from.Name, strings.Join(locs, ", ")))
+ }
+ findings = append(findings, f)
+ }
+
+ }
+ return findings
+}
diff --git a/warn/warn_load_test.go b/warn/warn_load_test.go
new file mode 100644
index 0000000..280271e
--- /dev/null
+++ b/warn/warn_load_test.go
@@ -0,0 +1,43 @@
+/*
+Copyright 2020 Google LLC
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package warn
+
+import (
+ "testing"
+
+ "github.com/bazelbuild/buildtools/tables"
+)
+
+func TestWarnLoadLocation(t *testing.T) {
+ tables.AllowedSymbolLoadLocations["s1"] = map[string]bool{":z.bzl": true}
+ tables.AllowedSymbolLoadLocations["s3"] = map[string]bool{":x.bzl": true, ":y.bzl": true}
+ tables.AllowedSymbolLoadLocations["s4"] = map[string]bool{":a.bzl": true}
+ checkFindingsAndFix(t, "allowed-symbol-load-locations", `
+load(":f.bzl", "s1", "s2")
+load(":a.bzl", "s3")
+load(":a.bzl", "s4")
+`, `
+load(":f.bzl", "s1", "s2")
+load(":a.bzl", "s3")
+load(":a.bzl", "s4")
+`,
+ []string{
+ ":1: Symbol \"s1\" must be loaded from :z.bzl.",
+ ":2: Symbol \"s3\" must be loaded from one of the allowed locations: :x.bzl, :y.bzl.",
+ },
+ scopeEverywhere)
+}
