Simplify select tests

Summary: Pass resolved select keys to `select_the_most_specific` instead of passing `ResolvedConfigurationSettings` which contains incompatible keys too.

Reviewed By: JakobDegen

Differential Revision: D56121980

fbshipit-source-id: 1d278c1a746a6fac92303f20a2cbc05a37730a79

app/buck2_core/src/configuration/config_setting.rs

@@ -41,6 +41,15 @@ impl ConfigSettingData {
             && Self::is_subset(&that.constraints, &self.constraints)
             && Self::is_subset(&that.buckconfigs, &self.buckconfigs)
     }
+
+    pub fn testing_new(
+        constraint_values: BTreeMap<ConstraintKey, ConstraintValue>,
+    ) -> ConfigSettingData {
+        ConfigSettingData {
+            constraints: constraint_values,
+            buckconfigs: BTreeMap::new(),
+        }
+    }
 }
 
 #[cfg(test)]

app/buck2_node/src/attrs/coerced_attr.rs

@@ -59,7 +59,6 @@ use crate::attrs::json::ToJsonWithContext;
 use crate::attrs::serialize::AttrSerializeWithContext;
 use crate::attrs::traversal::CoercedAttrTraversal;
 use crate::configuration::resolved::ConfigurationSettingKey;
-use crate::configuration::resolved::ResolvedConfigurationSettings;
 use crate::metadata::map::MetadataMap;
 use crate::visibility::VisibilitySpecification;
 use crate::visibility::WithinViewSpecification;
@@ -485,21 +484,32 @@ impl CoercedAttr {
     }
 
     /// If more than one select key matches, select the most specific.
-    pub fn select_the_most_specific<'a>(
-        resolved_cfg_settings: &ResolvedConfigurationSettings,
-        select_entries: &'a [(ConfigurationSettingKey, CoercedAttr)],
+    pub fn select_the_most_specific<'a, 'x>(
+        select_entries: impl IntoIterator<
+            Item = (
+                &'x ConfigurationSettingKey,
+                &'x ConfigSettingData,
+                &'a CoercedAttr,
+            ),
+        >,
     ) -> anyhow::Result<Option<&'a CoercedAttr>> {
-        let mut matching: Option<(&ConfigurationSettingKey, &ConfigSettingData, &CoercedAttr)> =
-            None;
-        for (k, v) in select_entries {
-            matching = match (resolved_cfg_settings.setting_matches(k), matching) {
-                (None, matching) => matching,
-                (Some(conf), None) => Some((k, conf, v)),
-                (Some(conf), Some((prev_k, prev_conf, prev_v))) => {
+        let mut select_entries = select_entries.into_iter();
+        let Some(mut matching): Option<(
+            &ConfigurationSettingKey,
+            &ConfigSettingData,
+            &CoercedAttr,
+        )> = select_entries.next() else {
+            return Ok(None);
+        };
+
+        for (k, conf, v) in select_entries {
+            let (prev_k, prev_conf, prev_v) = matching;
+            matching = {
+                {
                     if conf.refines(prev_conf) {
-                        Some((k, conf, v))
+                        (k, conf, v)
                     } else if prev_conf.refines(conf) {
-                        Some((prev_k, prev_conf, prev_v))
+                        (prev_k, prev_conf, prev_v)
                     } else {
                         return Err(SelectError::TwoKeysDoNotRefineEachOther(
                             prev_k.to_string(),
@@ -510,15 +520,21 @@ impl CoercedAttr {
                 }
             }
         }
-        Ok(matching.map(|(_k, _conf, v)| v))
+        Ok(Some(matching.2))
     }
 
     fn select<'a>(
         ctx: &dyn AttrConfigurationContext,
         select: &'a CoercedSelector,
     ) -> anyhow::Result<&'a CoercedAttr> {
         let CoercedSelector { entries, default } = select;
-        if let Some(v) = Self::select_the_most_specific(ctx.resolved_cfg_settings(), entries)? {
+        let resolved_cfg_settings = ctx.resolved_cfg_settings();
+        let resolved_entries = entries.iter().filter_map(|(k, v)| {
+            resolved_cfg_settings
+                .setting_matches(k)
+                .map(|conf| (k, conf, v))
+        });
+        if let Some(v) = Self::select_the_most_specific(resolved_entries)? {
             Ok(v)
         } else {
             default.as_ref().ok_or_else(|| {

app/buck2_node/src/configuration/resolved.rs

@@ -7,14 +7,11 @@
  * of this source tree.
  */
 
-use std::collections::BTreeMap;
 use std::hash::Hash;
 use std::sync::Arc;
 
 use allocative::Allocative;
 use buck2_core::configuration::config_setting::ConfigSettingData;
-use buck2_core::configuration::constraints::ConstraintKey;
-use buck2_core::configuration::constraints::ConstraintValue;
 use buck2_core::configuration::pair::ConfigurationNoExec;
 use buck2_core::target::label::TargetLabel;
 use dupe::Dupe;
@@ -113,13 +110,4 @@ impl ConfigurationNode {
     pub fn configuration_data(&self) -> Option<&ConfigSettingData> {
         self.0.config_setting.as_ref()
     }
-
-    pub fn testing_new_constraints(
-        constraints: BTreeMap<ConstraintKey, ConstraintValue>,
-    ) -> ConfigurationNode {
-        ConfigurationNode::new(Some(ConfigSettingData {
-            constraints,
-            buckconfigs: BTreeMap::new(),
-        }))
-    }
 }

app/buck2_node_tests/BUCK

@@ -13,6 +13,5 @@ rust_unittest(
         "//buck2/app/buck2_node:buck2_node",
         "//buck2/app/buck2_util:buck2_util",
         "//buck2/gazebo/dupe:dupe",
-        "//buck2/starlark-rust/starlark_map:starlark_map",
     ],
 )

app/buck2_node_tests/Cargo.toml

@@ -10,7 +10,6 @@ version = "0.1.0"
 anyhow = { workspace = true }
 
 dupe = { workspace = true }
-starlark_map = { workspace = true }
 
 buck2_core = { workspace = true }
 buck2_node = { workspace = true }

app/buck2_node_tests/src/attrs/coerced_attr.rs

@@ -9,20 +9,18 @@
 
 use std::collections::BTreeMap;
 
+use buck2_core::configuration::config_setting::ConfigSettingData;
 use buck2_core::configuration::constraints::ConstraintKey;
 use buck2_core::configuration::constraints::ConstraintValue;
 use buck2_node::attrs::attr_type::bool::BoolLiteral;
 use buck2_node::attrs::attr_type::string::StringLiteral;
 use buck2_node::attrs::coerced_attr::CoercedAttr;
 use buck2_node::attrs::coerced_attr::CoercedSelector;
 use buck2_node::attrs::fmt_context::AttrFmtContext;
-use buck2_node::configuration::resolved::ConfigurationNode;
 use buck2_node::configuration::resolved::ConfigurationSettingKey;
-use buck2_node::configuration::resolved::ResolvedConfigurationSettings;
 use buck2_util::arc_str::ArcSlice;
 use buck2_util::arc_str::ArcStr;
 use dupe::Dupe;
-use starlark_map::unordered_map::UnorderedMap;
 
 #[test]
 fn selector_equals_accounts_for_ordering() {
@@ -93,66 +91,49 @@ fn select_the_most_specific() {
     let linux_arm64 = ConfigurationSettingKey::testing_parse("config//:linux-arm64");
     let linux_x86_64 = ConfigurationSettingKey::testing_parse("config//:linux-x86_64");
 
-    let resolved_cfg_settings = ResolvedConfigurationSettings::new(UnorderedMap::from_iter([
-        (
-            linux.dupe(),
-            ConfigurationNode::testing_new_constraints(BTreeMap::from_iter([(
-                c_os.dupe(),
-                c_linux.dupe(),
-            )])),
-        ),
-        (
-            linux_arm64.dupe(),
-            ConfigurationNode::testing_new_constraints(BTreeMap::from_iter([
-                (c_os.dupe(), c_linux.dupe()),
-                (c_cpu.dupe(), c_arm64.dupe()),
-            ])),
-        ),
-        (
-            linux_x86_64.dupe(),
-            ConfigurationNode::testing_new_constraints(BTreeMap::from_iter([
-                (c_os.dupe(), c_linux.dupe()),
-                (c_cpu.dupe(), c_x86_64.dupe()),
-            ])),
-        ),
+    let linux_data =
+        ConfigSettingData::testing_new(BTreeMap::from_iter([(c_os.dupe(), c_linux.dupe())]));
+    let linux_arm64_data = ConfigSettingData::testing_new(BTreeMap::from_iter([
+        (c_os.dupe(), c_linux.dupe()),
+        (c_cpu.dupe(), c_arm64.dupe()),
+    ]));
+    let linux_x86_64_data = ConfigSettingData::testing_new(BTreeMap::from_iter([
+        (c_os.dupe(), c_linux.dupe()),
+        (c_cpu.dupe(), c_x86_64.dupe()),
     ]));
 
-    fn literal_true() -> CoercedAttr {
-        CoercedAttr::Bool(BoolLiteral(true))
-    }
-    fn literal_str() -> CoercedAttr {
-        CoercedAttr::String(StringLiteral(ArcStr::from("linux")))
-    }
+    let literal_true = CoercedAttr::Bool(BoolLiteral(true));
+    let literal_str = CoercedAttr::String(StringLiteral(ArcStr::from("linux")));
 
     // Test more specific is selected even if it is not first.
-    let select_entries = Box::new([
-        (linux.dupe(), literal_true()),
-        (linux_x86_64.dupe(), literal_str()),
-    ]);
+    let select_entries = [
+        (&linux, &linux_data, &literal_true),
+        (&linux_x86_64, &linux_x86_64_data, &literal_str),
+    ];
     assert_eq!(
-        Some(&literal_str()),
-        CoercedAttr::select_the_most_specific(&resolved_cfg_settings, &*select_entries).unwrap()
+        Some(&literal_str),
+        CoercedAttr::select_the_most_specific(select_entries).unwrap()
     );
 
     // Test more specific is selected even if it is first.
-    let select_entries = Box::new([
-        (linux_x86_64.dupe(), literal_str()),
-        (linux.dupe(), literal_true()),
-    ]);
+    let select_entries = [
+        (&linux_x86_64, &linux_x86_64_data, &literal_str),
+        (&linux, &linux_data, &literal_true),
+    ];
     assert_eq!(
-        Some(&literal_str()),
-        CoercedAttr::select_the_most_specific(&resolved_cfg_settings, &*select_entries).unwrap()
+        Some(&literal_str),
+        CoercedAttr::select_the_most_specific(select_entries).unwrap()
     );
 
     // Conflicting keys.
-    let select_entries = Box::new([
-        (linux_arm64.dupe(), literal_true()),
-        (linux_x86_64.dupe(), literal_str()),
-    ]);
+    let select_entries = [
+        (&linux_arm64, &linux_arm64_data, &literal_true),
+        (&linux_x86_64, &linux_x86_64_data, &literal_str),
+    ];
     assert_eq!(
         "Both select keys `config//:linux-arm64` and `config//:linux-x86_64` \
             match the configuration, but neither is more specific",
-        CoercedAttr::select_the_most_specific(&resolved_cfg_settings, &*select_entries)
+        CoercedAttr::select_the_most_specific(select_entries)
             .unwrap_err()
             .to_string()
     );
@@ -173,40 +154,24 @@ fn test_select_refines_bug() {
     let x86_64 = ConfigurationSettingKey::testing_parse("config//:x86_64");
     let windows_x86_64 = ConfigurationSettingKey::testing_parse("config//:windows-x86_64");
 
+    let windows_data = ConfigSettingData::testing_new(BTreeMap::from_iter([c_windows.dupe()]));
+    let x86_64_data = ConfigSettingData::testing_new(BTreeMap::from_iter([c_x86_64.dupe()]));
+    let windows_x86_64_data =
+        ConfigSettingData::testing_new(BTreeMap::from_iter([c_windows, c_x86_64]));
+
+    let value_windows = CoercedAttr::String(StringLiteral(ArcStr::from("windows")));
+    let value_x86_64 = CoercedAttr::String(StringLiteral(ArcStr::from("x86_64")));
+    let value_windows_x86_64 = CoercedAttr::String(StringLiteral(ArcStr::from("windows-x86_64")));
     let select_entries = [
-        (
-            windows.dupe(),
-            CoercedAttr::String(StringLiteral(ArcStr::from("windows"))),
-        ),
-        (
-            x86_64.dupe(),
-            CoercedAttr::String(StringLiteral(ArcStr::from("x86_64"))),
-        ),
-        (
-            windows_x86_64.dupe(),
-            CoercedAttr::String(StringLiteral(ArcStr::from("windows-x86_64"))),
-        ),
+        (&windows, &windows_data, &value_windows),
+        (&x86_64, &x86_64_data, &value_x86_64),
+        (&windows_x86_64, &windows_x86_64_data, &value_windows_x86_64),
     ];
 
-    let resolved_cfg_settings = ResolvedConfigurationSettings::new(UnorderedMap::from_iter([
-        (
-            windows.dupe(),
-            ConfigurationNode::testing_new_constraints(BTreeMap::from_iter([c_windows.dupe()])),
-        ),
-        (
-            x86_64.dupe(),
-            ConfigurationNode::testing_new_constraints(BTreeMap::from_iter([c_x86_64.dupe()])),
-        ),
-        (
-            windows_x86_64.dupe(),
-            ConfigurationNode::testing_new_constraints(BTreeMap::from_iter([c_windows, c_x86_64])),
-        ),
-    ]));
-
     // TODO(nga): T177093673: this should select `config//:windows-x86_64`.
     assert_eq!(
         "Both select keys `config//:windows` and `config//:x86_64` match the configuration, but neither is more specific",
-        CoercedAttr::select_the_most_specific(&resolved_cfg_settings, &select_entries)
+        CoercedAttr::select_the_most_specific(select_entries)
             .unwrap_err()
             .to_string()
     );