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