From 83403fe993ba95a2673f27343a2fff301bf24c52 Mon Sep 17 00:00:00 2001 From: Henri Chataing Date: Fri, 22 Dec 2023 01:16:18 -0800 Subject: [PATCH 1/2] Improve support for open enums in generated pytnon - previously generated tags x_START and x_END for tag ranges, but IntEnum declarations are closed so parsing a value in these ranges would fail - instead removed declaration for range bounds, and relax the generated code to use Union[IntEnum, int] instead of IntEnum - provide IntEnum.from_int to validate values to are not matched to tags, that they are correctly inside of the ranges --- doc/python-generated-code-guide.rst | 12 +++--- .../scripts/generate_python_backend.py | 37 +++++++++++++++---- pdl-compiler/scripts/pdl/core.py | 6 +++ pdl-compiler/tests/python_generator_test.py | 3 +- 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/doc/python-generated-code-guide.rst b/doc/python-generated-code-guide.rst index e49a0d7..58dd60d 100644 --- a/doc/python-generated-code-guide.rst +++ b/doc/python-generated-code-guide.rst @@ -47,14 +47,16 @@ Enum declarations | | | | enum TestEnum : 8 { | class TestEnum(enum.IntEnum): | | A = 1, | A = 1 | -| B = 2..3, | B_MIN = 2 | -| C = 4, | B_MAX = 3 | -| OTHER = .., | C = 4 | -| } | | +| B = 2..3, | C = 4 | +| C = 4, | | +| OTHER = .., | @staticmethod | +| } | def from_int(v: int) -> typing.Union[TestEnum, int]: | +| | pass | +---------------------------------------+---------------------------------------------------------------+ .. note:: - Python enums are open by construction, default cases in enum declarations are ignored. + Python enums are closed by construction, default cases in enum declarations are ignored. + The static method `from_int` provides validation for enum tag ranges. Packet declarations ^^^^^^^^^^^^^^^^^^^ diff --git a/pdl-compiler/scripts/generate_python_backend.py b/pdl-compiler/scripts/generate_python_backend.py index 8938173..7bd9106 100755 --- a/pdl-compiler/scripts/generate_python_backend.py +++ b/pdl-compiler/scripts/generate_python_backend.py @@ -33,7 +33,7 @@ def mask(width: int) -> str: def generate_prelude() -> str: return dedent("""\ from dataclasses import dataclass, field, fields - from typing import Optional, List, Tuple + from typing import Optional, List, Tuple, Union import enum import inspect import math @@ -412,7 +412,7 @@ def parse_bit_field_(self, field: ast.Field): self.unchecked_append_(f"if {v} != {hex(field.value)}:") self.unchecked_append_(f" raise Exception('Unexpected fixed field value')") elif isinstance(field, ast.TypedefField): - self.unchecked_append_(f"fields['{field.id}'] = {field.type_id}({v})") + self.unchecked_append_(f"fields['{field.id}'] = {field.type_id}.from_int({v})") elif isinstance(field, ast.SizeField): self.unchecked_append_(f"{field.field_id}_size = {v}") elif isinstance(field, ast.CountField): @@ -1012,17 +1012,37 @@ def generate_enum_declaration(decl: ast.EnumDeclaration) -> str: enum_name = decl.id tag_decls = [] for t in decl.tags: + # Enums in python are closed and ranges cannot be represented; + # instead the generated code uses Union[int, Enum] + # when ranges are used. if t.value is not None: tag_decls.append(f"{t.id} = {hex(t.value)}") - if t.range is not None: - tag_decls.append(f"{t.id}_START = {hex(t.range[0])}") - tag_decls.append(f"{t.id}_END = {hex(t.range[1])}") + + if core.is_open_enum(decl): + unknown_handler = ["return v"] + else: + unknown_handler = [] + for t in decl.tags: + if t.range is not None: + unknown_handler.append(f"if v >= 0x{t.range[0]:x} and v <= 0x{t.range[1]:x}:") + unknown_handler.append(f" return v") + unknown_handler.append("raise exn") return dedent("""\ class {enum_name}(enum.IntEnum): {tag_decls} - """).format(enum_name=enum_name, tag_decls=indent(tag_decls, 1)) + + @staticmethod + def from_int(v: int) -> Union[int, '{enum_name}']: + try: + return {enum_name}(v) + except ValueError as exn: + {unknown_handler} + + """).format(enum_name=enum_name, + tag_decls=indent(tag_decls, 1), + unknown_handler=indent(unknown_handler, 3)) def generate_packet_declaration(packet: ast.Declaration) -> str: @@ -1047,7 +1067,10 @@ def generate_packet_declaration(packet: ast.Declaration) -> str: elif isinstance(f, ast.ScalarField): field_decls.append(f"{f.id}: int = field(kw_only=True, default=0)") elif isinstance(f, ast.TypedefField): - if isinstance(f.type, ast.EnumDeclaration): + if isinstance(f.type, ast.EnumDeclaration) and f.type.tags[0].range: + field_decls.append( + f"{f.id}: {f.type_id} = field(kw_only=True, default={f.type.tags[0].range[0]})") + elif isinstance(f.type, ast.EnumDeclaration): field_decls.append( f"{f.id}: {f.type_id} = field(kw_only=True, default={f.type_id}.{f.type.tags[0].id})") elif isinstance(f.type, ast.ChecksumDeclaration): diff --git a/pdl-compiler/scripts/pdl/core.py b/pdl-compiler/scripts/pdl/core.py index 5ddbb02..27da56b 100644 --- a/pdl-compiler/scripts/pdl/core.py +++ b/pdl-compiler/scripts/pdl/core.py @@ -360,3 +360,9 @@ def is_bit_field(field: Field) -> bool: else: return False + +def is_open_enum(decl: EnumDeclaration) -> bool: + """Return true if the enum declaration is open, i.e. contains a tag + declaration of the form DEFAULT = ..""" + + return any(t.value is None for t in decl.tags) diff --git a/pdl-compiler/tests/python_generator_test.py b/pdl-compiler/tests/python_generator_test.py index a126c8d..97e4968 100644 --- a/pdl-compiler/tests/python_generator_test.py +++ b/pdl-compiler/tests/python_generator_test.py @@ -69,7 +69,8 @@ def create_object(typ, value): elif typ is bytearray: return bytearray(value) elif issubclass(typ, enum.Enum): - return typ(value) + from_int = getattr(typ, 'from_int') + return from_int(value) elif typ is int: return value else: From 529b4d37b39be4ade62301032d25f6ed3a1ecc80 Mon Sep 17 00:00:00 2001 From: Henri Chataing Date: Tue, 2 Jan 2024 10:47:26 +0100 Subject: [PATCH 2/2] Add exhaustive canonical tests for enum configurations Test enum declarations with exhaustive configurations for the following criterias: 1. truncated: is the enum width a multiple of 8 or not ? this characteristic has an impact for some language generators 2. complete: does the enum define all possible values ? 3. open: does the enum allow for unknown or undefined values ? 4. with range: does the enum use a tag range declarations ? --- pdl-compiler/scripts/generate_cxx_backend.py | 8 +- .../src/bin/generate-canonical-tests.rs | 7 + pdl-compiler/tests/canonical/le_test_file.pdl | 98 ++++++++ .../tests/canonical/le_test_vectors.json | 227 ++++++++++++++++++ 4 files changed, 338 insertions(+), 2 deletions(-) diff --git a/pdl-compiler/scripts/generate_cxx_backend.py b/pdl-compiler/scripts/generate_cxx_backend.py index 4937079..0bb0d9c 100755 --- a/pdl-compiler/scripts/generate_cxx_backend.py +++ b/pdl-compiler/scripts/generate_cxx_backend.py @@ -817,7 +817,9 @@ def generate_enum_declaration(decl: ast.EnumDeclaration) -> str: enum_type = get_cxx_scalar_type(decl.width) tag_decls = [] for t in decl.tags: - tag_decls.append(f"{t.id} = {hex(t.value)},") + # Exclude default tags: DEFAULT = .. + if t.value is not None: + tag_decls.append(f"{t.id} = {hex(t.value)},") return dedent("""\ @@ -833,7 +835,9 @@ def generate_enum_to_text(decl: ast.EnumDeclaration) -> str: enum_name = decl.id tag_cases = [] for t in decl.tags: - tag_cases.append(f"case {enum_name}::{t.id}: return \"{t.id}\";") + # Exclude default tags: DEFAULT = .. + if t.value is not None: + tag_cases.append(f"case {enum_name}::{t.id}: return \"{t.id}\";") return dedent("""\ diff --git a/pdl-compiler/src/bin/generate-canonical-tests.rs b/pdl-compiler/src/bin/generate-canonical-tests.rs index 4ea3c82..acb0c12 100644 --- a/pdl-compiler/src/bin/generate-canonical-tests.rs +++ b/pdl-compiler/src/bin/generate-canonical-tests.rs @@ -241,6 +241,13 @@ fn main() { "Struct_FixedScalar_Field", "Struct_Size_Field", "Struct_Struct_Field", + "Enum_Incomplete_Truncated_Closed", + "Enum_Incomplete_Truncated_Open", + "Enum_Incomplete_Truncated_Closed_WithRange", + "Enum_Incomplete_Truncated_Open_WithRange", + "Enum_Complete_Truncated", + "Enum_Complete_Truncated_WithRange", + "Enum_Complete_WithRange", ], &module_name, ); diff --git a/pdl-compiler/tests/canonical/le_test_file.pdl b/pdl-compiler/tests/canonical/le_test_file.pdl index fa87314..833b723 100644 --- a/pdl-compiler/tests/canonical/le_test_file.pdl +++ b/pdl-compiler/tests/canonical/le_test_file.pdl @@ -838,3 +838,101 @@ struct Struct_Optional_Struct_Field_ { packet Struct_Optional_Struct_Field { s: Struct_Optional_Struct_Field_, } + +// Enum declarations +// +// Test enum declarations with exhaustive configurations for the +// following criterias: +// +// 1. truncated: is the enum width a multiple of 8 or not ? +// this characteristic has an impact for some language generators +// 2. complete: does the enum define all possible values ? +// 3. open: does the enum allow for unknown or undefined values ? +// 4. with range: does the enum use a tag range declarations ? + +enum Enum_Incomplete_Truncated_Closed_ : 3 { + A = 0, + B = 1, +} + +packet Enum_Incomplete_Truncated_Closed { + e: Enum_Incomplete_Truncated_Closed_, + _reserved_ : 5, +} + +enum Enum_Incomplete_Truncated_Open_ : 3 { + A = 0, + B = 1, + UNKNOWN = .. +} + +packet Enum_Incomplete_Truncated_Open { + e: Enum_Incomplete_Truncated_Open_, + _reserved_ : 5, +} + +enum Enum_Incomplete_Truncated_Closed_WithRange_ : 3 { + A = 0, + B = 1..6 { + X = 1, + Y = 2, + } +} + +packet Enum_Incomplete_Truncated_Closed_WithRange { + e: Enum_Incomplete_Truncated_Closed_WithRange_, + _reserved_ : 5, +} + +enum Enum_Incomplete_Truncated_Open_WithRange_ : 3 { + A = 0, + B = 1..6 { + X = 1, + Y = 2, + }, + UNKNOWN = .. +} + +packet Enum_Incomplete_Truncated_Open_WithRange { + e: Enum_Incomplete_Truncated_Open_WithRange_, + _reserved_ : 5, +} + +enum Enum_Complete_Truncated_ : 3 { + A = 0, + B = 1, + C = 2, + D = 3, + E = 4, + F = 5, + G = 6, + H = 7, +} + +packet Enum_Complete_Truncated { + e: Enum_Complete_Truncated_, + _reserved_ : 5, +} + +enum Enum_Complete_Truncated_WithRange_ : 3 { + A = 0, + B = 1..7 { + X = 1, + Y = 2, + } +} + +packet Enum_Complete_Truncated_WithRange { + e: Enum_Complete_Truncated_WithRange_, + _reserved_ : 5, +} + +enum Enum_Complete_WithRange_ : 8 { + A = 0, + B = 1, + C = 2..255, +} + +packet Enum_Complete_WithRange { + e: Enum_Complete_WithRange_, +} diff --git a/pdl-compiler/tests/canonical/le_test_vectors.json b/pdl-compiler/tests/canonical/le_test_vectors.json index 9ddff45..e6fb3b8 100644 --- a/pdl-compiler/tests/canonical/le_test_vectors.json +++ b/pdl-compiler/tests/canonical/le_test_vectors.json @@ -5095,5 +5095,232 @@ } } ] + }, + { + "packet": "Enum_Incomplete_Truncated_Closed", + "tests": [ + { + "packed": "00", + "unpacked": { + "e": 0 + } + }, + { + "packed": "01", + "unpacked": { + "e": 1 + } + } + ] + }, + { + "packet": "Enum_Incomplete_Truncated_Open", + "tests": [ + { + "packed": "00", + "unpacked": { + "e": 0 + } + }, + { + "packed": "01", + "unpacked": { + "e": 1 + } + }, + { + "packed": "02", + "unpacked": { + "e": 2 + } + } + ] + }, + { + "packet": "Enum_Incomplete_Truncated_Closed_WithRange", + "tests": [ + { + "packed": "00", + "unpacked": { + "e": 0 + } + }, + { + "packed": "01", + "unpacked": { + "e": 1 + } + }, + { + "packed": "02", + "unpacked": { + "e": 2 + } + } + ] + }, + { + "packet": "Enum_Incomplete_Truncated_Open_WithRange", + "tests": [ + { + "packed": "00", + "unpacked": { + "e": 0 + } + }, + { + "packed": "01", + "unpacked": { + "e": 1 + } + }, + { + "packed": "02", + "unpacked": { + "e": 2 + } + }, + { + "packed": "03", + "unpacked": { + "e": 3 + } + } + ] + }, + { + "packet": "Enum_Complete_Truncated", + "tests": [ + { + "packed": "00", + "unpacked": { + "e": 0 + } + }, + { + "packed": "01", + "unpacked": { + "e": 1 + } + }, + { + "packed": "02", + "unpacked": { + "e": 2 + } + }, + { + "packed": "03", + "unpacked": { + "e": 3 + } + }, + { + "packed": "04", + "unpacked": { + "e": 4 + } + }, + { + "packed": "05", + "unpacked": { + "e": 5 + } + }, + { + "packed": "06", + "unpacked": { + "e": 6 + } + }, + { + "packed": "07", + "unpacked": { + "e": 7 + } + } + ] + }, + { + "packet": "Enum_Complete_Truncated_WithRange", + "tests": [ + { + "packed": "00", + "unpacked": { + "e": 0 + } + }, + { + "packed": "01", + "unpacked": { + "e": 1 + } + }, + { + "packed": "02", + "unpacked": { + "e": 2 + } + }, + { + "packed": "03", + "unpacked": { + "e": 3 + } + }, + { + "packed": "04", + "unpacked": { + "e": 4 + } + }, + { + "packed": "05", + "unpacked": { + "e": 5 + } + }, + { + "packed": "06", + "unpacked": { + "e": 6 + } + }, + { + "packed": "07", + "unpacked": { + "e": 7 + } + } + ] + }, + { + "packet": "Enum_Complete_WithRange", + "tests": [ + { + "packed": "00", + "unpacked": { + "e": 0 + } + }, + { + "packed": "01", + "unpacked": { + "e": 1 + } + }, + { + "packed": "02", + "unpacked": { + "e": 2 + } + }, + { + "packed": "ff", + "unpacked": { + "e": 255 + } + } + ] } ]