Improve error when a label is provided in config_setting's values

fmeum · copybara-github · commit bd5dbc5a4fb8 · 2023-09-11T06:49:24.000-07:00
Suggest to use `flag_values` instead. Previously, this only resulted in an ambiguous "Unrecognized option" error. Closes #19464. PiperOrigin-RevId: 564367170 Change-Id: Ibe9397be4300783ab6e5b5de9e08e40142de0b4c
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
@@ -67,6 +67,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import javax.annotation.Nullable;
 
 /**
@@ -83,6 +84,20 @@ public ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, ActionConflictException {
     AttributeMap attributes = NonconfigurableAttributeMapper.of(ruleContext.getRule());
 
+    Optional<String> likelyLabelInvalidSetting =
+        attributes.get(ConfigSettingRule.SETTINGS_ATTRIBUTE, Type.STRING_DICT).keySet().stream()
+            .filter(s -> s.startsWith("@") || s.startsWith("//") || s.startsWith(":"))
+            .findFirst();
+    if (likelyLabelInvalidSetting.isPresent()) {
+      ruleContext.attributeError(
+          ConfigSettingRule.SETTINGS_ATTRIBUTE,
+          String.format(
+              "'%s' is not a valid setting name, but appears to be a label. Did you mean to place"
+                  + " it in %s instead?",
+              likelyLabelInvalidSetting.get(), ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE));
+      return null;
+    }
+
     // Get the built-in Blaze flag settings that match this rule.
     ImmutableMultimap<String, String> nativeFlagSettings =
         ImmutableMultimap.<String, String>builder()
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
@@ -2317,4 +2317,20 @@ public void singleValueThatLooksLikeMultiValueIsOkay() throws Exception {
     assertThat(getConfiguredTarget("//test:fg")).isNotNull();
     assertNoEvents();
   }
+
+  @Test
+  public void labelInValuesError() throws Exception {
+    scratch.file(
+        "test/BUILD",
+        "config_setting(",
+        "    name = 'match',",
+        "    values = {'//foo:bar': 'value'},",
+        ")");
+    reporter.removeHandler(failFastHandler); // expect errors
+    assertThat(getConfiguredTarget("//test:match")).isNull();
+    assertContainsEvent(
+        "in values attribute of config_setting rule //test:match: '//foo:bar' is"
+            + " not a valid setting name, but appears to be a label. Did you mean to place it in"
+            + " flag_values instead?");
+  }
 }
