tree 4ee24e1670831a46d2fc02469d8632a55fd8c546
parent 8b07e7cfa479a6865e7b8dc10cd1ec7b78dc6814
author Tim Malmström <oreflow@users.noreply.github.com> 1789972202 +0200
committer GitHub <noreply@github.com> 1789972202 +0200
gpgsig -----BEGIN PGP SIGNATURE-----
 
 wsFcBAABCAAQBQJqsM7qCRC1aQ7uu5UhlAAALsQQAHPfAP3ZEyKYV65BveJGIeSt
 6CqGN24aP/kJXrLNCSm3FltlXcGGOVZYJCh/kJY0C68M63A0/HurBYsCqWOjJFVS
 zCkd6CPi+2qWmBo772iFq7YBMHBhMWHj+SBAvI4z5VxdEbRQMu5eJkNe8lZs55iz
 mrjNLuSUcgrN1sWgUmmxGDafgNFt1XoHQD1fjmbvfjKJePpkK1wFSDyy3Ftmm6wc
 4HoxLIpxACW3TbHSn1dk1yv1BZmzHXG/n/XbaQGH/Ig7E/Tn3OVswRhnTa6w6LMj
 ohqVj/trLilwOU5VLKn63xAq5coB1vZ9nn2zmEsM1HGt/PuxoC346tLJaaTgq92q
 0V+cfjJb+WQmxydDlKsDYUfxJW3CqA7u/t5TJ23fWJZ7Kuvx/Wl8Xip2lxSIR834
 HujMD/DGnleInmuA0WtCOz0JUE/+ZGwcAcN3rXw7kFJmwjJY8ljTncl4pJmuy+gs
 Ty8cjaA07DrdVG86ciVG7AJtgJs/vV923plrjsJdB96JpaOmdHQl10QBR9A7aloQ
 Th8Q9ccD8ljp0r9hWgYk6DNroDX8ifJc9S6eK3h3YQxCG832HKhl0g/XkOi9f4QG
 eD+ueKRkpw0tvhHxc5mBk1X8hEKFn54cYM2qkVIqi1voaqY7rsSt6WN0W5jhA7bC
 g4HR4dCN3S9AVgDL151D
 =Mcn6
 -----END PGP SIGNATURE-----
 

only remove visibility matching explicit default_visibility (#1512)

## Buildtools PR checklist

- [x] The code in this PR is covered by unit/integration tests.
- [x] I have tested these changes and provide testing instructions
below.
- [ ] I have either responded to, or resolved all Gemini comments on the
PR.
- [x] I have read Google Eng Practices on [Small
Changes](https://google.github.io/eng-practices/review/developer/small-cls.html),
this PR either follows these guidelines or the description provides
reasoning for why they can not be followed.

## Description

Previously, buildozer's fix.go:105 routine stripped `visibility`
attributes whenever they matched an explicit `default_visibility` or
defaulted to `["//visibility:private"]` when `default_visibility` was
omitted.

However, Starlark macros and custom rules often implement their own
default visibility logic (e.g., defaulting to public or custom targets
when `visibility` is omitted). Stripping `visibility =
["//visibility:private"]` when no package-level default is declared can
silently broaden visibility.

Rather than relying on fragile heuristics to identify macros and loaded
rules, this change refactors fix.go:105 to only strip visibility
attributes when `default_visibility` is **explicitly defined** in a
`package()` declaration:

- **Explicit `default_visibility` required**: If `default_visibility` is
not explicitly set on the package, rule `visibility` attributes are
preserved.
- **Unordered string list matching**: Compares string lists using
`slices.Sorted`, allowing redundant visibility to be removed even if
list elements appear in a different order.
- **Identifier matching**: Supports matching identifier expressions
(e.g. `package(default_visibility = PUBLIC)` and `visibility = PUBLIC`).


### (optional) These changes were tested using the following steps

Ran unit tests covering all cases:
```bash
    go test ./edit -run TestFix
```

Unit test cases in fix_test.go verify:

  • Preserving visibility when default_visibility is not set
  • Removing redundant visibility with differing slice order
  • Removing redundant visibility matching identifiers
  • Preserving non-matching visibility

---------

Co-authored-by: Kyle Dobitz <kyledobitz@gmail.com>
Co-authored-by: Tim Malmström <oreflow@google.com>