buildifier does not move package() to the top in some cases

[keith]

if you have a BUILD file like this:

some_target(
...
)

some_list = [
#
]

package(default_visibility = ["//visibility:public"])

buildifier does not warn about the package() being at the bottom, nor does it move it to the top when fixing

[keith]

the same goes for a BUILD file like this:

[
     some_target(...)
     for x in ["list"]
]

package(default_visibility = ["//visibility:public"])

[vladmos]

The second case looks like a bug, the first case IIRC was implemented this way to prevent situations when a package statement uses a variable defined above it, e.g.

some_list = [...]
package(default_visibility = some_list)

This behavior is still buggy and could have been implemented better, but at least false-negative is better than false-positive here (the latter can lead to an invalid file and broken builds).

[keith]

ah thanks for the context, in my case it was more like:

targets()

lists = []

targets()

package()

and so maybe it could be at least moved up to under lists

[keith]

potential improvement here to be a bit more greedy with it: #1422