From 9089c4eb356a98dc0828e47715b760bdbea3513f Mon Sep 17 00:00:00 2001
From: Keith Smiley <keithbsmiley@gmail.com>
Date: Thu, 17 Oct 2024 16:13:05 -0700
Subject: [PATCH] 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
---
 WARNINGS.md                      | 33 ++++++++++++++
 buildifier/config/config_test.go | 12 +++++
 buildifier/integration_test.sh   |  3 ++
 warn/docs/warnings.textproto     | 21 +++++++++
 warn/warn.go                     |  3 ++
 warn/warn_bazel_api.go           | 22 +++++++++-
 warn/warn_bazel_api_test.go      | 75 ++++++++++++++++++++++++++++++++
 7 files changed, 168 insertions(+), 1 deletion(-)

diff --git a/WARNINGS.md b/WARNINGS.md
index 5ee0af111..460d89ec2 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -44,6 +44,9 @@ Warning categories supported by buildifier's linter:
   * [`native-package`](#native-package)
   * [`native-proto`](#native-proto)
   * [`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)
@@ -754,6 +757,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/buildifier/config/config_test.go b/buildifier/config/config_test.go
index ff120d393..ea59e11dd 100644
--- a/buildifier/config/config_test.go
+++ b/buildifier/config/config_test.go
@@ -83,6 +83,9 @@ func ExampleExample() {
 	//     "native-package",
 	//     "native-proto",
 	//     "native-py",
+	//     "native-sh-binary",
+	//     "native-sh-library",
+	//     "native-sh-test",
 	//     "no-effect",
 	//     "output-group",
 	//     "overly-nested-depset",
@@ -264,6 +267,9 @@ func TestValidate(t *testing.T) {
 			"native-package",
 			"native-proto",
 			"native-py",
+			"native-sh-binary",
+			"native-sh-library",
+			"native-sh-test",
 			"no-effect",
 			"output-group",
 			"overly-nested-depset",
@@ -326,6 +332,9 @@ func TestValidate(t *testing.T) {
 			"native-package",
 			// "native-proto",
 			// "native-py",
+			"native-sh-binary",
+			"native-sh-library",
+			"native-sh-test",
 			"no-effect",
 			"output-group",
 			"overly-nested-depset",
@@ -387,6 +396,9 @@ func TestValidate(t *testing.T) {
 			"native-package",
 			// "native-proto",
 			// "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 3c1173b3b..85bee3d7b 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -288,6 +288,9 @@ cat > golden/.buildifier.example.json <<EOF
     "native-package",
     "native-proto",
     "native-py",
+    "native-sh-binary",
+    "native-sh-library",
+    "native-sh-test",
     "no-effect",
     "output-group",
     "overly-nested-depset",
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index fa42da71c..bbbf58d16 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -539,6 +539,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 6f7c24e51..06c97fb73 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -157,6 +157,9 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"native-package":            nativePackageWarning,
 	"native-proto":              nativeProtoRulesWarning,
 	"native-py":                 nativePyRulesWarning,
+	"native-sh-binary":          nativeShBinaryWarning,
+	"native-sh-library":         nativeShLibraryWarning,
+	"native-sh-test":            nativeShTestWarning,
 	"no-effect":                 noEffectWarning,
 	"output-group":              outputGroupWarning,
 	"overly-nested-depset":      overlyNestedDepsetWarning,
diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go
index ecce12fa4..b8ea277aa 100644
--- a/warn/warn_bazel_api.go
+++ b/warn/warn_bazel_api.go
@@ -716,6 +716,27 @@ func nativeProtoRulesWarning(f *build.File) []*LinterFinding {
 	return notLoadedUsageCheck(f, tables.ProtoNativeRules, tables.ProtoNativeSymbols, tables.ProtoLoadPath)
 }
 
+func nativeShBinaryWarning(f *build.File) []*LinterFinding {
+	if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
+		return nil
+	}
+	return NotLoadedFunctionUsageCheck(f, []string{"sh_binary"}, "@rules_shell//shell:sh_binary.bzl")
+}
+
+func nativeShLibraryWarning(f *build.File) []*LinterFinding {
+	if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
+		return nil
+	}
+	return NotLoadedFunctionUsageCheck(f, []string{"sh_library"}, "@rules_shell//shell:sh_library.bzl")
+}
+
+func nativeShTestWarning(f *build.File) []*LinterFinding {
+	if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
+		return nil
+	}
+	return NotLoadedFunctionUsageCheck(f, []string{"sh_test"}, "@rules_shell//shell:sh_test.bzl")
+}
+
 func contextArgsAPIWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeBzl {
 		return nil
@@ -1085,7 +1106,6 @@ func providerParamsWarning(f *build.File) []*LinterFinding {
 	return findings
 }
 
-
 func attrNameWarning(f *build.File, names []string) []*LinterFinding {
 	if f.Type != build.TypeBzl {
 		return nil
diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go
index b23f970ce..af9a8d468 100644
--- a/warn/warn_bazel_api_test.go
+++ b/warn/warn_bazel_api_test.go
@@ -701,6 +701,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)