Merge pull request #5801 from MetRonnie/parentheses

Handle parentheses on RHS of trigger expression

Author: MetRonnie

changes.d/5801.fix.md

@@ -0,0 +1 @@
+Fix traceback when using parentheses on right hand side of graph trigger.

cylc/flow/graph_parser.py

@@ -23,7 +23,8 @@
     Dict,
     List,
     Tuple,
-    Optional
+    Optional,
+    Union
 )
 
 import cylc.flow.flags
@@ -85,10 +86,10 @@ class GraphParser:
     store dependencies for the whole workflow (call parse_graph multiple times
     and key results by graph section).
 
-    The general form of a dependency is "EXPRESSION => NODE", where:
-        * On the right, NODE is a task or family name
+    The general form of a dependency is "LHS => RHS", where:
         * On the left, an EXPRESSION of nodes involving parentheses, and
           logical operators '&' (AND), and '|' (OR).
+        * On the right, an EXPRESSION of nodes NOT involving '|'
         * Node names may be parameterized (any number of parameters):
             NODE<i,j,k>
             NODE<i=0,j,k>  # specific parameter value
@@ -517,32 +518,33 @@ def _proc_dep_pair(
                 "Suicide markers must be"
                 f" on the right of a trigger: {left}")
 
+        # Check that parentheses match.
+        mismatch_msg = 'Mismatched parentheses in: "{}"'
+        if left and left.count("(") != left.count(")"):
+            raise GraphParseError(mismatch_msg.format(left))
+        if right.count("(") != right.count(")"):
+            raise GraphParseError(mismatch_msg.format(right))
+
         # Ignore cycle point offsets on the right side.
         # (Note we can't ban this; all nodes get process as left and right.)
         if '[' in right:
             return
 
-        # Check that parentheses match.
-        if left and left.count("(") != left.count(")"):
-            raise GraphParseError(
-                "Mismatched parentheses in: \"" + left + "\"")
-
         # Split right side on AND.
         rights = right.split(self.__class__.OP_AND)
         if '' in rights or right and not all(rights):
             raise GraphParseError(
                 f"Null task name in graph: {left} => {right}")
 
+        lefts: Union[List[str], List[Optional[str]]]
         if not left or (self.__class__.OP_OR in left or '(' in left):
-            # Treat conditional or bracketed expressions as a single entity.
+            # Treat conditional or parenthesised expressions as a single entity
             # Can get [None] or [""] here
-            lefts: List[Optional[str]] = [left]
+            lefts = [left]
         else:
             # Split non-conditional left-side expressions on AND.
             # Can get [""] here too
-            # TODO figure out how to handle this wih mypy:
-            #   assign List[str] to List[Optional[str]]
-            lefts = left.split(self.__class__.OP_AND)  # type: ignore
+            lefts = left.split(self.__class__.OP_AND)
         if '' in lefts or left and not all(lefts):
             raise GraphParseError(
                 f"Null task name in graph: {left} => {right}")
@@ -847,9 +849,14 @@ def _compute_triggers(
                 trigs += [f"{name}{offset}:{trigger}"]
 
         for right in rights:
+            right = right.strip('()')  # parentheses don't matter
             m = self.__class__.REC_RHS_NODE.match(right)
-            # This will match, bad nodes are detected earlier (type ignore):
-            suicide_char, name, output, opt_char = m.groups()  # type: ignore
+            if not m:
+                # Bad nodes should have been detected earlier; fail loudly
+                raise ValueError(  # pragma: no cover
+                    f"Unexpected graph expression: '{right}'"
+                )
+            suicide_char, name, output, opt_char = m.groups()
             suicide = (suicide_char == self.__class__.SUICIDE)
             optional = (opt_char == self.__class__.OPTIONAL)
             if output:

tests/unit/test_graph_parser.py

@@ -16,6 +16,7 @@
 """Unit tests for the GraphParser."""
 
 import logging
+from typing import Dict, List
 import pytest
 from itertools import product
 from pytest import param
@@ -86,45 +87,59 @@ def test_graph_syntax_errors_2(seq, graph, expected_err):
 @pytest.mark.parametrize(
     'graph, expected_err',
     [
-        [
+        (
             "a b => c",
             "Bad graph node format"
-        ],
-        [
+        ),
+        (
+            "a => b c",
+            "Bad graph node format"
+        ),
+        (
             "!foo => bar",
             "Suicide markers must be on the right of a trigger:"
-        ],
-        [
+        ),
+        (
             "( foo & bar => baz",
-            "Mismatched parentheses in:"
-        ],
-        [
+            'Mismatched parentheses in: "(foo&bar"'
+        ),
+        (
+            "a => b & c)",
+            'Mismatched parentheses in: "b&c)"'
+        ),
+        (
+            "(a => b & c)",
+            'Mismatched parentheses in: "(a"'
+        ),
+        (
+            "(a => b[+P1]",
+            'Mismatched parentheses in: "(a"'
+        ),
+        (
             """(a | b & c) => d
                foo => bar
                (a | b & c) => !d""",
             "can't trigger both d and !d"
-        ],
-        [
+        ),
+        (
             "a => b | c",
             "Illegal OR on right side"
-        ],
-        [
+        ),
+        (
             "foo && bar => baz",
             "The graph AND operator is '&'"
-        ],
-        [
+        ),
+        (
             "foo || bar => baz",
             "The graph OR operator is '|'"
-        ],
+        ),
     ]
 )
 def test_graph_syntax_errors(graph, expected_err):
     """Test various graph syntax errors."""
     with pytest.raises(GraphParseError) as cm:
         GraphParser().parse_graph(graph)
-    assert (
-        expected_err in str(cm.value)
-    )
+    assert expected_err in str(cm.value)
 
 
 def test_parse_graph_simple():
@@ -845,3 +860,39 @@ def test_fail_family_triggers_on_tasks(ftrig):
                 "family trigger on non-family namespace"
             )
         )
+
+
+@pytest.mark.parametrize(
+    'graph, expected_triggers',
+    [
+        param(
+            'a => b & c',
+            {'a': [''], 'b': ['a:succeeded'], 'c': ['a:succeeded']},
+            id="simple"
+        ),
+        param(
+            'a => (b & c)',
+            {'a': [''], 'b': ['a:succeeded'], 'c': ['a:succeeded']},
+            id="simple w/ parentheses"
+        ),
+        param(
+            'a => (b & (c & d))',
+            {
+                'a': [''],
+                'b': ['a:succeeded'],
+                'c': ['a:succeeded'],
+                'd': ['a:succeeded'],
+            },
+            id="more parentheses"
+        ),
+    ]
+)
+def test_RHS_AND(graph: str, expected_triggers: Dict[str, List[str]]):
+    """Test '&' operator on right hand side of trigger expression."""
+    gp = GraphParser()
+    gp.parse_graph(graph)
+    triggers = {
+        task: list(trigs.keys())
+        for task, trigs in gp.triggers.items()
+    }
+    assert triggers == expected_triggers