Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dnathe4th
Copy link

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
Copy link
Contributor

linzhp commented Mar 2, 2023

@vladmos Can you take a look?

@@ -95,3 +96,43 @@ func TestRewriterWithRewriter(t *testing.T) {
t.Error("Original Printer should not equal Modified Printer")
}
}

func TestBUILDFileOrderingRewriter(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

vladmos commented Mar 2, 2023

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
Copy link
Author

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
Copy link
Member

vladmos commented Mar 7, 2023

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
Copy link
Author

dnathe4th commented Mar 7, 2023 via email

@vladmos
Copy link
Member

vladmos commented Mar 15, 2023

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
Copy link
Contributor

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

@dnathe4th
Copy link
Author

dnathe4th commented Mar 15, 2023 via email

@laurentlb
Copy link
Contributor

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
Copy link
Author

dnathe4th commented Mar 15, 2023 via email

@aiuto
Copy link
Contributor

aiuto commented Nov 10, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants