Fix parsing algorithm in ValueParser (#522)

Fix parsing algorithm in `ValueParser`

Reviewed-by: gemini-code-assist[bot]
Reviewed-by: Nikola Forró
Reviewed-by: Laura Barcziová

Author: centosinfra-prod-github-app[bot]

specfile/sanitizer.py

@@ -8,12 +8,12 @@
 
 from specfile.exceptions import UnterminatedMacroException
 from specfile.value_parser import (
-    BuiltinMacro,
     ConditionalMacroExpansion,
     EnclosedMacroSubstitution,
     ExpressionExpansion,
     MacroSubstitution,
     ShellExpansion,
+    SingleArgEnclosedMacroSubstitution,
     ValueParser,
 )
 
@@ -968,7 +968,12 @@ def sanitize_nodes(nodes):
             while i < len(nodes):
                 node = nodes[i]
                 if isinstance(
-                    node, (MacroSubstitution, EnclosedMacroSubstitution, BuiltinMacro)
+                    node,
+                    (
+                        MacroSubstitution,
+                        EnclosedMacroSubstitution,
+                        SingleArgEnclosedMacroSubstitution,
+                    ),
                 ) and node.name in ("include", "load", "uncompress"):
                     removed += 1
                     # %include/%load followed by whitespace and argument
@@ -1019,18 +1024,18 @@ def sanitize_nodes(nodes):
                     else:
                         converted += 1
                     result.append(replacement)
-                elif isinstance(node, BuiltinMacro):
+                elif isinstance(node, SingleArgEnclosedMacroSubstitution):
                     if not is_name_safe(node.name):
                         removed += 1
                         result.append("%{nil}")
                     elif node.name == "lua":
-                        if not cls.is_lua_safe(node.body):
+                        if not cls.is_lua_safe(node.arg):
                             removed += 1
                             result.append("%{nil}")
                         else:
                             result.append(str(node))
                     else:
-                        sanitized_body, c, r = cls.sanitize(node.body, _depth + 1)
+                        sanitized_body, c, r = cls.sanitize(node.arg, _depth + 1)
                         converted += c
                         removed += r
                         result.append(f"%{{{node.name}:{sanitized_body}}}")

specfile/spec_parser.py

@@ -21,7 +21,11 @@
 from specfile.sections import Section
 from specfile.tags import Tags
 from specfile.utils import get_filename_from_location
-from specfile.value_parser import BuiltinMacro, ShellExpansion, ValueParser
+from specfile.value_parser import (
+    ShellExpansion,
+    SingleArgEnclosedMacroSubstitution,
+    ValueParser,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -286,10 +290,13 @@ def collect_loaded_sources(content):
             # collect sources loaded using %{load:...}
             sources = set()
             for node in ValueParser.flatten(ValueParser.parse(content)):
-                if isinstance(node, BuiltinMacro) and node.name == "load":
+                if (
+                    isinstance(node, SingleArgEnclosedMacroSubstitution)
+                    and node.name == "load"
+                ):
                     # we can expand macros here because the first non-build parse,
                     # even though it failed, populated the macro context
-                    source = Path(Macros.expand(node.body))
+                    source = Path(Macros.expand(node.arg))
                     # ignore files outside of sourcedir
                     if source.parent.samefile(self.sourcedir):
                         sources.add(source.name)

specfile/value_parser.py

@@ -42,8 +42,8 @@ def __eq__(self, other: object) -> bool:
         return self.value == other.value
 
 
-class ShellExpansion(Node):
-    """Node representing shell expansion, e.g. _%(whoami)_."""
+class Expansion(Node):
+    """Abstract base class for expansion nodes."""
 
     def __init__(self, body: str) -> None:
         self.body = body
@@ -54,16 +54,20 @@ def __repr__(self) -> str:
         # don't have to reimplement __repr__()
         return f"{self.__class__.__name__}({self.body!r})"
 
-    def __str__(self) -> str:
-        return f"%({self.body})"
-
     def __eq__(self, other: object) -> bool:
         if not isinstance(other, self.__class__):
             return NotImplemented
         return self.body == other.body
 
 
-class ExpressionExpansion(ShellExpansion):
+class ShellExpansion(Expansion):
+    """Node representing shell expansion, e.g. _%(whoami)_."""
+
+    def __str__(self) -> str:
+        return f"%({self.body})"
+
+
+class ExpressionExpansion(Expansion):
     """Node representing expression expansion, e.g. _%[1+1]_."""
 
     def __str__(self) -> str:
@@ -125,6 +129,29 @@ def __eq__(self, other: object) -> bool:
         )
 
 
+class SingleArgEnclosedMacroSubstitution(Node):
+    """
+    Node representing single-argument bracket-enclosed macro substitution,
+    e.g. _%{quote:Ancient Greek}_.
+    """
+
+    def __init__(self, name: str, arg: str) -> None:
+        self.name = name
+        self.arg = arg
+
+    @formatted
+    def __repr__(self) -> str:
+        return f"SingleArgEnclosedMacroSubstitution({self.name!r}, {self.arg!r})"
+
+    def __str__(self) -> str:
+        return f"%{{{self.name}:{self.arg}}}"
+
+    def __eq__(self, other: object) -> bool:
+        if not isinstance(other, self.__class__):
+            return NotImplemented
+        return self.name == other.name and self.arg == other.arg
+
+
 class ConditionalMacroExpansion(Node):
     """Node representing conditional macro expansion, e.g. _%{?prerel:0.}_."""
 
@@ -156,26 +183,6 @@ def __eq__(self, other: object) -> bool:
         )
 
 
-class BuiltinMacro(Node):
-    """Node representing built-in macro, e.g. _%{quote:Ancient Greek}_."""
-
-    def __init__(self, name: str, body: str) -> None:
-        self.name = name
-        self.body = body
-
-    @formatted
-    def __repr__(self) -> str:
-        return f"BuiltinMacro({self.name!r}, {self.body!r})"
-
-    def __str__(self) -> str:
-        return f"%{{{self.name}:{self.body}}}"
-
-    def __eq__(self, other: object) -> bool:
-        if not isinstance(other, self.__class__):
-            return NotImplemented
-        return self.name == other.name and self.body == other.body
-
-
 class ValueParser:
     @classmethod
     def flatten(cls, nodes: List[Node]) -> Generator[Node, None, None]:
@@ -243,7 +250,7 @@ def find_macro_end(index):
             return i
 
         result: List[Node] = []
-        start = 0
+        start = start0 = 0
         offset = 0
         while start < len(value):
             try:
@@ -258,8 +265,6 @@ def find_macro_end(index):
                 end = None
             if end is None:
                 break
-            if end > start:
-                result.append(StringLiteral(value[start:end]))
             start = end
             end = find_macro_end(start + 1)
             if end is None:
@@ -269,24 +274,46 @@ def find_macro_end(index):
             elif value[start + 1] == "[":
                 result.append(ExpressionExpansion(value[start + 2 : end - 1]))
             elif value[start + 1] == "{":
-                if ":" in value[start:end]:
-                    condition, body = value[start + 2 : end - 1].split(":", maxsplit=1)
+                index, delimiter = next(
+                    (
+                        (i, c)
+                        for i, c in enumerate(value[start + 2 : end - 1])
+                        if c in " :}"
+                    ),
+                    (-1, None),
+                )
+                if delimiter == ":":
+                    condition = value[start + 2 : start + index + 2]
+                    body = value[start + index + 3 : end - 1]
                     tokens = re.split(r"^([?!]+)", condition, maxsplit=1)
                     prefix = "" if len(tokens) == 1 else tokens[1]
                     if "?" in prefix:
                         result.append(
                             ConditionalMacroExpansion(condition, cls.parse(body))
                         )
                     else:
-                        result.append(BuiltinMacro(condition, body))
+                        result.append(
+                            SingleArgEnclosedMacroSubstitution(condition, body)
+                        )
                 else:
-                    result.append(EnclosedMacroSubstitution(value[start + 2 : end - 1]))
+                    body = value[start + 2 : end - 1]
+                    name = (
+                        value[start + 2 : start + index + 2]
+                        if delimiter is not None
+                        else body
+                    )
+                    if not re.match(r"[A-Za-z_]\w*$", name.lstrip("?!"), re.ASCII):
+                        start += 2
+                        continue
+                    result.append(EnclosedMacroSubstitution(body))
             else:
                 result.append(MacroSubstitution(value[start + 1 : end]))
-            start = end
+            if start > start0:
+                result.insert(-1, StringLiteral(value[start0:start]))
+            start = start0 = end
             offset = 0
-        if value[start:]:
-            result.append(StringLiteral(value[start:]))
+        if value[start0:]:
+            result.append(StringLiteral(value[start0:]))
         return result
 
     @classmethod
@@ -387,7 +414,14 @@ def evaluate(node):
                 tokens.append(("c", "", node))
             elif isinstance(node, StringLiteral):
                 tokens.append(("v", node.value, ""))
-            elif isinstance(node, (ShellExpansion, ExpressionExpansion, BuiltinMacro)):
+            elif isinstance(
+                node,
+                (
+                    ShellExpansion,
+                    ExpressionExpansion,
+                    SingleArgEnclosedMacroSubstitution,
+                ),
+            ):
                 const = expand(str(node))
                 tokens.append(("c", const, str(node)))
             elif isinstance(node, MacroSubstitution):

tests/unit/test_sanitizer.py

@@ -1215,9 +1215,9 @@ def test_expression_expansion_sanitized(value, expected_sanitized):
     "value, expected_sanitized",
     [
         # EnclosedMacroSubstitution without args - unsafe name
-        ("%{%(whoami)}", "%{nil}"),
+        ("%{%(whoami)}", "%{%{nil}}"),
         # EnclosedMacroSubstitution with args - unsafe name
-        ("%{%(whoami) arg1 arg2}", "%{nil}"),
+        ("%{%(whoami) arg1 arg2}", "%{%{nil} arg1 arg2}"),
         # BuiltinMacro - unsafe name
         ("%{%(whoami):body}", "%{nil}"),
         # BuiltinMacro - nested macro in name (splits at : giving unterminated name)

tests/unit/test_value_parser.py

@@ -5,12 +5,12 @@
 from flexmock import flexmock
 
 from specfile.value_parser import (
-    BuiltinMacro,
     ConditionalMacroExpansion,
     EnclosedMacroSubstitution,
     Macros,
     MacroSubstitution,
     ShellExpansion,
+    SingleArgEnclosedMacroSubstitution,
     StringLiteral,
     ValueParser,
 )
@@ -50,12 +50,26 @@
         (
             '%{lua:ver = string.gsub(rpm.expand("%{ver}"), "-", "~"); print(string.lower(ver))}',
             [
-                BuiltinMacro(
+                SingleArgEnclosedMacroSubstitution(
                     "lua",
                     'ver = string.gsub(rpm.expand("%{ver}"), "-", "~"); print(string.lower(ver))',
                 )
             ],
         ),
+        ("%{%{28}}", [StringLiteral("%{%{28}}")]),
+        (
+            "%{%{macro}}",
+            [
+                StringLiteral("%{"),
+                EnclosedMacroSubstitution("macro"),
+                StringLiteral("}"),
+            ],
+        ),
+        ("%{upper:%{name}}", [SingleArgEnclosedMacroSubstitution("upper", "%{name}")]),
+        (
+            "%{version_no_tilde %{quote:nil}}",
+            [EnclosedMacroSubstitution("version_no_tilde %{quote:nil}")],
+        ),
     ],
 )
 def test_parse(value, nodes):