From daf085f2678cc858e82f1cbe551be0b2568d000d Mon Sep 17 00:00:00 2001 From: Corentin Machu Date: Mon, 15 Apr 2024 16:20:32 +0200 Subject: [PATCH] Retrieve circuit when a cycle is detected Ref. eng/recordflux/RecordFlux#256 --- CHANGELOG.md | 6 ++ rflx/model/message.py | 125 +++++++++++++++++++++++--- tests/unit/model/message_test.py | 149 +++++++++++++++++++++++++++---- 3 files changed, 253 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e16186794..083c19c84 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/rflx/model/message.py b/rflx/model/message.py index 6d3f6b4eb..19ebc10ef 100644 --- a/rflx/model/message.py +++ b/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) diff --git a/tests/unit/model/message_test.py b/tests/unit/model/message_test.py index 7f7c4b082..b8250d818 100644 --- a/tests/unit/model/message_test.py +++ b/tests/unit/model/message_test.py @@ -371,13 +371,12 @@ 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} @@ -385,23 +384,23 @@ def test_cycle() -> None: assert_message_model_error( structure, types, - '^:10:8: model: error: structure of "P::M" contains cycle$', - # Eng/RecordFlux/RecordFlux#256 - # '\n' - # ':3:5: model: info: field "X" links to "Y"\n' - # ':4:5: model: info: field "Y" links to "Z"\n' - # ':5:5: model: info: field "Z" links to "X"\n', + '^:10:8: model: error: structure of "P::M" contains cycle\n' + ':4:5: model: info: field "X" links to "Y"\n' + ':5:5: model: info: field "Y" links to "Z"\n' + r':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} @@ -409,11 +408,129 @@ def test_direct_cycle() -> None: assert_message_model_error( structure, types, - '^:10:8: model: error: structure of "P::M" contains cycle$', + '^:10:8: model: error: structure of "P::M" contains cycle\n' + ':11:5: model: info: field "X" links to "Y"\n' + ':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, + '^:10:8: model: error: structure of "P::M" contains cycle\n' + ':12:5: model: info: field "B" links to "C"\n' + ':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, + '^:10:8: model: error: structure of "P::M" contains cycle\n' + ':12:5: model: info: field "B" links to "C"\n' + ':13:5: model: info: field "C" links to "B"\n' + ':10:8: model: error: structure of "P::M" contains cycle\n' + ':16:5: model: info: field "E" links to "F"\n' + ':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 == (