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
(cherry picked from commit 0656103)

site/en/concepts/visibility.md

@@ -232,18 +232,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

@@ -86,10 +86,10 @@ private void validateDirectPrerequisiteVisibility(
 
       // Determine if we should use the new visibility rules for tools.
       boolean toolCheckAtDefinition =
-          context
-              .getStarlarkSemantics()
-              .getBool(
-                  BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION);
+              context
+                      .getStarlarkSemantics()
+                      .getBool(
+                              BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION);
 
       if (!toolCheckAtDefinition
               || !attribute.isImplicit()
@@ -119,8 +119,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

@@ -472,13 +472,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(
@@ -865,7 +865,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");