Add Rewriter rule to position Package() after loads in BUILD file (#1138)

build/BUILD.bazel

@@ -40,14 +40,15 @@ go_test(
         "rule_test.go",
         "walk_test.go",
     ],
-    data = glob(["testdata/*"]) + [
+    data = glob([
+        "testdata/*",
+        "rewrite_test_files/*",
+    ]) + [
         # parse.y.go is checked in to satisfy the Go community
         # https://github.com/bazelbuild/buildtools/issues/14
         # this test ensures it doesn't get stale.
         "parse.y.baz.go",
         "parse.y.go",
-        "rewrite_test_files/original_formatted.star",
-        "rewrite_test_files/original.star",
     ],
     embed = [":go_default_library"],
     deps = [

build/rewrite.go

@@ -132,6 +132,7 @@ var rewrites = []struct {
 	{"formatdocstrings", formatDocstrings, scopeBoth},
 	{"reorderarguments", reorderArguments, scopeBoth},
 	{"editoctal", editOctals, scopeBoth},
+	{"reorderpackageafterloadbeforerules", reorderPackageAfterLoadBeforeRules, TypeBuild},
 }
 
 // leaveAlone reports whether any of the nodes on the stack are marked
@@ -1096,3 +1097,52 @@ func removeParens(f *File, _ *Rewriter) {
 
 	Edit(f, simplify)
 }
+
+// reorderPackageAfterLoadBeforeRules ensures a Package() only occurs after the last load().
+//
+// Note that this reordering is best effort and there may be already-invalid
+// BUILD.bazel file configurations (such as a load() after a rule) that result
+// in still-invalid output.
+//
+// Also note that we do not manipulate the Position{} structs within existing
+// expressions; only the order of statements in *File is modified. This should
+// be fine for printing (the only current usecase within this package), but
+// may cause confusion for future alternative usages of this Rewrite logic.
+func reorderPackageAfterLoadBeforeRules(f *File, _ *Rewriter) {
+	insertIndex := -1
+	insertIndexLocked := false
+	packageFuncIndex := -1
+	var packageFunc *CallExpr
+	for idx, s := range f.Stmt {
+		switch s := s.(type) {
+		case *LoadStmt, *CommentBlock, *StringExpr:
+			if !insertIndexLocked {
+				insertIndex = idx
+			}
+			continue
+		case *CallExpr:
+			if x, ok := s.X.(*Ident); ok && x.Name == "package" {
+				packageFunc = s
+				packageFuncIndex = idx
+			}
+		}
+		insertIndexLocked = true
+	}
+
+	if insertIndex == -1 || packageFuncIndex == -1 {
+		// no reordering necessary
+		return
+	}
+
+	// to get the package func after the last load, shift everything
+	// after the last load, up until the package func to make room.
+	//
+	// fun fact, the terminal condition on this loop can actually be
+	// > or >= and still result in correct output because the final
+	// packageFunc insertion would overright the final iteration
+	// in the latter case.
+	for i := packageFuncIndex - 1; i > insertIndex; i-- {
+		f.Stmt[i+1] = f.Stmt[i]
+	}
+	f.Stmt[insertIndex+1] = packageFunc
+}

build/rewrite_test.go

@@ -24,9 +24,9 @@ import (
 	"testing"
 )
 
-var workingDir string = path.Join(os.Getenv("TEST_SRCDIR"), os.Getenv("TEST_WORKSPACE"), "build")
-var originalFilePath = workingDir + "/rewrite_test_files/original.star"
-var formattedFilePath = workingDir + "/rewrite_test_files/original_formatted.star"
+var workingDir string = path.Join(os.Getenv("TEST_SRCDIR"), os.Getenv("TEST_WORKSPACE"), "build", "rewrite_test_files")
+var originalFilePath = path.Join(workingDir, "original.star")
+var formattedFilePath = path.Join(workingDir, "original_formatted.star")
 
 var originalBytes, _ = ioutil.ReadFile(originalFilePath)
 var originalFile, _ = ParseDefault(originalFilePath, originalBytes)
@@ -47,7 +47,8 @@ func TestRewriterRegular(t *testing.T) {
 	var modifyBytes, _ = ioutil.ReadFile(originalFilePath)
 	var modifiedFile, _ = ParseDefault(originalFilePath, modifyBytes)
 
-	// Perform rewrite on loaded file, rewrite should do nothing here
+	// Perform rewrite on loaded file, rewrite should do nothing here because the file
+	// has no needed modifations for the rewrites scoped to include scopeDefault.
 	Rewrite(modifiedFile)
 
 	// Initialize printers to obtain bytes later for different types of printers
@@ -60,7 +61,7 @@ func TestRewriterRegular(t *testing.T) {
 	originalPrinter.file(originalFile)
 	modifiedPrinter.file(modifiedFile)
 
-	// Assert that bytes to be writter are same as original bytes and different from formatted
+	// Assert that bytes to be written are same as original bytes and different from formatted
 	if !bytes.Equal(originalPrinter.Bytes(), modifiedPrinter.Bytes()) {
 		t.Error("Original Printer should equal Modified Printer")
 	}
@@ -74,7 +75,7 @@ func TestRewriterWithRewriter(t *testing.T) {
 	var modifyBytes, _ = ioutil.ReadFile(originalFilePath)
 	var modifiedFile, _ = ParseDefault(originalFilePath, modifyBytes)
 
-	// Perform rewrite with rewriter on loaded file, should proceed to reorder
+	// Perform rewrite with rewriter on loaded file, should proceed to reorder arguments
 	rewriter.Rewrite(modifiedFile)
 
 	// Initialize printers to obtain bytes later for different types of printers
@@ -95,3 +96,43 @@ func TestRewriterWithRewriter(t *testing.T) {
 		t.Error("Original Printer should not equal Modified Printer")
 	}
 }
+
+func TestBUILDFileOrderingRewriter(t *testing.T) {
+	originalFilePath := path.Join(workingDir, "ordering_test.original.bazel")
+	formattedFilePath := path.Join(workingDir, "ordering_test.formatted.bazel")
+
+	// Load the files as a BUILD
+	modifyBytes, _ := ioutil.ReadFile(originalFilePath)
+	modifiedFile, _ := ParseBuild(originalFilePath, modifyBytes)
+	originalBytes, _ := ioutil.ReadFile(originalFilePath)
+	originalFile, _ := ParseBuild(originalFilePath, originalBytes)
+	formattedBytes, _ := ioutil.ReadFile(formattedFilePath)
+	formattedFile, _ := ParseBuild(formattedFilePath, formattedBytes)
+
+	// Perform rewrite on loaded file, should proceed to reorder calls
+	Rewrite(modifiedFile)
+
+	// Initialize printers to obtain bytes later for different types of printers
+	// We will check bytes from printers because that is our source of truth before writing to new file
+	formattedPrinter := &printer{fileType: formattedFile.Type}
+	originalPrinter := &printer{fileType: originalFile.Type}
+	modifiedPrinter := &printer{fileType: modifiedFile.Type}
+
+	formattedPrinter.file(formattedFile)
+	originalPrinter.file(originalFile)
+	modifiedPrinter.file(modifiedFile)
+
+	// Assert that bytes to be written is same as formmatted bytes and different from original
+	if !bytes.Equal(formattedPrinter.Bytes(), modifiedPrinter.Bytes()) {
+		t.Errorf("Formatted Printer should equal Modified Printer\n\n\nmodified:\n%s\n\nformatted:\n%s\n\n",
+			modifiedPrinter.String(),
+			formattedPrinter.String(),
+		)
+	}
+	if bytes.Equal(originalPrinter.Bytes(), modifiedPrinter.Bytes()) {
+		t.Errorf("Original Printer should not equal Modified Printer\n\n\nmodified:\n%s\n\noriginal:\n%s\n\n",
+			modifiedPrinter.String(),
+			formattedPrinter.String(),
+		)
+	}
+}

build/rewrite_test_files/ordering_test.formatted.bazel

@@ -0,0 +1,20 @@
+"""
+Test file for rewrite.go
+"""
+# gazelle:load_directive
+load("//foo.bzl", "library_x")
+load("//keep.bzl", "library_keep") # keep
+
+load("//bar.bzl", "library_y")
+
+# gazelle:directive
+
+package(default_visibility="//:__subpackages__")
+
+library_x(
+	name = "default_name"
+)
+
+library_y(
+	name = "another_default_name"
+)

build/rewrite_test_files/ordering_test.original.bazel

@@ -0,0 +1,20 @@
+"""
+Test file for rewrite.go
+"""
+# gazelle:load_directive
+load("//foo.bzl", "library_x")
+load("//keep.bzl", "library_keep") # keep
+
+load("//bar.bzl", "library_y")
+
+# gazelle:directive
+
+library_x(
+	name = "default_name"
+)
+
+package(default_visibility="//:__subpackages__")
+
+library_y(
+	name = "another_default_name"
+)