Move package() up when safe

edit/fix.go

@@ -408,36 +408,71 @@ func cleanUnusedLoads(f *build.File) bool {
 }
 
 // movePackageDeclarationToTheTop ensures that the call to package() is done
-// before everything else (except comments).
+// before everything else (except comments, docstrings, and loads).
 func movePackageDeclarationToTheTop(f *build.File) bool {
 	pkg := ExistingPackageDeclaration(f)
 	if pkg == nil {
 		return false
 	}
-	all := []build.Expr{}
-	inserted := false // true when the package declaration has been inserted
+
+	// Collect variables defined by assignment statements before package()
+	definedVars := make(map[string]bool)
 	for _, stmt := range f.Stmt {
-		_, isComment := stmt.(*build.CommentBlock)
-		_, isString := stmt.(*build.StringExpr)     // typically a docstring
-		_, isAssignExpr := stmt.(*build.AssignExpr) // e.g. variable declaration
-		_, isLoad := stmt.(*build.LoadStmt)
-		if isComment || isString || isAssignExpr || isLoad {
-			all = append(all, stmt)
-			continue
-		}
 		if stmt == pkg.Call {
-			if inserted {
-				// remove the old package
-				continue
+			break
+		}
+		if assign, ok := stmt.(*build.AssignExpr); ok {
+			if ident, ok := assign.LHS.(*build.Ident); ok {
+				definedVars[ident.Name] = true
 			}
-			return false // the file was ok
 		}
-		if !inserted {
-			all = append(all, pkg.Call)
-			inserted = true
+	}
+
+	// Check if the package() call uses any of the defined variables.
+	// If so, we can't as easily move it.
+	usedSymbols := UsedSymbols(pkg.Call)
+	for varName := range definedVars {
+		if usedSymbols[varName] {
+			return false
 		}
-		all = append(all, stmt)
 	}
+
+	var beforePkg []build.Expr
+	var afterPkg []build.Expr
+	seenPkg := false
+	pkgMisplaced := false
+
+	for _, stmt := range f.Stmt {
+		if stmt == pkg.Call {
+			seenPkg = true
+			continue
+		}
+		_, isComment := stmt.(*build.CommentBlock)
+		_, isString := stmt.(*build.StringExpr)
+		_, isLoad := stmt.(*build.LoadStmt)
+		_, isLicenses := ExprToRule(stmt, "licenses")
+		_, isPackageGroup := ExprToRule(stmt, "package_group")
+		shouldSkip := isComment || isString || isLoad || isLicenses || isPackageGroup
+
+		if shouldSkip {
+			beforePkg = append(beforePkg, stmt)
+		} else {
+			if !seenPkg {
+				pkgMisplaced = true
+			}
+			afterPkg = append(afterPkg, stmt)
+		}
+	}
+
+	if !pkgMisplaced {
+		return false
+	}
+
+	all := make([]build.Expr, 0, len(f.Stmt))
+	all = append(all, beforePkg...)
+	all = append(all, pkg.Call)
+	all = append(all, afterPkg...)
+
 	f.Stmt = all
 	return true
 }

edit/fix_test.go

@@ -72,6 +72,74 @@ load(":path.bzl", "x")
 foo()`,
 			false,
 		},
+		{`"""Docstring."""
+
+# Some comment 1
+
+load(":path.bzl", "some_target")
+
+# Some comment 2
+
+package_group(name = "my_group")
+
+licenses(["my_license"])
+
+some_target(name = "foo")
+
+some_list = []
+
+[
+    some_target(name = "x")
+    for x in some_list
+]
+
+package(default_visibility = ["//visibility:public"])`,
+			`"""Docstring."""
+
+# Some comment 1
+
+load(":path.bzl", "some_target")
+
+# Some comment 2
+
+package_group(name = "my_group")
+
+licenses(["my_license"])
+
+package(default_visibility = ["//visibility:public"])
+
+some_target(name = "foo")
+
+some_list = []
+
+[
+    some_target(name = "x")
+    for x in some_list
+]
+`,
+			true},
+		{`VISIBILITY = ["//visibility:public"]
+
+foo()
+
+package(default_visibility = VISIBILITY)`,
+			`VISIBILITY = ["//visibility:public"]
+
+foo()
+
+package(default_visibility = VISIBILITY)`,
+			false},
+		{`VISIBILITY = "//visibility:public"
+
+foo()
+
+package(default_visibility = [VISIBILITY])`,
+			`VISIBILITY = "//visibility:public"
+
+foo()
+
+package(default_visibility = [VISIBILITY])`,
+			false},
 	}
 
 	for i, tst := range tests {

warn/warn_cosmetic.go

@@ -37,18 +37,19 @@ func packageOnTopWarning(f *build.File) []*LinterFinding {
 		return nil
 	}
 
-	// Find the misplaced load statements
+	// Collect variables defined by assignment statements and find misplaced packages
 	misplacedPackages := make(map[int]*build.CallExpr)
-	firstStmtIndex := -1 // index of the first seen non string, comment or load statement
+	firstStmtIndex := -1 // index of the first seen non-skippable statement
+	definedVars := make(map[string]bool)
+
 	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
+		// Track variables defined by assignment statements that could be used in the package() call
+		if assign, ok := stmt.(*build.AssignExpr); ok {
+			if ident, ok := assign.LHS.(*build.Ident); ok {
+				definedVars[ident.Name] = true
+			}
 		}
 
 		_, isString := stmt.(*build.StringExpr) // typically a docstring
@@ -69,36 +70,56 @@ func packageOnTopWarning(f *build.File) []*LinterFinding {
 		if firstStmtIndex == -1 {
 			continue
 		}
+		// Check if the package() call uses any of the defined variables.
+		// If so, we can't easily move it.
+		usedSymbols := edit.UsedSymbols(rule.Call)
+		usesDefinedVar := false
+		for varName := range definedVars {
+			if usedSymbols[varName] {
+				usesDefinedVar = true
+				break
+			}
+		}
+		if usesDefinedVar {
+			continue
+		}
 		misplacedPackages[i] = rule.Call
 	}
 	offset := len(misplacedPackages)
 	if offset == 0 {
 		return nil
 	}
 
-	// Calculate a fix:
+	// Calculate a fix by building the new statement order
 	if firstStmtIndex == -1 {
 		firstStmtIndex = 0
 	}
-	var replacements []LinterReplacement
-	for i := range f.Stmt {
+
+	// Build lists of different statement types
+	var beforePkg []build.Expr // statements before firstStmtIndex (comments, docstrings, loads)
+	var packages []build.Expr  // misplaced package() calls
+	var afterPkg []build.Expr  // other statements (assignments, rules, macros)
+
+	for i, stmt := range f.Stmt {
 		if i < firstStmtIndex {
-			// Docstring or comment or load in the beginning, skip
+			beforePkg = append(beforePkg, stmt)
+			continue
+		}
+		if _, ok := misplacedPackages[i]; ok {
+			packages = append(packages, stmt)
 			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]})
 		}
+		afterPkg = append(afterPkg, stmt)
+	}
+
+	newOrder := make([]build.Expr, 0, len(f.Stmt))
+	newOrder = append(newOrder, beforePkg...)
+	newOrder = append(newOrder, packages...)
+	newOrder = append(newOrder, afterPkg...)
+
+	var replacements []LinterReplacement
+	for i := firstStmtIndex; i < len(f.Stmt); i++ {
+		replacements = append(replacements, LinterReplacement{&f.Stmt[i], newOrder[i]})
 	}
 
 	var findings []*LinterFinding

warn/warn_cosmetic_test.go

@@ -126,19 +126,37 @@ irrelevant = baz
 
 foo()
 
-package()`,
+package(default_visibility = ["//visibility:public"])`,
 		`
 """This is a docstring"""
 
 load(":foo.bzl", "foo")
 load(":bar.bzl", baz = "bar")
 
+package(default_visibility = ["//visibility:public"])
 irrelevant = baz
 
-foo()
+foo()`,
+		[]string{":10: 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)
 
-package()`,
-		[]string{},
+	// Similar test case from issue #1420: package() after a list assignment
+	checkFindingsAndFix(t,
+		"package-on-top",
+		`package_group(name = "my_group")
+
+some_target(name = "foo")
+
+some_list = []
+
+package(default_visibility = ["//visibility:public"])`,
+		`package_group(name = "my_group")
+
+package(default_visibility = ["//visibility:public"])
+some_target(name = "foo")
+
+some_list = []`,
+		[]string{":7: 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)
 }