Flip --incompatible_visibility_private_attributes_at_definition and make it less breaking

[fmeum]

In addition to flipping the flag, also add a check for visibility from the target as a fallback to make the flip less breaking.

Closes #19330

RELNOTES[INC]: --incompatible_visibility_private_attributes_at_definition is flipped to true. See #19330 for details.

[fmeum]

@meteorcloudy I would like to see this flag flipped, but would like to see the results of a downstream run before I bring this up for discussion. Could you trigger one for me on this PR?

[meteorcloudy]

Here you go: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds?branch=pull%2F19141%2Fhead

But there are many existing failures in downstream pipeline, you may need to compare the results.

[fmeum]

Downstream is looking good: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1625

Not entirely sure which team this belongs to, but based on prior interactions about this flag and the relation to visibility, I will go with Team-Configurability.

@aranguyen Could you review this? I had a previous discussion about this with Greg on #19139. We can also wait for Greg to be back in office if you don't feel like you have sufficient context.

CC @laurentlb as the reviewer of the original proposal (https://github.com/bazelbuild/proposals/blob/main/designs/2019-10-15-tool-visibility.md). With load visibility available now, there doesn't seem to be any reason not to flip this for Bazel. Blaze may be a different story, but could be solved with an RC file entry if you don't want to perform a migration.

[comius]

This has a potential to show preexisting correctness bugs on unexpected edge cases. I'll run the internal tests and report back.

[comius]

@brandjon, can you provide a second pair of eyes?

This has been implemented in 2019: PiperOrigin-RevId: 277683498

Technically this could show us some edge cases / correctness issues. I remember fixing one related to this in the past that involved about 5 conditions to trigger.

I'll run internal tests and if we don't have a plan for other actions that could de-risk us, I propose we move forward.

This is a feature that both internal and external users were asking for and is a pain point for toolchain owners in general.

@fmeum I don't see aspects covered in the tests of the original implementation. Could you add some tests for that? Should an implicit dependency in that case be visible to an aspect, not rule?

cc @mai93

[comius]

The test I was referring to is: targetCompatibleWith_mismatchesExecCompatibleWithinAspect.

The aspect case is even more complex. Implicit dependency might be coming from the target an aspect is over or from an aspect. Please add tests. Undo approval.

[comius]

Please add tests that cover aspect's implicit dependecies and implicit dependencies of targets an aspect is over.

[comius]

I believe IntelliJ issues are related to aspects functionality. Please address them.

[fmeum]

@comius I made the change you suggested in #19330 (comment).

[comius]

There's one use case we haven't foreseen. Restricting visibility of tools to builtin rules.

[fmeum]

@comius Could you elaborate on this? Wouldn't something like visibility = ["@_builtins//..."] work?

[comius]

@comius Could you elaborate on this? Wouldn't something like visibility = ["@_builtins//..."] work?

I've just got a report from an internal user, that they tried "@_builtins//:__pkg__" and that it doesn't work. The error message indicates that fallback happened, however I didn't debug it.

It happened on an internal ruleset. Possibly a simple reproduction in Bazel is to reduce visibility of @bazel_tools/tools/def_parser:def_parser, which is used in cc_binary

[fmeum]

I checked that visibility = ["@@_builtins//:__subpackages__"] on def_parser suffices to build cc_binarys. Note the double @, which is necessary since _builtins does not have a repo mapping entry in bazel_tools.

@comius If a rule foo extends a rule bar, are implicit dependencies of bar checked against the definition of bar (instead of foo)? I could see this leading to problems if not.

[Wyverald]

cc also @brandjon; this is somewhat annoying since @_builtins is really a "pseudo-repo" -- it doesn't have a definition, you can't build any targets in it, and the only thing you're really allowed to do is to load .bzl files from it. I'm not too familiar with what this PR entails, but if it has now required @_builtins to be an actual repo, we might need to have a more principled solution.