Repository: bazelbuild/buildtools
Pull request #1303 (2024-10-17 23:13:56)

Add rules_shell fixing
Currently these are split into 3 rules because 1 rule cannot add 3 load
statements and this repo doesn't have a defs.bzl that exposes them all


keith: i don't feel strongly about the defs.bzl part since realistically these rules shouldn't be disabled, but happy to try to go that route and add that upstream to unify this into 1 rule
keith: @vladmos @comius another one for you, CI issue is infra
comius: cc @c-mita


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 9eed6aa..167eb98 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -61,6 +61,9 @@ Warning categories supported by buildifier's linter:
   * [`native-proto-lang-toolchain`](#native-proto-lang-toolchain)
   * [`native-proto-lang-toolchain-info`](#native-proto-lang-toolchain-info)
   * [`native-py`](#native-py)
+  * [`native-sh-binary`](#native-sh-binary)
+  * [`native-sh-library`](#native-sh-library)
+  * [`native-sh-test`](#native-sh-test)
   * [`no-effect`](#no-effect)
   * [`out-of-order-load`](#out-of-order-load)
   * [`output-group`](#output-group)
@@ -945,6 +948,36 @@ at the moment it's not required to load Starlark rules.
 
 --------------------------------------------------------------------------------
 
+## <a name="native-sh-binary"></a>sh_binary build rules should be loaded from Starlark
+
+  * Category name: `native-sh-binary`
+  * Automatic fix: yes
+  * [Suppress the warning](#suppress): `# buildifier: disable=native-sh-binary`
+
+The sh_binary build rules should be loaded from Starlark.
+
+--------------------------------------------------------------------------------
+
+## <a name="native-sh-library"></a>sh_library build rules should be loaded from Starlark
+
+  * Category name: `native-sh-library`
+  * Automatic fix: yes
+  * [Suppress the warning](#suppress): `# buildifier: disable=native-sh-library`
+
+The sh_library build rules should be loaded from Starlark.
+
+--------------------------------------------------------------------------------
+
+## <a name="native-sh-test"></a>sh_test build rules should be loaded from Starlark
+
+  * Category name: `native-sh-test`
+  * Automatic fix: yes
+  * [Suppress the warning](#suppress): `# buildifier: disable=native-sh-test`
+
+The sh_test build rules should be loaded from Starlark.
+
+--------------------------------------------------------------------------------
+
 ## <a name="no-effect"></a>Expression result is not used
 
   * Category name: `no-effect`
diff --git a/tables/tables.go b/tables/tables.go
index dbe0df8..6fce90f 100644
--- a/tables/tables.go
+++ b/tables/tables.go
@@ -248,6 +248,9 @@ var CcLoadPath = "@rules_cc//cc:defs.bzl"
 // JavaLoadPathPrefix is the load package for the Starlark Java Rules.
 var JavaLoadPathPrefix = "@rules_java//java"
 
+// ShellLoadPathPrefix is the load package for the Starlark Shell Rules.
+var ShellLoadPathPrefix = "@rules_shell//shell"
+
 // PyNativeRules lists all Python rules that are being migrated from Native to Starlark.
 var PyNativeRules = []string{
 	"py_library",
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index aa45c33..6db577f 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -692,6 +692,27 @@ warnings: {
   autofix: true
 }
 
+warnings: {
+  name: "native-sh-binary"
+  header: "sh_binary build rules should be loaded from Starlark"
+  description: "The sh_binary build rules should be loaded from Starlark."
+  autofix: true
+}
+
+warnings: {
+  name: "native-sh-library"
+  header: "sh_library build rules should be loaded from Starlark"
+  description: "The sh_library build rules should be loaded from Starlark."
+  autofix: true
+}
+
+warnings: {
+  name: "native-sh-test"
+  header: "sh_test build rules should be loaded from Starlark"
+  description: "The sh_test build rules should be loaded from Starlark."
+  autofix: true
+}
+
 warnings: {
   name: "no-effect"
   header: "Expression result is not used"
diff --git a/warn/warn.go b/warn/warn.go
index fd86998..db81304 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -197,6 +197,9 @@ var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader)
 	"native-proto-common":              nativeProtoSymbolsWarning("proto_common", "proto_common.bzl"),
 	"native-proto-lang-toolchain-info": nativeProtoSymbolsWarning("ProtoLangToolchainInfo", "proto_lang_toolchain_info.bzl"),
 	"native-py":                        nativePyRulesWarning,
+	"native-sh-binary":                 NativeShellRulesWarning("sh_binary"),
+	"native-sh-library":                NativeShellRulesWarning("sh_library"),
+	"native-sh-test":                   NativeShellRulesWarning("sh_test"),
 	"unnamed-macro":                    unnamedMacroWarning,
 }
 
diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go
index 8ca12f6..6757918 100644
--- a/warn/warn_bazel_api.go
+++ b/warn/warn_bazel_api.go
@@ -798,6 +798,15 @@ func nativeProtoSymbolsWarning(symbol string, bzlfile string) func(f *build.File
 	}
 }
 
+func NativeShellRulesWarning(rule string) func(f *build.File, fileReader *FileReader) []*LinterFinding {
+	return func(f *build.File, fileReader *FileReader) []*LinterFinding {
+		if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
+			return nil
+		}
+		return NotLoadedFunctionUsageCheck(f, fileReader, []string{rule}, tables.ShellLoadPathPrefix+":"+rule+".bzl")
+	}
+}
+
 func contextArgsAPIWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeBzl {
 		return nil


Test patch:
diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go
index dd3344e..eb8f692 100644
--- a/buildifier/config/config_test.go
+++ b/buildifier/config/config_test.go
@@ -100,6 +100,9 @@ func ExampleExample() {
 	//     "native-proto-lang-toolchain",
 	//     "native-proto-lang-toolchain-info",
 	//     "native-py",
+	//     "native-sh-binary",
+	//     "native-sh-library",
+	//     "native-sh-test",
 	//     "no-effect",
 	//     "output-group",
 	//     "overly-nested-depset",
@@ -298,6 +301,9 @@ func TestValidate(t *testing.T) {
 			"native-proto-lang-toolchain",
 			"native-proto-lang-toolchain-info",
 			"native-py",
+			"native-sh-binary",
+			"native-sh-library",
+			"native-sh-test",
 			"no-effect",
 			"output-group",
 			"overly-nested-depset",
@@ -377,6 +383,9 @@ func TestValidate(t *testing.T) {
 			"native-proto-lang-toolchain",
 			"native-proto-lang-toolchain-info",
 			"native-py",
+			"native-sh-binary",
+			"native-sh-library",
+			"native-sh-test",
 			"no-effect",
 			"output-group",
 			"overly-nested-depset",
@@ -456,6 +465,9 @@ func TestValidate(t *testing.T) {
 			"native-proto-lang-toolchain",
 			"native-proto-lang-toolchain-info",
 			"native-py",
+			"native-sh-binary",
+			"native-sh-library",
+			"native-sh-test",
 			"no-effect",
 			"output-group",
 			"overly-nested-depset",
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index f5457ea..57559b7 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -305,6 +305,9 @@ cat > golden/.buildifier.example.json <<EOF
     "native-proto-lang-toolchain",
     "native-proto-lang-toolchain-info",
     "native-py",
+    "native-sh-binary",
+    "native-sh-library",
+    "native-sh-test",
     "no-effect",
     "output-group",
     "overly-nested-depset",
diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go
index a3a734d..cac634c 100644
--- a/warn/warn_bazel_api_test.go
+++ b/warn/warn_bazel_api_test.go
@@ -763,6 +763,81 @@ def macro():
 		scopeBzl|scopeBuild)
 }
 
+func TestNativeShBinaryWarning(t *testing.T) {
+	checkFindingsAndFix(t, "native-sh-binary", `
+"""My file"""
+
+def macro():
+    native.sh_binary()
+
+sh_binary()
+`, `
+"""My file"""
+
+load("@rules_shell//shell:sh_binary.bzl", "sh_binary")
+
+def macro():
+    sh_binary()
+
+sh_binary()
+`,
+		[]string{
+			fmt.Sprintf(`:4: Function "sh_binary" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_binary.bzl".`),
+			fmt.Sprintf(`:6: Function "sh_binary" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_binary.bzl".`),
+		},
+		scopeBzl|scopeBuild)
+}
+
+func TestNativeShLibraryWarning(t *testing.T) {
+	checkFindingsAndFix(t, "native-sh-library", `
+"""My file"""
+
+def macro():
+    native.sh_library()
+
+sh_library()
+`, `
+"""My file"""
+
+load("@rules_shell//shell:sh_library.bzl", "sh_library")
+
+def macro():
+    sh_library()
+
+sh_library()
+`,
+		[]string{
+			fmt.Sprintf(`:4: Function "sh_library" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_library.bzl".`),
+			fmt.Sprintf(`:6: Function "sh_library" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_library.bzl".`),
+		},
+		scopeBzl|scopeBuild)
+}
+
+func TestNativeShTestWarning(t *testing.T) {
+	checkFindingsAndFix(t, "native-sh-test", `
+"""My file"""
+
+def macro():
+    native.sh_test()
+
+sh_test()
+`, `
+"""My file"""
+
+load("@rules_shell//shell:sh_test.bzl", "sh_test")
+
+def macro():
+    sh_test()
+
+sh_test()
+`,
+		[]string{
+			fmt.Sprintf(`:4: Function "sh_test" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_test.bzl".`),
+			fmt.Sprintf(`:6: Function "sh_test" is not global anymore and needs to be loaded from "@rules_shell//shell:sh_test.bzl".`),
+		},
+		scopeBzl|scopeBuild)
+}
+
 func TestKeywordParameters(t *testing.T) {
 	checkFindingsAndFix(t, "keyword-positional-params", `
 foo(key = value)