Enforce visibility on select() keys #13676
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Example:
Today,
//other/package:some_config
is exempt from visibility checking, even though it's technically a target dep ofbuildme
.While this dep is "special" vs. other deps in various ways, there's no obvious reason why it needs to be special in this way. It adds an unclear corner case exception to visibility's API.
Implementation:
select() keys are not "normal" dependencies and don't generally follow the same code path. Hence them not being automatically visibility checked like others.
In particular, normal dependencies are found in
ConfiguredTargetFunction
and validity-checked inRuleContext.Builder
. select() keys' only purpose is to figure out which other normal dependencies should exist. There's generally no need to pass them toRuleContext.Builder
. Instead, Blaze passes theirConfigMatchingProvider
s, which remain useful for analysis phase attribute lookups.RuleContext.Builder
needs aConfiguredTargetAndData
to do validity-checking. This patch propagates that information for select() keys too.We could alternatively refactor the validity checking logic. But that's an even more invasive change. Or do ad hoc validity checking directly in
ConfiguredTargetFunction
. But that's duplicating logic we really want to keep consolidated.Backward compatibility:
This would break existing builds if
config_setting
defaulted to private visibility. So this change specially defaultsconfig_setting
to public visibility, with clarifying documentation. When ready we'll want to create an incompatible change to makeconfig_setting
the same as everything else.Fixes #12669.
Closes #12877.
RELNOTES: config_setting now honors
visibility
attribute (and defaults to//visibility:public
)PiperOrigin-RevId: 354310777