Fix label_flag and label_setting to not have a dependency on the default

value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes bazelbuild#11291. Closes bazelbuild#13372. PiperOrigin-RevId: 369445041
fweikert · Apr 20, 2021 · c9f9eed · c9f9eed
1 parent 660e8a5
commit c9f9eed
Show file tree

Hide file tree

Showing 5 changed files with 14 additions and 11 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
@@ -16,6 +16,7 @@
 
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
+import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
 import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
 import static com.google.devtools.build.lib.packages.Type.INTEGER;
 import static com.google.devtools.build.lib.packages.Type.STRING;
@@ -57,6 +58,7 @@ public class CoreOptionConverters {
           .put(STRING_LIST, new CommaSeparatedOptionListConverter())
           .put(LABEL, new LabelConverter())
           .put(LABEL_LIST, new LabelListConverter())
+          .put(NODEP_LABEL, new LabelConverter())
           .build();
 
   /** A converter from strings to Starlark int values. */

diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java b/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java
@@ -14,6 +14,8 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.packages.Type.LabelClass;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkConfigApi.BuildSettingApi;
 import net.starlark.java.eval.Printer;
 
@@ -37,6 +39,9 @@ public static BuildSetting create(boolean isFlag, Type<?> type, boolean allowMul
   }
 
   public static BuildSetting create(boolean isFlag, Type<?> type) {
+    Preconditions.checkState(
+        type.getLabelClass() != LabelClass.DEPENDENCY,
+        "Build settings should not create a dependency with their default attribute");
     return new BuildSetting(isFlag, type, /* allowMultiple= */ false);
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.packages;
 
 import static com.google.common.collect.Streams.stream;
-import static com.google.devtools.build.lib.packages.Attribute.ANY_RULE;
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
@@ -58,7 +57,6 @@
 import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
-import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.StringUtil;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
@@ -941,10 +939,6 @@ public RuleClass build(String name, String key) {
             attr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, type)
                 .nonconfigurable(BUILD_SETTING_DEFAULT_NONCONFIGURABLE)
                 .mandatory();
-        if (BuildType.isLabelType(type)) {
-          defaultAttrBuilder.allowedFileTypes(FileTypeSet.ANY_FILE);
-          defaultAttrBuilder.allowedRuleClasses(ANY_RULE);
-        }
         this.add(defaultAttrBuilder);
 
         this.add(

diff --git a/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java b/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java
@@ -15,6 +15,7 @@
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
 import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
 
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
@@ -56,15 +57,15 @@ public class LabelBuildSettings {
           null,
           (rule, attributes, configuration) -> {
             if (rule == null || configuration == null) {
-              return attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL);
+              return attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL);
             }
             Object commandLineValue =
                 configuration.getOptions().getStarlarkOptions().get(rule.getLabel());
             Label asLabel;
             try {
               asLabel =
                   commandLineValue == null
-                      ? attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL)
+                      ? attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL)
                       : LABEL.convert(commandLineValue, "label_flag value resolution");
             } catch (ConversionException e) {
               throw new IllegalStateException(
@@ -81,7 +82,7 @@ private static RuleClass buildRuleClass(RuleClass.Builder builder, boolean flag)
         .removeAttribute("licenses")
         .removeAttribute("distribs")
         .add(attr(":alias", LABEL).value(ACTUAL))
-        .setBuildSetting(BuildSetting.create(flag, LABEL))
+        .setBuildSetting(BuildSetting.create(flag, NODEP_LABEL))
         .canHaveAnyProvider()
         .useToolchainResolution(ToolchainResolutionMode.DISABLED)
         .build();

diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -18,6 +18,7 @@
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
+import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.OUTPUT_LIST;
 import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.substitutePlaceholderIntoTemplate;
 import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
@@ -1097,7 +1098,7 @@ public void testBuildSetting_createsDefaultAttribute() {
         new RuleClass.Builder("label_flag", RuleClassType.NORMAL, false)
             .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
             .add(attr("tags", STRING_LIST))
-            .setBuildSetting(BuildSetting.create(true, LABEL))
+            .setBuildSetting(BuildSetting.create(true, NODEP_LABEL))
             .build();
     RuleClass stringSetting =
         new RuleClass.Builder("string_setting", RuleClassType.NORMAL, false)
@@ -1106,7 +1107,7 @@ public void testBuildSetting_createsDefaultAttribute() {
             .setBuildSetting(BuildSetting.create(false, STRING))
             .build();
 
-    assertThat(labelFlag.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL)).isTrue();
+    assertThat(labelFlag.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL)).isTrue();
     assertThat(stringSetting.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, STRING)).isTrue();
   }