bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Do not munge `visibility` attribute for targets created by symbolic macros

Open brandjon opened this issue 1 year ago • 2 comments

@bazel-io flag 8.0.0 This potentially blocks Bazel 8.0 because without fixing this, the bazel query-introspected visibility value of targets created in Symbolic Macros is misleading/inconsistent.


Currently, for targets created within a symbolic macro, the visibility attribute that gets stored on the target is munged to include the instantiating location. This contrasts with the raw visibility attribute that gets stored on targets not in symbolic macros, which matches whatever the visibility = argument passed to the instantiation site was.

We can't munge for targets not in symbolic macros, because it would likely imply a memory regression for the overwhelming majority of the build, and it would change the introspected visibility value for those targets relative to what is seen today in native.existing_rules and bazel query. But at the same time, the munging for targets in symbolic macros is also problematic, both due to the inconsistency and because tooling that reads the introspected values might expect them to match the source text of a BUILD file.

The proper solution is probably to be consistent by

  1. not munging visibility for any target
  2. computing the VisibilityProvider at analysis time without relying on said munging, deferring the concatenation of the callsite location to that point in time
  3. creating a synthetic attribute, $actual_visibility, to hold the munged value (regardless of whether the target is inside a symbolic macro), that can be introspected in bazel query but not in native.existing_rules. This attribute should not be materialized in normal builds.

Symbolic macros themselves also have a visibility attribute, and it must always be munged (as it currently is) so that the implementation function sees the correct value for its visibility parameter. But if we expose symbolic macro attributes via bazel query in the future, we may very well want to avoid the munging to preserve consistency with rules. Note that munging happens at every level of a chain of symbolic macro calls, and the raw visibility for one macro is the munged visibility for its caller.

Implementing this probably just requires a little work in RuleFactory and ConfiguredTargetFactory, plus updating some code comments that explain the outdated invariant. I'm not completely sure what the precedent is for synthetic query-only attributes.

brandjon avatar Oct 03 '24 02:10 brandjon

Let's track the synthetic attribute part separately as FR #23893, without necessarily blocking 8.0 on that part. (The rest of this issue still blocks 8.0.)

brandjon avatar Oct 07 '24 17:10 brandjon

@bazel-io fork 8.0.0

brandjon avatar Oct 15 '24 18:10 brandjon

Status update for 8.0, please?

Wyverald avatar Nov 13 '24 23:11 Wyverald

CL submitting imminently.

brandjon avatar Nov 14 '24 15:11 brandjon

The changes in this PR have been included in Bazel 8.0.0 RC3. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=8.0.0rc3. Thanks!

iancha1992 avatar Nov 22 '24 23:11 iancha1992