Repository: bazelbuild/buildtools
Pull request #1214 (2023-11-03 19:59:58)

Fix false positive warnings for package on top
A package declaration may be not at the top of the file if it depends on some variables defined earlier:

    VISIBILITY = [...]

    package(
        ...
        default_visibility = VISIBILITY,
    )

This dependance may also be indirect and it may be hard to determine whether the package declaration can safely be moved on top, and, if so, together with which assignment expressions. To avoid false positive warnings, with this buildifier will just suppress package-on-top warnings in case there's any assignment expression above package declarations. If the visibility scope in the example above has been loaded from another file it wouldn't have cause a similar issue because load statements come before package declarations by design (which is also enforced by other warnings) and loaded symbols are frozen and can't be mutated in the current file by other types of statements (non-assignment statements, e.g. `VISIBILITY.append("bar")` to modify the visibility before using it in the package declaration is not valid anyway).


Patch:
diff --git a/warn/warn_cosmetic.go b/warn/warn_cosmetic.go
index 9061619..04dfaa2 100644
--- a/warn/warn_cosmetic.go
+++ b/warn/warn_cosmetic.go
@@ -117,6 +117,15 @@ func packageOnTopWarning(f *build.File) []*LinterFinding {
 	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]
+
+		// Assign statements may define variables that are used by the package statement,
+		// e.g. visibility declarations. To avoid false positive detections and also
+		// for keeping things simple, the warning should be just suppressed if there's
+		// any assignment statement, even if it's not used by the package declaration.
+		if _, ok := stmt.(*build.AssignExpr); ok {
+			break
+		}
+
 		_, isString := stmt.(*build.StringExpr) // typically a docstring
 		_, isComment := stmt.(*build.CommentBlock)
 		_, isLoad := stmt.(*build.LoadStmt)
@@ -171,8 +180,8 @@ func packageOnTopWarning(f *build.File) []*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...))
+					"but before any call to a rule or a macro. "+
+					"package_group() and licenses() may be called before package().", replacements...))
 	}
 
 	return findings


Test patch:
diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go
index 0009ca6..81d9c13 100644
--- a/warn/warn_cosmetic_test.go
+++ b/warn/warn_cosmetic_test.go
@@ -185,6 +185,60 @@ package()
 foo(baz)`,
 		[]string{":11: 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()."},
 		scopeDefault|scopeBzl|scopeBuild)
+
+	checkFindingsAndFix(t,
+		"package-on-top",
+		`
+"""This is a docstring"""
+
+load(":foo.bzl", "foo")
+load(":bar.bzl", baz = "bar")
+
+VISIBILITY = baz
+
+foo()
+
+package(default_visibility = VISIBILITY)`,
+		`
+"""This is a docstring"""
+
+load(":foo.bzl", "foo")
+load(":bar.bzl", baz = "bar")
+
+VISIBILITY = baz
+
+foo()
+
+package(default_visibility = VISIBILITY)`,
+		[]string{},
+		scopeDefault|scopeBzl|scopeBuild)
+
+	checkFindingsAndFix(t,
+		"package-on-top",
+		`
+"""This is a docstring"""
+
+load(":foo.bzl", "foo")
+load(":bar.bzl", baz = "bar")
+
+irrelevant = baz
+
+foo()
+
+package()`,
+		`
+"""This is a docstring"""
+
+load(":foo.bzl", "foo")
+load(":bar.bzl", baz = "bar")
+
+irrelevant = baz
+
+foo()
+
+package()`,
+		[]string{},
+		scopeDefault|scopeBzl|scopeBuild)
 }
 
 func TestLoadOnTop(t *testing.T) {