Add Rewriter rule to position Package() after loads in BUILD file (#1138)

[dnathe4th]

This change adds a Rewrite rule to enforce the position of the package() function invocation within a BUILD file. This function must be called after all of the load()s, but before any rule, according to the documentation.

Making this change here in the Rewriter also simplifies gazelle extensions manipulating this function in BUILD files as they no longer need to inspect the rest of the file for positional information.

[linzhp]

@vladmos Can you take a look?

[vladmos]

What's the reason this test is implemented separately, not as a test file under build/testdata? Rewrites are also done after ordinary formatting, aren't they?

[vladmos]

Sorry for the delay. This is a backward-incompatible change (it may make some previously correctly formatted files not-formatted) and may require some repository cleanup, I'll do it in the near future (probably even merge this PR before and temporarily disable the reformatting rule internally).

[dnathe4th]

No worries on the delay, I understand we're all volunteers here :- )

Do you have a specific example of a correctly-formatted (i.e. package() in the right place) file that this change would render unformatted? My positioning of it after *LoadStmt, *CommentBlock, *StringExpr seems correct, but if there are additional expressions to skip that would reduce a false positive rate I'd like to include those in this logic as well.

[vladmos]

Currently if package() goes before load() it's still correctly formatted (it violates the convention that load() should go first, but buildifier doesn't reorder the statements). After the change it will start reordering them, making files that were previously considered as formatted non-formatted. That can in theory block presubmits if there's a check that buildifier doesn't modify BUILD files, even if the changes in the files are competely irrelevant to load or package statements.

[dnathe4th]

Is that convention (package allowed to go before load) something I should update this code to *not* reorganize? I am fine with that if it doesn't actually impact the correctness of the final build file.

…

On Tue, Mar 7, 2023 at 6:36 AM Vladimir Moskva ***@***.***> wrote: Currently if package() goes before load() it's still correctly formatted (it violates the convention that load() should go first, but buildifier doesn't reorder the statements). After the change it will start reordering them, making files that were previously considered as formatted non-formatted. That can in theory block presubmits if there's a check that buildifier doesn't modify BUILD files, even if the changes in the files are competely irrelevant to load or package statements. — Reply to this email directly, view it on GitHub <#1139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKQ7RSCF4WQLAZ6VEEMTDW25BYBANCNFSM6AAAAAAVFAF5IE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[vladmos]

No, by convention load()s should go first (the exception is WORKSPACE files). It's just what's currently allowed by the formatter, and won't be allowed once this change is in place.

[laurentlb]

Could you use buildifier --lint=fix --warnings=load-on-top instead of adding a new rewriter?

[dnathe4th]

But the goal is for the formatting process to enforce the requirements of the spec by default, not by opt-in. I understand this is a one-time breaking change *for existing invalid build files*, but the goal is for gazelle (and extensions) and buildifier and buildozer to have one fewer thing to worry about from a file formatting perspective because this core functionality is aligning the build file to the spec for them.

…

On Wed, Mar 15, 2023, 8:32 AM Laurent Le Brun ***@***.***> wrote: Could you use buildifier --lint=fix --warnings=load-on-top instead of adding a new rewriter? — Reply to this email directly, view it on GitHub <#1139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKQ7U2RTMFX6URHEU7V4DW4HOJZANCNFSM6AAAAAAVFAF5IE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[laurentlb]

Sure, but I'd (personally) prefer to see a pull request that makes load-on-top on by default, instead of implementing a new rewriter.

I'll let @vladmos decide how to proceed (we need to take care of the one time migration of the Google codebase).

[dnathe4th]

The problem I was actually trying to solve for was more like "package-between-load-and-rules". Maybe you're saying the path to this is to make it a fix first, release that so people can opt on, and then do a later release with it as the default?

…

On Wed, Mar 15, 2023, 9:02 AM Laurent Le Brun ***@***.***> wrote: Sure, but I'd (personally) prefer to see a pull request that makes load-on-top on by default, instead of implementing a new rewriter. I'll let @vladmos <https://github.com/vladmos> decide how to proceed (we need to take care of the one time migration of the Google codebase). — Reply to this email directly, view it on GitHub <#1139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKQ7WMKEKMWNJBZVZW7ATW4HRYTANCNFSM6AAAAAAVFAF5IE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[aiuto]

@sdtwigg is handling the google internal migration to package after load but before any rules.
Do not let that be a blocker for having this behavior.

FWIW, we are also well on the way towards making package(default_attr=something) apply to the entire file and not just following rules. That is a breaking change but results in the least surprise for users. The result is that if buildifier starts moving packages around, it will either be a logical no-op, OR will expose behavior which will soon break anyway.