Repository: bazelbuild/buildtools
Pull request #440 (2018-11-20 19:01:59)

[buildifier] Warn about and fix non-lexicographic load order.
This makes it easier to developers to locate loads, but more
importantly reduces changes for large-scale automated refactorings
to result in merge conflicts, since they usually add loads at the
end or some other fixed position.

ttsugriy: That is an awesome point, @vladmos! I have special-cased `@` and applied your great stylistic suggestions.

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 2732c73..b24db3b 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -367,6 +367,21 @@ they can follow only comments and docstrings.
 
 --------------------------------------------------------------------------------
 
+## <a name="load-out-of-order"></a>Load statements should be ordered by their labels.
+
+  * Category_name: `load-out-of-order`
+  * Automatic fix: yes
+
+Load statements should be ordered by their first argument - extension file label.
+This makes it easier to developers to locate loads of interest and reduces chances
+for conflicts when performing large-scale automated refactoring.
+
+When applying automated fixes, it's highly recommended to also use
+<a name="load-on-top">load-on-top</a> fixes, since otherwise the relative order
+of a symbol load and its usage can change resulting in runtime error.
+
+--------------------------------------------------------------------------------
+
 ## <a name="filetype"></a>The `FileType` function is deprecated
 
   * Category_name: `filetype`
diff --git a/warn/warn.go b/warn/warn.go
index 4adf6ae..dd8762d 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -333,6 +333,48 @@ func loadOnTopWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
+func outOfOrderLoadWarning(f *build.File, fix bool) []*Finding {
+	findings := []*Finding{}
+	sortedLoads := []*build.LoadStmt{}
+	for i := 0; i < len(f.Stmt); i++ {
+		load, ok := f.Stmt[i].(*build.LoadStmt)
+		if !ok {
+			continue
+		}
+		sortedLoads = append(sortedLoads, load)
+	}
+	if fix {
+		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
+		})
+		sortedLoadIndex := 0
+		for globalLoadIndex := 0; globalLoadIndex < len(f.Stmt); globalLoadIndex++ {
+			if _, ok := f.Stmt[globalLoadIndex].(*build.LoadStmt); !ok {
+				continue
+			}
+			f.Stmt[globalLoadIndex] = sortedLoads[sortedLoadIndex]
+			sortedLoadIndex++
+		}
+		return findings
+	}
+
+	for i := 1; i < len(sortedLoads); i++ {
+		if sortedLoads[i].Module.Value < sortedLoads[i-1].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))
+		}
+	}
+	return findings
+}
+
 func integerDivisionWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
@@ -819,6 +861,7 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"native-build":        nativeInBuildFilesWarning,
 	"native-package":      nativePackageWarning,
 	"no-effect":           noEffectWarning,
+	"out-of-order-load":   outOfOrderLoadWarning,
 	"output-group":        outputGroupWarning,
 	"package-name":        packageNameWarning,
 	"package-on-top":      packageOnTopWarning,


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 5badeb2..2457944 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -451,6 +451,60 @@ bar()`,
 		}, scopeBzl)
 }
 
+func TestOutOfOrderLoad(t *testing.T) {
+	checkFindingsAndFix(t, "out-of-order-load", `
+# b comment
+load(":b.bzl", "b")
+b += 2
+# a comment
+load(":a.bzl", "a")
+a + b`, `
+# a comment
+load(":a.bzl", "a")
+b += 2
+# b comment
+load(":b.bzl", "b")
+a + b`,
+		[]string{":5: Load statement is out of its lexicographical order."},
+		scopeEverywhere)
+
+	checkFindingsAndFix(t, "out-of-order-load", `
+# b comment
+load(":b.bzl", "b")
+# c comment
+load(":c.bzl", "c")
+# a comment
+load(":a.bzl", "a")
+a + b + c`, `
+# a comment
+load(":a.bzl", "a")
+# b comment
+load(":b.bzl", "b")
+# c comment
+load(":c.bzl", "c")
+a + b + c`,
+		[]string{":6: Load statement is out of its lexicographical order."},
+		scopeEverywhere)
+
+	checkFindingsAndFix(t, "out-of-order-load", `
+load(":a.bzl", "a")
+load("//a:a.bzl", "a")
+load("@a//a:a.bzl", "a")
+load("//b:b.bzl", "b")
+load(":b.bzl", "b")
+load("@b//b:b.bzl", "b")`, `
+load("@a//a:a.bzl", "a")
+load("@b//b:b.bzl", "b")
+load("//a:a.bzl", "a")
+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."},
+		scopeEverywhere)
+}
+
 func TestPositionalArguments(t *testing.T) {
 	checkFindings(t, "positional-args", `
 my_macro(foo = "bar")