[swift][swift6] fix: recursive schemas generate structs of infinite size - #24898
[swift][swift6] fix: recursive schemas generate structs of infinite size#24898wiebren wants to merge 2 commits into
Conversation
A struct that stores itself inline - through any chain of model-typed properties, Optional included - has infinite size: "value type cannot have a stored property that recursively contains it", and every struct that embeds it is infinite in turn. The generators emitted structs unconditionally, so any self- or mutually-referencing schema produced a client that does not compile; the only escape was useClasses=true, which turns every model into a class. Detect inline reference cycles in postProcessAllModels (containers break recursion on the heap and are not edges) and render only the models on a cycle as final classes - heap allocation provides the indirection, the wire format is unchanged, and everything else stays a struct. In swift6 a recursion-breaking class is @unchecked Sendable, the way readonlyProperties classes already are, so the Sendable structs embedding it still conform. The per-model rendering flag also carries the global useClasses value, and the useClasses samples (urlsession, vapor, swift5 and swift6 alike) regenerate byte-identical. Fixes OpenAPITools#15240 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
There was a problem hiding this comment.
4 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift6ClientCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift6ClientCodegen.java:854">
P1: When a cycle is expressed through `oneOf`, this flag does not break recursion because the one-of template still emits a non-`indirect` enum. Handle these cycles with an indirect representation (or a separate cycle strategy) instead of marking them as fixed by the class extension.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/swift5/modelObject.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/swift5/modelObject.mustache:124">
P1: When the default `hashableModels` setting is used, a newly class-rendered recursive model still gets recursive `==` and `hash(into:)` implementations. Hashing or comparing a cyclic instance then overflows the stack; suppress Hashable for cycle models or generate cycle-safe identity semantics.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java:782">
P2: The three new methods (markModelClassRendering, collectInlineModelRefs, isOnInlineReferenceCycle) are copied verbatim into both Swift5ClientCodegen and Swift6ClientCodegen (~70 lines each). Since both generators extend DefaultCodegen with no shared Swift parent, any future fix to cycle detection must be replicated twice and will drift. Move the graph construction and cycle detection into a shared helper (e.g. a static util or a common parent) parameterized by the models map and the useClasses/readonlyProperties flags, and have both generators call it.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java:806">
P3: The template now renders based on the vendor extension `x-swift-use-class`, which `markModelClassRendering` only writes (never clears) when `useClasses || recursive`. For a spec that already declares `x-swift-use-class: true` on a schema while `useClasses` is off and the model is not on a cycle, that stored `true` survives and the model silently switches from a struct to a final class — a behavior change from the previous global-flag template that ignored the extension. Use a dedicated internal extension name (or always set the value explicitly, including false) so user-provided spec extensions cannot change rendering unintentionally.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let additionalPropertiesContainer = try decoder.container(keyedBy: String.self) | ||
| additionalProperties = try additionalPropertiesContainer.decodeMap({{{additionalPropertiesType}}}.self, excludedKeys: nonAdditionalPropertyKeys) | ||
| }{{/additionalPropertiesType}}{{/generateModelAdditionalProperties}}{{^objcCompatible}}{{#useClasses}}{{#vendorExtensions.x-swift-hashable}} | ||
| }{{/additionalPropertiesType}}{{/generateModelAdditionalProperties}}{{^objcCompatible}}{{#vendorExtensions.x-swift-use-class}}{{#vendorExtensions.x-swift-hashable}} |
There was a problem hiding this comment.
P1: When the default hashableModels setting is used, a newly class-rendered recursive model still gets recursive == and hash(into:) implementations. Hashing or comparing a cyclic instance then overflows the stack; suppress Hashable for cycle models or generate cycle-safe identity semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/swift5/modelObject.mustache, line 124:
<comment>When the default `hashableModels` setting is used, a newly class-rendered recursive model still gets recursive `==` and `hash(into:)` implementations. Hashing or comparing a cyclic instance then overflows the stack; suppress Hashable for cycle models or generate cycle-safe identity semantics.</comment>
<file context>
@@ -121,7 +121,7 @@
let additionalPropertiesContainer = try decoder.container(keyedBy: String.self)
additionalProperties = try additionalPropertiesContainer.decodeMap({{{additionalPropertiesType}}}.self, excludedKeys: nonAdditionalPropertyKeys)
- }{{/additionalPropertiesType}}{{/generateModelAdditionalProperties}}{{^objcCompatible}}{{#useClasses}}{{#vendorExtensions.x-swift-hashable}}
+ }{{/additionalPropertiesType}}{{/generateModelAdditionalProperties}}{{^objcCompatible}}{{#vendorExtensions.x-swift-use-class}}{{#vendorExtensions.x-swift-hashable}}
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static func == (lhs: {{classname}}, rhs: {{classname}}) -> Bool {
</file context>
There was a problem hiding this comment.
This one predates the PR rather than being introduced by it: with useClasses=true, every model is already a class today and the generated ==/hash(into:) already recurse over whatever object graph they are given. Wire data cannot express a cycle - JSON is a tree - so a cyclic instance only exists if a caller builds one by hand, and this change does not make that any more reachable than useClasses already does. Switching these models to identity-based equality would be a real behavioural departure from that precedent, so I would rather not fold it in here; happy to file it separately if you think the useClasses case deserves a fix.
| for (CodegenModel cm : modelsByClassname.values()) { | ||
| boolean recursive = !useClasses && isOnInlineReferenceCycle(cm.classname, inlineRefs); | ||
| if (useClasses || recursive) { | ||
| cm.vendorExtensions.put("x-swift-use-class", true); |
There was a problem hiding this comment.
P1: When a cycle is expressed through oneOf, this flag does not break recursion because the one-of template still emits a non-indirect enum. Handle these cycles with an indirect representation (or a separate cycle strategy) instead of marking them as fixed by the class extension.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift6ClientCodegen.java, line 854:
<comment>When a cycle is expressed through `oneOf`, this flag does not break recursion because the one-of template still emits a non-`indirect` enum. Handle these cycles with an indirect representation (or a separate cycle strategy) instead of marking them as fixed by the class extension.</comment>
<file context>
@@ -814,6 +815,79 @@ public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs)
+ for (CodegenModel cm : modelsByClassname.values()) {
+ boolean recursive = !useClasses && isOnInlineReferenceCycle(cm.classname, inlineRefs);
+ if (useClasses || recursive) {
+ cm.vendorExtensions.put("x-swift-use-class", true);
+ }
+ if ((useClasses && readonlyProperties) || recursive) {
</file context>
There was a problem hiding this comment.
I do not think this holds. A cycle running through a oneOf enum (struct A -> enum E -> A) is broken by A becoming a class: E's associated value is then a reference, so E has finite size and so does A. That is exactly why the composedSchemas oneOf/anyOf references are collected as edges - they make A get marked. An enum-only cycle with no object model in between has no struct to convert, but it also cannot be expressed: something has to carry the reference inline. If you have a spec shape where a oneOf cycle survives this, I will happily take it as a fixture - I could not construct one.
| * | ||
| * @param objs the models | ||
| */ | ||
| private void markModelClassRendering(Map<String, ModelsMap> objs) { |
There was a problem hiding this comment.
P2: The three new methods (markModelClassRendering, collectInlineModelRefs, isOnInlineReferenceCycle) are copied verbatim into both Swift5ClientCodegen and Swift6ClientCodegen (~70 lines each). Since both generators extend DefaultCodegen with no shared Swift parent, any future fix to cycle detection must be replicated twice and will drift. Move the graph construction and cycle detection into a shared helper (e.g. a static util or a common parent) parameterized by the models map and the useClasses/readonlyProperties flags, and have both generators call it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java, line 782:
<comment>The three new methods (markModelClassRendering, collectInlineModelRefs, isOnInlineReferenceCycle) are copied verbatim into both Swift5ClientCodegen and Swift6ClientCodegen (~70 lines each). Since both generators extend DefaultCodegen with no shared Swift parent, any future fix to cycle detection must be replicated twice and will drift. Move the graph construction and cycle detection into a shared helper (e.g. a static util or a common parent) parameterized by the models map and the useClasses/readonlyProperties flags, and have both generators call it.</comment>
<file context>
@@ -766,6 +767,73 @@ public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs)
+ *
+ * @param objs the models
+ */
+ private void markModelClassRendering(Map<String, ModelsMap> objs) {
+ Map<String, CodegenModel> modelsByClassname = new HashMap<>();
+ for (ModelsMap modelsMap : objs.values()) {
</file context>
There was a problem hiding this comment.
Agreed that it is not pretty. It follows the existing relationship between the two generators, which share no Swift base class and already duplicate their reservedWords lists, option handling and postProcess logic - the swift6 codegen is a full copy of swift5 with its own divergences. Extracting a shared helper would be the first piece of common ground between them, which felt like a bigger call than this PR should make on its own. If maintainers want that refactor here, I am glad to do it.
| for (CodegenModel cm : modelsByClassname.values()) { | ||
| boolean recursive = !useClasses && isOnInlineReferenceCycle(cm.classname, inlineRefs); | ||
| if (useClasses || recursive) { | ||
| cm.vendorExtensions.put("x-swift-use-class", true); |
There was a problem hiding this comment.
P3: The template now renders based on the vendor extension x-swift-use-class, which markModelClassRendering only writes (never clears) when useClasses || recursive. For a spec that already declares x-swift-use-class: true on a schema while useClasses is off and the model is not on a cycle, that stored true survives and the model silently switches from a struct to a final class — a behavior change from the previous global-flag template that ignored the extension. Use a dedicated internal extension name (or always set the value explicitly, including false) so user-provided spec extensions cannot change rendering unintentionally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java, line 806:
<comment>The template now renders based on the vendor extension `x-swift-use-class`, which `markModelClassRendering` only writes (never clears) when `useClasses || recursive`. For a spec that already declares `x-swift-use-class: true` on a schema while `useClasses` is off and the model is not on a cycle, that stored `true` survives and the model silently switches from a struct to a final class — a behavior change from the previous global-flag template that ignored the extension. Use a dedicated internal extension name (or always set the value explicitly, including false) so user-provided spec extensions cannot change rendering unintentionally.</comment>
<file context>
@@ -766,6 +767,73 @@ public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs)
+ for (CodegenModel cm : modelsByClassname.values()) {
+ boolean recursive = !useClasses && isOnInlineReferenceCycle(cm.classname, inlineRefs);
+ if (useClasses || recursive) {
+ cm.vendorExtensions.put("x-swift-use-class", true);
+ }
+ }
</file context>
There was a problem hiding this comment.
Deliberate, and additive-only. x-swift-use-class: true on a schema is a per-model escape hatch that the global useClasses cannot express - opting a single model into class rendering - and since the extension is only ever written, never cleared, no spec that worked before this PR changes behaviour. If you would rather the extension were internal, I can namespace it (x-swift-use-class-internal) or clear it before writing; say the word.
Review pointed out that allOf parents are flattened into allVars rather than stored inline, so treating the composed reference as an edge could mark models that store nothing recursively. Real cycles introduced by the flattening still surface through the allVars properties themselves; oneOf/anyOf keep their edges, since those render as enums with inline associated values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
|
cubic's allOf remark was right and is fixed: allOf parents are flattened into On the other remarks:
|
A self- or mutually-referencing schema generates a struct that stores itself inline —
through any chain of model-typed properties,
Optionalincluded — which has infinite sizeand does not compile:
Every struct that embeds the recursive one is infinite in turn (56 such errors against the
production registry API spec this was found on). The generators emit structs
unconditionally, so the only escape has been
useClasses=true, which turns every modelinto a class. This is #15240, open since swift5.
The fix
postProcessAllModelsbuilds the inline-reference graph — a property whose type is a baremodel reference is an edge; containers store their elements on the heap and break the
recursion, so arrays and dictionaries are not — and every model on a cycle is rendered as
a
final classinstead of a struct. Heap allocation provides the indirection the structcannot have; the wire format (Codable) is unchanged; every acyclic model stays a struct.
Applies to swift5 and swift6 both.
Two details:
modelObject.mustachemoves from the globaluseClassesflag to a per-model
x-swift-use-classvendor extension that carries the global value —so
useClasses=truebehavior is untouched, and the fouruseClassessample configs(urlsession + vapor, swift5 and swift6) regenerate byte-identical.
Sendable, so a struct embedding a recursion-breakingclass needs that class to conform: it is declared
@unchecked Sendable, the wayuseClasses+readonlyPropertiesclasses already are.Tests
Swift5ClientCodegenTest/Swift6ClientCodegenTest#testRecursiveModelsBecomeClassesgenerate the new
3_0/swift/recursive-models.yamlfixture (a self-reference, a mutualcycle, a struct embedding the cyclic model, and an array-indirect self-reference) and
assert: the three cyclic models are
final class(swift6:@unchecked Sendable), theother two stay structs (swift6:
Sendable). Fails without the change.Verified end to end: clients generated from the fixture fail on master with exactly the
errors above and
swift buildclean with this change, swift5 and swift6 both.PR checklist
(
./bin/generate-samples.sh ./bin/configs/swift5-*.yaml ./bin/configs/swift6-*.yaml):zero diffs — no sample spec has an inline reference cycle, and the
useClassessamples are unchanged.
Generated with Claude Code
Summary by cubic
Fixes Swift codegen emitting structs of infinite size for self- or mutually-referencing schemas, which don't compile (#15240). Models on a reference cycle now render as
final classinstead; acyclic models stay structs and the wire format is unchanged.Details
useClassesvalue, so existinguseClassessamples regenerate byte-identical.@unchecked Sendable, so theSendablestructs embedding them still conform.Written for commit bb5244e. Summary will update on new commits.