Detect duplicate messages and channels aspects #930

mhadhbir · 2022-02-09T13:17:56Z

~~xCloses #714~~

This commit introduces extra checks to avoid duplicate aspects for messages and channels. Closes #714

rflx/specification/parser.py

tests/unit/specification/parser_test.py

treiher · 2022-02-09T14:29:38Z

rflx/specification/parser.py

@@ -533,10 +533,23 @@ def create_channel_decl(
    filename: Path,


please also have a look at other uses of aspects. For example, messages can have Checksum aspects and transitions in sessions can have Desc aspects. Maybe we have the same issue there.

See #714 (comment).

What about the Desc aspect mentioned above? Please add a unit test, even if a duplicate aspect is already detected (even as a syntax error).

I didn't do anything for Desc as I think the code doesn't allow more than one Desc anyway, so I don't think it is necessary to add a test for this as I didn't implement it.

Even if it is not possible to have duplicate Desc aspects at the moment, this doesn't mean that this cannot be the case in the future. Unit test are used to prevent regressions. That is why it still makes sense to add a unit test, even if you did not change anything.

test added.

Fixes #714

treiher · 2022-02-17T10:35:01Z

tests/unit/specification/parser_test.py

+def test_parse_error_duplicate_channel_aspect_readable() -> None:
+    assert_error_string(
+        """
+            package Test is
+
+               type M is
+                  message
+                     B : Opaque
+                        with Size => 8;
+                  end message;
+
+               generic
+                  Channel : Channel with Readable, Writable, Readable;
+               session Session with
+                  Initial => Start,
+                  Final => Terminated
+               is
+                  Message : M;
+               begin
+                  state Start is
+                  begin
+                     Channel'Read (Message);
+                  transition
+                     goto Reply
+                        if Message'Valid
+                     goto Terminated
+                  end Start;
+
+                  state Reply is
+                  begin
+                     Channel'Write (Message);
+                  transition
+                     goto Terminated
+                  end Reply;
+
+                  state Terminated is null state;
+               end Session;
+
+            end Test;
+        """,
+        r'^<stdin>:11:62: parser: error: duplicate aspect "Readable"',
+    )
+
+
+def test_parse_error_duplicate_channel_aspect_writable() -> None:
+    assert_error_string(
+        """
+            package Test is
+
+               type M is
+                  message
+                     B : Opaque
+                        with Size => 8;
+                  end message;
+
+               generic
+                  Channel : Channel with Readable, Writable, Writable;
+               session Session with
+                  Initial => Start,
+                  Final => Terminated
+               is
+                  Message : M;
+               begin
+                  state Start is
+                  begin
+                     Channel'Read (Message);
+                  transition
+                     goto Reply
+                        if Message'Valid
+                     goto Terminated
+                  end Start;
+
+                  state Reply is
+                  begin
+                     Channel'Write (Message);
+                  transition
+                     goto Terminated
+                  end Reply;
+
+                  state Terminated is null state;
+               end Session;
+
+            end Test;
+        """,
+        r'^<stdin>:11:62: parser: error: duplicate aspect "Writable"',
+    )


Duplicate code can be avoided by using @pytest.mark.parametrize.

treiher · 2022-02-17T10:36:46Z

tests/unit/specification/parser_test.py

+                          Byte_Order => Low_Order_First,
+                          Byte_Order => Low_Order_First;
+            end Test;
+        """,
+        r'^<stdin>:18:27: parser: error: duplicate aspect "ByteOrderAspect"',


The aspect is called Byte_Order, not ByteOrderAspect.

That's what the error message generates.

Then the error message needs to be changed.

That means changing aspect.kind_name, if you don't like the message I suggest you open a ticket to change the attribute _kind_name of the class ByteOrderAspect from librflxlang.

Changing kind_name is not possible. Please find another solution.

In that case I think the error message should stay the same to keep the code consistent then.

The error messages are intended for users of the tool. Having a error message mentioning "ByteOrderAspect", while the aspect is actually called "Byte_Order" in the specification, is confusing. Keeping the code as it as, just because it is easier, is no option here.

First, I don't think ByteOrderAspect is confusing, it literally literally has Byte Order and Aspect in it. and I already proposed a solution to change kind_name if you don't like the exact wording. I don't see another way to do it.

treiher · 2022-02-17T10:48:23Z

rflx/specification/parser.py

@@ -218,9 +219,10 @@ def create_session(
    session: lang.SessionDecl,
    package: ID,
    filename: Path,
+    error: RecordFluxError,


All create_* functions, which had an error parameter before, had this as the first parameter. Could you please make that consistent to the existing code? A consistent order of parameters is especially relevant when function pointers are used, which is done in several cases here. I also would like to prevent confusions in the future.

treiher · 2022-02-17T10:53:16Z

rflx/specification/parser.py

+    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),
+                    )
+                ],
+            )


Are checksum_parsed and byte_order_parsed required? Couldn't just checksum_result and byte_order_result be used?

Not really, fixed

using checksum_result is causing mypy to raise typing errors, I am putting checksum_parsed back

treiher · 2022-02-17T10:57:38Z

rflx/specification/parser.py

-        raise NotImplementedError(
-            f"Enumeration kind {enumeration.f_elements.kind_name} unsupported"
-        )
+        raise NotImplementedError(f"Enmeration kind {enumeration.f_elements.kind_name} unsupported")


A typo is added here: Enmeration.

treiher · 2022-02-17T11:01:24Z

tests/unit/specification/grammar_test.py

 def parse_formal_declaration(data: str) -> decl.Declaration:
    parser_declaration, filename = parse(data, lang.GrammarRule.session_parameter_rule)
    assert isinstance(parser_declaration, lang.FormalDecl)
-    declaration = create_formal_declaration(parser_declaration, ID("Package"), filename)
+    error = RecordFluxError()
+    declaration = create_formal_declaration(parser_declaration, ID("Package"), filename, error)
    assert isinstance(declaration, decl.Declaration)
    return declaration


error.propagate() should be called after create_*, otherwise any error in create_* will be just ignored.

treiher · 2022-02-17T11:01:39Z

tests/unit/specification/grammar_test.py

    if diagnostics_to_error(unit.diagnostics, error, STDIN):
        error.propagate()
    assert isinstance(unit.root, lang.SessionDecl)
-    return create_session(unit.root, ID("Package"), Path("<stdin>"))
+    return create_session(unit.root, ID("Package"), Path("<stdin>"), error)


error.propagate() should be called after create_*, otherwise any error in create_* will be just ignored.

treiher · 2022-02-17T11:01:56Z

tests/unit/specification/grammar_test.py

@@ -73,7 +74,7 @@ def parse_unproven_session(string: str) -> model.UnprovenSession:
    if diagnostics_to_error(unit.diagnostics, error, STDIN):
        error.propagate()
    assert isinstance(unit.root, lang.SessionDecl)
-    return create_unproven_session(unit.root, ID("Package"), Path("<stdin>"))
+    return create_unproven_session(unit.root, ID("Package"), Path("<stdin>"), error)


error.propagate() should be called after create_*, otherwise any error in create_* will be just ignored.

treiher · 2022-02-17T11:05:57Z

tests/unit/specification/parser_test.py

+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'^<stdin>:3:50: parser: error: duplicate aspect "Size"',
+    )


Are duplicate Size aspects also detected for range integers?

The logic of detecting duplicate Size aspects doesn't care about the format of the expression given to the aspect Size, so the answer to your question is Yes.

Please add a regression test nevertheless.

wouldn't be code duplication as well?

Fixes #714

Detect duplicate messages and channels aspects

f86678c

This commit introduces extra checks to avoid duplicate aspects for messages and channels. Closes #714

mhadhbir force-pushed the issue_714 branch from b70c1f9 to f86678c Compare February 9, 2022 13:50

mhadhbir requested review from treiher and senier February 9, 2022 13:51

treiher requested changes Feb 9, 2022

View reviewed changes

Get error from session down to create_channel_decl

fdd23bb

Fixes #714

mhadhbir force-pushed the issue_714 branch from a32a51f to fdd23bb Compare February 10, 2022 16:50

mhadhbir requested a review from treiher February 15, 2022 15:21

mhadhbir mentioned this pull request Feb 15, 2022

Investigate/Fix syntax error for duplicate Checksum aspect AdaCore/RecordFlux-parser#56

Closed

mhadhbir force-pushed the issue_714 branch 2 times, most recently from 006fab2 to fcb9cd0 Compare February 15, 2022 17:11

mhadhbir self-assigned this Feb 15, 2022

mhadhbir force-pushed the issue_714 branch from fcb9cd0 to e39e680 Compare February 16, 2022 11:55

treiher requested changes Feb 17, 2022

View reviewed changes

mhadhbir force-pushed the issue_714 branch from e39e680 to 41dbb1c Compare February 17, 2022 14:24

mhadhbir requested a review from treiher February 17, 2022 14:25

mhadhbir force-pushed the issue_714 branch 4 times, most recently from 210d1c2 to 4adca2e Compare February 17, 2022 16:39

mhadhbir marked this pull request as draft February 17, 2022 16:52

Detecting duplicate aspects

7f5f99a

Fixes #714

mhadhbir force-pushed the issue_714 branch from 4adca2e to 7f5f99a Compare February 18, 2022 10:27

mhadhbir marked this pull request as ready for review February 18, 2022 11:24

senier closed this Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect duplicate messages and channels aspects #930

Detect duplicate messages and channels aspects #930

mhadhbir commented Feb 9, 2022 •

edited by senier

Loading

treiher Feb 9, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022 •

edited

Loading

treiher Feb 17, 2022

mhadhbir Feb 18, 2022

treiher Feb 18, 2022

mhadhbir Feb 18, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

mhadhbir Feb 18, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 17, 2022

treiher Feb 17, 2022

mhadhbir Feb 18, 2022

		@@ -533,10 +533,23 @@ def create_channel_decl(
		filename: Path,

Detect duplicate messages and channels aspects #930

Detect duplicate messages and channels aspects #930

Conversation

mhadhbir commented Feb 9, 2022 • edited by senier Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhadhbir Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhadhbir commented Feb 9, 2022 •

edited by senier

Loading

mhadhbir Feb 17, 2022 •

edited

Loading