From 82a64aab0bba28d6921a4c08176ec60804fa4a98 Mon Sep 17 00:00:00 2001
From: Sergey Tyurin <sergey.tyurin@cloudkitchens.com>
Date: Mon, 16 Dec 2024 17:13:20 -0800
Subject: [PATCH 1/2] Add a buildifier warning to enforce rule load location

---
 WARNINGS.md                        | 21 ++++++++++++++++
 buildifier/config/config_test.go   |  5 +++-
 buildifier/integration_test.sh     |  1 +
 tables/jsonparser.go               |  5 ++--
 tables/jsonparser_test.go          |  1 +
 tables/tables.go                   | 11 +++++++--
 tables/testdata/simple_tables.json |  5 +++-
 warn/BUILD.bazel                   |  2 ++
 warn/docs/warnings.textproto       | 15 ++++++++++++
 warn/warn.go                       |  1 +
 warn/warn_load.go                  | 32 ++++++++++++++++++++++++
 warn/warn_load_test.go             | 39 ++++++++++++++++++++++++++++++
 12 files changed, 132 insertions(+), 6 deletions(-)
 create mode 100644 warn/warn_load.go
 create mode 100644 warn/warn_load_test.go

diff --git a/WARNINGS.md b/WARNINGS.md
index 9eed6aad5..7c1b11ecc 100644
--- 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)
@@ -1207,6 +1208,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": "//tools/bazel: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
index dd3344e87..32b9f4b06 100644
--- 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",
@@ -468,7 +471,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
index f5457ea44..b3511edf1 100755
--- 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
index d2ae1dcc4..3c3deb5db 100644
--- 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
index 800934de6..1bb1b67e3 100644
--- 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": "//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 dbe0df808..b9bb1d04c 100644
--- a/tables/tables.go
+++ b/tables/tables.go
@@ -277,8 +277,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
@@ -288,10 +291,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
 	}
@@ -315,4 +319,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
index 9243e2d52..40e4e9127 100644
--- 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": "//tools/bazel:genrule.bzl"
+  }
 }
diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel
index 225b5fcfa..377a38975 100644
--- 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",
@@ -42,6 +43,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
index aa45c3357..fb59b29f2 100644
--- 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\": \"//tools/bazel: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
index fd869987d..3ada2cc91 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -160,6 +160,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
new file mode 100644
index 000000000..fe2d6977f
--- /dev/null
+++ 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
new file mode 100644
index 000000000..537355875
--- /dev/null
+++ 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)
+}

From 283c8f970fd5727078b6fee5a11a028221a593db Mon Sep 17 00:00:00 2001
From: Sergey Tyurin <sergey.tyurin@cloudkitchens.com>
Date: Mon, 16 Dec 2024 18:57:00 -0800
Subject: [PATCH 2/2] Minor refactoring

---
 warn/warn_load.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/warn/warn_load.go b/warn/warn_load.go
index fe2d6977f..a16473343 100644
--- a/warn/warn_load.go
+++ b/warn/warn_load.go
@@ -15,7 +15,7 @@ func ruleLoadLocationWarning(f *build.File) []*LinterFinding {
 			continue
 		}
 
-		for i := 0; i < len(load.To); i++ {
+		for i := 0; i < len(load.From); i++ {
 			from := load.From[i]
 
 			expectedLocation, ok := tables.RuleLoadLocation[from.Name]
@@ -23,7 +23,7 @@ func ruleLoadLocationWarning(f *build.File) []*LinterFinding {
 				continue
 			}
 
-			f := makeLinterFinding(load.From[i], fmt.Sprintf("Rule %q must be loaded from %v.", from.Name, expectedLocation))
+			f := makeLinterFinding(from, fmt.Sprintf("Rule %q must be loaded from %v.", from.Name, expectedLocation))
 			findings = append(findings, f)
 		}