Enforce visibility on select() keys

[gregestren]

Example:

my_rule(
  name = "buildme",
  deps = select({
    "//other/package:some_config": [":mydeps"]
  }))

Today, //other/package:some_config is exempt from visibility checking, even though it's technically a target dep of
buildme.

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 in RuleContext.Builder. select() keys' only purpose is to figure out which other normal dependencies should exist. There's generally no need to pass them to RuleContext.Builder. Instead, Blaze passes their ConfigMatchingProviders, which remain useful for analysis phase attribute lookups.

RuleContext.Builder needs a ConfiguredTargetAndData 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 defaults config_setting to public visibility, with clarifying documentation. When ready we'll want to create an incompatible change to make config_setting the same as everything else.

Fixes #12669.

RELNOTES: config_setting now honors visibility attribute (and defaults to //visibility:public)

[gregestren]

This PR is proof-of-concepty. I haven't added tests and would like to resolve some philosophical questions before advancing:

1. Should we strive for all rules supporting visibility checking? I don't think select() keys are the only rules that skip it. @katre - what about toolchain deps?
2. This change highlights that select() deps follow a different code path than normal deps. That's why it's a 10-file change with an entirely new data class to support it. And that much more Bazel code to support. For just this maybe it's worth it. But if we had to repeat this ad hocness for X other rule types, maybe that suggests deeper design brittleness.
3. We also have a workaround: route through aliases.

[katre]

1. The actual toolchain targets that are identified with register_toolchains and --extra_toolchains probably skip visibility, but the toolchains themselves (the cc_toolchain or java_toolchain targets that are consumed by rules) are visibility checked.
   a. Also, visibility is skipped for platforms that are loaded via register_execution_platforms, --extra_execution_platforms, --platforms, and --host_platforms.
2. I'm not sure how we'd restructure configurable attributes to be more neatly supported. I think at this point the change makes sense.
3. The workaround requires everyone who uses a select to follow it, which seems onerous.

This PR LGTM.

[gregestren]

Thanks for all the feedback, @katre . Added tests and some documentation.

[gregestren]

Thanks for all the feedback, @katre . Added tests and some documentation.

[katre]

Ideally this entire method, or at least the part from here on, could be moved into ConfigConditions.

[gregestren]

I support this idea, and tried to make headway with it. But it's too complicated for this PR, in my view. The problem is that the method has various dependencies on ConfiguredTargetFunction that aren't trivial to extract. Trivially moving it would result in ConfiguredTargetFunction and ConfigConditions calling back and forth to each other.

I do support keeping ConfiguredTargetFunction (which is a beast) simple in any ways we can. So over a longer term I'd love to make progress on things like this.

[lberki]

Yay, this is great! I mean, great modulo the necessary depot cleanup required. Do you have a sense of how much work that is?

I think this is about the best approach you can take. I was somewhat surprised that you have to plumb through two maps in ConfigConditions (I would have guessed that the ConfiguredTarget is enough), but meh.

Do we have many other places that are exempt from visibility checking? I was hoping that centralizing it into RuleContext makes it catch every case, but I was apparently mistaken in at least one case.

[gregestren]

I think this is about the best approach you can take. I was somewhat surprised that you have to plumb through two maps in ConfigConditions (I would have guessed that the ConfiguredTarget is enough), but meh.

Yeah, I wasn't thrilled about that. But it's really necessary, without doing deeper Bazel refactoring.

About that refactoring... One possible alternative is to include config_settings in RuleContext.prerequisiteMap. I avoided that approach because it technically changes the semantics and I can't say with confidence that wouldn't break some other invariant. But I am intrigued. I'd like to submit this PR to get the functionality in, then try to whip up a version around that and share with you all to see if it ultimately looks viable.

Do we have many other places that are exempt from visibility checking? I was hoping that centralizing it into RuleContext makes it catch every case, but I was apparently mistaken in at least one case.

I believe the answer is near universally no. It's really only dependencies that get evaluated directly in ConfiguredTargetFunction through their own code paths that have this issue. That should only be this and some toolchain deps as described at #12877 (comment).