Merge pull request #86 from SimonThalvorsen/ENT-13880

ENT-13880: Added error-checks for half-promises / attrs without promiser outside of macros

Author: olehermanse

src/cfengine_cli/lint.py

@@ -27,12 +27,13 @@
   are a bit awkward. Could make iteration nicer.
 """
 
+from copy import deepcopy
 from enum import Enum
 import os
 import json
 from typing import Callable, Iterable
 import tree_sitter_cfengine as tscfengine
-from dataclasses import dataclass
+from dataclasses import dataclass, field
 from tree_sitter import Language, Node, Parser, Tree
 from cfbs.validate import validate_config
 from cfbs.cfbs_config import CFBSConfig
@@ -179,6 +180,13 @@ def visit(x):
 
         _walk_callback(tree.root_node, visit)
 
+    def __deepcopy__(self, _):
+        """Overrides deepcopy for state-snapshotting.
+        treesitter-tree is not pickleable, and PolicyFile
+        should not change across state snapshots
+        """
+        return self
+
 
 @dataclass
 class State:
@@ -199,6 +207,8 @@ class State:
     promise_type: str | None = None  # "vars" | "files" | "classes" | ... | None
     attribute_name: str | None = None  # "if" | "string" | "slist" | ... | None
     namespace: str = DEFAULT_NAMESPACE  # "ns" | "default" | ... |
+    macro: str | None = None  # "minimum_version()", "else", "between_versions()"
+    old_state: dict = field(default_factory=dict)
     mode: Mode = Mode.NONE
     walking: bool = False
     strict: bool = True
@@ -307,11 +317,14 @@ def _add_definition(self, name: str, node: Node, definitions: dict) -> None:
             parameters = list(filter(",".__ne__, iter(_text(x) for x in args)))
         else:
             parameters = []
+
         definition = {
             "filename": self.policy_file.filename,
             "line": node.range.start_point[0] + 1,
             "parameters": parameters,
         }
+        if self.macro:
+            definition["macro"] = self.macro
         if name not in definitions:
             definitions[name] = []
         definitions[name].append(definition)
@@ -429,6 +442,22 @@ def navigate(self, node) -> None:
             self.promise_type = None
             return
 
+        if node.type == "macro":
+            macro_type = _text(node)
+            if macro_type.startswith("@if"):
+                self.old_state = deepcopy(self.__dict__)
+                self.macro = macro_type.split(" ")[-1]
+            elif macro_type.startswith("@else"):
+                self.macro = "else"
+                self.old_state = deepcopy(self.__dict__)
+                self.__dict__.update(self.old_state)
+            elif macro_type.startswith("@endif"):
+                self.macro = None
+                self.__dict__.update(self.old_state)
+                # NOTE: this or that? maybe a dict of states based on the macro-type?
+                self.old_state = {}
+            return
+
 
 def _check_syntax(policy_file: PolicyFile, state: State) -> int:
     """Iterate over a syntax tree and print errors if it is empty or has syntax
@@ -746,7 +775,23 @@ def _lint_node(
                     f"Error: Expected {expected} arguments, received {len(args)} for body '{call}' {location}"
                 )
                 return 1
-
+    if node.type == "half_promise":
+        prev_sib = node.prev_named_sibling
+        while prev_sib and prev_sib.type == "comment":
+            prev_sib = prev_sib.prev_named_sibling
+        prev_type = prev_sib.type if prev_sib else None
+        if not state.macro:
+            _highlight_range(node, lines)
+            print(
+                f"Error: Found promise attribute with no parent-promiser outside of a macro {location}"
+            )
+            return 1
+        elif prev_type != "macro":
+            _highlight_range(node, lines)
+            print(
+                f"Error: Multiple promise attributes with ending semicolon found inside macro '{state.macro}' {location}"
+            )
+            return 1
     return 0
 
 

tests/lint/017_half_promises.cf

@@ -0,0 +1,12 @@
+bundle agent main
+{
+  vars:
+    "string" string => $(bar);
+
+    "string"
+@if minimum_version(3.18)
+      string => $($(baz));
+@else
+      string => $($(baz));
+@endif
+}

tests/lint/017_half_promises.expected.txt

@@ -0,0 +1,17 @@
+
+      string => $(bar);
+      string => $(bar);
+      ^---------------^
+Error: Found promise attribute with no parent-promiser outside of a macro at tests/lint/017_half_promises.x.cf:6:7
+
+      string => $($(baz));
+      string => $($(baz));
+      ^------------------^
+Error: Multiple promise attributes with ending semicolon found inside macro 'minimum_version(3.18)' at tests/lint/017_half_promises.x.cf:9:7
+
+    # comment
+      string => $($(baz));
+      ^------------------^
+Error: Multiple promise attributes with ending semicolon found inside macro 'else' at tests/lint/017_half_promises.x.cf:26:7
+FAIL: tests/lint/017_half_promises.x.cf (3 errors)
+Failure, 3 errors in total.

tests/lint/017_half_promises.x.cf

@@ -0,0 +1,29 @@
+bundle agent main
+{
+  vars:
+    "string"
+      string => $(bar);
+      string => $(bar);
+@if minimum_version(3.18)
+      string => $($(baz));
+      string => $($(baz));
+@else
+      string => $($(baz));
+@endif
+
+    "string"
+      # comment
+      # comment
+      # comment
+@if minimum_version(3.20)
+      string => $($(baz));
+
+    # comment
+@else
+      string => $($(baz));
+
+    # comment
+      string => $($(baz));
+  # comment
+@endif
+}