Repository: bazelbuild/buildtools
Pull request #1200 (2023-09-28 15:31:29)

Make package on top apply fix
Currently the package-on-top buildifier lint rule only shows a warning but is unable to offer a fix.
This PR addresses this issue by essentially cloning the behavior of the load-on-top rule, but placing the package statement after the load statements (as well as any docstrings or comments).
I updated the test to use checkFindingsAndFix as well and it passes.

AttilaTheFun: Thanks @vladmos ! I incorporated your suggestions and pushed the commit. I don't think I have permission to merge it, so if you could do so whenever you have a chance that would be great.
vladmos: I'll merge the pull request in a few days, I want to do it separately from other pull requests to make sure it doesn't break any internal projects.

Patch:
diff --git a/warn/warn_cosmetic.go b/warn/warn_cosmetic.go
index 08ae8e7..f618a52 100644
--- a/warn/warn_cosmetic.go
+++ b/warn/warn_cosmetic.go
@@ -102,30 +102,78 @@ func sameOriginLoadWarning(f *build.File) []*LinterFinding {
 	return findings
 }
 
+// packageOnTopWarning hoists package statements to the top after any comments / docstrings / load statments.
+// If applied together with loadOnTopWarning and/or outOfOrderLoadWarning, it should be applied after them.
+// This is currently guaranteed by sorting the warning categories names before applying them:
+// "load-on-top" < "out-of-order-load" < "package-on-top"
 func packageOnTopWarning(f *build.File) []*LinterFinding {
-	seenRule := false
-	for _, stmt := range f.Stmt {
+	if f.Type == build.TypeWorkspace || f.Type == build.TypeModule {
+		// Not applicable to WORKSPACE or MODULE files
+		return nil
+	}
+
+	// Find the misplaced load statements
+	misplacedPackages := make(map[int]*build.CallExpr)
+	firstStmtIndex := -1 // index of the first seen non string, comment or load statement
+	for i := 0; i < len(f.Stmt); i++ {
+		stmt := f.Stmt[i]
 		_, isString := stmt.(*build.StringExpr) // typically a docstring
 		_, isComment := stmt.(*build.CommentBlock)
-		_, isAssignExpr := stmt.(*build.AssignExpr) // e.g. variable declaration
 		_, isLoad := stmt.(*build.LoadStmt)
-		_, isPackageGroup := edit.ExprToRule(stmt, "package_group")
-		_, isLicense := edit.ExprToRule(stmt, "licenses")
-		if isString || isComment || isAssignExpr || isLoad || isPackageGroup || isLicense || stmt == nil {
+		if isString || isComment || isLoad || stmt == nil {
 			continue
 		}
-		if rule, ok := edit.ExprToRule(stmt, "package"); ok {
-			if !seenRule { // OK: package is on top of the file
-				return nil
+		rule, ok := edit.ExprToRule(stmt, "package")
+		if !ok {
+			if firstStmtIndex == -1 {
+				firstStmtIndex = i
 			}
-			return []*LinterFinding{makeLinterFinding(rule.Call,
-				"Package declaration should be at the top of the file, after the load() statements, "+
-					"but before any call to a rule or a macro. "+
-					"package_group() and licenses() may be called before package().")}
+			continue
+		}
+		if firstStmtIndex == -1 {
+			continue
 		}
-		seenRule = true
+		misplacedPackages[i] = rule.Call
+	}
+	offset := len(misplacedPackages)
+	if offset == 0 {
+		return nil
 	}
-	return nil
+
+	// Calculate a fix:
+	if firstStmtIndex == -1 {
+		firstStmtIndex = 0
+	}
+	var replacements []LinterReplacement
+	for i := range f.Stmt {
+		if i < firstStmtIndex {
+			// Docstring or comment or load in the beginning, skip
+			continue
+		} else if _, ok := misplacedPackages[i]; ok {
+			// A misplaced load statement, should be moved up to the `firstStmtIndex` position
+			replacements = append(replacements, LinterReplacement{&f.Stmt[firstStmtIndex], f.Stmt[i]})
+			firstStmtIndex++
+			offset--
+			if offset == 0 {
+				// No more statements should be moved
+				break
+			}
+		} else {
+			// An actual statement (not a docstring or a comment in the beginning), should be moved
+			// `offset` positions down.
+			replacements = append(replacements, LinterReplacement{&f.Stmt[i+offset], f.Stmt[i]})
+		}
+	}
+
+	var findings []*LinterFinding
+	for _, load := range misplacedPackages {
+		findings = append(findings, makeLinterFinding(load,
+			"Package declaration should be at the top of the file, after the load() statements, "+
+				"but before any call to a rule or a macro. "+
+				"package_group() and licenses() may be called before package().", replacements...))
+	}
+
+	return findings
 }
 
 func loadOnTopWarning(f *build.File) []*LinterFinding {


Test patch:
diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go
index 7286244..a32bd60 100644
--- a/warn/warn_cosmetic_test.go
+++ b/warn/warn_cosmetic_test.go
@@ -118,13 +118,20 @@ func TestWarnSameOriginLoad(t *testing.T) {
 }
 
 func TestPackageOnTop(t *testing.T) {
-	checkFindings(t, "package-on-top", `
+	checkFindingsAndFix(t,
+		"package-on-top",
+		`
 my_macro(name = "foo")
 package()`,
+		`
+package()
+my_macro(name = "foo")`,
 		[]string{":2: Package declaration should be at the top of the file, after the load() statements, but before any call to a rule or a macro. package_group() and licenses() may be called before package()."},
-		scopeEverywhere)
+		scopeDefault|scopeBzl|scopeBuild)
 
-	checkFindings(t, "package-on-top", `
+	checkFindingsAndFix(t,
+		"package-on-top",
+		`
 # Some comments
 
 """This is a docstring"""
@@ -134,10 +141,20 @@ load(":bar.bzl", baz = "bar")
 
 package()
 
-foo(baz)
-`,
+foo(baz)`,
+		`
+# Some comments
+
+"""This is a docstring"""
+
+load(":foo.bzl", "foo")
+load(":bar.bzl", baz = "bar")
+
+package()
+
+foo(baz)`,
 		[]string{},
-		scopeEverywhere)
+		scopeDefault|scopeBzl|scopeBuild)
 }
 
 func TestLoadOnTop(t *testing.T) {