From e1701b0e62b6fd557414e05fcf22bf84aa6550aa Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Thu, 28 Mar 2024 10:43:28 +0000
Subject: [PATCH] Error if graph has a dependency offset exclusively on the RHS
 (#5924)

---
 changes.d/fix.5924.md           |  1 +
 cylc/flow/graph_parser.py       | 39 ++++++++++++++++++++++----------
 tests/unit/test_graph_parser.py | 40 ++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 13 deletions(-)
 create mode 100644 changes.d/fix.5924.md

diff --git a/changes.d/fix.5924.md b/changes.d/fix.5924.md
new file mode 100644
index 00000000000..7ce2caf7777
--- /dev/null
+++ b/changes.d/fix.5924.md
@@ -0,0 +1 @@
+Validation: a cycle offset can only appear on the right of a dependency if the task's cycling is defined elsewhere with no offset.
\ No newline at end of file
diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py
index ad0ec280a3d..92427b3cc4b 100644
--- a/cylc/flow/graph_parser.py
+++ b/cylc/flow/graph_parser.py
@@ -462,8 +462,12 @@ def parse_graph(self, graph_string: str) -> None:
             for i in range(0, len(chain) - 1):
                 pairs.add((chain[i], chain[i + 1]))
 
+        # Get a set of RH nodes which are not at the LH of another pair:
+        pairs_dict = dict(pairs)
+        terminals = set(pairs_dict.values()).difference(pairs_dict.keys())
+
         for pair in pairs:
-            self._proc_dep_pair(pair)
+            self._proc_dep_pair(pair, terminals)
 
     @classmethod
     def _report_invalid_lines(cls, lines: List[str]) -> None:
@@ -493,19 +497,23 @@ def _report_invalid_lines(cls, lines: List[str]) -> None:
 
     def _proc_dep_pair(
         self,
-        pair: Tuple[Optional[str], str]
+        pair: Tuple[Optional[str], str],
+        terminals: Set[str],
     ) -> None:
         """Process a single dependency pair 'left => right'.
 
-        'left' can be a logical expression of qualified node names.
-        'left' can be None, when triggering a left-side or lone node.
-        'left' can be "", if null task name in graph error (a => => b).
-        'right' can be one or more node names joined by AND.
-        'right' can't be None or "".
         A node is an xtrigger, or a task or a family name.
         A qualified name is NAME([CYCLE-POINT-OFFSET])(:QUALIFIER).
-        Trigger qualifiers, but not cycle offsets, are ignored on the right to
-        allow chaining.
+
+        Args:
+            pair:
+                'left' can be a logical expression of qualified node names.
+                'left' can be None, when triggering a left-side or lone node.
+                'left' can be "", if null task name in graph error (a => => b).
+                'right' can be one or more node names joined by AND.
+                'right' can't be None or "".
+            terminals:
+                Nodes which are _only_ on the RH end of chains.
         """
         left, right = pair
         # Raise error for right-hand-side OR operators.
@@ -525,10 +533,17 @@ def _proc_dep_pair(
         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.)
+        # Raise error for cycle point offsets at the end of chains
         if '[' in right:
-            return
+            if left and (right in terminals):
+                # This right hand side is at the end of a chain:
+                raise GraphParseError(
+                    'Invalid cycle point offsets only on right hand '
+                    'side of a dependency (must be on left hand side):'
+                    f' {left} => {right}')
+            else:
+                # This RHS is also a LHS in a chain:
+                return
 
         # Split right side on AND.
         rights = right.split(self.__class__.OP_AND)
diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py
index ddd443a3597..4da3a8c22d9 100644
--- a/tests/unit/test_graph_parser.py
+++ b/tests/unit/test_graph_parser.py
@@ -133,6 +133,12 @@ def test_graph_syntax_errors_2(seq, graph, expected_err):
             "foo || bar => baz",
             "The graph OR operator is '|'"
         ),
+        param(
+            # See https://github.com/cylc/cylc-flow/issues/5844
+            "foo => bar[1649]",
+            'Invalid cycle point offsets only on right',
+            id='no-cycle-point-RHS'
+        ),
     ]
 )
 def test_graph_syntax_errors(graph, expected_err):
@@ -377,7 +383,16 @@ def test_line_continuation():
             foo => bar
             bar:succeed => baz
             """
-        ]
+        ],
+        [
+            """
+            foo => bar[1649] => baz
+            """,
+            """
+            foo => bar[1649]
+            bar[1649] => baz
+            """
+        ],
     ]
 )
 def test_trigger_equivalence(graph1, graph2):
@@ -896,3 +911,26 @@ def test_RHS_AND(graph: str, expected_triggers: Dict[str, List[str]]):
         for task, trigs in gp.triggers.items()
     }
     assert triggers == expected_triggers
+
+
+@pytest.mark.parametrize(
+    'args, err',
+    (
+        # Error if offset in terminal RHS:
+        param((('a', 'b[-P42M]'), {'b[-P42M]'}), 'Invalid cycle point offset'),
+        # No error if offset in NON-terminal RHS:
+        param((('a', 'b[-P42M]'), {}), None),
+        # Don't check the left hand side if this has a non-terminal RHS:
+        param((('a &', 'b[-P42M]'), {}), None),
+    )
+)
+def test_proc_dep_pair(args, err):
+    """
+    Unit tests for _proc_dep_pair.
+    """
+    gp = GraphParser()
+    if err:
+        with pytest.raises(GraphParseError, match=err):
+            gp._proc_dep_pair(*args)
+    else:
+        assert gp._proc_dep_pair(*args) is None