Retrieve circuit when a cycle is detected

Ref. eng/recordflux/RecordFlux#256

CHANGELOG.md

@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+
+### Changed
+
+- Display message fields involved in a cycle (eng/recordflux/RecordFlux#256)
+
 ## [0.21.0] - 2024-04-23
 
 ### Changed

rflx/model/message.py

@@ -1209,20 +1209,123 @@ def _compute_topological_sorting(self, has_unreachable: bool) -> Optional[tuple[
                 if set(self.incoming(e.target)) <= visited:
                     fields.append(e.target)
         if not has_unreachable and set(self.structure) - visited:
-            self.error.extend(
-                [
-                    (
-                        f'structure of "{self.identifier}" contains cycle',
-                        Subsystem.MODEL,
-                        Severity.ERROR,
-                        self.location,
-                    ),
-                ],
-            )
-            # Eng/RecordFlux/RecordFlux#256
+            for cycle in self._find_cycles():
+                self.error.extend(
+                    [
+                        (
+                            f'structure of "{self.identifier}" contains cycle',
+                            Subsystem.MODEL,
+                            Severity.ERROR,
+                            self.location,
+                        ),
+                    ],
+                )
+
+                self.error.extend(
+                    [
+                        (
+                            f'field "{link.source.name}" links to "{link.target.name}"',
+                            Subsystem.MODEL,
+                            Severity.INFO,
+                            link.location,
+                        )
+                        for link in cycle
+                    ],
+                )
+
             return None
         return tuple(f for f in result if f not in [INITIAL, FINAL])
 
+    def _find_cycles(self) -> list[list[Link]]:
+        """
+        Retrieve all cycles in the current model.
+
+        Returns
+        -------
+        A list of cycles. Each cycle is represented by a list of fields.
+
+        """
+
+        def search_cycles(
+            current_link: Link,
+            visited: dict[Field, bool],
+            cycle: list[Link],
+        ) -> bool:
+            """
+            Perform a depth-first search to find cycles in a graph starting from `current_link`.
+
+            If a `cycle` is found (True is returned), the cycle parameter represents the found
+            cycle. The content of `cycle` remains unchanged if no cycles have been detected.
+
+            Returns
+            -------
+                bool: True if a cycle is found, False otherwise.
+
+            """
+            visited[current_link.source] = True
+            cycle.append(current_link)
+
+            for outgoing_link in self.outgoing(current_link.target):
+                adj = outgoing_link.target
+                if not visited[adj]:
+                    if search_cycles(outgoing_link, visited, cycle):
+                        return True
+                else:
+                    cycle.append(outgoing_link)
+                    start_index = cycle.index(outgoing_link)
+                    cycle = cycle[start_index:]
+                    visited[current_link.target] = True
+                    return True
+
+            cycle.pop()
+            return False
+
+        visited = {
+            **{l.source: False for l in self.structure},
+            FINAL: False,
+        }
+
+        cycles = []
+
+        for node in [l for l in self.structure if l.source != INITIAL and l.target != FINAL]:
+            if not visited[node.source]:
+                cycle: list[Link] = []
+                if search_cycles(node, visited, cycle):
+                    cycles.append(cycle)
+
+        def _filter_relevant_links(cycle: list[Link]) -> list[Link]:
+            """
+            Remove the first links that are not part of the cycle.
+
+            Sometimes the algorithm might report cycles like this:
+                `A -> B -> C -> B`
+            Even if this cycle is correct, it's preferable to report only the following to the
+            user as it is more readable:
+                `B -> C -> B`
+
+            Parameters
+            ----------
+            cycle : list[Link]
+                A list of Link objects representing a cycle.
+
+            Returns
+            -------
+            list[Link]
+                A list containing only relevant links for the cycle.
+
+            """
+            loop_field = cycle[-1].target
+            filter_offset = 0
+
+            # Iterate through the cycle to find the first link that belongs to the cycle
+            while filter_offset < len(cycle) and cycle[filter_offset].source != loop_field:
+                filter_offset += 1
+
+            # Return a sublist starting from the first link that belongs to the cycle
+            return cycle[filter_offset:]
+
+        return [_filter_relevant_links(cycle) for cycle in cycles]
+
     def _set_types(self) -> None:
         def set_types(expression: expr.Expr) -> expr.Expr:
             return self.typed_expression(expression, self.types)

tests/unit/model/message_test.py

@@ -371,49 +371,166 @@ def test_unreachable_field() -> None:
 
 def test_cycle() -> None:
     t = Integer("P::T", Number(0), Number(1), Number(1))
-
     structure = [
-        Link(INITIAL, Field("X")),
-        Link(Field(ID("X", Location((3, 5)))), Field("Y")),
-        Link(Field(ID("Y", Location((3, 5)))), Field("Z")),
-        Link(Field(ID("Z", Location((3, 5)))), Field("X")),
-        Link(Field("X"), FINAL),
+        Link(INITIAL, Field(ID("X")), location=Location((3, 5))),
+        Link(Field(ID("X")), Field(ID("Y")), location=Location((4, 5))),
+        Link(Field(ID("Y")), Field(ID("Z")), location=Location((5, 5))),
+        Link(Field(ID("Z")), Field(ID("X")), location=Location((6, 5))),
+        Link(Field(ID("X")), FINAL),
     ]
 
     types = {Field("X"): t, Field("Y"): t, Field("Z"): t}
 
     assert_message_model_error(
         structure,
         types,
-        '^<stdin>:10:8: model: error: structure of "P::M" contains cycle$',
-        # Eng/RecordFlux/RecordFlux#256
-        # '\n'
-        # '<stdin>:3:5: model: info: field "X" links to "Y"\n'
-        # '<stdin>:4:5: model: info: field "Y" links to "Z"\n'
-        # '<stdin>:5:5: model: info: field "Z" links to "X"\n',
+        '^<stdin>:10:8: model: error: structure of "P::M" contains cycle\n'
+        '<stdin>:4:5: model: info: field "X" links to "Y"\n'
+        '<stdin>:5:5: model: info: field "Y" links to "Z"\n'
+        r'<stdin>:6:5: model: info: field "Z" links to "X"$',
         location=Location((10, 8)),
     )
 
 
 def test_direct_cycle() -> None:
     t = Integer("P::T", Number(0), Number(1), Number(1))
+    x = Field(ID("X"))
+    y = Field(ID("Y"))
 
     structure = [
-        Link(INITIAL, Field("X")),
-        Link(Field("X"), Field("Y")),
-        Link(Field(ID("Y", Location((3, 5)))), Field("X")),
+        Link(INITIAL, x, location=Location((10, 5))),
+        Link(x, y, location=Location((11, 5))),
+        Link(y, x, location=Location((12, 5))),
     ]
 
     types = {Field("X"): t, Field("Y"): t}
 
     assert_message_model_error(
         structure,
         types,
-        '^<stdin>:10:8: model: error: structure of "P::M" contains cycle$',
+        '^<stdin>:10:8: model: error: structure of "P::M" contains cycle\n'
+        '<stdin>:11:5: model: info: field "X" links to "Y"\n'
+        '<stdin>:12:5: model: info: field "Y" links to "X"$',
+        location=Location((10, 8)),
+    )
+
+
+def test_nested_cycle() -> None:
+    t = Integer("P::T", Number(0), Number(1), Number(1))
+    a = Field(ID("A"))
+    b = Field(ID("B"))
+    c = Field(ID("C"))
+    d = Field(ID("D"))
+    e = Field(ID("E"))
+
+    structure = [
+        Link(INITIAL, a, location=Location((10, 5))),
+        Link(a, b, location=Location((11, 5))),
+        Link(b, c, location=Location((12, 5))),
+        Link(
+            c,
+            Field(ID("B")),
+            condition=Equal(Literal("B"), Number(4)),
+            location=Location((13, 5)),
+        ),
+        Link(c, d, condition=NotEqual(Literal("D"), Number(4)), location=Location((14, 5))),
+        Link(d, e, location=Location((15, 5))),
+        Link(e, FINAL, location=Location((16, 5))),
+    ]
+
+    types = {
+        Field("A"): t,
+        Field("B"): t,
+        Field("C"): t,
+        Field("D"): t,
+        Field("E"): t,
+    }
+
+    assert_message_model_error(
+        structure,
+        types,
+        '^<stdin>:10:8: model: error: structure of "P::M" contains cycle\n'
+        '<stdin>:12:5: model: info: field "B" links to "C"\n'
+        '<stdin>:13:5: model: info: field "C" links to "B"$',
+        location=Location((10, 8)),
+    )
+
+
+def test_two_cycles() -> None:
+    t = Integer("P::T", Number(0), Number(1), Number(1))
+    a = Field(ID("A"))
+    b = Field(ID("B"))
+    c = Field(ID("C"))
+    d = Field(ID("D"))
+    e = Field(ID("E"))
+    f = Field(ID("F"))
+
+    structure = [
+        Link(INITIAL, a, location=Location((10, 5))),
+        Link(a, b, location=Location((11, 5))),
+        Link(b, c, location=Location((12, 5))),
+        Link(
+            c,
+            Field(ID("B")),
+            condition=Equal(Literal("B"), Number(4)),
+            location=Location((13, 5)),
+        ),
+        Link(c, d, condition=NotEqual(Literal("D"), Number(4)), location=Location((14, 5))),
+        Link(d, e, location=Location((15, 5))),
+        Link(e, f, location=Location((16, 5))),
+        Link(
+            f,
+            Field(ID("E")),
+            condition=Equal(Literal("B"), Number(5)),
+            location=Location((17, 5)),
+        ),
+        Link(f, FINAL, condition=NotEqual(Literal("B"), Number(5)), location=Location((18, 5))),
+    ]
+
+    types = {
+        Field("A"): t,
+        Field("B"): t,
+        Field("C"): t,
+        Field("D"): t,
+        Field("E"): t,
+        Field("F"): t,
+    }
+
+    assert_message_model_error(
+        structure,
+        types,
+        '^<stdin>:10:8: model: error: structure of "P::M" contains cycle\n'
+        '<stdin>:12:5: model: info: field "B" links to "C"\n'
+        '<stdin>:13:5: model: info: field "C" links to "B"\n'
+        '<stdin>:10:8: model: error: structure of "P::M" contains cycle\n'
+        '<stdin>:16:5: model: info: field "E" links to "F"\n'
+        '<stdin>:17:5: model: info: field "F" links to "E"$',
         location=Location((10, 8)),
     )
 
 
+def test_cycle_detection_no_false_positive() -> None:
+    t = Integer("P::T", Number(0), Number(10), Number(8))
+    x = Field(ID("X"))
+    y = Field(ID("Y"))
+    z = Field(ID("Z"))
+
+    structure = [
+        Link(INITIAL, x),
+        Link(x, y),
+        Link(y, z),
+        Link(z, FINAL),
+    ]
+
+    types = {Field("X"): t, Field("Y"): t, Field("Z"): t}
+
+    assert not Message(  # noqa: SLF001
+        "P::M",
+        structure,
+        types,
+    )._find_cycles(), "expected no cycles"
+
+
 def test_parameters() -> None:
     assert not models.ethernet_frame().parameters
     assert parameterized_message().parameters == (