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) }