From caf03bda753be8f464f6d6f6b3982476d4a0f1a2 Mon Sep 17 00:00:00 2001 From: Dom Narducci Date: Wed, 22 Feb 2023 16:53:58 -0800 Subject: [PATCH 1/5] Add Rewriter rule to position Package() after loads in BUILD file (#1138) --- build/BUILD.bazel | 4 +- build/rewrite.go | 44 +++++++++++++++ build/rewrite_test.go | 53 ++++++++++++++++--- .../ordering_test.formatted.bazel | 20 +++++++ .../ordering_test.original.bazel | 20 +++++++ 5 files changed, 132 insertions(+), 9 deletions(-) create mode 100644 build/rewrite_test_files/ordering_test.formatted.bazel create mode 100644 build/rewrite_test_files/ordering_test.original.bazel diff --git a/build/BUILD.bazel b/build/BUILD.bazel index 973a8af2f..d91636dfd 100644 --- a/build/BUILD.bazel +++ b/build/BUILD.bazel @@ -40,14 +40,12 @@ go_test( "rule_test.go", "walk_test.go", ], - data = glob(["testdata/*"]) + [ + data = glob(["testdata/*", "rewrite_test_files/*"]) + [ # parse.y.go is checked in to satisfy the Go community # https://github.com/bazelbuild/buildtools/issues/14 # this test ensures it doesn't get stale. "parse.y.baz.go", "parse.y.go", - "rewrite_test_files/original_formatted.star", - "rewrite_test_files/original.star", ], embed = [":go_default_library"], deps = [ diff --git a/build/rewrite.go b/build/rewrite.go index 10ac2edf3..8fc385b83 100644 --- a/build/rewrite.go +++ b/build/rewrite.go @@ -132,6 +132,7 @@ var rewrites = []struct { {"formatdocstrings", formatDocstrings, scopeBoth}, {"reorderarguments", reorderArguments, scopeBoth}, {"editoctal", editOctals, scopeBoth}, + {"reorderpackageafterloadbeforerules", reorderPackageAfterLoadBeforeRules, TypeBuild}, } // leaveAlone reports whether any of the nodes on the stack are marked @@ -1096,3 +1097,46 @@ func removeParens(f *File, _ *Rewriter) { Edit(f, simplify) } + +// reorderPackageAfterLoadBeforeRules ensures a Package() only occurs after the last load(). +// +// Note that this reordering is best effort and there may be already-invalid +// BUILD.bazel file configurations (such as a load() after a rule) that result +// in still-invalid output. +// +// Also note that we do not manipulate the Position{} structs within existing +// expressions; only the order of statements in *File is modified. This should +// be fine for printing (the only current usecase within this package), but +// may cause confusion for future alternative usages of this Rewrite logic. +func reorderPackageAfterLoadBeforeRules(f *File, _ *Rewriter) { + var lastLoadIndex, packageFuncIndex int + var packageFunc *CallExpr + for idx, s := range f.Stmt { + switch s := s.(type) { + case *LoadStmt: + lastLoadIndex = idx + case *CallExpr: + if x, ok := s.X.(*Ident); ok && x.Name == "package" { + packageFunc = s + packageFuncIndex = idx + } + } + } + + if packageFunc == nil { + // no reordering necessary + return + } + + // to get the package func after the last load, shift everything + // after the last load, up until the package func to make room. + // + // fun fact, the terminal condition on this loop can actually be + // > or >= and still result in correct output because the final + // packageFunc insertion would overright the final iteration + // in the latter case. + for i := packageFuncIndex - 1; i >= lastLoadIndex; i-- { + f.Stmt[i+1] = f.Stmt[i] + } + f.Stmt[lastLoadIndex+1] = packageFunc +} diff --git a/build/rewrite_test.go b/build/rewrite_test.go index 9dfa4a113..2a4ffd7c7 100644 --- a/build/rewrite_test.go +++ b/build/rewrite_test.go @@ -24,9 +24,9 @@ import ( "testing" ) -var workingDir string = path.Join(os.Getenv("TEST_SRCDIR"), os.Getenv("TEST_WORKSPACE"), "build") -var originalFilePath = workingDir + "/rewrite_test_files/original.star" -var formattedFilePath = workingDir + "/rewrite_test_files/original_formatted.star" +var workingDir string = path.Join(os.Getenv("TEST_SRCDIR"), os.Getenv("TEST_WORKSPACE"), "build", "rewrite_test_files") +var originalFilePath = path.Join(workingDir, "original.star") +var formattedFilePath = path.Join(workingDir, "original_formatted.star") var originalBytes, _ = ioutil.ReadFile(originalFilePath) var originalFile, _ = ParseDefault(originalFilePath, originalBytes) @@ -47,7 +47,8 @@ func TestRewriterRegular(t *testing.T) { var modifyBytes, _ = ioutil.ReadFile(originalFilePath) var modifiedFile, _ = ParseDefault(originalFilePath, modifyBytes) - // Perform rewrite on loaded file, rewrite should do nothing here + // Perform rewrite on loaded file, rewrite should do nothing here because the file + // has no needed modifations for the rewrites scoped to include scopeDefault. Rewrite(modifiedFile) // Initialize printers to obtain bytes later for different types of printers @@ -60,7 +61,7 @@ func TestRewriterRegular(t *testing.T) { originalPrinter.file(originalFile) modifiedPrinter.file(modifiedFile) - // Assert that bytes to be writter are same as original bytes and different from formatted + // Assert that bytes to be written are same as original bytes and different from formatted if !bytes.Equal(originalPrinter.Bytes(), modifiedPrinter.Bytes()) { t.Error("Original Printer should equal Modified Printer") } @@ -74,7 +75,7 @@ func TestRewriterWithRewriter(t *testing.T) { var modifyBytes, _ = ioutil.ReadFile(originalFilePath) var modifiedFile, _ = ParseDefault(originalFilePath, modifyBytes) - // Perform rewrite with rewriter on loaded file, should proceed to reorder + // Perform rewrite with rewriter on loaded file, should proceed to reorder arguments rewriter.Rewrite(modifiedFile) // Initialize printers to obtain bytes later for different types of printers @@ -95,3 +96,43 @@ func TestRewriterWithRewriter(t *testing.T) { t.Error("Original Printer should not equal Modified Printer") } } + +func TestBUILDFileOrderingRewriter(t *testing.T) { + originalFilePath := path.Join(workingDir, "ordering_test.original.bazel") + formattedFilePath := path.Join(workingDir, "ordering_test.formatted.bazel") + + // Load the files as a BUILD + modifyBytes, _ := ioutil.ReadFile(originalFilePath) + modifiedFile, _ := ParseBuild(originalFilePath, modifyBytes) + originalBytes, _ := ioutil.ReadFile(originalFilePath) + originalFile, _ := ParseBuild(originalFilePath, originalBytes) + formattedBytes, _ := ioutil.ReadFile(formattedFilePath) + formattedFile, _ := ParseBuild(formattedFilePath, formattedBytes) + + // Perform rewrite on loaded file, should proceed to reorder calls + Rewrite(modifiedFile) + + // Initialize printers to obtain bytes later for different types of printers + // We will check bytes from printers because that is our source of truth before writing to new file + formattedPrinter := &printer{fileType: formattedFile.Type} + originalPrinter := &printer{fileType: originalFile.Type} + modifiedPrinter := &printer{fileType: modifiedFile.Type} + + formattedPrinter.file(formattedFile) + originalPrinter.file(originalFile) + modifiedPrinter.file(modifiedFile) + + // Assert that bytes to be written is same as formmatted bytes and different from original + if bytes.Equal(formattedPrinter.Bytes(), modifiedPrinter.Bytes()) { + t.Errorf("Formatted Printer should equal Modified Printer\n\n\nmodified:\n%s\n\nformatted:\n%s\n\n", + modifiedPrinter.String(), + formattedPrinter.String(), + ) + } + if bytes.Equal(originalPrinter.Bytes(), modifiedPrinter.Bytes()) { + t.Errorf("Original Printer should not equal Modified Printer\n\n\nmodified:\n%s\n\noriginal:\n%s\n\n", + modifiedPrinter.String(), + formattedPrinter.String(), + ) + } +} diff --git a/build/rewrite_test_files/ordering_test.formatted.bazel b/build/rewrite_test_files/ordering_test.formatted.bazel new file mode 100644 index 000000000..ff7a7c93b --- /dev/null +++ b/build/rewrite_test_files/ordering_test.formatted.bazel @@ -0,0 +1,20 @@ +""" +Test file for rewrite.go +""" +# gazelle:load_directive +load("//foo.bzl", "library_x") +load("//keep.bzl", "library_keep") # keep + +load("//bar.bzl", "library_y") + +package(default_visibility="//:__subpackages__") + +# gazelle:directive + +library_x( + name = "default_name" +) + +library_y( + name = "another_default_name" +) diff --git a/build/rewrite_test_files/ordering_test.original.bazel b/build/rewrite_test_files/ordering_test.original.bazel new file mode 100644 index 000000000..29eafa69f --- /dev/null +++ b/build/rewrite_test_files/ordering_test.original.bazel @@ -0,0 +1,20 @@ +""" +Test file for rewrite.go +""" +# gazelle:load_directive +load("//foo.bzl", "library_x") +load("//keep.bzl", "library_keep") # keep + +load("//bar.bzl", "library_y") + +# gazelle:directive + +library_x( + name = "default_name" +) + +package(default_visibility="//:__subpackages__") + +library_y( + name = "another_default_name" +) From 4be7a2f0aad002560541d68a091b0dfbc06cc83c Mon Sep 17 00:00:00 2001 From: Dom Narducci Date: Fri, 24 Feb 2023 11:29:46 -0800 Subject: [PATCH 2/5] Fix index tracking for files without loads or package --- build/rewrite.go | 5 +++-- build/rewrite_test.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/build/rewrite.go b/build/rewrite.go index 8fc385b83..775a812dc 100644 --- a/build/rewrite.go +++ b/build/rewrite.go @@ -1109,7 +1109,8 @@ func removeParens(f *File, _ *Rewriter) { // be fine for printing (the only current usecase within this package), but // may cause confusion for future alternative usages of this Rewrite logic. func reorderPackageAfterLoadBeforeRules(f *File, _ *Rewriter) { - var lastLoadIndex, packageFuncIndex int + lastLoadIndex := -1 + packageFuncIndex := -1 var packageFunc *CallExpr for idx, s := range f.Stmt { switch s := s.(type) { @@ -1123,7 +1124,7 @@ func reorderPackageAfterLoadBeforeRules(f *File, _ *Rewriter) { } } - if packageFunc == nil { + if lastLoadIndex == -1 || packageFuncIndex == -1 { // no reordering necessary return } diff --git a/build/rewrite_test.go b/build/rewrite_test.go index 2a4ffd7c7..433b75ae6 100644 --- a/build/rewrite_test.go +++ b/build/rewrite_test.go @@ -123,7 +123,7 @@ func TestBUILDFileOrderingRewriter(t *testing.T) { modifiedPrinter.file(modifiedFile) // Assert that bytes to be written is same as formmatted bytes and different from original - if bytes.Equal(formattedPrinter.Bytes(), modifiedPrinter.Bytes()) { + if !bytes.Equal(formattedPrinter.Bytes(), modifiedPrinter.Bytes()) { t.Errorf("Formatted Printer should equal Modified Printer\n\n\nmodified:\n%s\n\nformatted:\n%s\n\n", modifiedPrinter.String(), formattedPrinter.String(), From 746d6a4bb0597f4b179aa23b0fa552f6f3985b8f Mon Sep 17 00:00:00 2001 From: Dom Narducci Date: Fri, 24 Feb 2023 11:58:39 -0800 Subject: [PATCH 3/5] consider string expressions and comments as skippable at the beginning of a build file --- build/rewrite.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/build/rewrite.go b/build/rewrite.go index 775a812dc..3d49dd86b 100644 --- a/build/rewrite.go +++ b/build/rewrite.go @@ -1109,22 +1109,27 @@ func removeParens(f *File, _ *Rewriter) { // be fine for printing (the only current usecase within this package), but // may cause confusion for future alternative usages of this Rewrite logic. func reorderPackageAfterLoadBeforeRules(f *File, _ *Rewriter) { - lastLoadIndex := -1 + insertIndex := -1 + insertIndexLocked := false packageFuncIndex := -1 var packageFunc *CallExpr for idx, s := range f.Stmt { switch s := s.(type) { - case *LoadStmt: - lastLoadIndex = idx + case *LoadStmt, *CommentBlock, *StringExpr: + if !insertIndexLocked { + insertIndex = idx + } + continue case *CallExpr: if x, ok := s.X.(*Ident); ok && x.Name == "package" { packageFunc = s packageFuncIndex = idx } } + insertIndexLocked = true } - if lastLoadIndex == -1 || packageFuncIndex == -1 { + if insertIndex == -1 || packageFuncIndex == -1 { // no reordering necessary return } @@ -1136,8 +1141,8 @@ func reorderPackageAfterLoadBeforeRules(f *File, _ *Rewriter) { // > or >= and still result in correct output because the final // packageFunc insertion would overright the final iteration // in the latter case. - for i := packageFuncIndex - 1; i >= lastLoadIndex; i-- { + for i := packageFuncIndex - 1; i > insertIndex; i-- { f.Stmt[i+1] = f.Stmt[i] } - f.Stmt[lastLoadIndex+1] = packageFunc + f.Stmt[insertIndex+1] = packageFunc } From 486ba9d7e549377fa52546afabff1bccd3697c69 Mon Sep 17 00:00:00 2001 From: Dom Narducci Date: Fri, 24 Feb 2023 12:01:23 -0800 Subject: [PATCH 4/5] Format build file --- build/BUILD.bazel | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/build/BUILD.bazel b/build/BUILD.bazel index d91636dfd..b1a58ecbf 100644 --- a/build/BUILD.bazel +++ b/build/BUILD.bazel @@ -40,7 +40,10 @@ go_test( "rule_test.go", "walk_test.go", ], - data = glob(["testdata/*", "rewrite_test_files/*"]) + [ + data = glob([ + "testdata/*", + "rewrite_test_files/*", + ]) + [ # parse.y.go is checked in to satisfy the Go community # https://github.com/bazelbuild/buildtools/issues/14 # this test ensures it doesn't get stale. From 43e37cad6612de0907f10bf6d0ea765f99371ac3 Mon Sep 17 00:00:00 2001 From: Dom Narducci Date: Fri, 24 Feb 2023 12:02:39 -0800 Subject: [PATCH 5/5] fix test case --- build/rewrite_test_files/ordering_test.formatted.bazel | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build/rewrite_test_files/ordering_test.formatted.bazel b/build/rewrite_test_files/ordering_test.formatted.bazel index ff7a7c93b..5efe519f3 100644 --- a/build/rewrite_test_files/ordering_test.formatted.bazel +++ b/build/rewrite_test_files/ordering_test.formatted.bazel @@ -7,10 +7,10 @@ load("//keep.bzl", "library_keep") # keep load("//bar.bzl", "library_y") -package(default_visibility="//:__subpackages__") - # gazelle:directive +package(default_visibility="//:__subpackages__") + library_x( name = "default_name" )