From 6784cd657e8ded116ff4da721766d8fe1cc6f7e3 Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Wed, 3 Dec 2025 01:48:27 +0000 Subject: [PATCH] Move package() up when safe If there are assignments above package() calls they could contain variables used in the package() call. We now check for that case so we can move it as far up as possible. Fixes https://github.com/bazelbuild/buildtools/issues/1420 --- edit/fix.go | 73 ++++++++++++++++++++++++++++---------- edit/fix_test.go | 68 +++++++++++++++++++++++++++++++++++ warn/warn_cosmetic.go | 71 +++++++++++++++++++++++------------- warn/warn_cosmetic_test.go | 26 +++++++++++--- 4 files changed, 190 insertions(+), 48 deletions(-) diff --git a/edit/fix.go b/edit/fix.go index f45224aed..ecf95ad7d 100644 --- a/edit/fix.go +++ b/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 } diff --git a/edit/fix_test.go b/edit/fix_test.go index d7b8927f9..87b3c5a61 100644 --- a/edit/fix_test.go +++ b/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 { diff --git a/warn/warn_cosmetic.go b/warn/warn_cosmetic.go index 83a6f7037..a6b620017 100644 --- a/warn/warn_cosmetic.go +++ b/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,6 +70,19 @@ 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) @@ -76,29 +90,36 @@ func packageOnTopWarning(f *build.File) []*LinterFinding { 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 diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go index 06aa6c0be..2ade63e2c 100644 --- a/warn/warn_cosmetic_test.go +++ b/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) }