Do not munge visibility attribute for targets created by symbolic macros
#23855
Assignees
Labels
P1
I'll work on this now. (Assignee required)
team-Loading-API
BUILD file and macro processing: labels, package(), visibility, glob
type: bug
Comments
brandjon
added
type: bug
P1
|
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.) |
|
@bazel-io fork 8.0.0 |
copybara-service bot
pushed a commit
that referenced
this issue
Oct 24, 2024
This CL simplifies RuleVisibility concatenation by inlining concatWithElement() into concatWithPackage(). The latter was the only caller of the former in production. The implementation no longer has to handle the case where the RHS is public or private -- something that was not needed in practice, but which was required in abstract for the deleted method's API to be correct. It also now doesn't need to parse an untrusted label, so there's no need to catch or throw EvalException. concatWithPackage() is also made non-static, which looks nicer at the call sites. The validate() method is renamed to checkForVisibilityMisspelling(), since that's the only thing it does now. (A previous CL removed its other job of detecting formerly illegal combinations of public/private visibility with other items in a list.) Added an equals() and hashCode() while we're at it, to avoid depending on singleton identity (I'm not sure whether @SerializationConstant lets us assume that). It's based on just getDeclaredLabels() since that should superset any information returned by getDependencyLabels(). Changed RuleVisibility from an interface to an abstract class too -- I would've made it `sealed`, but the singleton PUBLIC and PRIVATE anonymous subclasses prevent that and it's not worth making them named classes. MacroInstance.java - Implement the TODO. (I went with "getDefinitionPackage" over "getDefiningPackage" because the latter might be confused with the package that defines the macro *instance*.) MacroClass.java - Condense to if-expr form now that the else branch is more concise. RuleVisibilityTest.java - Add pkg() helper for concatenation() test case. - Remove assertEqual() now that we have a RuleVisibility#equals() method. - Rename "B" -> "B_PG" for clarity/contrast with "A" and "C". - Delete test cases for concatWithElement that are no longer applicable (where the RHS is public or private visibility) Work toward #23855. PiperOrigin-RevId: 689400241 Change-Id: I6b7a1c9f87c4b3c9e5cd99d5dc4d906b5d798d28
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
@bazel-io flag 8.0.0
This potentially blocks Bazel 8.0 because without fixing this, the
bazel query-introspectedvisibilityvalue of targets created in Symbolic Macros is misleading/inconsistent.Currently, for targets created within a symbolic macro, the
visibilityattribute that gets stored on the target is munged to include the instantiating location. This contrasts with the rawvisibilityattribute that gets stored on targets not in symbolic macros, which matches whatever thevisibility =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
visibilityvalue for those targets relative to what is seen today innative.existing_rulesandbazel 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
visibilityfor any target$actual_visibility, to hold the munged value (regardless of whether the target is inside a symbolic macro), that can be introspected inbazel querybut not innative.existing_rules. This attribute should not be materialized in normal builds.Symbolic macros themselves also have a
visibilityattribute, and it must always be munged (as it currently is) so that the implementation function sees the correct value for itsvisibilityparameter. But if we expose symbolic macro attributes viabazel queryin 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
RuleFactoryandConfiguredTargetFactory, plus updating some code comments that explain the outdated invariant. I'm not completely sure what the precedent is for syntheticquery-only attributes.The text was updated successfully, but these errors were encountered: