Fix mixin arg merge order for lists and scalars (fixes #40)

Author: PiousCrossten

colcon_mixin/mixin/mixin_argument.py

@@ -181,16 +181,38 @@ def collect_parsers_by_verb(root, parsers, parent_verbs=()):
         # update args based on selected mixins
         if 'mixin_verb' in args:
             mixins = mixins_by_verb.get(args.mixin_verb, {})
+            context = '.'.join(args.mixin_verb)
+
+            # validate all mixins first
             for mixin in args.mixin or ():
                 if mixin not in mixins:
-                    context = '.'.join(args.mixin_verb)
                     self._parser.error(
                         "Mixin '{mixin}' is not available for '{context}'"
                         .format_map(locals()))
+
+            # pre-merge all mixin args in order before applying to final args:
+            # - lists are concatenated in mixin order (first mixin's values
+            #   come first)
+            # - scalars: later mixin overrides earlier mixin (last wins)
+            # this avoids the bug where iterative application causes lists to
+            # be reversed and scalars from later mixins to be silently skipped
+            merged_mixin_args = {}
+            for mixin in args.mixin or ():
                 mixin_args = mixins[mixin]
                 logger.debug(
                     "Using mixin '{mixin}': {mixin_args}".format_map(locals()))
-                self._update_args(args, mixin_args, '.'.join(args.mixin_verb))
+                for key, value in mixin_args.items():
+                    if key not in merged_mixin_args:
+                        merged_mixin_args[key] = value
+                    elif isinstance(value, list):
+                        merged_mixin_args[key] = merged_mixin_args[key] + value
+                    else:
+                        # later mixin overrides earlier mixin for scalars
+                        merged_mixin_args[key] = value
+
+            if merged_mixin_args:
+                self._update_args(
+                    args, merged_mixin_args, context)
 
         # undo default value wrapping injected in the add_argument() method
         for k, v in args.__dict__.items():

test/test_mixin_merge_order.py

@@ -0,0 +1,107 @@
+# Copyright 2024 colcon contributors
+# Licensed under the Apache License, Version 2.0
+
+"""
+Regression tests for mixin merge order (Issue #40).
+
+When using --mixin a b:
+- list arguments should be concatenated in the given order (a's values
+  first, then b's values)
+- scalar arguments should be overridden by the later mixin (b wins over a)
+"""
+
+
+def _pre_merge(mixins, mixin_names):
+    """Pre-merge mixin args in order — mirrors the fix in mixin_argument.py."""
+    merged = {}
+    for name in mixin_names:
+        for key, value in mixins[name].items():
+            if key not in merged:
+                merged[key] = value
+            elif isinstance(value, list):
+                merged[key] = merged[key] + value
+            else:
+                merged[key] = value
+    return merged
+
+
+class TestMixinMergeOrder:
+
+    def test_list_args_concatenated_in_mixin_order(self):
+        """
+        Lists from --mixin a b should appear as [a_values, b_values].
+
+        Previously they were reversed: [b_values, a_values].
+        Regression test for Issue #40.
+        """
+        mixins = {
+            'a': {'list-arg': ['val-a']},
+            'b': {'list-arg': ['val-b']},
+        }
+        merged = _pre_merge(mixins, ['a', 'b'])
+        assert merged['list-arg'] == ['val-a', 'val-b']
+
+    def test_scalar_args_later_mixin_wins(self):
+        """
+        For scalar args, --mixin a b should result in b's value winning.
+
+        Previously a's value was set first and b's was silently skipped.
+        Regression test for Issue #40.
+        """
+        mixins = {
+            'a': {'scalar-arg': 'val-a'},
+            'b': {'scalar-arg': 'val-b'},
+        }
+        merged = _pre_merge(mixins, ['a', 'b'])
+        assert merged['scalar-arg'] == 'val-b'
+
+    def test_mixed_list_and_scalar_together(self):
+        """
+        Both fixes should work together in a realistic scenario.
+
+        Mixin a and b both define a list arg and a scalar arg.
+        """
+        mixins = {
+            'a': {'list-arg': ['val-a'], 'scalar-arg': 'val-a'},
+            'b': {'list-arg': ['val-b'], 'scalar-arg': 'val-b'},
+        }
+        merged = _pre_merge(mixins, ['a', 'b'])
+        assert merged['list-arg'] == ['val-a', 'val-b']
+        assert merged['scalar-arg'] == 'val-b'
+
+    def test_single_mixin_unaffected(self):
+        """Single mixin usage should behave exactly as before."""
+        mixins = {
+            'a': {'list-arg': ['val-a'], 'scalar-arg': 'val-a'},
+        }
+        merged = _pre_merge(mixins, ['a'])
+        assert merged['list-arg'] == ['val-a']
+        assert merged['scalar-arg'] == 'val-a'
+
+    def test_three_mixins_list_order_preserved(self):
+        """
+        Order should be preserved across more than two mixins.
+
+        --mixin a b c should produce [a_val, b_val, c_val].
+        """
+        mixins = {
+            'a': {'list-arg': ['val-a']},
+            'b': {'list-arg': ['val-b']},
+            'c': {'list-arg': ['val-c']},
+        }
+        merged = _pre_merge(mixins, ['a', 'b', 'c'])
+        assert merged['list-arg'] == ['val-a', 'val-b', 'val-c']
+
+    def test_three_mixins_scalar_last_wins(self):
+        """
+        With three mixins, the last one defining a scalar should win.
+
+        --mixin a b c should give c's scalar value.
+        """
+        mixins = {
+            'a': {'scalar-arg': 'val-a'},
+            'b': {'scalar-arg': 'val-b'},
+            'c': {'scalar-arg': 'val-c'},
+        }
+        merged = _pre_merge(mixins, ['a', 'b', 'c'])
+        assert merged['scalar-arg'] == 'val-c'