Enforce visibility on select() keys

site/docs/visibility.md

@@ -62,6 +62,13 @@ 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` targets default to `//visibility:public, regardless of how the
+package's [`default_visibility`](be/functions.html#package.default_visibility)
+is set. This is purely for legacy reasons. Best practice is to treat
+`config_setting` targets as if they use the private default: any
+`config_setting` intended for use by other packages should set its visibility
+explicitly.
+
 ### 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",
@@ -1596,6 +1597,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.RuleContext.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;
@@ -186,7 +186,7 @@ public final ConfiguredTarget createConfiguredTarget(
       BuildConfiguration hostConfig,
       ConfiguredTargetKey configuredTargetKey,
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
-      ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+      ConfigConditions configConditions,
       @Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts)
       throws InterruptedException, ActionConflictException, InvalidExecGroupException {
     if (target instanceof Rule) {
@@ -292,7 +292,7 @@ private ConfiguredTarget createRule(
       BuildConfiguration hostConfiguration,
       ConfiguredTargetKey configuredTargetKey,
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
-      ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+      ConfigConditions configConditions,
       @Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts)
       throws InterruptedException, ActionConflictException, InvalidExecGroupException {
     ConfigurationFragmentPolicy configurationFragmentPolicy =
@@ -323,7 +323,7 @@ private ConfiguredTarget createRule(
                     configuration,
                     ruleClassProvider.getUniversalFragments(),
                     configurationFragmentPolicy,
-                    configConditions,
+                    configConditions.asProviders(),
                     prerequisiteMap.values()))
             .build();
 
@@ -500,7 +500,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,
@@ -545,7 +545,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

@@ -46,6 +46,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;
@@ -1764,7 +1765,7 @@ public static final class Builder implements RuleErrorConsumer {
     private final PrerequisiteValidator prerequisiteValidator;
     private final RuleErrorConsumer reporter;
     private OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap;
-    private ImmutableMap<Label, ConfigMatchingProvider> configConditions = ImmutableMap.of();
+    private ConfigConditions configConditions;
     private String toolsRepository;
     private StarlarkSemantics starlarkSemantics;
     private Mutability mutability;
@@ -1813,11 +1814,15 @@ 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();
+      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());
       if (rawExecProperties == null) {
         if (!attributes.has(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)) {
           rawExecProperties = ImmutableMap.of();
@@ -1831,7 +1836,7 @@ public RuleContext build() throws InvalidExecGroupException {
           attributes,
           targetMap,
           filesetEntryMap,
-          configConditions,
+          configConditions.asProviders(),
           universalFragments,
           getRuleClassNameForLogging(),
           actionOwnerSymbol,
@@ -1906,11 +1911,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/Rule.java

@@ -732,7 +732,11 @@ public RuleVisibility getVisibility() {
       return visibility;
     }
 
-    return pkg.getDefaultVisibility();
+    // TODO(bazel-team): give config_setting the same default visibility as everything else. The
+    // only reason this isn't trivial is depot cleanup.
+    return ruleClass.getName().equals("config_setting")
+        ? ConstantRuleVisibility.PUBLIC
+        : pkg.getDefaultVisibility();
   }
 
   public boolean isVisibilitySpecified() {

src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java

@@ -37,7 +37,7 @@
 import com.google.devtools.build.lib.analysis.ToolchainCollection;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
-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.DependencyEvaluationException;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget;
@@ -403,7 +403,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       }
 
       // Get the configuration targets that trigger this rule's configurable attributes.
-      ImmutableMap<Label, ConfigMatchingProvider> configConditions =
+      ConfigConditions configConditions =
           ConfiguredTargetFunction.getConfigConditions(
               env,
               originalTargetAndAspectConfiguration,
@@ -423,7 +423,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                 resolver,
                 originalTargetAndAspectConfiguration,
                 aspectPath,
-                configConditions,
+                configConditions.asProviders(),
                 unloadedToolchainContext == null
                     ? null
                     : ToolchainCollection.builder()
@@ -640,7 +640,7 @@ private AspectValue createAspect(
       ConfiguredAspectFactory aspectFactory,
       ConfiguredTargetAndData associatedTarget,
       BuildConfiguration aspectConfiguration,
-      ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+      ConfigConditions configConditions,
       ResolvedToolchainContext toolchainContext,
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> directDeps,
       @Nullable NestedSetBuilder<Package> transitivePackagesForPackageRootResolution)

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

@@ -234,6 +234,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_key",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",