Roll forward config_setting visibility enforcement behind a flag.

This was rolled back in 36d228b
 because of depot breakages.

This version adds incompatible flags to safely introduce enforcement.
See #12932 and
#12933 for flag settings.

*** Original change description ***

Roll back #12877.

***

Fixes #12669.

RELNOTES: enforce config_setting visibility. See #12932 for details.
PiperOrigin-RevId: 354930807

Author: gregestren

site/docs/visibility.md

@@ -59,6 +59,25 @@ specified in the [`package`](be/functions.html#package) statement of the
 target's BUILD file. If there is no such `default_visibility` declaration, the
 visibility is `//visibility:private`.
 
+`config_setting` visibility has historically not been enforced.
+`--incompatible_enforce_config_setting_visibility` and
+`--incompatible_config_setting_private_default_visibility` provide migration
+logic for converging with other rules.
+
+If `--incompatible_enforce_config_setting_visibility=false`, every
+`config_setting` is unconditionally visible to all targets.
+
+Else if `--incompatible_config_setting_private_default_visibility=false`, any
+`config_setting` that doesn't explicitly set visibility is `//visibility:public`
+(ignoring package [`default_visibility`](be/functions.html#package.default_visibility)).
+
+Else if `--incompatible_config_setting_private_default_visibility=true`,
+`config_setting` uses the same visibility logic as all other rules.
+
+Best practice is to treat all `config_setting` targets like other rules:
+explicitly set `visibility` on any `config_setting` used anywhere outside its
+package.
+
 ### Example
 
 File `//frobber/bin/BUILD`:

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -292,6 +292,7 @@ java_library(
         ":buildinfo/build_info_key",
         ":config/build_configuration",
         ":config/build_options",
+        ":config/config_conditions",
         ":config/config_matching_provider",
         ":config/core_options",
         ":config/execution_transition_factory",
@@ -1619,6 +1620,20 @@ java_library(
     ],
 )
 
+java_library(
+    name = "config/config_conditions",
+    srcs = ["config/ConfigConditions.java"],
+    deps = [
+        ":config/config_matching_provider",
+        ":configured_target",
+        "//src/main/java/com/google/devtools/build/lib/analysis/platform",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
+        "//third_party:auto_value",
+        "//third_party:guava",
+    ],
+)
+
 java_library(
     name = "config/core_option_converters",
     srcs = ["config/CoreOptionConverters.java"],

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

@@ -27,7 +27,7 @@
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
-import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
+import com.google.devtools.build.lib.analysis.config.ConfigConditions;
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.RequiredFragmentsUtil;
 import com.google.devtools.build.lib.analysis.configuredtargets.EnvironmentGroupConfiguredTarget;
@@ -185,7 +185,7 @@ public final ConfiguredTarget createConfiguredTarget(
       BuildConfiguration hostConfig,
       ConfiguredTargetKey configuredTargetKey,
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
-      ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+      ConfigConditions configConditions,
       @Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
       ExecGroupCollection.Builder execGroupCollectionBuilder)
       throws InterruptedException, ActionConflictException, InvalidExecGroupException {
@@ -293,7 +293,7 @@ private ConfiguredTarget createRule(
       BuildConfiguration hostConfiguration,
       ConfiguredTargetKey configuredTargetKey,
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
-      ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+      ConfigConditions configConditions,
       @Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
       ExecGroupCollection.Builder execGroupCollectionBuilder)
       throws InterruptedException, ActionConflictException, InvalidExecGroupException {
@@ -323,7 +323,7 @@ private ConfiguredTarget createRule(
                     configuration,
                     ruleClassProvider.getUniversalFragments(),
                     configurationFragmentPolicy,
-                    configConditions,
+                    configConditions.asProviders(),
                     prerequisiteMap.values()))
             .build();
 
@@ -498,7 +498,7 @@ public ConfiguredAspect createAspect(
       ConfiguredAspectFactory aspectFactory,
       Aspect aspect,
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
-      ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+      ConfigConditions configConditions,
       @Nullable ResolvedToolchainContext toolchainContext,
       BuildConfiguration aspectConfiguration,
       BuildConfiguration hostConfiguration,
@@ -541,7 +541,7 @@ public ConfiguredAspect createAspect(
                     aspectConfiguration,
                     ruleClassProvider.getUniversalFragments(),
                     aspect.getDefinition().getConfigurationFragmentPolicy(),
-                    configConditions,
+                    configConditions.asProviders(),
                     prerequisiteMap.values()))
             .build();
 

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

@@ -47,6 +47,7 @@
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.config.ConfigConditions;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.CoreOptions.IncludeConfigFragmentsEnum;
@@ -79,6 +80,7 @@
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.OutputFile;
+import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.RequiredProviders;
@@ -1601,6 +1603,7 @@ public static final class Builder implements RuleErrorConsumer  {
     private final PrerequisiteValidator prerequisiteValidator;
     private final RuleErrorConsumer reporter;
     private OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap;
+    private ConfigConditions configConditions;
     private ImmutableMap<Label, ConfigMatchingProvider> configConditions = ImmutableMap.of();
     private NestedSet<PackageGroupContents> visibility;
     private ImmutableMap<String, Attribute> aspectAttributes;
@@ -1645,17 +1648,27 @@ public RuleContext build() throws InvalidExecGroupException {
       Preconditions.checkNotNull(visibility);
       Preconditions.checkNotNull(constraintSemantics);
       AttributeMap attributes =
-          ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions);
+          ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions.asProviders());
       checkAttributesNonEmpty(attributes);
       ListMultimap<String, ConfiguredTargetAndData> targetMap = createTargetMap();
+      // This conditionally checks visibility on config_setting rules based on
+      // --config_setting_visibility_policy. This should be removed as soon as it's deemed safe
+      // to unconditionally check visibility. See https://github.com/bazelbuild/bazel/issues/12669.
+      if (target.getPackage().getConfigSettingVisibilityPolicy()
+          != ConfigSettingVisibilityPolicy.LEGACY_OFF) {
+        Attribute configSettingAttr = attributes.getAttributeDefinition("$config_dependencies");
+        for (ConfiguredTargetAndData condition : configConditions.asConfiguredTargets().values()) {
+          validateDirectPrerequisite(configSettingAttr, condition);
+        }
+      }
       ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
-          createFilesetEntryMap(target.getAssociatedRule(), configConditions);
+          createFilesetEntryMap(target.getAssociatedRule(), configConditions.asProviders());
       return new RuleContext(
           this,
           attributes,
           targetMap,
           filesetEntryMap,
-          configConditions,
+          configConditions.asProviders(),
           universalFragments,
           getRuleClassNameForLogging(),
           actionOwnerSymbol,
@@ -1726,11 +1739,10 @@ public Builder setAspectAttributes(Map<String, Attribute> aspectAttributes) {
     }
 
     /**
-     * Sets the configuration conditions needed to determine which paths to follow for this
-     * rule's configurable attributes.
+     * Sets the configuration conditions needed to determine which paths to follow for this rule's
+     * configurable attributes.
      */
-    public Builder setConfigConditions(
-        ImmutableMap<Label, ConfigMatchingProvider> configConditions) {
+    public Builder setConfigConditions(ConfigConditions configConditions) {
       this.configConditions = Preconditions.checkNotNull(configConditions);
       return this;
     }

src/main/java/com/google/devtools/build/lib/analysis/config/ConfigConditions.java

@@ -0,0 +1,81 @@
+// Copyright 2021 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.analysis.config;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+
+/**
+ * Utility class for temporarily tracking {@code select()} keys' {@link ConfigMatchingProvider}s and
+ * {@link ConfiguredTarget}s.
+ *
+ * <p>This is a utility class because its only purpose is to maintain {@link ConfiguredTarget} long
+ * enough for {@link RuleContext.Builder} to do prerequisite validation on it (for example,
+ * visibility checks).
+ *
+ * <p>Once {@link RuleContext} is instantiated, it should only have access to {@link
+ * ConfigMatchingProvider}, on the principle that providers are the correct interfaces for storing
+ * and sharing target metadata. {@link ConfiguredTarget} isn't meant to persist that long.
+ */
+@AutoValue
+public abstract class ConfigConditions {
+  public abstract ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets();
+
+  public abstract ImmutableMap<Label, ConfigMatchingProvider> asProviders();
+
+  public static ConfigConditions create(
+      ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets,
+      ImmutableMap<Label, ConfigMatchingProvider> asProviders) {
+    return new AutoValue_ConfigConditions(asConfiguredTargets, asProviders);
+  }
+
+  public static final ConfigConditions EMPTY =
+      ConfigConditions.create(ImmutableMap.of(), ImmutableMap.of());
+
+  /** Exception for when a {@code select()} has an invalid key (for example, wrong target type). */
+  public static class InvalidConditionException extends Exception {}
+
+  /**
+   * Returns a {@link ConfigMatchingProvider} from the given configured target if appropriate, else
+   * triggers a {@link InvalidConditionException}.
+   *
+   * <p>This is the canonical place to extract {@link ConfigMatchingProvider}s from configured
+   * targets. It's not as simple as {@link ConfiguredTarget#getProvider}.
+   */
+  public static ConfigMatchingProvider fromConfiguredTarget(
+      ConfiguredTargetAndData selectKey, PlatformInfo targetPlatform)
+      throws InvalidConditionException {
+    ConfiguredTarget selectable = selectKey.getConfiguredTarget();
+    // The below handles config_setting (which natively provides ConfigMatchingProvider) and
+    // constraint_value (which needs a custom-built ConfigMatchingProvider).
+    ConfigMatchingProvider matchingProvider = selectable.getProvider(ConfigMatchingProvider.class);
+    if (matchingProvider != null) {
+      return matchingProvider;
+    }
+    ConstraintValueInfo constraintValueInfo = selectable.get(ConstraintValueInfo.PROVIDER);
+    if (constraintValueInfo != null && targetPlatform != null) {
+      // If platformInfo == null, that means the owning target doesn't invoke toolchain
+      // resolution, in which case depending on a constraint_value is nonsensical.
+      return constraintValueInfo.configMatchingProvider(targetPlatform);
+    }
+
+    // Not a valid provider for configuration conditions.
+    throw new InvalidConditionException();
+  }
+}

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -143,6 +143,24 @@ private NameConflictException(String message) {
   private RuleVisibility defaultVisibility;
   private boolean defaultVisibilitySet;
 
+  /**
+   * How to enforce config_setting visibility settings.
+   *
+   * <p>This is a temporary setting in service of https://github.com/bazelbuild/bazel/issues/12669.
+   * After enough depot cleanup, config_setting will have the same visibility enforcement as all
+   * other rules.
+   */
+  public enum ConfigSettingVisibilityPolicy {
+    /** Don't enforce visibility for any config_setting. */
+    LEGACY_OFF,
+    /** Honor explicit visibility settings on config_setting, else  use //visibility:public. */
+    DEFAULT_PUBLIC,
+    /** Enforce config_setting visibility exactly the same as all other rules. */
+    DEFAULT_STANDARD
+  }
+
+  private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
+
   /**
    * Default package-level 'testonly' value for rules that do not specify it.
    */
@@ -436,6 +454,7 @@ private void finishInit(Builder builder) {
     this.targets = ImmutableSortedKeyMap.copyOf(builder.targets);
     this.defaultVisibility = builder.defaultVisibility;
     this.defaultVisibilitySet = builder.defaultVisibilitySet;
+    this.configSettingVisibilityPolicy = builder.configSettingVisibilityPolicy;
     if (builder.defaultCopts == null) {
       this.defaultCopts = ImmutableList.of();
     } else {
@@ -700,6 +719,14 @@ public RuleVisibility getDefaultVisibility() {
     return defaultVisibility;
   }
 
+  /**
+   * How to enforce visibility on <code>config_setting</code> See
+   * {@link ConfigSettingVisibilityPolicy} for details.
+   */
+  public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() {
+    return configSettingVisibilityPolicy;
+  }
+
   /**
    * Returns the default testonly value.
    */
@@ -920,6 +947,7 @@ public boolean recordLoadedModules() {
     // serialized representation is deterministic.
     private final TreeMap<String, String> makeEnv = new TreeMap<>();
     private RuleVisibility defaultVisibility = ConstantRuleVisibility.PRIVATE;
+    private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
     private boolean defaultVisibilitySet;
     private List<String> defaultCopts = null;
     private final List<String> features = new ArrayList<>();
@@ -1151,6 +1179,12 @@ Builder setDefaultVisibilitySet(boolean defaultVisibilitySet) {
       return this;
     }
 
+    /** Sets visibility enforcement policy for <code>config_setting</code>. */
+    public Builder setConfigSettingVisibilityPolicy(ConfigSettingVisibilityPolicy policy) {
+      this.configSettingVisibilityPolicy = policy;
+      return this;
+    }
+
     /** Sets the default value of 'testonly'. Rule-level 'testonly' will override this. */
     Builder setDefaultTestonly(boolean defaultTestonly) {
       pkg.setDefaultTestOnly(defaultTestonly);

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -842,7 +842,8 @@ Package.Builder evaluateBuildFile(
             // and can be derived from Package.loads (if available) on demand.
             .setStarlarkFileDependencies(transitiveClosureOfLabels(loadedModules))
             .setThirdPartyLicenceExistencePolicy(
-                ruleClassProvider.getThirdPartyLicenseExistencePolicy());
+                ruleClassProvider.getThirdPartyLicenseExistencePolicy())
+            .setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy);
     if (packageSettings.recordLoadedModules()) {
       pkgBuilder.setLoads(loadedModules);
     }

src/main/java/com/google/devtools/build/lib/packages/Rule.java

@@ -32,6 +32,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.License.DistributionType;
+import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
 import com.google.devtools.build.lib.util.BinaryPredicate;
 import java.util.Collection;
 import java.util.LinkedHashSet;
@@ -728,10 +729,27 @@ public String toString() {
    */
   @Override
   public RuleVisibility getVisibility() {
+    // Temporary logic to relax config_setting's visibility enforcement while depot migrations set
+    // visibility settings properly (legacy code may have visibility settings that would break if
+    // enforced). See https://github.com/bazelbuild/bazel/issues/12669. Ultimately this entire
+    // conditional should be removed.
+    if (ruleClass.getName().equals("config_setting")) {
+      ConfigSettingVisibilityPolicy policy = getPackage().getConfigSettingVisibilityPolicy();
+      if (policy == ConfigSettingVisibilityPolicy.LEGACY_OFF) {
+        return ConstantRuleVisibility.PUBLIC; // Always public, no matter what.
+      } else if (visibility != null) {
+        return visibility; // Use explicitly set visibility
+      } else if (policy == ConfigSettingVisibilityPolicy.DEFAULT_PUBLIC) {
+        return ConstantRuleVisibility.PUBLIC; // Default: //visibility:public.
+      } else {
+        return pkg.getDefaultVisibility(); // Default: same as all other rules.
+      }
+    }
+
+    // All other rules.
     if (visibility != null) {
       return visibility;
     }
-
     return pkg.getDefaultVisibility();
   }