diff --git a/WARNINGS.md b/WARNINGS.md index 0d6270449..0733a8805 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -315,7 +315,7 @@ the [`function-docstring`](#function-docstring) warning. * [Suppress the warning](#suppress): `# buildifier: disable=depset-items` The `items` parameter for [`depset`](https://docs.bazel.build/versions/master/skylark/lib/globals.html#depset) -is deprecated. In it's old form it's either a list of direct elements to be +is deprecated. In its old form it's either a list of direct elements to be added (use the `direct` or unnamed first parameter instead) or a depset that becomes a transitive element of the new depset (use the `transitive` parameter instead). @@ -611,8 +611,11 @@ If you want to keep the load, you can disable the warning by adding a comment * Category name: `load-on-top` * Flag in Bazel: [`--incompatible_bzl_disallow_load_after_statement`](https://github.com/bazelbuild/bazel/issues/5815) * Automatic fix: yes + * Not supported by the latest version of Buildifier * [Suppress the warning](#suppress): `# buildifier: disable=load-on-top` +Obsolete; the warning has been implemented in the formatter and the fix is now automatically applied to all files except `WORKSPACE` files (unless suppressed). + Load statements should be first statements (with the exception of `WORKSPACE` files), they can follow only comments and docstrings. @@ -765,8 +768,11 @@ The statement has no effect. Consider removing it or storing its result in a var * Category name: `out-of-order-load` * Automatic fix: yes + * Not supported by the latest version of Buildifier * [Suppress the warning](#suppress): `# buildifier: disable=out-of-order-load` +Obsolete; the warning has been implemented in the formatter and the fix is now automatically applied to all files (unless suppressed). + Load statements should be ordered by their first argument - extension file label. This makes it easier to developers to locate loads of interest and reduces chances for conflicts when performing large-scale automated refactoring. @@ -1016,8 +1022,11 @@ or lists of providers instead. * Category name: `same-origin-load` * Automatic fix: yes + * Not supported by the latest version of Buildifier * [Suppress the warning](#suppress): `# buildifier: disable=same-origin-load` +Obsolete; the warning has been implemented in the formatter and the fix is now automatically applied to all files except `WORKSPACE` files (unless suppressed). + ### Background [load](https://docs.bazel.build/versions/master/skylark/concepts.html#loading-an-extension) diff --git a/build/rewrite.go b/build/rewrite.go index 906e3bcc7..30a766d20 100644 --- a/build/rewrite.go +++ b/build/rewrite.go @@ -28,7 +28,7 @@ import ( "github.com/bazelbuild/buildtools/tables" ) -// For debugging: flag to disable certain rewrites. +// DisableRewrites disables certain rewrites (for debugging). var DisableRewrites []string // disabled reports whether the named rewrite is disabled. @@ -41,7 +41,7 @@ func disabled(name string) bool { return false } -// For debugging: allow sorting of these lists even with sorting otherwise disabled. +// AllowSort allows sorting of these lists even with sorting otherwise disabled (for debugging). var AllowSort []string // allowedSort reports whether sorting is allowed in the named context. @@ -71,6 +71,7 @@ type Rewriter struct { ShortenAbsoluteLabelsToRelative bool } +// Rewrite applies rewrites to a file func Rewrite(f *File) { var rewriter = &Rewriter{ IsLabelArg: tables.IsLabelArg, @@ -127,6 +128,9 @@ var rewrites = []struct { {"label", fixLabels, scopeBuild}, {"listsort", sortStringLists, scopeBoth}, {"multiplus", fixMultilinePlus, scopeBuild}, + {"loadTop", moveLoadOnTop, scopeBoth}, + {"sameOriginLoad", compressSameOriginLoads, scopeBoth}, + {"sortLoadStatements", sortLoadStatements, scopeBoth}, {"loadsort", sortAllLoadArgs, scopeBoth}, {"useRepoPositionalsSort", sortUseRepoPositionals, TypeModule}, {"formatdocstrings", formatDocstrings, scopeBoth}, @@ -159,9 +163,33 @@ func hasComment(x Expr, text string) bool { return true } } + for _, com := range x.Comment().After { + if strings.Contains(strings.ToLower(com.Token), text) { + return true + } + } + for _, com := range x.Comment().Suffix { + if strings.Contains(strings.ToLower(com.Token), text) { + return true + } + } return false } +// isCommentAnywhere checks whether there's a comment containing the given text +// anywhere in the file. +func isCommentAnywhere(f *File, text string) bool { + commentExists := false + WalkInterruptable(f, func(node Expr, stack []Expr) (err error) { + if hasComment(node, text) { + commentExists = true + return &StopTraversalError{} + } + return nil + }) + return commentExists +} + // leaveAlone1 reports whether x is marked with a comment containing // "buildifier: leave-alone", case-insensitive. func leaveAlone1(x Expr) bool { @@ -460,8 +488,8 @@ func sortStringLists(f *File, w *Rewriter) { continue } if w.IsSortableListArg[key.Name] || - w.SortableAllowlist[context] || - (!disabled("unsafesort") && allowedSort(context)) { + w.SortableAllowlist[context] || + (!disabled("unsafesort") && allowedSort(context)) { if doNotSort(as) { deduplicateStringList(as.RHS) } else { @@ -911,6 +939,245 @@ func fixMultilinePlus(f *File, _ *Rewriter) { }) } +// isFunctionCall checks whether expr is a call of a function with a given name +func isFunctionCall(expr Expr, name string) bool { + call, ok := expr.(*CallExpr) + if !ok { + return false + } + if ident, ok := call.X.(*Ident); ok && ident.Name == name { + return true + } + return false +} + +// moveLoadOnTop moves all load statements to the top of the file +func moveLoadOnTop(f *File, _ *Rewriter) { + if f.Type == TypeWorkspace { + // Moving load statements in Workspace files can break the semantics + return + } + if isCommentAnywhere(f, "disable=load-on-top") { + // For backward compatibility. This rewrite used to be a suppressible warning, + // in some cases it's hard to maintain the position of load statements (e.g. + // when the file is automatically generated or has automatic transformations + // applied to it). The rewrite checks for the comment anywhere in the file + // because it's hard to determine which statement is out of order. + return + } + + // Find the misplaced load statements + misplacedLoads := make(map[int]*LoadStmt) + firstStmtIndex := -1 // index of the first seen non-load statement + for i := 0; i < len(f.Stmt); i++ { + stmt := f.Stmt[i] + _, isString := stmt.(*StringExpr) // typically a docstring + _, isComment := stmt.(*CommentBlock) + isWorkspace := isFunctionCall(stmt, "workspace") + if isString || isComment || isWorkspace || stmt == nil { + continue + } + load, ok := stmt.(*LoadStmt) + if !ok { + if firstStmtIndex == -1 { + firstStmtIndex = i + } + continue + } + if firstStmtIndex == -1 { + continue + } + misplacedLoads[i] = load + } + + // Calculate a fix + if firstStmtIndex == -1 { + firstStmtIndex = 0 + } + offset := len(misplacedLoads) + if offset == 0 { + return + } + stmtCopy := append([]Expr{}, f.Stmt...) + for i := range f.Stmt { + if i < firstStmtIndex { + // Docstring or a comment in the beginning, skip + continue + } else if _, ok := misplacedLoads[i]; ok { + // A misplaced load statement, should be moved up to the `firstStmtIndex` position + f.Stmt[firstStmtIndex] = stmtCopy[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. + f.Stmt[i+offset] = stmtCopy[i] + } + } +} + +// compressSameOriginLoads merges loads from the same source into one load statement +func compressSameOriginLoads(f *File, _ *Rewriter) { + // Load statements grouped by their source files + loads := make(map[string]*LoadStmt) + + for stmtIndex := 0; stmtIndex < len(f.Stmt); stmtIndex++ { + load, ok := f.Stmt[stmtIndex].(*LoadStmt) + if !ok { + continue + } + + previousLoad, ok := loads[load.Module.Value] + if !ok { + loads[load.Module.Value] = load + continue + } + if hasComment(previousLoad, "disable=same-origin-load") || + hasComment(load, "disable=same-origin-load") { + continue + } + + // Move loaded symbols to the existing load statement + previousLoad.From = append(previousLoad.From, load.From...) + previousLoad.To = append(previousLoad.To, load.To...) + // Remove the current statement + f.Stmt[stmtIndex] = nil + } +} + +// comparePaths compares two strings as if they were paths (the path delimiter, +// '/', should be treated as smallest symbol). +func comparePaths(path1, path2 string) bool { + if path1 == path2 { + return false + } + + chunks1 := strings.Split(path1, "/") + chunks2 := strings.Split(path2, "/") + + for i, chunk1 := range chunks1 { + if i >= len(chunks2) { + return false + } + chunk2 := chunks2[i] + // Compare case-insensitively + chunk1Lower := strings.ToLower(chunk1) + chunk2Lower := strings.ToLower(chunk2) + if chunk1Lower != chunk2Lower { + return chunk1Lower < chunk2Lower + } + } + + // No case-insensitive difference detected. Likely the difference is just in + // the case of some symbols, compare case-sensitively for the determinism. + return path1 <= path2 +} + +// compareLoadLabels compares two module names +// If one label has explicit repository path (starts with @), it goes first +// If the packages are different, labels are sorted by package name (empty package goes first) +// If the packages are the same, labels are sorted by their name +func compareLoadLabels(load1Label, load2Label string) bool { + // handle absolute labels with explicit repositories separately to + // make sure they precede absolute and relative labels without repos + isExplicitRepo1 := strings.HasPrefix(load1Label, "@") + isExplicitRepo2 := strings.HasPrefix(load2Label, "@") + + if isExplicitRepo1 != isExplicitRepo2 { + // Exactly one label has an explicit repository name, it should be the first one. + return isExplicitRepo1 + } + + // Either both labels have explicit repository names or both don't, compare their packages + // and break ties using file names if necessary + module1Parts := strings.SplitN(load1Label, ":", 2) + package1, filename1 := "", module1Parts[0] + if len(module1Parts) == 2 { + package1, filename1 = module1Parts[0], module1Parts[1] + } + module2Parts := strings.SplitN(load2Label, ":", 2) + package2, filename2 := "", module2Parts[0] + if len(module2Parts) == 2 { + package2, filename2 = module2Parts[0], module2Parts[1] + } + + // in case both packages are the same, use file names to break ties + if package1 == package2 { + return comparePaths(filename1, filename2) + } + + // in case one of the packages is empty, the empty one goes first + if len(package1) == 0 || len(package2) == 0 { + return len(package1) > 0 + } + + // both packages are non-empty and not equal, so compare them + return comparePaths(package1, package2) +} + +// sortLoadStatements reorders sorts loads lexicographically by the source file, +// but absolute loads have priority over local loads. +func sortLoadStatements(f *File, _ *Rewriter) { + if isCommentAnywhere(f, "disable=out-of-order-load") { + // For backward compatibility. This rewrite used to be a suppressible warning, + // in some cases it's hard to maintain the position of load statements (e.g. + // when the file is automatically generated or has automatic transformations + // applied to it). The rewrite checks for the comment anywhere in the file + // because it's hard to determine which statement is out of order. + return + } + + // Consequent chunks of load statements (i.e. without statements of other types between them) + var loadsChunks [][]*LoadStmt + newChunk := true // Create a new chunk for the next seen load statement + + for _, stmt := range f.Stmt { + if stmt == nil { + // nil statements are no-op and shouldn't break consecutive chains of + // load statements + continue + } + load, ok := stmt.(*LoadStmt) + if !ok { + newChunk = true + continue + } + if newChunk { + // There's a non-load statement between this load and the previous load + loadsChunks = append(loadsChunks, []*LoadStmt{}) + newChunk = false + } + loadsChunks[len(loadsChunks)-1] = append(loadsChunks[len(loadsChunks)-1], load) + } + + // Sort and flatten the chunks + var sortedLoads []*LoadStmt + for _, chunk := range loadsChunks { + sortedChunk := append([]*LoadStmt{}, chunk...) + + sort.SliceStable(sortedChunk, func(i, j int) bool { + load1Label := (sortedChunk)[i].Module.Value + load2Label := (sortedChunk)[j].Module.Value + return compareLoadLabels(load1Label, load2Label) + }) + sortedLoads = append(sortedLoads, sortedChunk...) + } + + // Calculate the replacements + loadIndex := 0 + for stmtIndex := range f.Stmt { + if _, ok := f.Stmt[stmtIndex].(*LoadStmt); !ok { + continue + } + f.Stmt[stmtIndex] = sortedLoads[loadIndex] + loadIndex++ + } +} + // sortAllLoadArgs sorts all load arguments in the file func sortAllLoadArgs(f *File, _ *Rewriter) { Walk(f, func(v Expr, stk []Expr) { @@ -973,17 +1240,17 @@ type loadArgs struct { modified bool } -func (args loadArgs) Len() int { +func (args *loadArgs) Len() int { return len(args.From) } -func (args loadArgs) Swap(i, j int) { +func (args *loadArgs) Swap(i, j int) { args.From[i], args.From[j] = args.From[j], args.From[i] args.To[i], args.To[j] = args.To[j], args.To[i] args.modified = true } -func (args loadArgs) Less(i, j int) bool { +func (args *loadArgs) Less(i, j int) bool { // Arguments with equal "from" and "to" parts are prioritized equalI := args.From[i].Name == args.To[i].Name equalJ := args.From[j].Name == args.To[j].Name @@ -995,10 +1262,33 @@ func (args loadArgs) Less(i, j int) bool { return args.To[i].Name < args.To[j].Name } +// deduplicate assumes that the args are sorted and prioritize arguments that +// are defined later (e.g. `a = "x", a = "y"` is shortened to `a = "y"`). +func (args *loadArgs) deduplicate() { + var from, to []*Ident + for i := range args.To { + if len(to) > 0 && to[len(to)-1].Name == args.To[i].Name { + // Overwrite + from[len(from)-1] = args.From[i] + to[len(to)-1] = args.To[i] + args.modified = true + } else { + // Just append + from = append(from, args.From[i]) + to = append(to, args.To[i]) + } + } + args.From = from + args.To = to +} + // SortLoadArgs sorts a load statement arguments (lexicographically, but positional first) func SortLoadArgs(load *LoadStmt) bool { args := loadArgs{From: load.From, To: load.To} - sort.Sort(args) + sort.Sort(&args) + args.deduplicate() + load.From = args.From + load.To = args.To return args.modified } diff --git a/build/testdata/055.golden b/build/testdata/055.golden index 69bc2c8e9..174bb00a0 100644 --- a/build/testdata/055.golden +++ b/build/testdata/055.golden @@ -1,12 +1,12 @@ -load(":foo.bzl", "a", "d", "foo", b = "c", c = "b") load(":bar.bzl", "bar") +load(":baz.bzl", "baz") +# after-comment + +load(":foo.bzl", "a", "d", "foo", b = "c", c = "b") load( ":foobar.bzl", "foobar", ) -load(":baz.bzl", "baz") -# after-comment - load( # 0 "foobar.bzl", # 1 diff --git a/build/testdata/067.build.golden b/build/testdata/067.build.golden index 6a8bacd3e..50d698f17 100644 --- a/build/testdata/067.build.golden +++ b/build/testdata/067.build.golden @@ -1,10 +1,10 @@ +load(":a", "b") load( # A - "foo.bzl", + ":foo.bzl", # B "bar", ) -load(":a", "b") cc_binary( # A diff --git a/build/testdata/067.bzl.golden b/build/testdata/067.bzl.golden index 2b5f63ec1..d2f3a0bee 100644 --- a/build/testdata/067.bzl.golden +++ b/build/testdata/067.bzl.golden @@ -1,10 +1,10 @@ +load(":a", "b") load( # A - "foo.bzl", + ":foo.bzl", # B "bar", ) -load(":a", "b") cc_binary( # A diff --git a/build/testdata/067.in b/build/testdata/067.in index c80f0396c..ca7f8f31c 100644 --- a/build/testdata/067.in +++ b/build/testdata/067.in @@ -1,7 +1,7 @@ load(,,, # A , - "foo.bzl",,, + ":foo.bzl",,, # B , "bar",,, diff --git a/build/testdata/071.golden b/build/testdata/071.golden new file mode 100644 index 000000000..f96604111 --- /dev/null +++ b/build/testdata/071.golden @@ -0,0 +1,7 @@ +foo() + +# buildifier: disable=load-on-top + +load(":foo", "bar") + +bar() diff --git a/build/testdata/071.in b/build/testdata/071.in new file mode 100644 index 000000000..f96604111 --- /dev/null +++ b/build/testdata/071.in @@ -0,0 +1,7 @@ +foo() + +# buildifier: disable=load-on-top + +load(":foo", "bar") + +bar() diff --git a/build/testdata/072.golden b/build/testdata/072.golden new file mode 100644 index 000000000..d6aa84d25 --- /dev/null +++ b/build/testdata/072.golden @@ -0,0 +1,5 @@ +load(":a.bzl", "one") # disable=same-origin-load +load(":a.bzl", "two") +load(":b.bzl", "one") +load(":b.bzl", "two") # buildifier: disable=same-origin-load +load(":c.bzl", "one", "two", "x", y = "four") diff --git a/build/testdata/072.in b/build/testdata/072.in new file mode 100644 index 000000000..87bb35d2d --- /dev/null +++ b/build/testdata/072.in @@ -0,0 +1,6 @@ +load(":a.bzl", "one") # disable=same-origin-load +load(":a.bzl", "two") +load(":b.bzl", "one") +load(":b.bzl", "two") # buildifier: disable=same-origin-load +load(":c.bzl", "one", "x", y = "three") +load(":c.bzl", "x", "two", y = "four") diff --git a/build/testdata/073.golden b/build/testdata/073.golden new file mode 100644 index 000000000..2822b02c4 --- /dev/null +++ b/build/testdata/073.golden @@ -0,0 +1,4 @@ +# disable=out-of-order-load + +load(":foo", "foo") +load(":bar", "bar") diff --git a/build/testdata/073.in b/build/testdata/073.in new file mode 100644 index 000000000..2822b02c4 --- /dev/null +++ b/build/testdata/073.in @@ -0,0 +1,4 @@ +# disable=out-of-order-load + +load(":foo", "foo") +load(":bar", "bar") diff --git a/build/testdata/074.golden b/build/testdata/074.golden new file mode 100644 index 000000000..ee9e49ad5 --- /dev/null +++ b/build/testdata/074.golden @@ -0,0 +1,4 @@ +load("bar", "baz") +load("foo", "x", "y", "z") + +package() diff --git a/build/testdata/074.in b/build/testdata/074.in new file mode 100644 index 000000000..aa0eb8360 --- /dev/null +++ b/build/testdata/074.in @@ -0,0 +1,5 @@ +package() + +load("foo", "x", "y") +load("foo", "x", "z") +load("bar", "baz") diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index 741dc05ec..86f998273 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -75,7 +75,6 @@ func ExampleExample() { // "keyword-positional-params", // "list-append", // "load", - // "load-on-top", // "module-docstring", // "name-conventions", // "native-android", @@ -86,7 +85,6 @@ func ExampleExample() { // "native-proto", // "native-py", // "no-effect", - // "out-of-order-load", // "output-group", // "overly-nested-depset", // "package-name", @@ -98,7 +96,6 @@ func ExampleExample() { // "repository-name", // "return-value", // "rule-impl-return", - // "same-origin-load", // "skylark-comment", // "skylark-docstring", // "string-iteration", @@ -259,7 +256,6 @@ func TestValidate(t *testing.T) { "keyword-positional-params", "list-append", "load", - "load-on-top", "module-docstring", "name-conventions", "native-android", @@ -270,7 +266,6 @@ func TestValidate(t *testing.T) { "native-proto", "native-py", "no-effect", - "out-of-order-load", "output-group", "overly-nested-depset", "package-name", @@ -282,7 +277,6 @@ func TestValidate(t *testing.T) { "repository-name", "return-value", "rule-impl-return", - "same-origin-load", "skylark-comment", "skylark-docstring", "string-iteration", @@ -324,7 +318,6 @@ func TestValidate(t *testing.T) { "keyword-positional-params", "list-append", "load", - "load-on-top", "module-docstring", "name-conventions", // "native-android", @@ -335,7 +328,6 @@ func TestValidate(t *testing.T) { // "native-proto", // "native-py", "no-effect", - "out-of-order-load", "output-group", "overly-nested-depset", "package-name", @@ -347,7 +339,6 @@ func TestValidate(t *testing.T) { "repository-name", "return-value", "rule-impl-return", - "same-origin-load", "skylark-comment", "skylark-docstring", "string-iteration", @@ -389,7 +380,6 @@ func TestValidate(t *testing.T) { "keyword-positional-params", "list-append", "load", - "load-on-top", "module-docstring", "name-conventions", // "native-android", @@ -399,7 +389,6 @@ func TestValidate(t *testing.T) { // "native-proto", // "native-py", "no-effect", - "out-of-order-load", "output-group", "overly-nested-depset", "package-name", @@ -411,7 +400,7 @@ func TestValidate(t *testing.T) { "repository-name", "return-value", "rule-impl-return", - "same-origin-load", + "skylark-comment", "skylark-docstring", "string-iteration", diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh index 87cf9caf5..2db6cc32c 100755 --- a/buildifier/integration_test.sh +++ b/buildifier/integration_test.sh @@ -279,7 +279,6 @@ cat > golden/.buildifier.example.json < golden/.buildifier.example.json < golden/.buildifier.example.json <= len(chunks2) { - return false - } - chunk2 := chunks2[i] - // Compare case-insensitively - chunk1Lower := strings.ToLower(chunk1) - chunk2Lower := strings.ToLower(chunk2) - if chunk1Lower != chunk2Lower { - return chunk1Lower < chunk2Lower - } - } - - // No case-insensitive difference detected. Likely the difference is just in - // the case of some symbols, compare case-sensitively for the determinism. - return path1 <= path2 -} - -// compareLoadLabels compares two module names -// If one label has explicit repository path (starts with @), it goes first -// If the packages are different, labels are sorted by package name (empty package goes first) -// If the packages are the same, labels are sorted by their name -func compareLoadLabels(load1Label, load2Label string) bool { - // handle absolute labels with explicit repositories separately to - // make sure they precede absolute and relative labels without repos - isExplicitRepo1 := strings.HasPrefix(load1Label, "@") - isExplicitRepo2 := strings.HasPrefix(load2Label, "@") - - if isExplicitRepo1 != isExplicitRepo2 { - // Exactly one label has an explicit repository name, it should be the first one. - return isExplicitRepo1 - } - - // Either both labels have explicit repository names or both don't, compare their packages - // and break ties using file names if necessary - module1Parts := strings.SplitN(load1Label, ":", 2) - package1, filename1 := "", module1Parts[0] - if len(module1Parts) == 2 { - package1, filename1 = module1Parts[0], module1Parts[1] - } - module2Parts := strings.SplitN(load2Label, ":", 2) - package2, filename2 := "", module2Parts[0] - if len(module2Parts) == 2 { - package2, filename2 = module2Parts[0], module2Parts[1] - } - - // in case both packages are the same, use file names to break ties - if package1 == package2 { - return comparePaths(filename1, filename2) - } - - // in case one of the packages is empty, the empty one goes first - if len(package1) == 0 || len(package2) == 0 { - return len(package1) > 0 - } - - // both packages are non-empty and not equal, so compare them - return comparePaths(package1, package2) -} - -// outOfOrderLoadWarning only sorts consequent chunks of load statements. If applied together with -// loadOnTopWarning, should be applied after it. This is currently guaranteed by sorting the -// warning categories names before applying them ("load-on-top" < "out-of-order-load") -func outOfOrderLoadWarning(f *build.File) []*LinterFinding { - var findings []*LinterFinding - - // Consequent chunks of load statements (i.e. without statements of other types between them) - var loadsChunks [][]*build.LoadStmt - lastLoadIndex := -2 - - for i := 0; i < len(f.Stmt); i++ { - load, ok := f.Stmt[i].(*build.LoadStmt) - if !ok { - continue - } - if i-lastLoadIndex > 1 { - // There's a non-load statement between this load and the previous load - loadsChunks = append(loadsChunks, []*build.LoadStmt{}) - } - loadsChunks[len(loadsChunks)-1] = append(loadsChunks[len(loadsChunks)-1], load) - lastLoadIndex = i - } - - // Sort and flatten the chunks - var sortedLoads []*build.LoadStmt - for _, chunk := range loadsChunks { - sortedChunk := append([]*build.LoadStmt{}, chunk...) - - sort.SliceStable(sortedChunk, func(i, j int) bool { - load1Label := (sortedChunk)[i].Module.Value - load2Label := (sortedChunk)[j].Module.Value - return compareLoadLabels(load1Label, load2Label) - }) - sortedLoads = append(sortedLoads, sortedChunk...) - } - - // Calculate the replacements - var replacements []LinterReplacement - loadIndex := 0 - for stmtIndex := range f.Stmt { - if _, ok := f.Stmt[stmtIndex].(*build.LoadStmt); !ok { - continue - } - replacements = append(replacements, LinterReplacement{&f.Stmt[stmtIndex], sortedLoads[loadIndex]}) - loadIndex++ - } - - for _, chunk := range loadsChunks { - for i, load := range chunk { - if i == 0 || !compareLoadLabels(chunk[i].Module.Value, chunk[i-1].Module.Value) { - // Correct position - continue - } - findings = append(findings, makeLinterFinding(load, "Load statement is out of its lexicographical order.", replacements...)) - } - } return findings } diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go index 81d9c13de..cbf5e12c9 100644 --- a/warn/warn_cosmetic_test.go +++ b/warn/warn_cosmetic_test.go @@ -18,105 +18,6 @@ package warn import "testing" -func TestWarnSameOriginLoad(t *testing.T) { - category := "same-origin-load" - - checkFindingsAndFix(t, category, ` - load( - ":f.bzl", - "s2" - ) - load(":t.bzl", "s3") - load( - ":f.bzl", - "s1" - )`, ` - load( - ":f.bzl", - "s1", - "s2" - ) - load(":t.bzl", "s3")`, - []string{`:7: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`}, - scopeEverywhere, - ) - - checkFindingsAndFix(t, category, ` - """Module""" - - load( - ":f.bzl", - "s1" - ) - load( - ":f.bzl", - "s2" - ) - load( - ":f.bzl", - "s3" - )`, ` - """Module""" - - load( - ":f.bzl", - "s1", - "s2", - "s3" - )`, - []string{`:8: There is already a load from ":f.bzl" on line 3. Please merge all loads from the same origin into a single one.`, - `:12: There is already a load from ":f.bzl" on line 3. Please merge all loads from the same origin into a single one.`}, - scopeEverywhere, - ) - - checkFindingsAndFix(t, category, ` - load(":f.bzl", "s1") - load(":f.bzl", "s2", "s3") - `, ` - load(":f.bzl", "s1", "s2", "s3") - `, - []string{`:2: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`}, - scopeEverywhere, - ) - - checkFindingsAndFix(t, category, ` - load(":g.bzl", "s0") - load(":f.bzl", "s1") - load(":f.bzl", - "s2", - "s3") - `, ` - load(":g.bzl", "s0") - load( - ":f.bzl", - "s1", - "s2", - "s3", - )`, - []string{`:3: There is already a load from ":f.bzl" on line 2. Please merge all loads from the same origin into a single one.`}, - scopeEverywhere, - ) - - checkFindingsAndFix(t, category, ` - load(":f.bzl", "s1") - load(":f.bzl", "s2", "s3") - load(":f.bzl", - "s4") - `, ` - load( - ":f.bzl", - "s1", - "s2", - "s3", - "s4", - )`, - []string{ - `:2: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`, - `:3: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`, - }, scopeEverywhere, - ) -} - func TestPackageOnTop(t *testing.T) { checkFindingsAndFix(t, "package-on-top", @@ -241,194 +142,6 @@ package()`, scopeDefault|scopeBzl|scopeBuild) } -func TestLoadOnTop(t *testing.T) { - checkFindingsAndFix(t, "load-on-top", ` -foo() -load(":f.bzl", "x") -x()`, ` -load(":f.bzl", "x") - -foo() - -x()`, - []string{ - ":2: Load statements should be at the top of the file.", - }, scopeDefault|scopeBzl|scopeBuild) - - checkFindingsAndFix(t, "load-on-top", ` -"""Docstring""" - -# Comment block - -# this is foo -foo() - -# load -load(":f.bzl", "bar") - -# this is bar -bar() - -# another load -load(":f.bzl", "foobar")`, ` -"""Docstring""" - -# Comment block - -# load -load(":f.bzl", "bar") - -# another load -load(":f.bzl", "foobar") - -# this is foo -foo() - -# this is bar -bar()`, - []string{ - ":9: Load statements should be at the top of the file.", - ":15: Load statements should be at the top of the file.", - }, scopeDefault|scopeBzl|scopeBuild) - - checkFindingsAndFix(t, "load-on-top", ` -load(":f.bzl", "x") -# after-comment - -x() - -load(":g.bzl", "y") -`, ` -load(":f.bzl", "x") -# after-comment - -load(":g.bzl", "y") - -x() -`, - []string{ - ":6: Load statements should be at the top of the file.", - }, scopeDefault|scopeBzl|scopeBuild) -} - -func TestOutOfOrderLoad(t *testing.T) { - checkFindingsAndFix(t, "out-of-order-load", ` -# b comment -load(":b.bzl", "b") -b += 2 -# c comment -load(":c.bzl", "c") -load(":a.bzl", "a") -a + b + c`, ` -# b comment -load(":b.bzl", "b") -b += 2 -load(":a.bzl", "a") - -# c comment -load(":c.bzl", "c") -a + b + c`, - []string{":6: Load statement is out of its lexicographical order."}, - scopeEverywhere) - - checkFindingsAndFix(t, "out-of-order-load", ` -# b comment -load(":b.bzl", "b") -# c comment -load(":c.bzl", "c") -# a comment -load(":a.bzl", "a") -a + b + c`, ` -# a comment -load(":a.bzl", "a") -# b comment -load(":b.bzl", "b") -# c comment -load(":c.bzl", "c") -a + b + c`, - []string{":6: Load statement is out of its lexicographical order."}, - scopeEverywhere) - - checkFindingsAndFix(t, "out-of-order-load", ` -load(":a.bzl", "a") -load("//a:a.bzl", "a") -load("@a//a:a.bzl", "a") -load("//b:b.bzl", "b") -load(":b.bzl", "b") -load("@b//b:b.bzl", "b")`, ` -load("@a//a:a.bzl", "a") -load("@b//b:b.bzl", "b") -load("//a:a.bzl", "a") -load("//b:b.bzl", "b") -load(":a.bzl", "a") -load(":b.bzl", "b") -`, - []string{ - ":2: Load statement is out of its lexicographical order.", - ":3: Load statement is out of its lexicographical order.", - ":6: Load statement is out of its lexicographical order.", - }, scopeEverywhere) - - checkFindingsAndFix(t, "out-of-order-load", ` -load(":a.bzl", "a") -load(":a.bzl", "a") -`, ` -load(":a.bzl", "a") -load(":a.bzl", "a")`, - []string{}, scopeEverywhere) - - checkFindingsAndFix(t, "out-of-order-load", ` -load("//foo-bar:xyz.bzl", "xyz") -load("//foo/bar:baz/mno.bzl", "mno") -load("//foo/bar-baz:mno/prs.bzl", "prs") -load("//foo/bar-baz:mno-prs.bzl", "prs") -`, ` -load("//foo/bar:baz/mno.bzl", "mno") -load("//foo/bar-baz:mno/prs.bzl", "prs") -load("//foo/bar-baz:mno-prs.bzl", "prs") -load("//foo-bar:xyz.bzl", "xyz")`, - []string{ - "2: Load statement is out of its lexicographical order.", - }, scopeEverywhere) - - checkFindingsAndFix(t, "out-of-order-load", ` -load("//foo:xyz.bzl", "xyz") -load("//foo2:mno.bzl", "mno") -`, ` -load("//foo:xyz.bzl", "xyz") -load("//foo2:mno.bzl", "mno")`, - []string{}, scopeEverywhere) - - checkFindingsAndFix(t, "out-of-order-load", ` -load("//foo:b.bzl", "b") -load("//foo:a.bzl", "a") -`, ` -load("//foo:a.bzl", "a") -load("//foo:b.bzl", "b")`, - []string{ - ":2: Load statement is out of its lexicographical order.", - }, scopeEverywhere) - - checkFindingsAndFix(t, "out-of-order-load", ` -load("//Foo:aaa.bzl", "bar1") -load("//Foo:Bbb.bzl", "bar2") -load("//Foo:BBB.bzl", "bar3") -load("//bar/Baz2:bar.bzl", "bar4") -load("//bar/baz1:bar.bzl", "bar5") - -`, ` -load("//bar/baz1:bar.bzl", "bar5") -load("//bar/Baz2:bar.bzl", "bar4") -load("//Foo:aaa.bzl", "bar1") -load("//Foo:BBB.bzl", "bar3") -load("//Foo:Bbb.bzl", "bar2")`, - []string{ - ":3: Load statement is out of its lexicographical order.", - ":4: Load statement is out of its lexicographical order.", - ":5: Load statement is out of its lexicographical order.", - }, scopeEverywhere) -} - func TestUnsortedDictItems(t *testing.T) { checkFindingsAndFix(t, "unsorted-dict-items", ` d = {