cfengine format: Fixed issue with multilining function calls inside macros

.github/workflows/lint-policy-in-other-repos.yml

@@ -44,12 +44,15 @@ jobs:
           python -m pip install --upgrade pip
           python -m pip install uv
           make install
-      - name: Run cfengine lint
+      - name: Check other repos with cfengine format and cfengine lint
         working-directory: cfengine-cli
         run: |
-          uv run cfengine lint --strict no ../masterfiles
-          uv run cfengine lint --strict no ../modules
-          uv run cfengine lint --strict no ../documentation
+          # Linting should work with no errors on all the other repos:
+          uv run cfengine lint --strict no ../masterfiles ../documentation ../modules
+          # Formatting should work with no errors on all the other repos:
+          uv run cfengine format ../masterfiles ../documentation ../modules
+          # Linting should still work after formatting:
+          uv run cfengine lint --strict no ../masterfiles ../documentation ../modules
           # TODO: Add core when ready
           # TODO: Do all of them together, with strict, when ready;
           #       uv run cfengine lint ../core ../masterfiles ../documentation ../modules

src/cfengine_cli/format.py

@@ -236,13 +236,21 @@ def split_generic_list(
     """Split list elements into one-per-line strings, each pre-indented."""
     has_macros = _contains_macro(middle)
     elements: list[str] = []
+    # Indices of the lines that end a top-level value element. For a nested
+    # element, like a function call inside an slist, this is only the final line,
+    # outside the function call, so we do not add trailing commas into function
+    # call argument lists.
+    value_end_indices: list[int] = []
     for element in middle:
         if elements and element.type == ",":
             elements[-1] = elements[-1] + ","
             continue
         if element.type == "macro":
             elements.append(text(element))
             continue
+        if element.type == "comment":
+            elements.append(" " * indent + text(element))
+            continue
         line = " " * indent + stringify_single_line_node(element)
         # Strict < reserves 1 char for the comma appended after this check
         if len(line) < line_length:
@@ -251,14 +259,18 @@ def split_generic_list(
             lines = split_generic_value(element, indent, line_length)
             elements.append(" " * indent + lines[0])
             elements.extend(lines[1:])
+        value_end_indices.append(len(elements) - 1)
 
-    # Adjust trailing commas: with macros, fix every non-macro element
-    # (one per branch); without, fix only the last non-comment element.
+    # Set trailing commas
     if has_macros:
-        for i, e in enumerate(elements):
-            if not e.lstrip().startswith(("@", "#")):
-                elements[i] = _set_trailing_comma(e, trailing_comma)
+        # With macros: Use the indices we counted earlier to ensure there
+        # are trailing commas at the right places (before macro, inside macro,
+        # after macros), but NOT inside the nested function calls which may
+        # be multi line
+        for i in value_end_indices:
+            elements[i] = _set_trailing_comma(elements[i], trailing_comma)
     else:
+        # No macros: Just ensure the last non-comment element has a trailing comma
         for i in reversed(range(len(elements))):
             if not elements[i].lstrip().startswith("#"):
                 elements[i] = _set_trailing_comma(elements[i], trailing_comma)

tests/format/011_macros.expected.cf

@@ -200,3 +200,94 @@ bundle agent test(a)
     "$(a)";
 }
 @endif
+bundle agent test_macro_call_param
+{
+  classes:
+    "a" -> { "CFE-3927" }
+      and => {
+        # The variable must be defined
+        isvariable("default:def.control_agent_environment_vars"),
+        # The length of the variable must be greater than 0 (can't be an empty list)
+        isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+        # Each element of the list must be of the form KEY=VALUE
+        every(".+=.+", "default:def.control_agent_environment_vars"),
+        # In 3.18 and greater we can validate the type of variable in use
+@if minimum_version(3.18.0)
+        regcmp(
+          "(policy slist|data array)",
+          type("default:def.control_agent_environment_vars", "true")
+        ),
+@endif
+      };
+
+    "b" -> { "CFE-3927" }
+      and => {
+        # The variable must be defined
+        isvariable("default:def.control_agent_environment_vars"),
+        isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+@if minimum_version(3.18.0)
+        regcmp(
+          "(policy slist|data array)",
+          type("default:def.control_agent_environment_vars", "true")
+        ),
+@endif
+        every(".+=.+", "default:def.control_agent_environment_vars"),
+      };
+
+    "c" -> { "CFE-3927" }
+      and => {
+        # The variable must be defined
+        isvariable("default:def.control_agent_environment_vars"),
+        isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+@if minimum_version(3.18.0)
+        regcmp(
+          "(policy slist|data array)",
+          type("default:def.control_agent_environment_vars", "true")
+        ),
+        every(".+=.+", "default:def.control_agent_environment_vars"),
+@endif
+      };
+
+    "d" -> { "CFE-3927" }
+      and => {
+@if minimum_version(3.18.0)
+        every(".+=.+", "default:def.control_agent_environment_vars"),
+@endif
+        isvariable("default:def.control_agent_environment_vars"),
+        isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+        regcmp(
+          "(policy slist|data array)",
+          type("default:def.control_agent_environment_vars", "true")
+        ),
+      };
+
+    "e" -> { "CFE-3927" }
+      and => {
+@if minimum_version(3.18.0)
+        "a",
+@endif
+        "b",
+        "c",
+        "d",
+        isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+        regcmp(
+          "(policy slist|data array)",
+          type("default:def.control_agent_environment_vars", "true")
+        ),
+      };
+
+    "f" -> { "CFE-3927" }
+      and => {
+        isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+        regcmp(
+          "(policy slist|data array)",
+          type("default:def.control_agent_environment_vars", "true")
+        ),
+        "a",
+        "b",
+        "c",
+@if minimum_version(3.18.0)
+        "d",
+@endif
+      };
+}

tests/format/011_macros.input.cf

@@ -193,3 +193,59 @@ reports:
 "$(a)";
 }
 @endif
+
+bundle agent test_macro_call_param
+{
+classes:
+"a" -> { "CFE-3927" }
+and => {
+# The variable must be defined
+isvariable("default:def.control_agent_environment_vars"),
+# The length of the variable must be greater than 0 (can't be an empty list)
+isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+# Each element of the list must be of the form KEY=VALUE
+every(".+=.+", "default:def.control_agent_environment_vars"),
+# In 3.18 and greater we can validate the type of variable in use
+@if minimum_version(3.18.0)
+regcmp("(policy slist|data array)",type("default:def.control_agent_environment_vars", "true")),
+@endif
+};
+"b" -> { "CFE-3927" }
+and => {
+# The variable must be defined
+isvariable("default:def.control_agent_environment_vars"),isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+@if minimum_version(3.18.0)
+regcmp( "(policy slist|data array)", type("default:def.control_agent_environment_vars", "true") ),
+@endif
+every(".+=.+", "default:def.control_agent_environment_vars"),
+};
+"c" -> { "CFE-3927" }
+and => {
+# The variable must be defined
+isvariable("default:def.control_agent_environment_vars"), isgreaterthan(length("default:def.control_agent_environment_vars"), 0),
+@if minimum_version(3.18.0)
+regcmp("(policy slist|data array)",type("default:def.control_agent_environment_vars", "true")),every(".+=.+", "default:def.control_agent_environment_vars"),
+@endif
+};
+"d" -> { "CFE-3927" }
+and => {
+@if minimum_version(3.18.0)
+every(".+=.+", "default:def.control_agent_environment_vars"),
+@endif
+isvariable("default:def.control_agent_environment_vars"),isgreaterthan(length("default:def.control_agent_environment_vars"), 0),regcmp("(policy slist|data array)",type("default:def.control_agent_environment_vars", "true")),
+};
+"e" -> { "CFE-3927" }
+and => {
+@if minimum_version(3.18.0)
+"a",
+@endif
+"b","c","d",isgreaterthan(length("default:def.control_agent_environment_vars"),0),regcmp("(policy slist|data array)",type("default:def.control_agent_environment_vars", "true")),
+};
+"f" -> { "CFE-3927" }
+and => {
+isgreaterthan(length("default:def.control_agent_environment_vars"), 0),regcmp("(policy slist|data array)",type("default:def.control_agent_environment_vars", "true")),"a","b","c",
+@if minimum_version(3.18.0)
+"d",
+@endif
+};
+}