From 5cf72487376a21dccf77fe06cd964daa78fce752 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 May 2026 04:06:58 +0000 Subject: [PATCH] Fix issues 1-6: comment loops, round-trip guards, dead branch, discriminator enum helpers, shared _check, cross-reference comments --- src/struct_frame/__init__.py | 4 +-- src/struct_frame/base.py | 31 ++++++++++++++++ src/struct_frame/boilerplate/js/frame-base.js | 2 ++ src/struct_frame/boilerplate/ts/frame-base.ts | 5 +-- src/struct_frame/c_gen.py | 35 ++++++++++--------- src/struct_frame/csharp_gen.py | 29 +++++++-------- src/struct_frame/js_gen.py | 33 ++++++++--------- src/struct_frame/py_gen.py | 29 +++++++-------- src/struct_frame/ts_gen.py | 33 ++++++++--------- tests/run_tests.py | 4 +-- tests/test_caching.py | 5 +-- tests/test_equality.py | 5 +-- tests/test_magic_bytes.py | 9 +---- tests/test_no_packed.py | 5 +-- tests/test_proto_field_types.py | 5 +-- tests/test_utils.py | 13 +++++++ tests/test_validate_flag.py | 5 +-- tests/test_wire_evolution.py | 4 +-- 18 files changed, 142 insertions(+), 114 deletions(-) create mode 100644 tests/test_utils.py diff --git a/src/struct_frame/__init__.py b/src/struct_frame/__init__.py index 91b3d022..caaa0d97 100644 --- a/src/struct_frame/__init__.py +++ b/src/struct_frame/__init__.py @@ -1,4 +1,4 @@ -from .base import version, NamingStyleC, NamingStyleCpp, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values +from .base import version, NamingStyleC, NamingStyleCpp, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values, get_discriminator_enum_name, build_discriminator_enum_values from .c_gen import FileCGen, TestCGen from .ts_gen import FileTsGen, TestTsGen @@ -12,4 +12,4 @@ from .generate import main __all__ = ["main", "FileCGen", "TestCGen", "FileTsGen", "TestTsGen", "FileJsGen", "TestJsGen", "FilePyGen", "TestPyGen", "FileGqlGen", "FileCppGen", "TestCppGen", "FileCSharpGen", "TestCSharpGen", "FileRustGen", "TestRustGen", "version", - "NamingStyleC", "NamingStyleCpp", "camel_to_snake_case", "pascal_case", "build_enum_leading_comments", "build_enum_values"] + "NamingStyleC", "NamingStyleCpp", "camel_to_snake_case", "pascal_case", "build_enum_leading_comments", "build_enum_values", "get_discriminator_enum_name", "build_discriminator_enum_values"] diff --git a/src/struct_frame/base.py b/src/struct_frame/base.py index 24c42bcf..23480de4 100644 --- a/src/struct_frame/base.py +++ b/src/struct_frame/base.py @@ -195,3 +195,34 @@ def build_enum_values(field, naming_style, value_format=None, comment_formatter= enum_values.append(enum_value) return enum_values + + +def get_discriminator_enum_name(oneof, msg_name): + """Return the standard discriminator enum type name used by most generators. + + Produces ``{msg_name}{PascalCase(oneof.name)}Field``, which is the shared + convention for C, C#, JavaScript, Python, and TypeScript generators. + (C++ uses a scoped name without the message prefix and calls its own helper.) + """ + return f'{msg_name}{pascal_case(oneof.name)}Field' + + +def build_discriminator_enum_values(oneof, none_entry_fn, field_entry_fn): + """Build the ``None`` + per-field entries for a field_order discriminator enum. + + Args: + oneof: The oneof model object with ``.fields`` (ordered dict of field_name→field). + none_entry_fn: Callable() → str — returns the formatted ``None = 0`` line. + field_entry_fn: Callable(field_name, field_order, is_last) → str — returns one + formatted enum-value line for the given field. + + Returns: + List of formatted enum-value lines (strings, without trailing newlines). + """ + lines = [none_entry_fn()] + items = list(oneof.fields.items()) + for idx, (field_name, _field) in enumerate(items): + field_order = idx + 1 + is_last = idx == len(items) - 1 + lines.append(field_entry_fn(field_name, field_order, is_last)) + return lines diff --git a/src/struct_frame/boilerplate/js/frame-base.js b/src/struct_frame/boilerplate/js/frame-base.js index 54df9b3d..dcf6b89d 100644 --- a/src/struct_frame/boilerplate/js/frame-base.js +++ b/src/struct_frame/boilerplate/js/frame-base.js @@ -1,4 +1,6 @@ // Struct-frame boilerplate: frame parser base utilities +// NOTE: This file is the JavaScript twin of boilerplate/ts/frame-base.ts. Any algorithm +// fix to GenericFrameParser or related helpers MUST also be applied to the TypeScript source. // Fletcher-16 checksum calculation diff --git a/src/struct_frame/boilerplate/ts/frame-base.ts b/src/struct_frame/boilerplate/ts/frame-base.ts index e58fbc75..3125e277 100644 --- a/src/struct_frame/boilerplate/ts/frame-base.ts +++ b/src/struct_frame/boilerplate/ts/frame-base.ts @@ -1,4 +1,7 @@ // Struct-frame boilerplate: frame parser base utilities +// NOTE: This file is the TypeScript source of the GenericFrameParser and related helpers. +// The JavaScript twin is boilerplate/js/frame-base.js. Any algorithm fix MUST be applied +// to both files. // Fletcher-16 checksum calculation export function fletcherChecksum(buffer: Uint8Array | number[], start: number = 0, end?: number, @@ -240,8 +243,6 @@ export class GenericFrameParser { private getInitialState(): GenericParserState { if (this.config.startBytes.length === 0) { return GenericParserState.GETTING_MSG_ID; - } else if (this.config.startBytes.length === 1) { - return GenericParserState.LOOKING_FOR_START1; } else { return GenericParserState.LOOKING_FOR_START1; } diff --git a/src/struct_frame/c_gen.py b/src/struct_frame/c_gen.py index 9ea0908e..ddafc370 100644 --- a/src/struct_frame/c_gen.py +++ b/src/struct_frame/c_gen.py @@ -7,7 +7,7 @@ functions for binary compatibility across platforms. """ -from struct_frame import version, NamingStyleC, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values +from struct_frame import version, NamingStyleC, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values, get_discriminator_enum_name, build_discriminator_enum_values import time _style_c = NamingStyleC() @@ -199,27 +199,28 @@ def generate_discriminator_enum(oneof, msg_name, package=None): """Generate a discriminator enum for field_order oneofs in C.""" if not oneof.auto_discriminator or oneof.discriminator_type != "field_order": return '' - - result = '' - enum_name = f'{msg_name}{pascal_case(oneof.name)}Field' - - result += f'/* Discriminator enum for {msg_name}::{oneof.name} oneof */\n' + + enum_name = get_discriminator_enum_name(oneof, msg_name) + prefix = f'{camel_to_snake_case(msg_name).upper()}_{camel_to_snake_case(oneof.name).upper()}_FIELD' + + def none_entry(): + return f' {prefix}_NONE = 0,' + + def field_entry(field_name, field_order, is_last): + value = f'{prefix}_{camel_to_snake_case(field_name).upper()}' + return f' {value} = {field_order},' + + lines = build_discriminator_enum_values(oneof, none_entry, field_entry) + result = f'/* Discriminator enum for {msg_name}::{oneof.name} oneof */\n' result += f'typedef enum {enum_name} {{\n' - result += f' {camel_to_snake_case(msg_name).upper()}_{camel_to_snake_case(oneof.name).upper()}_FIELD_NONE = 0,\n' - - for idx, (field_name, field) in enumerate(oneof.fields.items()): - field_order = idx + 1 - # Use SCREAMING_SNAKE_CASE for enum values with message prefix - enum_value = f'{camel_to_snake_case(msg_name).upper()}_{camel_to_snake_case(oneof.name).upper()}_FIELD_{camel_to_snake_case(field_name).upper()}' - result += f' {enum_value} = {field_order},\n' - + result += '\n'.join(lines) + '\n' result += f'}} {enum_name};\n\n' return result - + @staticmethod def get_discriminator_enum_name(oneof, msg_name): """Get the enum type name for a field_order discriminator.""" - return f'{msg_name}{pascal_case(oneof.name)}Field' + return get_discriminator_enum_name(oneof, msg_name) @staticmethod def generate(oneof, package=None, msg_name=None): @@ -332,7 +333,7 @@ def generate(msg, package=None, equality=False): result = '' if leading_comment: for c in msg.comments: - result = '%s\n' % c + result += '%s\n' % c structName = '%s%s' % (pascal_case(msg.package), msg.name) result += 'typedef struct %s {' % structName diff --git a/src/struct_frame/csharp_gen.py b/src/struct_frame/csharp_gen.py index 4006f1cf..cd163fe4 100644 --- a/src/struct_frame/csharp_gen.py +++ b/src/struct_frame/csharp_gen.py @@ -7,7 +7,7 @@ classes with manual Pack/Unpack methods for binary compatibility. """ -from struct_frame import version, NamingStyleC, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values +from struct_frame import version, NamingStyleC, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values, get_discriminator_enum_name, build_discriminator_enum_values import os import time @@ -124,27 +124,28 @@ def generate_discriminator_enum(oneof, msg_name): """Generate a discriminator enum for field_order oneofs in C#.""" if not oneof.auto_discriminator or oneof.discriminator_type != "field_order": return '' - - enum_name = f'{msg_name}{pascal_case(oneof.name)}Field' + + enum_name = get_discriminator_enum_name(oneof, msg_name) + + def none_entry(): + return ' None = 0,' + + def field_entry(field_name, field_order, is_last): + comma = '' if is_last else ',' + return f' {pascal_case(field_name)} = {field_order}{comma}' + + lines = build_discriminator_enum_values(oneof, none_entry, field_entry) result = f' /// Discriminator enum for {msg_name}.{oneof.name} oneof\n' result += f' public enum {enum_name} : byte\n' result += f' {{\n' - result += f' None = 0,\n' - - for idx, (field_name, field) in enumerate(oneof.fields.items()): - field_order = idx + 1 - # Use PascalCase for C# enum values - enum_value = pascal_case(field_name) - comma = ',' if idx < len(oneof.fields) - 1 else '' - result += f' {enum_value} = {field_order}{comma}\n' - + result += '\n'.join(lines) + '\n' result += f' }}\n' return result - + @staticmethod def get_discriminator_enum_name(oneof, msg_name): """Get the enum type name for a field_order discriminator.""" - return f'{msg_name}{pascal_case(oneof.name)}Field' + return get_discriminator_enum_name(oneof, msg_name) class FieldCSharpGen(): diff --git a/src/struct_frame/js_gen.py b/src/struct_frame/js_gen.py index 09137a0f..8e4a0463 100644 --- a/src/struct_frame/js_gen.py +++ b/src/struct_frame/js_gen.py @@ -8,7 +8,7 @@ but outputs JavaScript syntax (CommonJS) instead of TypeScript. """ -from struct_frame import version, NamingStyleC, pascal_case, build_enum_leading_comments, build_enum_values +from struct_frame import version, NamingStyleC, pascal_case, build_enum_leading_comments, build_enum_values, get_discriminator_enum_name, build_discriminator_enum_values from struct_frame.ts_js_base import ( common_types, common_typed_array_methods, @@ -87,27 +87,28 @@ def generate_discriminator_enum(oneof, msg_name): """Generate a discriminator enum for field_order oneofs in JavaScript.""" if not oneof.auto_discriminator or oneof.discriminator_type != "field_order": return '' - - enum_name = f'{msg_name}{pascal_case(oneof.name)}Field' + + enum_name = get_discriminator_enum_name(oneof, msg_name) + + def none_entry(): + return ' None: 0,' + + def field_entry(field_name, field_order, is_last): + comma = '' if is_last else ',' + return f' {pascal_case(field_name)}: {field_order}{comma}' + + lines = build_discriminator_enum_values(oneof, none_entry, field_entry) result = f'/** Discriminator enum for {msg_name}.{oneof.name} oneof */\n' result += f'const {enum_name} = Object.freeze({{\n' - result += f' None: 0,\n' - - for idx, (field_name, field) in enumerate(oneof.fields.items()): - field_order = idx + 1 - # Use PascalCase for JavaScript enum values - enum_value = pascal_case(field_name) - comma = ',' if idx < len(oneof.fields) - 1 else '' - result += f' {enum_value}: {field_order}{comma}\n' - + result += '\n'.join(lines) + '\n' result += f'}});\n' result += f'module.exports.{enum_name} = {enum_name};\n' return result - + @staticmethod def get_discriminator_enum_name(oneof, msg_name): """Get the enum type name for a field_order discriminator.""" - return f'{msg_name}{pascal_case(oneof.name)}Field' + return get_discriminator_enum_name(oneof, msg_name) class FieldJsGen(): @@ -134,7 +135,7 @@ def generate(msg, package_name, package=None): result = '' if leading_comment: for c in msg.comments: - result = '%s\n' % c + result += '%s\n' % c package_msg_name = '%s%s' % (package_name, msg.name) @@ -198,7 +199,7 @@ def generate(msg, package_name, package, packages, equality=False): result = '' if leading_comment: for c in msg.comments: - result = '%s\n' % c + result += '%s\n' % c package_msg_name = msg.name diff --git a/src/struct_frame/py_gen.py b/src/struct_frame/py_gen.py index b8975b9e..2ba67877 100644 --- a/src/struct_frame/py_gen.py +++ b/src/struct_frame/py_gen.py @@ -7,7 +7,7 @@ module for binary packing/unpacking with dataclass-style message definitions. """ -from struct_frame import version, NamingStyleC, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values +from struct_frame import version, NamingStyleC, camel_to_snake_case, pascal_case, build_enum_leading_comments, build_enum_values, get_discriminator_enum_name, build_discriminator_enum_values import time _style_c = NamingStyleC() @@ -81,20 +81,21 @@ def generate_discriminator_enum(oneof, msg_name): """Generate a discriminator enum for field_order oneofs in Python.""" if not oneof.auto_discriminator or oneof.discriminator_type != "field_order": return '' - - enum_name = f'{msg_name}{pascal_case(oneof.name)}Field' + + enum_name = get_discriminator_enum_name(oneof, msg_name) + + def none_entry(): + return ' NONE = 0' + + def field_entry(field_name, field_order, is_last): + return f' {camel_to_snake_case(field_name).upper()} = {field_order}' + + lines = build_discriminator_enum_values(oneof, none_entry, field_entry) result = f'class {enum_name}(Enum):\n' result += f' """Discriminator enum for {msg_name}.{oneof.name} oneof"""\n' - result += f' NONE = 0\n' - - for idx, (field_name, field) in enumerate(oneof.fields.items()): - field_order = idx + 1 - # Use SCREAMING_SNAKE_CASE for enum values - enum_value = camel_to_snake_case(field_name).upper() - result += f' {enum_value} = {field_order}\n' - + result += '\n'.join(lines) + '\n' return result - + @staticmethod def generate_nested(field): """Generate a nested enum class indented inside the message class body.""" @@ -112,7 +113,7 @@ def generate_nested(field): @staticmethod def get_discriminator_enum_name(oneof, msg_name): """Get the enum type name for a field_order discriminator.""" - return f'{msg_name}{pascal_case(oneof.name)}Field' + return get_discriminator_enum_name(oneof, msg_name) class FieldPyGen(): @@ -600,7 +601,7 @@ def generate(msg, equality=False): result = '' if leading_comment: for c in msg.comments: - result = '#%s\n' % c + result += '#%s\n' % c structName = msg.name result += 'class %s:\n' % structName diff --git a/src/struct_frame/ts_gen.py b/src/struct_frame/ts_gen.py index 7a11f7d2..50a0f876 100644 --- a/src/struct_frame/ts_gen.py +++ b/src/struct_frame/ts_gen.py @@ -7,7 +7,7 @@ ES6 module syntax (import/export). """ -from struct_frame import version, NamingStyleC, pascal_case, build_enum_leading_comments, build_enum_values +from struct_frame import version, NamingStyleC, pascal_case, build_enum_leading_comments, build_enum_values, get_discriminator_enum_name, build_discriminator_enum_values from struct_frame.ts_js_base import ( common_types, common_typed_array_methods, @@ -83,26 +83,27 @@ def generate_discriminator_enum(oneof, msg_name): """Generate a discriminator enum for field_order oneofs in TypeScript.""" if not oneof.auto_discriminator or oneof.discriminator_type != "field_order": return '' - - enum_name = f'{msg_name}{pascal_case(oneof.name)}Field' + + enum_name = get_discriminator_enum_name(oneof, msg_name) + + def none_entry(): + return ' None = 0,' + + def field_entry(field_name, field_order, is_last): + comma = '' if is_last else ',' + return f' {pascal_case(field_name)} = {field_order}{comma}' + + lines = build_discriminator_enum_values(oneof, none_entry, field_entry) result = f'/** Discriminator enum for {msg_name}.{oneof.name} oneof */\n' result += f'export enum {enum_name} {{\n' - result += f' None = 0,\n' - - for idx, (field_name, field) in enumerate(oneof.fields.items()): - field_order = idx + 1 - # Use PascalCase for TypeScript enum values - enum_value = pascal_case(field_name) - comma = ',' if idx < len(oneof.fields) - 1 else '' - result += f' {enum_value} = {field_order}{comma}\n' - + result += '\n'.join(lines) + '\n' result += f'}}\n' return result - + @staticmethod def get_discriminator_enum_name(oneof, msg_name): """Get the enum type name for a field_order discriminator.""" - return f'{msg_name}{pascal_case(oneof.name)}Field' + return get_discriminator_enum_name(oneof, msg_name) class FieldTsGen(): @@ -129,7 +130,7 @@ def generate(msg, package_name, package=None): result = '' if leading_comment: for c in msg.comments: - result = '%s\n' % c + result += '%s\n' % c package_msg_name = msg.name @@ -195,7 +196,7 @@ def generate(msg, package_name, package, packages, equality=False): result = '' if leading_comment: for c in msg.comments: - result = '%s\n' % c + result += '%s\n' % c package_msg_name = msg.name diff --git a/tests/run_tests.py b/tests/run_tests.py index df2296d7..f2f7594b 100644 --- a/tests/run_tests.py +++ b/tests/run_tests.py @@ -1959,7 +1959,7 @@ def run_roundtrip_tests(self) -> bool: # ---- C++ ---- cpp = self.languages.get("cpp") - if cpp and self.results["compilation"].get("cpp", True): + if cpp and "cpp" not in self.skipped_languages and self.results["compilation"].get("cpp", False): gen_dir = self.project_root / cpp.gen_output_dir sources = sorted(gen_dir.glob("test_roundtrip_*.cpp")) if not sources: @@ -2013,7 +2013,7 @@ def run_roundtrip_tests(self) -> bool: # ---- C ---- c_lang = self.languages.get("c") - if c_lang and "c" not in self.skipped_languages and self.results["compilation"].get("c", True): + if c_lang and "c" not in self.skipped_languages and self.results["compilation"].get("c", False): gen_dir = self.project_root / c_lang.gen_output_dir sources = sorted(gen_dir.glob("test_roundtrip_*.c")) if not sources: diff --git a/tests/test_caching.py b/tests/test_caching.py index 40e30114..0ab93122 100644 --- a/tests/test_caching.py +++ b/tests/test_caching.py @@ -22,6 +22,7 @@ import os import subprocess import sys +from test_utils import _check import tempfile from pathlib import Path @@ -47,10 +48,6 @@ def _run_generator(proto: Path, out_dir: Path, force: bool = False) -> tuple[int return result.returncode, result.stdout + result.stderr -def _check(condition, msg): - if not condition: - print(f"FAIL: {msg}") - sys.exit(1) def test_hash_file_created(): diff --git a/tests/test_equality.py b/tests/test_equality.py index 961059d1..7cfea1ea 100644 --- a/tests/test_equality.py +++ b/tests/test_equality.py @@ -27,6 +27,7 @@ import shutil import subprocess import sys +from test_utils import _check import tempfile from pathlib import Path @@ -62,10 +63,6 @@ def _run_generator(out_dir: Path, *extra_args: str) -> None: subprocess.check_call(cmd, env=env) -def _check(condition, msg): - if not condition: - print(f"FAIL: {msg}") - sys.exit(1) # --------------------------------------------------------------------------- diff --git a/tests/test_magic_bytes.py b/tests/test_magic_bytes.py index eedac9b0..02fbf5e0 100644 --- a/tests/test_magic_bytes.py +++ b/tests/test_magic_bytes.py @@ -22,6 +22,7 @@ import os import subprocess import sys +from test_utils import _check import tempfile import textwrap from pathlib import Path @@ -34,14 +35,6 @@ # Helpers # --------------------------------------------------------------------------- -def _fail(msg: str) -> None: - print(f"FAIL: {msg}", file=sys.stderr) - sys.exit(1) - - -def _check(condition: bool, msg: str) -> None: - if not condition: - _fail(msg) def _load_model(sf_text: str): diff --git a/tests/test_no_packed.py b/tests/test_no_packed.py index afba983a..fa92c426 100644 --- a/tests/test_no_packed.py +++ b/tests/test_no_packed.py @@ -21,6 +21,7 @@ import shutil import subprocess import sys +from test_utils import _check import tempfile from pathlib import Path @@ -51,10 +52,6 @@ def _read(path: Path) -> str: return path.read_text(encoding="utf-8") -def _check(condition, msg): - if not condition: - print(f"FAIL: {msg}") - sys.exit(1) def test_no_packed_flag(): diff --git a/tests/test_proto_field_types.py b/tests/test_proto_field_types.py index d3f91523..3db810e4 100644 --- a/tests/test_proto_field_types.py +++ b/tests/test_proto_field_types.py @@ -17,6 +17,7 @@ import os import subprocess import sys +from test_utils import _check import tempfile from pathlib import Path @@ -25,10 +26,6 @@ PROTO_FILE = REPO_ROOT / "tests" / "proto" / "test_messages.sf" -def _check(condition, msg): - if not condition: - print(f"FAIL: {msg}", file=sys.stderr) - sys.exit(1) def _generate(out_dir: Path) -> None: diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 00000000..a213b9d6 --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,13 @@ +#!/usr/bin/env python3 +""" +Shared test utilities for struct-frame test suite. +""" + +import sys + + +def _check(condition: bool, msg: str) -> None: + """Assert *condition* is truthy; print a FAIL message and exit if not.""" + if not condition: + print(f"FAIL: {msg}", file=sys.stderr) + sys.exit(1) diff --git a/tests/test_validate_flag.py b/tests/test_validate_flag.py index e25ebeef..300ff0fa 100644 --- a/tests/test_validate_flag.py +++ b/tests/test_validate_flag.py @@ -16,6 +16,7 @@ import os import subprocess import sys +from test_utils import _check import tempfile from pathlib import Path @@ -37,10 +38,6 @@ def _run_validate(proto_path: Path, extra_args=()) -> tuple[int, str, str]: return result.returncode, result.stdout, result.stderr -def _check(condition, msg): - if not condition: - print(f"FAIL: {msg}") - sys.exit(1) def _validation_rejected(code: int, stdout: str, stderr: str) -> bool: diff --git a/tests/test_wire_evolution.py b/tests/test_wire_evolution.py index 89dfb85d..c8c42297 100644 --- a/tests/test_wire_evolution.py +++ b/tests/test_wire_evolution.py @@ -30,6 +30,7 @@ import struct import subprocess import sys +from test_utils import _check import tempfile from pathlib import Path @@ -47,9 +48,6 @@ def _fail(msg: str) -> None: sys.exit(1) -def _check(condition: bool, msg: str) -> None: - if not condition: - _fail(msg) def _generate(out_dir: Path) -> None: