Fix the out-of-order-load check (#459)

warn/warn.go

@@ -334,6 +334,20 @@ func loadOnTopWarning(f *build.File, fix bool) []*Finding {
 }
 
 func outOfOrderLoadWarning(f *build.File, fix bool) []*Finding {
+	// compareLoadLabels compares two module names
+	compareLoadLabels := func(load1Label, load2Label string) bool {
+		// handle absolute labels with explicit repositories separately to
+		// make sure they preceed absolute and relative labels without repos
+		isExplicitRepo1 := strings.HasPrefix(load1Label, "@")
+		isExplicitRepo2 := strings.HasPrefix(load2Label, "@")
+		if isExplicitRepo1 == isExplicitRepo2 {
+			// Either both labels have explicit repository names or both don't, compare lexicographically
+			return load1Label < load2Label
+		}
+		// Exactly one label has an explicit repository name, it should be the first one.
+		return isExplicitRepo1
+	}
+
 	findings := []*Finding{}
 
 	if f.Type == build.TypeWorkspace {
@@ -353,12 +367,7 @@ func outOfOrderLoadWarning(f *build.File, fix bool) []*Finding {
 		sort.SliceStable(sortedLoads, func(i, j int) bool {
 			load1Label := sortedLoads[i].Module.Value
 			load2Label := sortedLoads[j].Module.Value
-			// handle absolute labels with explicit repositories separately to
-			// make sure they preceed absolute and relative labels without repos
-			if strings.HasPrefix(load1Label, "@") && !strings.HasPrefix(load2Label, "@") {
-				return true
-			}
-			return load1Label < load2Label
+			return compareLoadLabels(load1Label, load2Label)
 		})
 		sortedLoadIndex := 0
 		for globalLoadIndex := 0; globalLoadIndex < len(f.Stmt); globalLoadIndex++ {
@@ -372,7 +381,7 @@ func outOfOrderLoadWarning(f *build.File, fix bool) []*Finding {
 	}
 
 	for i := 1; i < len(sortedLoads); i++ {
-		if sortedLoads[i].Module.Value < sortedLoads[i-1].Module.Value {
+		if !compareLoadLabels(sortedLoads[i-1].Module.Value, sortedLoads[i].Module.Value) {
 			start, end := sortedLoads[i].Span()
 			findings = append(findings, makeFinding(f, start, end, "out-of-order-load",
 				"Load statement is out of its lexicographical order.", true, nil))

warn/warn_test.go

@@ -97,17 +97,17 @@ func checkFindings(t *testing.T, category, input string, expected []string, scop
 }
 
 func checkFindingsAndFix(t *testing.T, category, input, output string, expected []string, scope build.FileType) {
-	// BUILD file
-	compareFindings(t, category, input, expected, scope, build.TypeBuild)
-	checkFix(t, category, input, output, scope, build.TypeBuild)
-
-	// Bzl file
-	compareFindings(t, category, input, expected, scope, build.TypeDefault)
-	checkFix(t, category, input, output, scope, build.TypeDefault)
+	fileTypes := []build.FileType{
+		build.TypeDefault,
+		build.TypeBuild,
+		build.TypeWorkspace,
+	}
 
-	// WORKSPACE file
-	compareFindings(t, category, input, expected, scope, build.TypeWorkspace)
-	checkFix(t, category, input, output, scope, build.TypeWorkspace)
+	for _, fileType := range fileTypes {
+		compareFindings(t, category, input, expected, scope, fileType)
+		checkFix(t, category, input, output, scope, fileType)
+		checkFix(t, category, output, output, scope, fileType)
+	}
 }
 
 func TestNoEffect(t *testing.T) {
@@ -204,7 +204,7 @@ py_library(name = "z")
 php_library(name = "x")`,
 		[]string{":3: A rule with name `x' was already found on line 1",
 			":5: A rule with name `x' was already found on line 1"},
-		scopeBuild | scopeWorkspace)
+		scopeBuild|scopeWorkspace)
 }
 
 func TestWarnUnusedLoad(t *testing.T) {
@@ -327,7 +327,7 @@ z = "name"
 cc_library(name = z)`,
 		[]string{":2: Variable \"x\" is unused.",
 			":3: Variable \"y\" is unused."},
-		scopeBuild | scopeWorkspace)
+		scopeBuild|scopeWorkspace)
 
 	checkFindings(t, "unused-variable", `
 a = 1
@@ -338,7 +338,7 @@ e = 5 # @unused
 # @unused
 f = 7`,
 		[]string{":4: Variable \"d\" is unused."},
-		scopeBuild | scopeWorkspace)
+		scopeBuild|scopeWorkspace)
 
 	checkFindings(t, "unused-variable", `
 a = 1
@@ -353,7 +353,7 @@ def foo():
   g = 8
   return g`,
 		[]string{":6: Variable \"d\" is unused."},
-		scopeBuild | scopeWorkspace)
+		scopeBuild|scopeWorkspace)
 
 	checkFindings(t, "unused-variable", `
 a = 1
@@ -370,7 +370,7 @@ def bar(b):
 			":4: Variable \"b\" is unused.",
 			":8: Variable \"c\" is unused.",
 		},
-		scopeBuild | scopeWorkspace)
+		scopeBuild|scopeWorkspace)
 }
 
 func TestRedefinedVariable(t *testing.T) {
@@ -415,7 +415,7 @@ load(":f.bzl", "x")
 foo()
 
 x()`,
-		[]string{":2: Load statements should be at the top of the file."}, scopeBuild | scopeBzl)
+		[]string{":2: Load statements should be at the top of the file."}, scopeBuild|scopeBzl)
 
 	checkFindingsAndFix(t, "load-on-top", `
 """Docstring"""
@@ -451,7 +451,7 @@ bar()`,
 		[]string{
 			":9: Load statements should be at the top of the file.",
 			":15: Load statements should be at the top of the file.",
-		}, scopeBuild | scopeBzl)
+		}, scopeBuild|scopeBzl)
 }
 
 func TestOutOfOrderLoad(t *testing.T) {
@@ -469,7 +469,7 @@ b += 2
 load(":b.bzl", "b")
 a + b`,
 		[]string{":5: Load statement is out of its lexicographical order."},
-		scopeBuild | scopeBzl)
+		scopeBuild|scopeBzl)
 
 	checkFindingsAndFix(t, "out-of-order-load", `
 # b comment
@@ -487,7 +487,7 @@ load(":b.bzl", "b")
 load(":c.bzl", "c")
 a + b + c`,
 		[]string{":6: Load statement is out of its lexicographical order."},
-		scopeBuild | scopeBzl)
+		scopeBuild|scopeBzl)
 
 	checkFindingsAndFix(t, "out-of-order-load", `
 load(":a.bzl", "a")
@@ -503,17 +503,19 @@ load("//b:b.bzl", "b")
 load(":a.bzl", "a")
 load(":b.bzl", "b")
 `,
-		[]string{":2: Load statement is out of its lexicographical order.",
-			":4: Load statement is out of its lexicographical order."},
-		scopeBuild | scopeBzl)
+		[]string{
+			":2: Load statement is out of its lexicographical order.",
+			":3: Load statement is out of its lexicographical order.",
+			":6: Load statement is out of its lexicographical order.",
+		}, scopeBuild|scopeBzl)
 }
 
 func TestPositionalArguments(t *testing.T) {
 	checkFindings(t, "positional-args", `
 my_macro(foo = "bar")
 my_macro("foo", "bar")`,
 		[]string{":2: All calls to rules or macros should pass arguments by keyword (arg_name=value) syntax."},
-		scopeBuild | scopeWorkspace)
+		scopeBuild|scopeWorkspace)
 }
 
 func TestIntegerDivision(t *testing.T) {