Repository navigation
fix(compiler): fix Go, C++, Swift and Dart codegen bugs - #4159
ayush00git wants to merge 5 commits into
Conversation
- Go: only timestamp and duration add the "time" import; date maps to fory.Date, so generated Go failed to compile with an unused import - Go gRPC: nested RPC payload types now use the owning schema's nested type naming instead of invalid dot-qualified selectors like *Local.Deep - C++: list element optional/ref/encoding specs are no longer silently dropped from generated field metadata (wire-level loss of element tagged encoding and nullability) - Swift: a message visited inside a union cycle no longer caches a provisional Equatable=true result that a later union case overturns - Swift: ref-target resolution threads the full parent lineage, so a ref field in a nested message resolves sibling nested types to final classes - Swift: keyword packages compose flattened identifiers from raw components and escape the whole identifier once (Any_User instead of the invalid `Any`_User), including gRPC service symbols - Dart: struct annotations and field type ids honor the file-level "option evolving = false" default like the other generators Claude-Session: https://claude.ai/code/session_01YcPbRyTJYmU8t6mkGCbnUf
| name = self.to_pascal_case(service.name) | ||
| prefix = self._grpc_prefix() | ||
| return f"{prefix}_{name}" if prefix else name | ||
| return self.safe_type_identifier(f"{prefix}_{name}" if prefix else name) |
There was a problem hiding this comment.
With no package, a valid service Any { rpc Echo (Request) returns (Request); } now makes _service_symbol() return `Any`. Its callers append Provider, Metadata, Message, etc. afterward, producing identifiers such as `Any`Provider, which are invalid Swift; this previously emitted AnyProvider. Please keep the base unescaped and escape each complete symbol after adding its suffix. A regression case with an unqualified Any service would cover the case missed by the new keyword-package test.
There was a problem hiding this comment.
Fixed in 12530bc. _service_symbol now returns the raw composed base; every emitted symbol appends a PascalCase suffix (Provider, Metadata, ...), and the complete composed name can never equal a Swift keyword, so no mid-identifier escaping can occur. Added test_swift_grpc_keyword_service_name_without_package covering an unqualified service Any (asserts AnyProvider/AnyMetadata and no backticks); it fails on the previous revision.
| return False | ||
|
|
||
| self._equatable_cache[key] = True | ||
| if is_cycle_root: |
There was a problem hiding this comment.
is_cycle_root is false for every nested call, even when the type graph has no cycle, so this also disables memoization of fully resolved acyclic dependencies. For example, declare A0 through An in that order, give each Ai two list<A(i+1)> fields, and give An an int32 field. While generating A0, both fields independently traverse the entire remaining graph because none of the successful child results are cached. The number of recursive visits grows as O(2^n), whereas the previous implementation reused each child's result. Please retain memoization for results that do not depend on a provisional cycle back-edge, and add a repeated-dependency DAG regression case alongside the cycle test.
There was a problem hiding this comment.
Fixed in 12530bc. A provisional back-edge hit (message or union) now records the in-progress type in a pending-dependency set; a computed True is cached unless a pending dependency remains after the node completes, and each node discards itself from the set on completion. Acyclic traversals never take a provisional hit, so every fully resolved result memoizes and the repeated-dependency chain is linear again; cycle-tainted True results stay uncached until recomputed from a resolved root. Added test_acyclic_equatable_results_are_memoized (asserts every message in a two-fields-per-level chain lands in the cache after one root query); it fails on the previous revision.
There was a problem hiding this comment.
There is still an exponential case in 12530bccc: a DAG reached after a sibling back-edge. Prepend this message to the new test's A0 through An chain and query Root with a fresh generator:
message Root {
list<Root> children = 1;
list<A0> data = 2;
}
Visiting children puts Root in _equatable_pending, where it stays until the entire root finishes. Consequently, the new if not self._equatable_pending condition prevents every successful Ai result from being cached, although none of those nodes depends on Root. The two fields per level still cause O(2^n) recursive visits. The new test queries the acyclic chain directly, so it misses this case. Please base cache eligibility on the current result's unresolved dependencies and add this mixed cycle/DAG case. This follows from the call path; I have not run a local timing measurement.
…ce symbol base raw Review follow-ups: provisional back-edge hits now mark the in-progress type as a pending dependency, so acyclic results memoize (linear traversal) while cycle-tainted True results stay uncached; the gRPC service symbol base is no longer escaped before suffixes are appended, so an unqualified service named Any emits AnyProvider instead of invalid backtick-embedded identifiers. Claude-Session: https://claude.ai/code/session_01YcPbRyTJYmU8t6mkGCbnUf
| outer optional/ref would wrap the container spec in an extra | ||
| ::fory::T::inner(...) that get_field_meta cannot dispatch on. | ||
| """ | ||
| return self.get_type_spec(field.field_type) |
There was a problem hiding this comment.
For list<ref TreeNode>, this now emits .list(::fory::FieldNodeSpec{}), which selects write_configured_list_data() instead of the ordinary collection writer. That configured writer never sets COLL_TRACKING_REF and passes only RefMode::None or NullOnly to the element serializer, even with global reference tracking enabled. Repeated pointers therefore lose their identity on round-trip. The existing tree fixture uses {child_a, child_a, child_b}, and the current C++ IDL job fails with IDL roundtrip failed: tree children mismatch. Please preserve reference tracking in this configured list path and make the existing IDL round-trip pass.
The configured list writer never set COLL_TRACKING_REF and wrote elements with RefMode::None/NullOnly, so shared-ref elements reached by a configured field spec lost identity on round-trip. Mirror the ordinary collection writer: set the bit and write with RefMode::Tracking when the element is a shared ref and global reference tracking is enabled. The configured read path already honors the bit. Claude-Session: https://claude.ai/code/session_01YcPbRyTJYmU8t6mkGCbnUf
… fix/generator-codegen-bugs
What does this PR do?
collect_importsaddedimport "time"fordatefields, butdatemaps tofory.Date, so generated Go failed to compile with an unused import. Onlytimestampanddurationimporttimenow.*Local.Deep/*shared.Outer.Inner, which are invalid Go selectors. Local types now resolve through the nested-type naming used by the model generator, and imported types are formatted with the imported schema'sgo_nested_type_style.get_field_type_specpassed the field-level element optional/ref copies as the outer optional/ref of a container, wrapping the spec in an extra::fory::T::inner(...)that matched no dispatch prefix inget_field_meta, so the whole field spec was silently dropped and element tagged encoding/nullability was lost on the wire. The spec is now built from the type node, which owns the element flags.Equatable = trueresult; when a later union case (for exampleany) overturned the union's answer, the poisoned cache emittedstruct Node: Equatablewith a non-Equatable stored property that did not compile. Only a cycle-root call may cache a true result now.parent_stack=[message]instead of the real lineage, so areffield in a nested message could not resolve a sibling nested type, and the target silently stayed a value struct instead of a final class, breaking reference identity against Java/C++. The recursion now threads the full parent lineage.anyproduced invalid identifiers such as`Any`_Userand gRPC`Any`_GreeterProvider. Flattened identifiers are now composed from raw PascalCase components and escaped once as a whole (Any_User); enum-style namespaces keep per-component escaping since`Any`.Useris valid.@ForyStructannotations and message field type ids read only the message-levelevolvingoption and ignored the file-leveloption evolving = false;default every other generator honors, producingTypeIds.compatibleStructwhile peers emitted schema-consistent structs (wire mismatch). Dart now resolves the effective evolving flag like the JavaScript generator, with imported types using their own file's default.Rust (#4155) was not changed: the duplicate
Unknownunion variant is already fixed on main (unknown_case_namerenames the generated carrier, covered bytest_union_unknown_carrier_names), andoptional anyis now rejected by the IR validator, so the optional-any Default/Debug bug is no longer reachable. The C++optional anyaccessor bug is unreachable for the same reason.Verification: each new regression test fails without its fix and passes with it; the full compiler suite (
python3 -m pytest fory_compiler/tests -q) passes with 544 passed, 1 skipped.Closes #4154
Closes #4156
Closes #4157
Closes #4158