Add a buildifier warning to enforce rule load location

styurin · styurin · commit 7f228eea788d · 2024-12-16T17:16:57.000-08:00
diff --git a/WARNINGS.md b/WARNINGS.md
@@ -74,6 +74,7 @@ Warning categories supported by buildifier's linter:
   * [`repository-name`](#repository-name)
   * [`return-value`](#return-value)
   * [`rule-impl-return`](#rule-impl-return)
+  * [`rule-load-location`](#rule-load-location)
   * [`same-origin-load`](#same-origin-load)
   * [`skylark-comment`](#skylark-comment)
   * [`skylark-docstring`](#skylark-docstring)
@@ -1210,6 +1211,26 @@ consider using
 [providers](https://docs.bazel.build/versions/main/skylark/rules.html#providers)
 or lists of providers instead.
 
+--------------------------------------------------------------------------------
+
+## <a name="rule-load-location"></a>Rule must be loaded from a specific location
+
+  * Category name: `rule-load-location`
+  * Automatic fix: no
+  * [Suppress the warning](#suppress): `# buildifier: disable=rule-load-location`
+
+Warns when a rule is loaded from a location other than the expected one.
+Expected locations are specified in the tables file:
+
+```json
+{
+  "RuleLoadLocation": {
+    "genrule": "//:genrule.bzl"
+  }
+}
+```
+
+
 --------------------------------------------------------------------------------
 
 ## <a name="same-origin-load"></a>Same label is used for multiple loads
diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go
@@ -112,6 +112,7 @@ func ExampleExample() {
 	//     "repository-name",
 	//     "return-value",
 	//     "rule-impl-return",
+	//     "rule-load-location",
 	//     "skylark-comment",
 	//     "skylark-docstring",
 	//     "string-iteration",
@@ -310,6 +311,7 @@ func TestValidate(t *testing.T) {
 			"repository-name",
 			"return-value",
 			"rule-impl-return",
+			"rule-load-location",
 			"skylark-comment",
 			"skylark-docstring",
 			"string-iteration",
@@ -389,6 +391,7 @@ func TestValidate(t *testing.T) {
 			"repository-name",
 			"return-value",
 			"rule-impl-return",
+			"rule-load-location",
 			"skylark-comment",
 			"skylark-docstring",
 			"string-iteration",
@@ -467,7 +470,7 @@ func TestValidate(t *testing.T) {
 			"repository-name",
 			"return-value",
 			"rule-impl-return",
-
+			"rule-load-location",
 			"skylark-comment",
 			"skylark-docstring",
 			"string-iteration",
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
@@ -317,6 +317,7 @@ cat > golden/.buildifier.example.json <<EOF
     "repository-name",
     "return-value",
     "rule-impl-return",
+    "rule-load-location",
     "skylark-comment",
     "skylark-docstring",
     "string-iteration",
diff --git a/tables/jsonparser.go b/tables/jsonparser.go
@@ -31,6 +31,7 @@ type Definitions struct {
 	NamePriority                    map[string]int
 	StripLabelLeadingSlashes        bool
 	ShortenAbsoluteLabelsToRelative bool
+	RuleLoadLocation                map[string]string
 }
 
 // ParseJSONDefinitions reads and parses JSON table definitions from file.
@@ -55,9 +56,9 @@ func ParseAndUpdateJSONDefinitions(file string, merge bool) error {
 	}
 
 	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.RuleLoadLocation)
 	} 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.RuleLoadLocation)
 	}
 	return nil
 }
diff --git a/tables/jsonparser_test.go b/tables/jsonparser_test.go
@@ -37,6 +37,7 @@ func TestParseJSONDefinitions(t *testing.T) {
 		SortableAllowlist:        map[string]bool{},
 		NamePriority:             map[string]int{"name": -1},
 		StripLabelLeadingSlashes: true,
+		RuleLoadLocation:         map[string]string{"genrule": "//: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
@@ -271,8 +271,11 @@ var IsModuleOverride = map[string]bool{
 	"single_version_override":   true,
 }
 
+// RuleLoadLocation contains custom locations for loading rules.
+var RuleLoadLocation = map[string]string{}
+
 // 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, ruleLoadLocation map[string]string) {
 	IsLabelArg = labelArg
 	LabelDenylist = denylist
 	IsListArg = listArg
@@ -282,10 +285,11 @@ func OverrideTables(labelArg, denylist, listArg, sortableListArg, sortDenylist,
 	NamePriority = namePriority
 	StripLabelLeadingSlashes = stripLabelLeadingSlashes
 	ShortenAbsoluteLabelsToRelative = shortenAbsoluteLabelsToRelative
+	RuleLoadLocation = ruleLoadLocation
 }
 
 // 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, ruleLoadLocation map[string]string) {
 	for k, v := range labelArg {
 		IsLabelArg[k] = v
 	}
@@ -309,4 +313,7 @@ func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sor
 	}
 	StripLabelLeadingSlashes = stripLabelLeadingSlashes || StripLabelLeadingSlashes
 	ShortenAbsoluteLabelsToRelative = shortenAbsoluteLabelsToRelative || ShortenAbsoluteLabelsToRelative
+	for k, v := range ruleLoadLocation {
+		RuleLoadLocation[k] = v
+	}
 }
diff --git a/tables/testdata/simple_tables.json b/tables/testdata/simple_tables.json
@@ -19,5 +19,8 @@
   "NamePriority": {
     "name": -1
   },
-  "StripLabelLeadingSlashes": true
+  "StripLabelLeadingSlashes": true,
+  "RuleLoadLocation": {
+    "genrule": "//:genrule.bzl"
+  }
 }
diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel
@@ -13,6 +13,7 @@ go_library(
         "warn_cosmetic.go",
         "warn_deprecated.go",
         "warn_docstring.go",
+        "warn_load.go",
         "warn_macro.go",
         "warn_naming.go",
         "warn_operation.go",
@@ -41,6 +42,7 @@ go_test(
         "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
@@ -897,6 +897,21 @@ warnings: {
     "or lists of providers instead."
 }
 
+warnings: {
+  name: "rule-load-location"
+  header: "Rule must be loaded from a specific location"
+  description:
+      "Warns when a rule is loaded from a location other than the expected one.\n"
+      "Expected locations are specified in the tables file:\n\n"
+  "```json\n"
+  "{\n"
+  "  \"RuleLoadLocation\": {\n"
+  "    \"genrule\": \"//:genrule.bzl\"\n"
+  "  }\n"
+  "}\n"
+  "```\n"
+}
+
 warnings: {
   name: "same-origin-load"
   header: "Same label is used for multiple loads"
diff --git a/warn/warn.go b/warn/warn.go
@@ -184,6 +184,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"redefined-variable":               redefinedVariableWarning,
 	"repository-name":                  repositoryNameWarning,
 	"rule-impl-return":                 ruleImplReturnWarning,
+	"rule-load-location":               ruleLoadLocationWarning,
 	"return-value":                     missingReturnValueWarning,
 	"skylark-comment":                  skylarkCommentWarning,
 	"skylark-docstring":                skylarkDocstringWarning,
diff --git a/warn/warn_load.go b/warn/warn_load.go
@@ -0,0 +1,32 @@
+package warn
+
+import (
+	"fmt"
+	"github.com/bazelbuild/buildtools/build"
+	"github.com/bazelbuild/buildtools/tables"
+)
+
+func ruleLoadLocationWarning(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.To); i++ {
+			from := load.From[i]
+
+			expectedLocation, ok := tables.RuleLoadLocation[from.Name]
+			if !ok || expectedLocation == load.Module.Value {
+				continue
+			}
+
+			f := makeLinterFinding(load.From[i], fmt.Sprintf("Rule %q must be loaded from %v.", from.Name, expectedLocation))
+			findings = append(findings, f)
+		}
+
+	}
+	return findings
+}
diff --git a/warn/warn_load_test.go b/warn/warn_load_test.go
@@ -0,0 +1,39 @@
+/*
+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 (
+	"github.com/bazelbuild/buildtools/tables"
+	"testing"
+)
+
+func TestWarnLoadLocation(t *testing.T) {
+	tables.RuleLoadLocation["s1"] = ":z.bzl"
+	tables.RuleLoadLocation["s3"] = ":y.bzl"
+	checkFindingsAndFix(t, "rule-load-location", `
+load(":f.bzl", "s1", "s2")
+load(":a.bzl", "s3")
+`, `
+load(":f.bzl", "s1", "s2")
+load(":a.bzl", "s3")
+`,
+		[]string{
+			":1: Rule \"s1\" must be loaded from :z.bzl.",
+			":2: Rule \"s3\" must be loaded from :y.bzl.",
+		},
+		scopeEverywhere)
+}

Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,7 @@ type Definitions struct {`
`31`	`31`	`NamePriority map[string]int`
`32`	`32`	`StripLabelLeadingSlashes bool`
`33`	`33`	`ShortenAbsoluteLabelsToRelative bool`
	`34`	`+ RuleLoadLocation map[string]string`
`34`	`35`	`}`
`35`	`36`
`36`	`37`	`// ParseJSONDefinitions reads and parses JSON table definitions from file.`
`@@ -55,9 +56,9 @@ func ParseAndUpdateJSONDefinitions(file string, merge bool) error {`
`55`	`56`	`}`
`56`	`57`
`57`	`58`	`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.RuleLoadLocation)`
`59`	`60`	`} 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.RuleLoadLocation)`
`61`	`62`	`}`
`62`	`63`	`return nil`
`63`	`64`	`}`
Original file line number	Diff line number	Diff line change
`@@ -37,6 +37,7 @@ func TestParseJSONDefinitions(t *testing.T) {`
`37`	`37`	`SortableAllowlist: map[string]bool{},`
`38`	`38`	`NamePriority: map[string]int{"name": -1},`
`39`	`39`	`StripLabelLeadingSlashes: true,`
	`40`	`+ RuleLoadLocation: map[string]string{"genrule": "//:genrule.bzl"},`
`40`	`41`	`}`
`41`	`42`	`if !reflect.DeepEqual(expected, definitions) {`
`42`	`43`	`t.Errorf("ParseJSONDefinitions(simple_tables.json) = %v; want %v", definitions, expected)`