diff --git a/warn/warn.go b/warn/warn.go index 26e16792e..f1c0197d5 100644 --- a/warn/warn.go +++ b/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)) diff --git a/warn/warn_test.go b/warn/warn_test.go index 3c8530202..f1ba13924 100644 --- a/warn/warn_test.go +++ b/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,9 +503,11 @@ 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) { @@ -513,7 +515,7 @@ func TestPositionalArguments(t *testing.T) { 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) {