Skip to content

Commit bbf01ec

Browse files
styurinstyurin
authored
Add a buildifier warning to enforce symbol load location (#1318)
* Add a buildifier warning to enforce rule load location * Minor refactoring * Rename to AllowedSymbolLoadLocations
1 parent 9bdafcf commit bbf01ec

File tree

12 files changed

+264
-65
lines changed

12 files changed

+264
-65
lines changed

‎WARNINGS.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
Warning categories supported by buildifier's linter:
44

5+
* [`allowed-symbol-load-locations`](#allowed-symbol-load-locations)
56
* [`attr-applicable_licenses`](#attr-applicable_licenses)
67
* [`attr-cfg`](#attr-cfg)
78
* [`attr-license`](#attr-license)
@@ -128,6 +129,27 @@ if debug:
128129

129130
--------------------------------------------------------------------------------
130131

132+
## <a name="allowed-symbol-load-locations"></a>Symbol must be loaded from a specific location
133+
134+
* Category name: `allowed-symbol-load-locations`
135+
* Automatic fix: no
136+
* [Suppress the warning](#suppress): `# buildifier: disable=allowed-symbol-load-locations`
137+
138+
Warns when a symbol is loaded from a location other than the expected ones.
139+
Expected locations are specified in the tables file:
140+
141+
```json
142+
{
143+
"AllowedSymbolLoadLocations": {
144+
"genrule": [
145+
"//tools/bazel:genrule.bzl"
146+
]
147+
}
148+
}
149+
```
150+
151+
--------------------------------------------------------------------------------
152+
131153
## <a name="attr-applicable_licenses"></a>Do not use `applicable_licenses` as an attribute name.
132154

133155
* Category name: `attr-applicable_licenses`

‎buildifier/config/config_test.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ func ExampleExample() {
4343
// "mode": "fix",
4444
// "lint": "fix",
4545
// "warningsList": [
46+
// "allowed-symbol-load-locations",
4647
// "attr-applicable_licenses",
4748
// "attr-cfg",
4849
// "attr-license",
@@ -263,6 +264,7 @@ func TestValidate(t *testing.T) {
263264
"type auto": {options: "--type=auto"},
264265
"type error": {options: "--type=foo", wantErr: fmt.Errorf("unrecognized input type foo; valid types are build, bzl, workspace, default, module, auto")},
265266
"warnings all": {options: "--warnings=all", wantWarnings: []string{
267+
"allowed-symbol-load-locations",
266268
"attr-applicable_licenses",
267269
"attr-cfg",
268270
"attr-license",
@@ -364,6 +366,7 @@ func TestValidate(t *testing.T) {
364366
"unused-variable",
365367
}},
366368
"warnings default": {options: "--warnings=default", wantWarnings: []string{
369+
"allowed-symbol-load-locations",
367370
"attr-applicable_licenses",
368371
"attr-cfg",
369372
"attr-license",
@@ -465,6 +468,7 @@ func TestValidate(t *testing.T) {
465468
"unused-variable",
466469
}},
467470
"warnings plus/minus": {options: "--warnings=+unsorted-dict-items,-print,-deprecated-function", wantWarnings: []string{
471+
"allowed-symbol-load-locations",
468472
"attr-applicable_licenses",
469473
"attr-cfg",
470474
"attr-license",
@@ -556,7 +560,6 @@ func TestValidate(t *testing.T) {
556560
"repository-name",
557561
"return-value",
558562
"rule-impl-return",
559-
560563
"skylark-comment",
561564
"skylark-docstring",
562565
"string-iteration",
@@ -567,6 +570,7 @@ func TestValidate(t *testing.T) {
567570
"unused-variable",
568571
}},
569572
"warnings no duplicates": {options: "--warnings=+unused-variable", wantWarnings: []string{
573+
"allowed-symbol-load-locations",
570574
"attr-applicable_licenses",
571575
"attr-cfg",
572576
"attr-license",
@@ -658,7 +662,6 @@ func TestValidate(t *testing.T) {
658662
"repository-name",
659663
"return-value",
660664
"rule-impl-return",
661-
662665
"skylark-comment",
663666
"skylark-docstring",
664667
"string-iteration",

‎buildifier/integration_test.sh‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ cat > golden/.buildifier.example.json <<EOF
258258
"mode": "fix",
259259
"lint": "fix",
260260
"warningsList": [
261+
"allowed-symbol-load-locations",
261262
"attr-applicable_licenses",
262263
"attr-cfg",
263264
"attr-license",
@@ -691,3 +692,36 @@ EOF
691692

692693
$buildifier --lint=warn --warnings=deprecated-function BUILD 2> report || ret=$?
693694
diff -u report_golden report || die "$1: wrong console output for multifile warnings (WORKSPACE exists)"
695+
696+
cd ..
697+
698+
# Test allowed symbol load locations
699+
700+
mkdir test_dir/allowed_locations
701+
cd test_dir/allowed_locations
702+
703+
cat > BUILD <<EOF
704+
load(":f.bzl", "s1", "s2")
705+
load(":a.bzl", "s3")
706+
load(":a.bzl", "s4")
707+
EOF
708+
709+
cat > buildifier.tables <<EOF
710+
{
711+
"AllowedSymbolLoadLocations": {
712+
"s1": [":z.bzl"],
713+
"s3": [":y.bzl", ":x.bzl"],
714+
"s4": [":a.bzl"]
715+
}
716+
}
717+
EOF
718+
719+
cat > report_golden <<EOF
720+
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)
721+
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)
722+
EOF
723+
724+
$buildifier --lint=warn --warnings=allowed-symbol-load-locations -tables=buildifier.tables BUILD 2> report || true
725+
diff -u report_golden report || die "$1: wrong console output for allowed symbol load locations"
726+
727+
cd ../..

‎tables/jsonparser.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ type Definitions struct {
3131
NamePriority map[string]int
3232
StripLabelLeadingSlashes bool
3333
ShortenAbsoluteLabelsToRelative bool
34+
AllowedSymbolLoadLocations map[string][]string
3435
}
3536

3637
// ParseJSONDefinitions reads and parses JSON table definitions from file.
@@ -55,9 +56,9 @@ func ParseAndUpdateJSONDefinitions(file string, merge bool) error {
5556
}
5657

5758
if merge {
58-
MergeTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative)
59+
MergeTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative, definitions.AllowedSymbolLoadLocations)
5960
} else {
60-
OverrideTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative)
61+
OverrideTables(definitions.IsLabelArg, definitions.LabelDenylist, definitions.IsListArg, definitions.IsSortableListArg, definitions.SortableDenylist, definitions.SortableAllowlist, definitions.NamePriority, definitions.StripLabelLeadingSlashes, definitions.ShortenAbsoluteLabelsToRelative, definitions.AllowedSymbolLoadLocations)
6162
}
6263
return nil
6364
}

‎tables/jsonparser_test.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,14 @@ func TestParseJSONDefinitions(t *testing.T) {
3030
}
3131

3232
expected := Definitions{
33-
IsLabelArg: map[string]bool{"srcs": true},
34-
LabelDenylist: map[string]bool{},
35-
IsSortableListArg: map[string]bool{"srcs": true, "visibility": true},
36-
SortableDenylist: map[string]bool{"genrule.srcs": true},
37-
SortableAllowlist: map[string]bool{},
38-
NamePriority: map[string]int{"name": -1},
39-
StripLabelLeadingSlashes: true,
33+
IsLabelArg: map[string]bool{"srcs": true},
34+
LabelDenylist: map[string]bool{},
35+
IsSortableListArg: map[string]bool{"srcs": true, "visibility": true},
36+
SortableDenylist: map[string]bool{"genrule.srcs": true},
37+
SortableAllowlist: map[string]bool{},
38+
NamePriority: map[string]int{"name": -1},
39+
StripLabelLeadingSlashes: true,
40+
AllowedSymbolLoadLocations: map[string][]string{"genrule": {"//tools/bazel:genrule.bzl"}},
4041
}
4142
if !reflect.DeepEqual(expected, definitions) {
4243
t.Errorf("ParseJSONDefinitions(simple_tables.json) = %v; want %v", definitions, expected)

‎tables/tables.go‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,11 @@ var IsModuleOverride = map[string]bool{
265265
"single_version_override": true,
266266
}
267267

268+
// AllowedSymbolLoadLocations contains locations for loading rules that are allowed to be used.
269+
var AllowedSymbolLoadLocations = map[string]map[string]bool{}
270+
268271
// OverrideTables allows a user of the build package to override the special-case rules. The user-provided tables replace the built-in tables.
269-
func OverrideTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool) {
272+
func OverrideTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool, symbolLoadLocation map[string][]string) {
270273
IsLabelArg = labelArg
271274
LabelDenylist = denylist
272275
IsListArg = listArg
@@ -276,10 +279,19 @@ func OverrideTables(labelArg, denylist, listArg, sortableListArg, sortDenylist,
276279
NamePriority = namePriority
277280
StripLabelLeadingSlashes = stripLabelLeadingSlashes
278281
ShortenAbsoluteLabelsToRelative = shortenAbsoluteLabelsToRelative
282+
283+
AllowedSymbolLoadLocations = map[string]map[string]bool{}
284+
for k, v := range symbolLoadLocation {
285+
locations := map[string]bool{}
286+
for _, l := range v {
287+
locations[l] = true
288+
}
289+
AllowedSymbolLoadLocations[k] = locations
290+
}
279291
}
280292

281293
// 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.
282-
func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool) {
294+
func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool, symbolLoadLocation map[string][]string) {
283295
for k, v := range labelArg {
284296
IsLabelArg[k] = v
285297
}
@@ -303,4 +315,12 @@ func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sor
303315
}
304316
StripLabelLeadingSlashes = stripLabelLeadingSlashes || StripLabelLeadingSlashes
305317
ShortenAbsoluteLabelsToRelative = shortenAbsoluteLabelsToRelative || ShortenAbsoluteLabelsToRelative
318+
319+
for k, v := range symbolLoadLocation {
320+
locations := map[string]bool{}
321+
for _, l := range v {
322+
locations[l] = true
323+
}
324+
AllowedSymbolLoadLocations[k] = locations
325+
}
306326
}

‎tables/testdata/simple_tables.json‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,5 +19,10 @@
1919
"NamePriority": {
2020
"name": -1
2121
},
22-
"StripLabelLeadingSlashes": true
22+
"StripLabelLeadingSlashes": true,
23+
"AllowedSymbolLoadLocations": {
24+
"genrule": [
25+
"//tools/bazel:genrule.bzl"
26+
]
27+
}
2328
}

‎warn/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ go_library(
1313
"warn_cosmetic.go",
1414
"warn_deprecated.go",
1515
"warn_docstring.go",
16+
"warn_load.go",
1617
"warn_macro.go",
1718
"warn_naming.go",
1819
"warn_operation.go",
@@ -42,6 +43,7 @@ go_test(
4243
"warn_cosmetic_test.go",
4344
"warn_deprecated_test.go",
4445
"warn_docstring_test.go",
46+
"warn_load_test.go",
4547
"warn_macro_test.go",
4648
"warn_naming_test.go",
4749
"warn_operation_test.go",

‎warn/docs/warnings.textproto‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,23 @@
22
# proto-message: Warnings
33
# After modifying this file, run `bazel build //warn/docs:warnings_docs && cp bazel-bin/warn/docs/WARNINGS.md .`
44

5+
warnings: {
6+
name: "allowed-symbol-load-locations"
7+
header: "Symbol must be loaded from a specific location"
8+
description:
9+
"Warns when a symbol is loaded from a location other than the expected ones.\n"
10+
"Expected locations are specified in the tables file:\n\n"
11+
"```json\n"
12+
"{\n"
13+
" \"AllowedSymbolLoadLocations\": {\n"
14+
" \"genrule\": [\n"
15+
" \"//tools/bazel:genrule.bzl\"\n"
16+
" ]\n"
17+
" }\n"
18+
"}\n"
19+
"```"
20+
}
21+
522
warnings: {
623
name: "attr-cfg"
724
header: "`cfg = \"data\"` for attr definitions has no effect"

‎warn/warn.go‎

Lines changed: 52 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -116,57 +116,58 @@ var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFi
116116

117117
// FileWarningMap lists the warnings that run on the whole file.
118118
var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
119-
"attr-applicable_licenses": attrApplicableLicensesWarning,
120-
"attr-cfg": attrConfigurationWarning,
121-
"attr-license": attrLicenseWarning,
122-
"attr-licenses": attrLicensesWarning,
123-
"attr-non-empty": attrNonEmptyWarning,
124-
"attr-output-default": attrOutputDefaultWarning,
125-
"attr-single-file": attrSingleFileWarning,
126-
"build-args-kwargs": argsKwargsInBuildFilesWarning,
127-
"canonical-repository": canonicalRepositoryWarning,
128-
"confusing-name": confusingNameWarning,
129-
"constant-glob": constantGlobWarning,
130-
"ctx-actions": ctxActionsWarning,
131-
"ctx-args": contextArgsAPIWarning,
132-
"depset-items": depsetItemsWarning,
133-
"depset-iteration": depsetIterationWarning,
134-
"depset-union": depsetUnionWarning,
135-
"dict-method-named-arg": dictMethodNamedArgWarning,
136-
"dict-concatenation": dictionaryConcatenationWarning,
137-
"duplicated-name": duplicatedNameWarning,
138-
"external-path": externalPathWarning,
139-
"filetype": fileTypeWarning,
140-
"function-docstring": functionDocstringWarning,
141-
"function-docstring-header": functionDocstringHeaderWarning,
142-
"function-docstring-args": functionDocstringArgsWarning,
143-
"function-docstring-return": functionDocstringReturnWarning,
144-
"integer-division": integerDivisionWarning,
145-
"keyword-positional-params": keywordPositionalParametersWarning,
146-
"list-append": listAppendWarning,
147-
"load": unusedLoadWarning,
148-
"module-docstring": moduleDocstringWarning,
149-
"name-conventions": nameConventionsWarning,
150-
"native-build": nativeInBuildFilesWarning,
151-
"native-package": nativePackageWarning,
152-
"no-effect": noEffectWarning,
153-
"output-group": outputGroupWarning,
154-
"overly-nested-depset": overlyNestedDepsetWarning,
155-
"package-name": packageNameWarning,
156-
"package-on-top": packageOnTopWarning,
157-
"print": printWarning,
158-
"provider-params": providerParamsWarning,
159-
"redefined-variable": redefinedVariableWarning,
160-
"repository-name": repositoryNameWarning,
161-
"rule-impl-return": ruleImplReturnWarning,
162-
"return-value": missingReturnValueWarning,
163-
"skylark-comment": skylarkCommentWarning,
164-
"skylark-docstring": skylarkDocstringWarning,
165-
"string-iteration": stringIterationWarning,
166-
"uninitialized": uninitializedVariableWarning,
167-
"unreachable": unreachableStatementWarning,
168-
"unsorted-dict-items": unsortedDictItemsWarning,
169-
"unused-variable": unusedVariableWarning,
119+
"allowed-symbol-load-locations": symbolLoadLocationWarning,
120+
"attr-applicable_licenses": attrApplicableLicensesWarning,
121+
"attr-cfg": attrConfigurationWarning,
122+
"attr-license": attrLicenseWarning,
123+
"attr-licenses": attrLicensesWarning,
124+
"attr-non-empty": attrNonEmptyWarning,
125+
"attr-output-default": attrOutputDefaultWarning,
126+
"attr-single-file": attrSingleFileWarning,
127+
"build-args-kwargs": argsKwargsInBuildFilesWarning,
128+
"canonical-repository": canonicalRepositoryWarning,
129+
"confusing-name": confusingNameWarning,
130+
"constant-glob": constantGlobWarning,
131+
"ctx-actions": ctxActionsWarning,
132+
"ctx-args": contextArgsAPIWarning,
133+
"depset-items": depsetItemsWarning,
134+
"depset-iteration": depsetIterationWarning,
135+
"depset-union": depsetUnionWarning,
136+
"dict-method-named-arg": dictMethodNamedArgWarning,
137+
"dict-concatenation": dictionaryConcatenationWarning,
138+
"duplicated-name": duplicatedNameWarning,
139+
"external-path": externalPathWarning,
140+
"filetype": fileTypeWarning,
141+
"function-docstring": functionDocstringWarning,
142+
"function-docstring-header": functionDocstringHeaderWarning,
143+
"function-docstring-args": functionDocstringArgsWarning,
144+
"function-docstring-return": functionDocstringReturnWarning,
145+
"integer-division": integerDivisionWarning,
146+
"keyword-positional-params": keywordPositionalParametersWarning,
147+
"list-append": listAppendWarning,
148+
"load": unusedLoadWarning,
149+
"module-docstring": moduleDocstringWarning,
150+
"name-conventions": nameConventionsWarning,
151+
"native-build": nativeInBuildFilesWarning,
152+
"native-package": nativePackageWarning,
153+
"no-effect": noEffectWarning,
154+
"output-group": outputGroupWarning,
155+
"overly-nested-depset": overlyNestedDepsetWarning,
156+
"package-name": packageNameWarning,
157+
"package-on-top": packageOnTopWarning,
158+
"print": printWarning,
159+
"provider-params": providerParamsWarning,
160+
"redefined-variable": redefinedVariableWarning,
161+
"repository-name": repositoryNameWarning,
162+
"rule-impl-return": ruleImplReturnWarning,
163+
"return-value": missingReturnValueWarning,
164+
"skylark-comment": skylarkCommentWarning,
165+
"skylark-docstring": skylarkDocstringWarning,
166+
"string-iteration": stringIterationWarning,
167+
"uninitialized": uninitializedVariableWarning,
168+
"unreachable": unreachableStatementWarning,
169+
"unsorted-dict-items": unsortedDictItemsWarning,
170+
"unused-variable": unusedVariableWarning,
170171
}
171172

172173
// MultiFileWarningMap lists the warnings that run on the whole file, but may use other files.

0 commit comments

Comments
 (0)