From 6dc88419901071391679a5642a335bd68b8e3260 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Fri, 24 May 2024 14:45:09 +0200
Subject: [PATCH] Fix sorting of relative and `@foo.bzl` loads

Load sorting now properly parses load labels and compares repo, package and
target name in order. Relative labels are forced to sort last.
---
 build/BUILD.bazel         |  1 +
 build/rewrite.go          | 34 ++++++++++++++++------------------
 build/testdata/075.golden |  5 +++++
 build/testdata/075.in     |  5 +++++
 4 files changed, 27 insertions(+), 18 deletions(-)
 create mode 100644 build/testdata/075.golden
 create mode 100644 build/testdata/075.in

diff --git a/build/BUILD.bazel b/build/BUILD.bazel
index 274211020..2b7fcf0dc 100644
--- a/build/BUILD.bazel
+++ b/build/BUILD.bazel
@@ -23,6 +23,7 @@ go_library(
     importpath = "github.com/bazelbuild/buildtools/build",
     visibility = ["//visibility:public"],
     deps = [
+        "//labels",
         "//tables",
     ],
 )
diff --git a/build/rewrite.go b/build/rewrite.go
index 30a766d20..e297d7c11 100644
--- a/build/rewrite.go
+++ b/build/rewrite.go
@@ -25,6 +25,7 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/bazelbuild/buildtools/labels"
 	"github.com/bazelbuild/buildtools/tables"
 )
 
@@ -1081,6 +1082,7 @@ func comparePaths(path1, path2 string) bool {
 // If one label has explicit repository path (starts with @), it goes first
 // If the packages are different, labels are sorted by package name (empty package goes first)
 // If the packages are the same, labels are sorted by their name
+// Relative labels go last
 func compareLoadLabels(load1Label, load2Label string) bool {
 	// handle absolute labels with explicit repositories separately to
 	// make sure they precede absolute and relative labels without repos
@@ -1092,31 +1094,27 @@ func compareLoadLabels(load1Label, load2Label string) bool {
 		return isExplicitRepo1
 	}
 
-	// Either both labels have explicit repository names or both don't, compare their packages
-	// and break ties using file names if necessary
-	module1Parts := strings.SplitN(load1Label, ":", 2)
-	package1, filename1 := "", module1Parts[0]
-	if len(module1Parts) == 2 {
-		package1, filename1 = module1Parts[0], module1Parts[1]
-	}
-	module2Parts := strings.SplitN(load2Label, ":", 2)
-	package2, filename2 := "", module2Parts[0]
-	if len(module2Parts) == 2 {
-		package2, filename2 = module2Parts[0], module2Parts[1]
-	}
+	// ensure that relative labels go last by giving them a fake package name
+	// that sorts after all valid package names
+	label1 := labels.ParseRelative(load1Label, string(rune(0x7F)))
+	label2 := labels.ParseRelative(load2Label, string(rune(0x7F)))
 
-	// in case both packages are the same, use file names to break ties
-	if package1 == package2 {
-		return comparePaths(filename1, filename2)
+	if label1.Repository != label2.Repository {
+		return label1.Repository < label2.Repository
 	}
 
 	// in case one of the packages is empty, the empty one goes first
-	if len(package1) == 0 || len(package2) == 0 {
-		return len(package1) > 0
+	if (len(label1.Package) == 0) != (len(label2.Package) == 0) {
+		return len(label1.Package) == 0
 	}
 
 	// both packages are non-empty and not equal, so compare them
-	return comparePaths(package1, package2)
+	if label1.Package != label2.Package {
+		return comparePaths(label1.Package, label2.Package)
+	}
+
+	// in case both packages are the same, use file names to break ties
+	return comparePaths(label1.Target, label2.Target)
 }
 
 // sortLoadStatements reorders sorts loads lexicographically by the source file,
diff --git a/build/testdata/075.golden b/build/testdata/075.golden
new file mode 100644
index 000000000..da21653a3
--- /dev/null
+++ b/build/testdata/075.golden
@@ -0,0 +1,5 @@
+load("@bar.bzl", "x", "y")
+load("@foo//:foo.bzl", "x", "y")
+load("//:baa.bzl", "x", "y")
+load("//:bac.bzl", "x", "y")
+load(":bab.bzl", "x", "y")
diff --git a/build/testdata/075.in b/build/testdata/075.in
new file mode 100644
index 000000000..33b43cfcc
--- /dev/null
+++ b/build/testdata/075.in
@@ -0,0 +1,5 @@
+load("//:bac.bzl", "x", "y")
+load(":bab.bzl", "x", "y")
+load("//:baa.bzl", "x", "y")
+load("@foo//:foo.bzl", "x", "y")
+load("@bar.bzl", "x", "y")