bazel visibility extension should allow override default_visibility

[sluongng]

Let's say I have a setup like this

foo/BUILD
foo/bar/BUILD

with these file contents

cat foo/BUILD
# gazelle:default_visibility //foo:__subpackages

cat foo/bar/BUILD
# gazelle:default_visibility //foo/bar:__subpackages

Running gazelle bazel/visibility extension would generate something like this

cat foo/bar/BUILD
# gazelle:default_visibility //foo/bar:__subpackages
package(default_visibility = [
    "//foo:__subpackages__",
    "//foo/bar:__subpackages__",
])

while the sensible expected result should be

cat foo/bar/BUILD
# gazelle:default_visibility //foo/bar:__subpackages
package(default_visibility = ["//foo/bar:__subpackages__"])

[sluongng]

cc: @linzhp @moisesvega @dnathe4th

[dnathe4th]

Yup you're right. I accumulated visibilities here with the idea that it would allow someone to define multiple back to back directives, but I did not consider the case that it is even appending directive values it finds in other files.

Maybe it is better to only accumulate within the same file (i.e. same call to Configure) and overwrite the config's list of targets at the end of that method?

[sluongng]

Yeah, I think it's better to treat a subpackage's default_visibility directive as an override of the parent package's default_visibility directive. In my mind, I cannot think of a use case where I would want to inherit the parent's visibility config. Usually, it's always going to be some complete override.

If a change were made to parent scope visibility, the change's author should be responsible to replicate such changes to children's packages that have overridden parents' visibility. That way, such change would impact code ownership and trigger various code review systems.

So it's gonna be a +1 for me to get rid of the accumulation.

Side note: Imo this behavior is relatively safe to change. Although the language plugin was part of the last release and we recently added documentation for the new directive, the language is not yet included as part of DEFAULT_LANGUAGE exported in def.bzl so folks are not enabling this by default. Given that (a) this is a relatively new feature and (b) it's an opt-in feature without clear documentation, I think an update to this behavior should be ok.