Skip to content

fix(compiler): fix Go, C++, Swift and Dart codegen bugs - #4159

Open
ayush00git wants to merge 5 commits into
apache:mainfrom
ayush00git:fix/generator-codegen-bugs
Open

ayush00git wants to merge 5 commits into
apache:mainfrom
ayush00git:fix/generator-codegen-bugs

Conversation

@ayush00git

Copy link
Copy Markdown
Contributor

What does this PR do?

  • Go: collect_imports added import "time" for date fields, but date maps to fory.Date, so generated Go failed to compile with an unused import. Only timestamp and duration import time now.
  • Go gRPC: nested RPC payload types were emitted as dot-qualified Go types such as *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's go_nested_type_style.
  • C++: get_field_type_spec passed 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 in get_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.
  • Swift: a message visited inside a union cycle permanently cached a provisional Equatable = true result; when a later union case (for example any) overturned the union's answer, the poisoned cache emitted struct Node: Equatable with a non-Equatable stored property that did not compile. Only a cycle-root call may cache a true result now.
  • Swift: ref-target resolution used parent_stack=[message] instead of the real lineage, so a ref field 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.
  • Swift: package components were keyword-escaped individually and then joined, so package any produced invalid identifiers such as `Any`_User and 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`.User is valid.
  • Dart: @ForyStruct annotations and message field type ids read only the message-level evolving option and ignored the file-level option evolving = false; default every other generator honors, producing TypeIds.compatibleStruct while 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 Unknown union variant is already fixed on main (unknown_case_name renames the generated carrier, covered by test_union_unknown_carrier_names), and optional any is now rejected by the IR validator, so the optional-any Default/Debug bug is no longer reachable. The C++ optional any accessor 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

- 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
@ayush00git
ayush00git requested a review from chaokunyang October 9, 2026 12:04
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ayush00git
ayush00git requested a review from chaokunyang October 9, 2026 12:18
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants