Merge list values across config files.

Fixes pantsbuild#14679

[ci skip-rust]

[ci skip-build-wheels]

src/python/pants/option/config.py

@@ -128,16 +128,22 @@ def update_seed_values(key: str, *, default_dir: str) -> None:
 
         return all_seed_values
 
-    def get(self, section, option) -> str | None:
+    def get(self, section, option, option_type: type) -> str | None:
         """Retrieves option from the specified section (or 'DEFAULT').
 
         If the specified section does not exist or is missing a definition for the option, the value
         is looked up in the DEFAULT section.  If there is still no definition found, returns None.
         """
-        for vals in reversed(self.values):
+        available_vals = []
+        for vals in self.values:
             val = vals.get_value(section, option)
             if val is not None:
-                return val
+                available_vals.append(val)
+        if available_vals:
+            if option_type == list:
+                return ",".join(available_vals)
+            else:
+                return available_vals[-1]
         return None
 
     def sources(self) -> list[str]:

src/python/pants/option/config_test.py

@@ -51,6 +51,13 @@ class ConfigFile:
         add = 0
         remove = 0
         nested = { nested_key = 'foo' }
+
+        [list_merging]
+        list1 = []
+        list2 = [1, 2]
+        list3.add = [3, 4]
+        list4.remove = [5]
+        list5 = [6, 7]
         """
     ),
     default_values={
@@ -70,6 +77,13 @@ class ConfigFile:
             "recursively_interpolated_from_section": "overridden_from_default is interpolated (again)",
         },
         "d": {"dict_val": "{'add': 0, 'remove': 0, 'nested': {'nested_key': 'foo'}"},
+        "list_merging": {
+            "list1": "[]",
+            "list2": "[1, 2]",
+            "list3": "+[3, 4]",
+            "list4": "-[5]",
+            "list5": "[6, 7]",
+        },
     },
 )
 
@@ -88,6 +102,14 @@ class ConfigFile:
         list.remove = [8, 9]
 
         [empty_section]
+
+        [list_merging]
+        list1 = [11, 22]
+        list2.add = [33]
+        list3.add = [8, 9]
+        list3.remove = [4, 55]
+        list4 = [66]
+        list6.add = [77, 88]
         """
     ),
     default_values={},
@@ -96,6 +118,13 @@ class ConfigFile:
         "b": {"preempt": "False"},
         "d": {"list": "+[0, 1],-[8, 9]"},
         "empty_section": {},
+        "list_merging": {
+            "list1": "[11, 22]",
+            "list2": "+[33]",
+            "list3": "+[8, 9],-[4, 55]",
+            "list4": "[66]",
+            "list6": "+[77, 88]",
+        },
     },
 )
 
@@ -118,43 +147,79 @@ def setUp(self) -> None:
         self.default_seed_values = Config._determine_seed_values(
             seed_values={"buildroot": "fake_buildroot"},
         )
-        self.expected_combined_values = {
-            **FILE_1.expected_options,
-            **FILE_2.expected_options,
-            "a": {**FILE_2.expected_options["a"], **FILE_1.expected_options["a"]},
+        self.expected_combined_values: dict[str, dict[str, str]] = {
+            "a": {
+                "list": '["1", "2", "3", "42"]',
+                "list2": "+[7, 8, 9]",
+                "list3": '-["x", "y", "z"]',
+                "fast": "True",
+            },
+            "b": {"preempt": "False"},
+            "c": {
+                "name": "overridden_from_default",
+                "interpolated_from_section": "overridden_from_default is interpolated",
+                "recursively_interpolated_from_section": "overridden_from_default is interpolated (again)",
+            },
+            "d": {
+                "dict_val": "{'add': 0, 'remove': 0, 'nested': {'nested_key': 'foo'}}",
+                "list": "+[0, 1],-[8, 9]",
+            },
+            "empty_section": {},
+            "list_merging": {
+                "list1": "[],[11, 22]",
+                "list2": "[1, 2],+[33]",
+                "list3": "+[3, 4],+[8, 9],-[4, 55]",
+                "list4": "-[5],[66]",
+                "list5": "[6, 7]",
+                "list6": "+[77, 88]",
+            },
         }
 
     def test_default_values(self) -> None:
         # This is used in `options_bootstrapper.py` to ignore default values when validating options.
-        file1_config_values = self.config.values[0]
-        file2_config_values = self.config.values[1]
+        file1_values = self.config.values[0]
+        file2_values = self.config.values[1]
         # NB: string interpolation should only happen when calling _ConfigValues.get_value(). The
         # values for _ConfigValues.defaults are not yet interpolated.
         default_file1_values_unexpanded = {
             **FILE_1.default_values,
             "path": "/a/b/%(answer)s",
             "embed": "%(path)s::foo",
         }
-        assert file1_config_values.defaults == {
+        assert file1_values.defaults == {
             **self.default_seed_values,
             **default_file1_values_unexpanded,
         }
-        assert file2_config_values.defaults == self.default_seed_values
+        assert file2_values.defaults == self.default_seed_values
 
     def test_get(self) -> None:
         # Check the DEFAULT section
         for option, value in {**self.default_seed_values, **FILE_1.default_values}.items():
-            assert self.config.get(section="DEFAULT", option=option) == value
+            assert (
+                self.config.get(
+                    section="DEFAULT",
+                    option=option,
+                    option_type=list if option.startswith("list") else str,
+                )
+                == value
+            )
         # Check the combined values, including that each section has the default seed values
         for section, section_values in self.expected_combined_values.items():
             for option, value in {**section_values, **self.default_seed_values}.items():
-                assert self.config.get(section=section, option=option) == value
+                assert (
+                    self.config.get(
+                        section=section,
+                        option=option,
+                        option_type=list if option.startswith("list") else str,
+                    )
+                    == value
+                )
         # Check that each section from file1 also has file1's default values, unless that section
         # explicitly overrides the default
         for section, section_values in FILE_1.expected_options.items():
             for option, default_value in FILE_1.default_values.items():
                 expected = default_value if option not in section_values else section_values[option]
-                assert self.config.get(section=section, option=option) == expected
+                assert self.config.get(section=section, option=option, option_type=str) == expected
 
     def test_empty(self) -> None:
         config = Config.load([])

src/python/pants/option/options_bootstrapper.py

@@ -170,7 +170,7 @@ def filecontent_for(path: str) -> FileContent:
             # https://github.com/pantsbuild/pants/pull/13228#discussion_r728223889
             alias_dict = parse_expression(
                 name="cli.alias",
-                val=post_bootstrap_config.get("cli", "alias") or "{}",
+                val=post_bootstrap_config.get("cli", "alias", dict) or "{}",
                 acceptable_types=dict,
             )
             alias = CliAlias.from_dict(alias_dict)

src/python/pants/option/options_test.py

@@ -795,9 +795,21 @@ def check(
         check(
             flags="--listy=+[8,9]",
             env_val="+[6,7]",
-            config_val="+[4,5]",
-            expected=[*default, 4, 5, 6, 7, 8, 9],
+            config_val="+[4,5],+[45]",
+            expected=[*default, 4, 5, 45, 6, 7, 8, 9],
+        )
+        check(
+            config_val="+[4,5],-[4]",
+            expected=[*default, 5],
         )
+        # TODO: Make this work.
+        #  Currently, splitting a string like this requires that all components be modifications
+        #  (i.e., starting with a + or a -). But it might be the case that a config file overrides
+        #  defaults and then another config file appends further, so we should support that.
+        # check(
+        #     config_val="[4,5],-[4]",
+        #     expected=[*default, 5],
+        # )
 
         # Appending and filtering across env, config and flags (in the right order).
         check(

src/python/pants/option/parser.py

@@ -584,8 +584,8 @@ def expand(val_or_str):
         # Get value from config files, and capture details about its derivation.
         config_details = None
         config_section = GLOBAL_SCOPE_CONFIG_SECTION if self._scope == GLOBAL_SCOPE else self._scope
-        config_default_val_or_str = expand(self._config.get(DEFAULT_SECTION, dest))
-        config_val_or_str = expand(self._config.get(config_section, dest))
+        config_default_val_or_str = expand(self._config.get(DEFAULT_SECTION, dest, type_arg))
+        config_val_or_str = expand(self._config.get(config_section, dest, type_arg))
         config_source_files = self._config.get_sources_for_option(
             config_section, dest
         ) or self._config.get_sources_for_option(DEFAULT_SECTION, dest)