From 41dbb1c618de1ce7ecc944db1611228ec1fad24b Mon Sep 17 00:00:00 2001 From: mhadhbi Date: Mon, 14 Feb 2022 18:37:52 +0100 Subject: [PATCH] Detecting duplicate aspects Fixes #714 --- rflx/specification/parser.py | 82 +++++++++++++++++-------- tests/unit/specification/parser_test.py | 79 ++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 25 deletions(-) diff --git a/rflx/specification/parser.py b/rflx/specification/parser.py index 90a75cfcac..57a3e956b4 100644 --- a/rflx/specification/parser.py +++ b/rflx/specification/parser.py @@ -835,7 +835,7 @@ def create_message( error, identifier, parameters, fields, types, filename ) structure = create_message_structure(error, fields, filename) - checksum_aspects, byte_order_aspect = parse_aspects(message.f_aspects, filename) + checksum_aspects, byte_order_aspect = parse_aspects(message.f_aspects, filename, error) try: result = create_proven_message( model.UnprovenMessage( @@ -1193,12 +1193,27 @@ def merge_field_condition( def parse_aspects( - aspects: lang.MessageAspectList, filename: Path + aspects: lang.MessageAspectList, filename: Path, error: RecordFluxError ) -> Tuple[Mapping[ID, Sequence[expr.Expr]], Optional[model.ByteOrder]]: + checksum_parsed = False checksum_result = {} + byte_order_parsed = False byte_order_result = None for aspect in aspects: - if isinstance(aspect, lang.ChecksumAspect): + if (isinstance(aspect, lang.ChecksumAspect) and checksum_parsed) or ( + isinstance(aspect, lang.ByteOrderAspect) and byte_order_parsed + ): + error.extend( + [ + ( + f'duplicate aspect "{aspect.kind_name}"', + Subsystem.PARSER, + Severity.ERROR, + node_location(aspect, filename), + ) + ], + ) + elif isinstance(aspect, lang.ChecksumAspect): for assoc in aspect.f_associations: exprs = [] for value in assoc.f_covered_fields: @@ -1214,11 +1229,14 @@ def parse_aspects( else: raise NotImplementedError(f"Invalid checksum association {value.kind_name}") checksum_result[create_id(assoc.f_identifier, filename)] = exprs - if isinstance(aspect, lang.ByteOrderAspect): + checksum_parsed = True + else: + assert isinstance(aspect, lang.ByteOrderAspect) if isinstance(aspect.f_byte_order, lang.ByteOrderTypeLoworderfirst): byte_order_result = model.ByteOrder.LOW_ORDER_FIRST else: byte_order_result = model.ByteOrder.HIGH_ORDER_FIRST + byte_order_parsed = True return checksum_result, byte_order_result @@ -1297,28 +1315,42 @@ def create_aspects(aspects: lang.AspectList) -> Tuple[expr.Expr, bool]: always_valid = False size = None for a in aspects: - if a.f_identifier.text == "Size": - size = create_math_expression(a.f_value, filename) - if a.f_identifier.text == "Always_Valid": - if a.f_value: - av_expr = create_bool_expression(a.f_value, filename) - if av_expr == expr.Variable("True"): - always_valid = True - elif av_expr == expr.Variable("False"): - always_valid = False - else: - error.extend( - [ - ( - f"invalid Always_Valid expression: {av_expr}", - Subsystem.PARSER, - Severity.ERROR, - node_location(a.f_value, filename), - ) - ], + if (a.f_identifier.text == "Size" and size) or ( + a.f_identifier.text == "Always_Valid" and always_valid + ): + error.extend( + [ + ( + f'duplicate aspect "{a.f_identifier.text}"', + Subsystem.PARSER, + Severity.ERROR, + node_location(a, filename), ) - else: - always_valid = True + ], + ) + else: + if a.f_identifier.text == "Size": + size = create_math_expression(a.f_value, filename) + if a.f_identifier.text == "Always_Valid": + if a.f_value: + av_expr = create_bool_expression(a.f_value, filename) + if av_expr == expr.Variable("True"): + always_valid = True + elif av_expr == expr.Variable("False"): + always_valid = False + else: + error.extend( + [ + ( + f"invalid Always_Valid expression: {av_expr}", + Subsystem.PARSER, + Severity.ERROR, + node_location(a.f_value, filename), + ) + ], + ) + else: + always_valid = True if not size: error.extend( [ diff --git a/tests/unit/specification/parser_test.py b/tests/unit/specification/parser_test.py index 39599d503f..99d59686f2 100644 --- a/tests/unit/specification/parser_test.py +++ b/tests/unit/specification/parser_test.py @@ -2043,6 +2043,85 @@ def test_parse_error_duplicate_channel_aspect_writable() -> None: ) +def test_parse_error_duplicate_checksum_aspect() -> None: + assert_error_string( + """ + package Test is + type T is mod 2**8; + type M is + message + F1 : T; + F2 : T; + C1 : T; + F3 : T; + C2 : T + then F4 + if C1'Valid_Checksum and C2'Valid_Checksum; + F4 : T; + end message + with Checksum => (C1 => (F3, F1'First .. F2'Last), + C2 => (C1'Last + 1 .. C2'First - 1)), + Checksum => (C1 => (F3, F1'First .. F2'Last), + C2 => (C1'Last + 1 .. C2'First - 1)), + Byte_Order => Low_Order_First; + end Test; + """, + # ISSUE: Componolit/RecordFlux-parser#56 + # Delete the following line when ticket mentioned above is solved. + "^:17:27: parser: error: Expected 'Byte_Order', got 'First'", + # Uncomment this when ticket #56 is solved. + # r'^:17:27: parser: error: duplicate aspect "Checksum"', + ) + + +def test_parse_error_duplicate_byte_order_aspect() -> None: + assert_error_string( + """ + package Test is + type T is mod 2**8; + type M is + message + F1 : T; + F2 : T; + C1 : T; + F3 : T; + C2 : T + then F4 + if C1'Valid_Checksum and C2'Valid_Checksum; + F4 : T; + end message + with Checksum => (C1 => (F3, F1'First .. F2'Last), + C2 => (C1'Last + 1 .. C2'First - 1)), + Byte_Order => Low_Order_First, + Byte_Order => Low_Order_First; + end Test; + """, + r'^:18:27: parser: error: duplicate aspect "ByteOrderAspect"', + ) + + +def test_parse_error_duplicate_always_valid_aspect() -> None: + assert_error_string( + """ + package Test is + type T is (A, B) with Always_Valid => True, Always_Valid => False; + end Test; + """, + r'^:3:60: parser: error: duplicate aspect "Always_Valid"', + ) + + +def test_parse_error_duplicate_size_aspect() -> None: + assert_error_string( + """ + package Test is + type T is (A, B) with Size => 12, Size => 10; + end Test; + """, + r'^:3:50: parser: error: duplicate aspect "Size"', + ) + + @pytest.mark.parametrize("spec", ["empty_file", "comment_only", "empty_package", "context"]) def test_create_model_no_messages(spec: str) -> None: assert_messages_files([f"{SPEC_DIR}/{spec}.rflx"], [])