From 7cd0bcee4f48cb203b7051219e04991042cc1da4 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Tue, 16 Jan 2024 10:57:59 +0000
Subject: [PATCH] Error if graph has a dependency offset exclusively on the RHS
 of a graph pair.

---
 changes.d/fix.5924.md           |  1 +
 cylc/flow/graph_parser.py       | 38 ++++++++++++++++++++++-----------
 tests/unit/test_graph_parser.py | 24 +++++++++++++++++++--
 3 files changed, 49 insertions(+), 14 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..be3ba7ad429
--- /dev/null
+++ b/changes.d/fix.5924.md
@@ -0,0 +1 @@
+Ensure that graphs with dependency offsets on the right hand side are flagged to users.
\ No newline at end of file
diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py
index ad0ec280a3d..f63d7193417 100644
--- a/cylc/flow/graph_parser.py
+++ b/cylc/flow/graph_parser.py
@@ -462,8 +462,13 @@ 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:
+        terminals = {
+            right for right in {right for _, right in pairs}
+            if right not in {left for left, _ in pairs}}
+
         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 +498,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:
+                Lits of nodes which are _only_ on the RH end of chains.
         """
         left, right = pair
         # Raise error for right-hand-side OR operators.
@@ -525,10 +534,15 @@ 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.)
+        # Ignore/Error cycle point offsets on the right side:
+        # (Note we can only ban this for nodes at the end of chains)
         if '[' in right:
-            return
+            if right in terminals:
+                raise GraphParseError(
+                    'ERROR, illegal cycle point offset on the right:'
+                    f' {left} => {right}')
+            else:
+                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..f739a79d09b 100644
--- a/tests/unit/test_graph_parser.py
+++ b/tests/unit/test_graph_parser.py
@@ -133,13 +133,33 @@ 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]",
+            'illegal cycle point offset on the right',
+            id='no-cycle-point-RHS'
+        ),
+        param(
+            "foo => bar[1649] => baz",
+            '!illegal cycle point offset on the right',
+            id='ignore-not-exclusively-RHS-cycle-point'
+        ),
+        param(
+            "foo => bar[1649]\nbar[1649] => baz",
+            '!illegal cycle point offset on the right',
+            id='ignore-not-exclusively-RHS-cycle-point2'
+        ),
     ]
 )
 def test_graph_syntax_errors(graph, expected_err):
     """Test various graph syntax errors."""
-    with pytest.raises(GraphParseError) as cm:
+    if expected_err[0] == '!':
+        # Assert method doesn't fail:
         GraphParser().parse_graph(graph)
-    assert expected_err in str(cm.value)
+    else:
+        with pytest.raises(GraphParseError) as cm:
+            GraphParser().parse_graph(graph)
+        assert expected_err in str(cm.value)
 
 
 def test_parse_graph_simple():