Flip --incompatible_visibility_private_attributes_at_definition and…

… make it less breaking

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.

Closes #19141.

PiperOrigin-RevId: 569556377
Change-Id: Ic691c5585d9ba2d95b6ff3c32b9979f777147d8b

site/en/concepts/visibility.md

@@ -234,18 +234,22 @@ every instance of that rule. For example, a `cc_library` rule might create an
 implicit dependency from each of its rule targets to an executable target
 representing a C++ compiler.
 
-Currently, for visibility purposes these implicit dependencies are treated like
-any other dependency. This means that the target being depended on (such as our
-C++ compiler) must be visible to every instance of the rule. In practice this
-usually means the target must have public visibility.
-
-You can change this behavior by setting
-[`--incompatible_visibility_private_attributes_at_definition`](https://github.com/bazelbuild/proposals/blob/master/designs/2019-10-15-tool-visibility.md){: .external}. When enabled, the
-target in question need only be visible to the rule declaring it an implicit
-dependency. That is, it must be visible to the package containing the `.bzl`
-file in which the rule is defined. In our example, the C++ compiler could be
-private so long as it lives in the same package as the definition of the
-`cc_library` rule.
+The visibility of such an implicit dependency is checked with respect to the
+package containing the `.bzl` file in which the rule (or aspect) is defined. In
+our example, the C++ compiler could be private so long as it lives in the same
+package as the definition of the `cc_library` rule. As a fallback, if the
+implicit dependency is not visible from the definition, it is checked with
+respect to the `cc_library` target.
+
+You can change this behavior by disabling
+[`--incompatible_visibility_private_attributes_at_definition`](https://github.com/bazelbuild/proposals/blob/master/designs/2019-10-15-tool-visibility.md){: .external}.
+When disabled, implicit dependencies are treated like any other dependency.
+This means that the target being depended on (such as our C++ compiler) must be
+visible to every instance of the rule. In practice this usually means the target
+must have public visibility.
+
+If you want to restrict the usage of a rule to certain packages, use
+[load visibility](#load-visibility) instead.
 
 ## Load visibility {:#load-visibility}
 

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java

@@ -133,8 +133,14 @@ private void validateDirectPrerequisiteVisibility(
         implicitDefinition =
             checkNotNull(rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel());
       }
+      // Check that the prerequisite is visible from the definition. As a fallback, check if the
+      // prerequisite is visible from the target so that adopting this new style of checking
+      // visibility is not a breaking change.
       if (implicitDefinition != null
-          && !RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())) {
+          && !RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())
+          && !context.isVisible(prerequisite.getConfiguredTarget())) {
+        // In the error message, always suggest making the prerequisite visible from the definition,
+        // not the target.
         handleVisibilityConflict(context, prerequisite, implicitDefinition);
       }
     }

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -444,13 +444,13 @@ public final class BuildLanguageOptions extends OptionsBase {
 
   @Option(
       name = "incompatible_visibility_private_attributes_at_definition",
-      defaultValue = "false",
+      defaultValue = "true",
       documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
       effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
       metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
       help =
           "If set to true, the visibility of private rule attributes is checked with respect "
-              + "to the rule definition, rather than the rule usage.")
+              + "to the rule definition, falling back to rule usage if not visible.")
   public boolean incompatibleVisibilityPrivateAttributesAtDefinition;
 
   @Option(
@@ -882,7 +882,7 @@ public StarlarkSemantics toStarlarkSemantics() {
   public static final String INCOMPATIBLE_UNAMBIGUOUS_LABEL_STRINGIFICATION =
       "+incompatible_unambiguous_label_stringification";
   public static final String INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION =
-      "-incompatible_visibility_private_attributes_at_definition";
+      "+incompatible_visibility_private_attributes_at_definition";
   public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS =
       "-incompatible_top_level_aspects_require_providers";
   public static final String INCOMPATIBLE_DISABLE_STARLARK_HOST_TRANSITIONS =

src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java

@@ -93,14 +93,15 @@ public void testToolVisibilityUseCheckAtUse() throws Exception {
   }
 
   @Test
-  public void testToolVisibilityUseCheckAtRule() throws Exception {
+  public void testToolVisibilityUseCheckAtRule_fallbackToUse() throws Exception {
     setupArgsScenario();
     scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])");
     scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//use:__pkg__'])");
     useConfiguration("--incompatible_visibility_private_attributes_at_definition");
 
-    reporter.removeHandler(failFastHandler);
-    assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
+    update("//use:world");
+
+    assertThat(hasErrors(getConfiguredTarget("//use:world"))).isFalse();
   }
 
   @Test
@@ -274,7 +275,7 @@ public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exc
   }
 
   @Ignore(
-      "TODO(b/206127051): The aspects path implicit dependendencies with same name are incorrectly"
+      "TODO(b/206127051): The aspects path implicit dependencies with same name are incorrectly"
           + " merged, enable this test when this is fixed.")
   @Test
   public void testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision()
@@ -402,7 +403,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_outerAspectToolNotVi
         "  visibility = [",
         "    '//inner_aspect:__pkg__',",
         "    '//rule:__pkg__',",
-        "    '//foo:__pkg__',",
         "  ],",
         ")",
         "sh_binary(",
@@ -485,7 +485,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_innerAspectToolNotVi
         "  visibility = [",
         "    '//outer_aspect:__pkg__',",
         "    '//rule:__pkg__',",
-        "    '//foo:__pkg__',",
         "  ],",
         ")",
         "sh_binary(",
@@ -568,7 +567,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_ruleToolNotVisible()
         "  visibility = [",
         "    '//outer_aspect:__pkg__',",
         "    '//inner_aspect:__pkg__',",
-        "    '//foo:__pkg__',",
         "  ],",
         ")");
     useConfiguration("--incompatible_visibility_private_attributes_at_definition");