Skip to content

feat: inheritance mode, body-field descriptions, keep soft keywords - #5

Merged
goduni merged 8 commits into
goduni:mainfrom
K1rL3s:feat/inheritance-and-body-descriptions
Aug 7, 2026
Merged

feat: inheritance mode, body-field descriptions, keep soft keywords#5
goduni merged 8 commits into
goduni:mainfrom
K1rL3s:feat/inheritance-and-body-descriptions

Conversation

@K1rL3s

@K1rL3s K1rL3s commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Three additions, driven by generating a hand-written client's shape:

  • --inheritance renders allOf: [{$ref: Base}, ...] as a real base class instead of merging the parent's fields into every subtype. A discriminated base stays a model (its own properties survive) and its mapped subtypes inherit from it, re-declaring only the discriminator tag. Declarations are emitted parent-first so a class Sub(Base) statement resolves, and model constructors become keyword-only because a subclass may pin an inherited field to a default while adding required fields of its own.

  • IRBodyField.description carries the schema description of a spread request body field, which was silently dropped. IRParameter already had it.

  • sanitize_identifier no longer suffixes soft keywords: type is a legal attribute name and an extremely common spec field, so type_ was noise. _ stays reserved.

Verified on a real 130-schema spec: both file layouts x all three serializers generate code that passes ruff --isolated and mypy --strict.

Description

Please include a summary of the change and specify which issue is being addressed. Additionally, provide relevant motivation and context.

Fixes # (issue number)

Type of change

Please delete options that are not relevant.

  • Documentation (typos, code examples, or any documentation updates)
  • Bug fix (a non-breaking change that resolves an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a fix or feature that would disrupt existing functionality)
  • This change requires a documentation update

Checklist

  • My code adheres to the style guidelines of this project (uv run ruff check and uv run ruff format --check show no errors)
  • I have conducted a self-review of my own code
  • I have made the necessary changes to the documentation
  • My changes do not generate any new warnings
  • I have added tests to validate the effectiveness of my fix or the functionality of my new feature
  • I have ensured that type checking passes by running uv run mypy
  • I have included code examples to illustrate the modifications

K1rL3s added 2 commits July 23, 2026 17:37
Three additions, driven by generating a hand-written client's shape:

- `--inheritance` renders `allOf: [{$ref: Base}, ...]` as a real base class
  instead of merging the parent's fields into every subtype. A discriminated
  base stays a model (its own properties survive) and its mapped subtypes
  inherit from it, re-declaring only the discriminator tag. Declarations are
  emitted parent-first so a `class Sub(Base)` statement resolves, and model
  constructors become keyword-only because a subclass may pin an inherited
  field to a default while adding required fields of its own.

- `IRBodyField.description` carries the schema description of a spread request
  body field, which was silently dropped. `IRParameter` already had it.

- `sanitize_identifier` no longer suffixes soft keywords: `type` is a legal
  attribute name and an extremely common spec field, so `type_` was noise.
  `_` stays reserved.

Verified on a real 130-schema spec: both file layouts x all three serializers
generate code that passes `ruff --isolated` and `mypy --strict`.
Review of 566dc7c found the inheritance path silently changing decoded data
in several shapes. Fixes, most severe first:

- A `oneOf` + `discriminator` union holder is no longer force-built as a class.
  It declares no properties, so `--inheritance` produced `class Button: pass`
  and every `list[Button]` payload decoded into it, dropping each variant's
  fields. It stays a union alias; only bases with their own properties become
  classes. Verified: pydantic again decodes `CallbackButton(payload=...)` and
  `isinstance(x, ButtonBase)` still holds.

- The pinned discriminator tag is reserved against the subtype's existing field
  names. A sibling property whose wire name only differs in case (`Type` vs
  `type`) snake-cases to the same identifier, and the two class attributes
  collapsed into one -- destroying the tag, with ruff reporting nothing.

- A subtype that restates an inherited property only to attach prose, or to
  relax it to nullable, now inherits it instead of emitting an override.
  `v: str | None` over the base's `v: str` is an `[assignment]` error under
  `mypy --strict`, so `--inheritance --check` failed on ordinary specs. Genuine
  narrowings (a `Literal` tag over a `str`) are kept.

- The base class is resolved from the schema, not from the half-built
  `_declarations` registry. A base whose own body refers back to its subtype
  (a recursive hierarchy) has no entry yet, so inheritance silently degraded to
  a field merge based on nothing but graph traversal order.

- Keyword-only constructors are limited to the models in a hierarchy. One
  `allOf` subtype used to flip every model in the package, breaking positional
  construction for models with no relation to it.

- A discriminated base kept as a class emits its mapping as a comment. No
  serializer resolves a subtype from a base-class annotation on its own, and
  `IRModel.discriminator` was read by nobody, so that was dropped on the floor.

- `IRBodyField.description` (and `IRParameter.description`) now reach the
  generated code as PEP 258 attribute docstrings. Both were carried through the
  IR and rendered nowhere.

Also: `IRModel.base` -> `base_model`, so it can't be confused with
`IREnum.base` (the enum's "str"/"int" value type); soft keywords use an
allow-list over `keyword.issoftkeyword` so a future Python's new soft keyword
stays guarded; `_ordered_declarations` breaks an inheritance cycle instead of
emitting a class before its base; the two discriminator-mapping loops share one
helper; `_inherited_ref` is computed once and passed down.

Verified: both file layouts x all three serializers generate code that passes
`ruff check --isolated` and `mypy --strict` (only the pre-existing
`BaseMethod.__init_subclass__` no-untyped-call remains).
@K1rL3s
K1rL3s force-pushed the feat/inheritance-and-body-descriptions branch from 32d314d to 53f0765 Compare July 23, 2026 14:39
@K1rL3s

K1rL3s commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Review of 566dc7c → fixes in 53f0765

Went through the diff for recall. Found 12 issues, all fixed in a follow-up commit. What was breaking, and what it does now.

Critical: silently corrupted decoded data

1. oneOf + discriminator was turned into an empty class

The most common polymorphism shape in OpenAPI is a union holder with no properties of its own:

Button:
  oneOf: [CallbackButton, LinkButton]
  discriminator: {propertyName: type, mapping: {...}}

The _build_named branch went into _build_object unconditionally, so --inheritance produced:

@dataclass(kw_only=True)
class Button:
    pass

@dataclass(kw_only=True)
class Keyboard:
    buttons: list[Button]     # ← everything decoded into this, and everything was lost

There is nothing to inherit from such a schema. Only a base that declares its own properties (_is_object) becomes a class now; the rest stays a union alias. Verified: pydantic again returns CallbackButton(payload='P'), while isinstance(x, ButtonBase) still holds — both goals of the feature coexist.

2. The discriminator tag was overwritten by a sibling field

_apply_discriminator_tag inserted the field bypassing the model's field_names registry. If a subtype declared Type while propertyName was type, both snake-cased to the same identifier:

class A(B):
    type: Literal['a'] = 'a'
    type: int | None = None      # ← the second one wins

ruff --isolated lets this through (F811 does not fire on annotated class attributes) and dataclasses.fields(A) shows only int | None. The tag was destroyed, and name_mapping mapped type -> 'Type' — the wrong wire name. The name is now reserved against the fields already on the model.

3. Re-declaring an inherited field broke mypy --strict

Specs routinely restate a base property to attach prose or to allow null. That produced an unsound override:

error: Incompatible types in assignment (expression has type "str | None",
base class "P" defined the type as "str")  [assignment]

So --inheritance --check failed on ordinary specs. Pure restatements and widenings are now inherited; genuine narrowings (a Literal over a str) are kept. _is_narrowing is deliberately conservative: a false yes emits code that does not type-check, a false no merely inherits a slightly less precise type.

4. Inheritance depended on graph traversal order

The base was looked up in self._declarations, which has no entry yet while the base itself is being built. If the base's own body refers back to its subtype (a recursive hierarchy), the entry was missing:

before: LeafNode base=None  fields=['id', 'child', 'value']   ← merged, --inheritance did nothing
after:  LeafNode base=Node  fields=['value']

The decision is now made from the schema (_declares_model), not from a half-built registry.

The rest

5. kw_only is now per-model. A single allOf subtype used to flip every model in the package to keyword-only, breaking positional construction for models unrelated to any hierarchy. Only hierarchy members are marked now; Keyboard from the example stays a plain @dataclass.

6. IRModel.discriminator was read by nobody. For a base kept as a class, no serializer resolves the concrete subtype from a base-class annotation on its own. The mapping is now emitted as a comment — exactly what you need to wire tagged decoding in _serialization.py. The limitation is documented in the README.

7. IRBodyField.description was not rendered. The commit message says it "was silently dropped" — but the drop point had only moved from the builder to the renderer: no template reads field descriptions. Both IRBodyField.description and IRParameter.description now reach the generated code as PEP 258 attribute docstrings, without touching the constructor signature:

title: Body[str]
"""Doc title."""
type: Query[Omittable[str]] = Omitted()
"""Kind of send."""

8. IRModel.basebase_model. IREnum.base is the enum's value type ("str"/"int"). Both new call sites already guarded with isinstance(...), but any future getattr(decl, "base", None) loop would read an enum's "str" as a superclass name.

9. Soft keywords use an allow-list. frozenset({"_"}) in place of keyword.issoftkeyword() would stop catching whatever word the next Python release promotes (that is how type arrived in 3.12). It is now issoftkeyword(c) and c not in {"type", "match", "case"} — the reviewed names are hardcoded, not the forbidden ones.

10-12. _ordered_declarations breaks an inheritance cycle explicitly (marking visited before recursing prevented infinite recursion but not the wrong emit order); the two discriminator.mapping traversals share one _convert_mapped_subtypes helper; _inherited_ref is computed once and passed into _flatten_object instead of a keep_base flag, so the caller's decision and the merge cannot disagree.

Verification

  • 208 tests green (+6 regression tests, one per bug 1-4, +1 for per-model kw_only, +1 for mapping visibility, +1 for attribute docstrings).
  • ruff format --check, ruff check, mypy --strict clean across the repo.
  • End-to-end generation: both file layouts × all three serializers → the output passes ruff check --isolated and mypy --strict. The only remaining error is BaseMethod.__init_subclass__ [no-untyped-call], which comes from unihttp and predates this PR.

One call worth a second opinion

For the widening case (item 3) I chose to inherit the base's declaration, which means a subtype's nullable: true is dropped. The alternative is to keep the override and accept the mypy failure. I went with code that type-checks, but if annotation fidelity matters more for your specs, that is a one-line flip in _is_narrowing.

used = NameRegistry()
for existing in decl.fields:
used.reserve(existing.name)
decl.fields.insert(

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.

The pinned tag bypasses _is_narrowing, so --inheritance --check fails on the most common discriminator shape.

_drop_unsafe_overrides runs inside _build_object; this insert runs later, from _convert_ref -> _apply_discriminator_tag. Nothing re-checks the field it adds, so Literal[tag] is emitted over whatever the base declared. When the base types the discriminator property as a $ref to an enum (the idiomatic OpenAPI form) that is not a narrowing.

Worse, the two paths fight each other: if the subtype does restate type, _drop_unsafe_overrides correctly drops it, and then this branch puts it right back unchecked.

Reproduced with Button.type: {$ref: ButtonKind} + mapping: {callback: CallbackButton, link: LinkButton}:

@dataclass(kw_only=True)
class Button:
    type: ButtonKind
    text: str

@dataclass(kw_only=True)
class CallbackButton(Button):
    payload: str
    type: Literal["callback"] = "callback"
models.py:26: error: Incompatible types in assignment (expression has type "Literal['callback']",
  base class "Button" defined the type as "ButtonKind")  [assignment]
models.py:32: error: ... "Literal['link']" ... "ButtonKind"  [assignment]

The same happens in a 3-level hierarchy where the middle class already pinned the tag to its own Literal.

The conservative rule the rest of the module follows applies here too: only pin when _is_narrowing(LiteralType((value,)), inherited_type) holds, otherwise inherit the base's declaration.

return self.render_alias(decl)
return self.render_model(decl)
body = self.render_model(decl)
if decl.discriminator is not None:

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.

Regression in the default (non---inheritance) path.

This fires for any IRModel whose discriminator is set, and in merge mode _flatten_object copies the base's discriminator down into every allOf subtype (discriminator = discriminator or d). So concrete subtypes now carry a header announcing them as tagged-union bases, with a partial, self-referential mapping - the mapping is resolved while the subtype is being built, so _ref_to_name only knows the subtypes converted so far.

No flag needed to hit it; --serializer adaptix on a plain Pet/Dog/Cat spec:

# discriminator: petType (dog=Dog)          <- claims Dog is a base that maps "dog" to itself
# subtype resolution is left to the serializer config
@dataclass
class Dog:
    ...

# discriminator: petType (cat=Cat, dog=Dog) <- and Cat is a base of Dog?
@dataclass
class Cat:
    ...

The root cause is in _flatten_object: a discriminator belongs to the schema that declares it - the same reasoning the PR already applies to the inherited member. Dropping the discriminator or d merge entirely fixes this and is safe: IRModel.discriminator has no other reader, and the inheritance base re-resolves its own in _build_named.

parent = self._declarations.get(base)
if not isinstance(parent, IRModel):
return
inherited_types = {f.wire_name: f.type for f in parent.fields}

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.

wire_name is the wrong key: shadowing happens on the python name.

Type/type and packSize/pack_size are distinct wire names that produce one identifier. The lookup misses, the field is kept, and the subclass attribute shadows an inherited one of an unrelated type. _build_object's field_names registry only dedups within the model's own fields, so nothing else catches it either.

Reproduced (base B has type: string + packSize: integer; subtype A adds Type: integer + pack_size: string):

@dataclass(kw_only=True)
class A(B):
    type2: Literal["a"] = "a"
    type: int | None = None        # wire "Type"  -> shadows B.type: str
    pack_size: str | None = None   # wire "pack_size" -> shadows B.pack_size: int
models.py:20: error: Incompatible types in assignment (expression has type "int | None",
  base class "B" defined the type as "str")  [assignment]
models.py:21: error: ... "str | None" ... "int"  [assignment]

Dropping is also the wrong remedy here - the subtype's property is genuinely its own, it just needs a non-colliding identifier. Reserving the subtype's field names against the inherited ones (and letting only a same-wire-name re-declaration keep the name) preserves both fields and both aliases.

already covers the field, so anything that is not a genuine narrowing is
dropped and simply inherited.
"""
parent = self._declarations.get(base)

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.

Only one level of the base chain is checked.

parent.fields holds a subclass's own fields only - by construction, that is the whole point of this mode. So in A <- B <- C, A's fields are invisible when C is checked, and an incompatible re-declaration of an A field goes straight through.

Reproduced with A{v: string}, B: allOf[A] + {b}, C: allOf[B] + {v: integer, c}:

@dataclass(kw_only=True)
class C(B):
    v: int
    c: str | None = None
models.py:20: error: Incompatible types in assignment (expression has type "int",
  base class "A" defined the type as "str")  [assignment]

The inherited set has to be accumulated by walking base_model to the root (nearest declaration winning, since that is what mypy compares against).

dropped and simply inherited.
"""
parent = self._declarations.get(base)
if not isinstance(parent, IRModel):

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.

The half-built-registry problem _resolve_base_model fixes is reintroduced here.

_resolve_base_model goes to some length to avoid reading self._declarations for a base that has no entry yet, and documents exactly why:

a base whose own body refers back to this subtype (a recursive hierarchy) is still mid-build and has no entry yet, which would silently downgrade the subtype [...] based on nothing but graph traversal order

This method then does that read anyway and returns silently. In the Node / LeafNode shape covered by test_inheritance_recursive_base_still_subclasses, LeafNode is built while Node is mid-flight, so parent is None, no override is pruned, and a widening restatement (v: str | None over v: str) reaches the output and fails mypy --strict.

Since the decision needs every base's final field set anyway (see the grandparent case), the natural place for this pass is after the whole graph is built - build() already computes a base-before-subclass ordering, so walking that list makes the inherited set final and both this and the multi-level gap disappear.

@@ -515,34 +620,173 @@ def _flatten_object(
if not isinstance(sub_schema, dict):
continue
p, r, a, d = self._flatten_object(sub_schema, sub_base)

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.

Wasted recursion, and it makes the new cycle guard dead code.

The recursive _flatten_object call happens before the sub is not inherited test, so in inheritance mode the base's entire property set is merged and then thrown away on the next line (only required survives). That is O(depth) redundant work per subtype.

It also has a correctness consequence for the guard added in _ordered_declarations. A base_model cycle can only come from an allOf cycle, and an allOf cycle dies here first:

A: allOf: [{$ref: B}, {properties: {a}}]
B: allOf: [{$ref: A}, {properties: {b}}]

  File "ir/builder.py", line 622, in _flatten_object
    p, r, a, d = self._flatten_object(sub_schema, sub_base)
  [Previous line repeated 980 more times]
RecursionError: maximum recursion depth exceeded

So _ordered_declarations' grey-marking branch, the logger.warning and the assert are unreachable. (The RecursionError itself predates this PR - merge mode crashes identically - but the guard is presented as handling it.)

Worth noting what that branch would do if it were reachable: by the time it runs, _flatten_object has already skipped the base's properties and _drop_unsafe_overrides has already removed the subtype's re-declarations. Clearing base_model at that point emits a class that has lost every inherited field rather than one that fails to import.

assignable, because a false yes emits code that fails ``mypy --strict`` while a
false no merely inherits a slightly less precise type.
"""
if sub.annotation() == base.annotation():

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.

A restatement with the same annotation is dropped wholesale, but annotation is not all a restatement can carry. default and description ride on the same IRField, and specs restate a property to change exactly those:

Base:  {properties: {mode: {type: string, default: "fast"}}}
Sub:
  allOf:
    - $ref: '#/components/schemas/Base'
    - properties: {mode: {type: string, default: "slow", description: "Slower here."}}

Sub silently inherits mode = "fast". The description loss is the more awkward one given this same PR adds descriptions to body fields - the prose the spec author attached to the subtype never reaches the output.

Keeping the field when only default/description differ is sound (identical annotation is always a legal override), so the return False could be conditioned on the restatement adding nothing at all.

lines.append(f"{spec.py_name}: {spec.marker}[Omittable[{spec.inner}]] = Omitted()")
# PEP 258 attribute docstring: the only place a parameter's / body field's
# schema prose can land without changing the constructor signature.
doc = docstring(spec.description, "")

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.

docstring(text, indent) uses indent for two things: the prefix it writes, and the wrap width (88 - len(indent)). Passing "" here and letting render_method_class add the four spaces gets the prefix right and the width wrong.

 16 |    q: Query[str]|
 17 |    r"""First paragraph that is long enough to need wrapping across several lines in the output|  <- 95 cols
 18 |    file.|
 19 |    |  <- trailing whitespace
 20 |    Second paragraph with a backslash: curl \ -H "X: y".|

Model field docstrings pass " " and come out right. E501/W291 are outside ruff's default select so --check stays green, but the 88-column target this helper exists to hit is missed. Rendering with the real indent and stripping it back off before render_method_class re-adds it fixes the width; skipping the prefix on empty lines fixes the trailing whitespace.

if "enum" in schema and "properties" not in schema:
return False
disc = schema.get("discriminator")
if isinstance(disc, dict) and isinstance(disc.get("mapping"), dict):

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.

The docstring is candid that this mirrors _build_named ("kept next to nothing else so the two stay reviewable side by side"), but the mirror is already off by one branch.

_build_named, for a discriminated base that is not _is_object, falls through to _build_discriminated_base, which ends with:

if not members:
    self._declarations[name] = self._build_object(name, schema, base_uri)
    return

So an empty or fully unresolvable mapping yields an IRModel, while this predicate returns False for it - a subtype pointing at that base silently downgrades to a merge.

The drift is minor today, but the shape is the concern: two hand-synchronised copies of a dispatch, in a mode where a wrong answer changes the class hierarchy. Having _build_named record its decision (or splitting the classification out and calling it from both) removes the class of bug rather than this instance.

hierarchy.add(model.base_model)
self._kw_only_models = frozenset(hierarchy)

def is_kw_only(self, model: IRModel) -> bool:

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.

_kw_only_models starts empty, so is_kw_only answers False for every model until bind_document has run. Both production paths happen to bind (render_models_module does it internally, _write_per_object_layout does it explicitly), but render_declaration / render_model are public and neither documents the requirement.

Get it wrong and the failure is not a missing keyword - it is @dataclass on a subclass whose base ends in a defaulted field while the subclass declares a required one:

TypeError: non-default argument 'payload' follows default argument

at import time of the generated package.

The property is a fact about the IR ("this model is in a hierarchy"), not about a rendering session; computing it from model.base_model plus the document once in the builder, or storing it on IRModel, would make the ordering unrepresentable.


def render_model(self, model: IRModel) -> str:
lines = [f"class {model.name}(BaseModel):"]
lines = [f"class {model.name}({model.base_model or 'BaseModel'}):"]

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.

Pydantic merges model_config down the MRO, so on a subclass this line is a no-op:

class CallbackButton(Button):
    model_config = ConfigDict(populate_by_name=True)   # already true via Button
    type: Literal["callback"] = "callback"

It also reads as if the subclass were deliberately overriding the parent's config. Guarding on model.base_model is None keeps the emitted hierarchy honest.


@staticmethod
def _discriminator_comment(disc: Discriminator) -> str:
mapping = ", ".join(f"{value}={name}" for value, name in sorted(disc.mapping.items()))

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.

_discriminator_comment emits

# discriminator: type (callback=CallbackButton, link=LinkButton)
# subtype resolution is left to the serializer config

and render_alias, just below, emits

# discriminator: type (tagged-union wiring is left to the serializer config)

Same information, same audience, two wordings and two layouts - and the alias form drops the value->class mapping even though IRAlias.discriminator.mapping carries it, which is the part a reader actually needs to wire tagged decoding. Routing render_alias through the new helper gives both forms the mapping and leaves one string to maintain.

declared = self._declarations.get(base_type.name)
if declared is not None:
return base_type.name if isinstance(declared, IRModel) else None
resolved = self._resolver.resolve_ref(inherited["$ref"], base_uri)

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.

_convert_ref on the line above already resolved this $ref (it needs the pointer to key _ref_to_name); this resolves it again for the same pointer. RefResolver.resolve_ref has no memoisation and re-splits/re-walks the document each call, and this now runs for every allOf subtype in the spec.

Only the key and resolved.value are needed here, both of which _convert_ref had in hand - returning them (or looking the pointer up from _ref_to_name) drops one full resolution per subtype.

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.

Correction to the scope claim above: the extra resolve_ref is guarded by if declared is not None: return ..., so it only runs when the base has no _declarations entry yet - the recursive-hierarchy case, not every allOf subtype. The duplicate resolution is real but rare, so I left it alone.

Follow-up review of the inheritance mode found four shapes where the generated
package failed `mypy --strict`, so `--inheritance --check` broke on ordinary
specs, plus one regression in the default (merge) path.

Override pruning moves out of `_build_object` into `_reconcile_inheritance`, a
whole-graph pass over the already base-before-subclass ordering. What a subtype
may keep depends on the final field set of every class above it, which is not
knowable while the subtype is being converted. That single move fixes:

- The pinned discriminator tag is now checked like any other override. It is
  added after the model is built, so nothing used to look at it, and a base that
  types the discriminator property as a `$ref` to an enum -- the idiomatic
  OpenAPI form -- got `type: Literal['callback']` over `type: ButtonKind`. The
  two paths also fought: a subtype restating the tag had it dropped as unsound
  and then put straight back unchecked.

- The full base chain is consulted, not just the direct parent. A subclass
  carries only its own fields, so in `A <- B <- C` a re-declaration of an `A`
  field was invisible from `B`.

- A base that is still mid-build no longer silently skips the check. That is the
  recursive-hierarchy case `_resolve_base_model` was rewritten for; reading the
  half-built registry here reintroduced the same order dependence.

Two more override bugs, fixed alongside:

- Inherited fields were keyed by wire name, but shadowing happens on the python
  name: `packSize` on the base and `pack_size` on the subtype are different
  properties that collapse onto one attribute. Neither may be dropped, so the
  subtype's is renamed and aliased back instead of shadowing the inherited one.

- A field re-declaring an inherited wire name now lands on the inherited
  attribute. Otherwise the class has two attributes for one wire key, which
  adaptix rejects outright ("fields point to the same path").

- A restatement that changes only the `default` is kept. An identical annotation
  is always a legal override, and dropping it handed the subtype the base's
  value.

Default-mode regression: `_flatten_object` copied a base's discriminator down
through `allOf`, and the new renderer turns any model carrying one into a
`# discriminator:` header. Every concrete subtype was announced as a
tagged-union base, with a mapping resolved only as far as the graph walk had
got (`# discriminator: petType (dog=Dog)` above `class Dog`). A discriminator
describes the schema that declares it, so it is no longer merged in at all --
`IRModel.discriminator` has no other reader, and an inheritance base re-resolves
its own in `_build_named`.

Also: parameter/body-field docstrings are rendered at the indent they actually
sit at, so wrapping targets 88 columns instead of 92 and blank paragraph
separators no longer carry trailing whitespace; `render_alias` reuses
`_discriminator_comment`, which also gives the alias form the value -> class
mapping it was dropping; pydantic subclasses no longer repeat the inherited
`model_config`.

Verified: 7 specs x 3 serializers x 2 layouts generate packages that pass
`mypy --strict` (only the pre-existing `BaseMethod.__init_subclass__`
no-untyped-call remains), adaptix builds a dumper for every generated model, and
all three serializers round-trip the collision cases.
@K1rL3s

K1rL3s commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 2847eee addressing the review above.

Fixed

# Finding Fix
1 Pinned tag bypassed _is_narrowing (enum-typed discriminator property) override pruning moved into _reconcile_inheritance, a whole-graph pass, so the tag is checked like any other override
2 # discriminator: header on concrete subtypes in default mode _flatten_object no longer merges a discriminator in from allOf
3 Inherited fields keyed by wire_name, not the python name _rename_shadowed_fields: the subtype's field is renamed and aliased instead of shadowing
4 Only the direct parent was consulted _inherited_fields walks the chain to the root, nearest declaration winning
5 Check no-opped on a mid-build base same whole-graph pass - every ancestor is final by then
7 Restatement dropped its own default kept when the annotation is identical and the default differs
8 Attribute docstrings wrapped for the wrong column, trailing whitespace rendered at the real indent, blank separators left unindented
11 model_config repeated on pydantic subclasses emitted only on a root model (with a pass fallback for an empty subclass body)
12 Two formats for the discriminator note render_alias reuses _discriminator_comment, which also gives the alias form the value → class mapping it was dropping

One extra bug surfaced while fixing #3: renaming the sibling freed the name but left the tag on type2, so A had two attributes for wire key type and adaptix refused to build the retort ("fields point to the same path"). _align_override_names now puts a re-declared wire name back on the inherited attribute.

Skipped

  • fix: resolve ruff and mypy from the generator's own environment #6 (redundant _flatten_object recursion / dead cycle guard) — the recursion cannot be dropped without regressing the required merge: a narrowing restatement that relies on the base's required would flip to optional and then be pruned as a widening. The underlying RecursionError predates this PR (merge mode dies identically), so the guard stays as-is rather than being reworked here.
  • #9 (_declares_model mirrors _build_named) — the real fix is having _build_named record its decision, which is a wider restructuring than this PR's surface.
  • #10 (is_kw_only depends on bind_document) — moving the flag onto IRModel touches the IR contract and every strategy; both production paths do bind today.
  • #13 — overstated, see the correction in-thread.

Verification: 7 specs × 3 serializers × 2 layouts. Every package passes mypy --strict (only the pre-existing BaseMethod.__init_subclass__ no-untyped-call remains), adaptix builds a dumper for every generated model, and all three serializers round-trip the collision cases. 213 unit tests pass, including 5 new regression tests.

…ursing

Review of 566dc7c..2847eee found the mode itself sound -- seven adversarial specs
x 3 serializers x 2 layouts generate packages that pass `ruff` and `mypy --strict`
and round-trip every wire key -- but two gaps worth closing before merge.

`--inheritance` had no compile gate. Both follow-up commits fixed generated code
that failed `mypy --strict`, which the unit tests cannot see: they assert on the IR,
and a subclass declaration is exactly where a slightly-wrong IR turns into an
`[assignment]` error. The gate now runs the hierarchy spec through all three
serializers x both layouts. Per-object is not redundant: the base class is the one
model reference that has to be imported at runtime rather than deferred into the
`TYPE_CHECKING` block.

Alongside it, a round-trip test in `test_behavior.py` for what ruff and mypy cannot
catch: two attributes collapsing onto one identifier silently drops a value, and two
attributes left pointing at one wire key makes adaptix refuse to build the retort.
Both were real bugs in 53f0765/2847eee. The shared `hierarchy_spec` fixture carries
one schema per rule -- enum-typed tag, prose-only restatement, nullable relaxation,
default-only override, `packSize`/`pack_size` and `type`/`Type` collisions, a
three-level chain, and a base whose body refers back to its subtype.

The cycle fix: `_ordered_declarations` breaks an inheritance cycle by dropping the
base edge, but nothing could ever reach it. An inheritance cycle needs an `allOf`
cycle, and `_flatten_object` followed one until the interpreter's stack gave out --
a pre-existing crash in both modes, now a skipped member and a warning. Output for
every acyclic spec is byte-identical.
@K1rL3s

K1rL3s commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Review of 566dc7c..2847eee — ready to merge, with one commit added

Verdict: the inheritance mode is correct. I could not break it. What I did add is the safety net the two follow-up commits argue for.

What I verified independently

Built two adversarial specs (beyond the ones in the tests) and generated 3 serializers x 2 layouts from each:

  • discriminated base with its own properties, plus a bare oneOf holder (stays a union alias);
  • a subtype that is itself a discriminated base with properties (three levels of polymorphism);
  • tag typed as $ref to an enum vs. as a plain string;
  • restatements that only add prose / relax to nullable / change the default;
  • packSize + pack_size on one class, in both declaration orders, and type + Type;
  • a three-level chain re-declaring the grandparent's fields;
  • a base whose own body refers back to its subtype;
  • allOf with two refs (mixin) and with a ref to an enum — both keep the merge;
  • --optional omitted on top of --inheritance.

Every package: ruff check clean, mypy --strict clean, generation deterministic. Runtime round-trip through pydantic / msgspec / adaptix keeps all seven colliding wire keys distinct, isinstance holds across the hierarchy, the default-only override wins over the base's value, and the recursive hierarchy constructs. _align_override_names running after _rename_shadowed_fields is load-bearing and provably collision-free — the only field that can hold an inherited python name after the rename pass is the override itself.

Gaps closed (712779d)

1. --inheritance had no compile gate. Both follow-up commits fixed generated code that failed mypy --strict; the unit tests assert on the IR, and a subclass declaration is exactly where a slightly-wrong IR becomes an [assignment] error. Added to test_compile_gate.py: the hierarchy spec through all three serializers x both layouts. Per-object is not redundant — the base class is the one model reference that must be imported at runtime rather than deferred into TYPE_CHECKING.

2. No round-trip test. ruff and mypy are both blind to the failure that 53f0765 fixed: two attributes collapsing onto one identifier silently drops a value, and two attributes on one wire key makes adaptix refuse to build the retort. Added to test_behavior.py, parametrized over all three serializers. Shared hierarchy_spec fixture in conftest.py, one schema per rule.

3. _ordered_declarations' cycle break was unreachable. An inheritance cycle requires an allOf cycle, and _flatten_object followed one until the stack gave out — pre-existing on main, in both modes. _flatten_object now tracks the schemas open on the current path and skips a repeated member with a warning, so the cycle break actually runs and emits resolvable classes. Output for every acyclic spec is byte-identical; verified by diffing the full generated tree before and after.

Known limitations, deliberate and documented

  • A discriminated base that stays a class decodes into the base, not the subtype — the # discriminator: comment carries the mapping. Documented in the README, correct call.
  • When the base types the tag as an enum, the subtype's Literal tag is dropped rather than pinned. Type-safe, and the base still requires the field. Also documented.
  • An allOf cycle still crashes before the builder ever runs, inside openapi-spec-validator — third-party, out of scope here.

223 passed, ruff + mypy --strict clean on the repo itself.

@K1rL3s
K1rL3s marked this pull request as ready for review August 2, 2026 17:09
goduni added 3 commits August 7, 2026 14:40
Review of the branch found six shapes where `--inheritance` decoded less than the
default merge mode, or emitted a package that fails `mypy --strict`. Most severe
first:

- `properties: {}` no longer counts as "declares its own structure". `_is_object`
  tests key presence, which is right for "model vs dict[str, Any]" but not for
  "may this be a base class": a bare discriminator holder written with an empty
  properties map -- common generator output -- became `class Shape: pass`, and
  every payload annotated with it decoded into that, losing the variant's fields
  entirely. Split off `_has_own_structure` for the base-class question.

- `_is_narrowing` checks the literal values' own python types. Knowing the base is
  *some* scalar says nothing about which one, so `Literal['one', 'two']` was kept
  over an `int` base and `--inheritance --check` failed on an ordinary spec with
  an `[assignment]` error.

- A subtype that tightens an inherited property by naming it in `required`, and
  restates nothing, now re-declares it. That is the ordinary way a spec narrows a
  base; the property stayed on the base and nothing carried the tightening down,
  so a spec-required field silently kept `T | None = None`. The re-declaration
  keeps the base's annotation: the IR cannot tell "optional" from "nullable", and
  narrowing to `T` would reject a null the spec may allow.

- A restatement that only tightens `constraints` is kept. Comparing annotations
  alone dropped it, so `maxLength`/`pattern`/`minimum` stopped being enforced and
  the client accepted payloads the API rejects.

- Breaking an `allOf` cycle merges the dropped base back in. Clearing `base_model`
  alone left the orphaned end with only its own properties, so it decoded strictly
  less than merge mode, which flattens the same cycle and keeps everything.

- The discriminator tag is pinned over an enum-typed property instead of being
  dropped. `Literal['callback']` is not assignable to `ButtonKind`, so the tag
  used to disappear -- leaving the subtype constructible with any sibling's tag
  and encoded without one. It now pins the matching member (`type: ButtonKind =
  ButtonKind.CALLBACK`) via `IRField.default_expr`, which keeps the base's
  annotation and so survives as a legal override. `IRModel.runtime_refs` reports
  the enum alongside the base class, because both are evaluated at class-definition
  time and a split layout must import them for real.

Also: the `# discriminator:` header is gated on there being a mapping or a base
class. A plain object that merely declares `discriminator: {propertyName: ...}`
was getting announced as a tagged-union base in the *default* mode, changing the
output of users who never asked for `--inheritance`. Dropping an override logs at
warning when it loses information and at debug when the restatement was verbatim.
Docstring wrapping counts the opening quotes, which share the first line, so it no
longer overshoots the budget by three columns.

Verified: 6 specs x 3 serializers x 2 layouts x 2 modes generate packages that pass
`ruff check --isolated` and `mypy --strict`, and the pinned enum default imports
cleanly in the per-object layout, where mypy cannot see a deferred name.
Three findings from reviewing the previous commit; none change generated output.

- `_own_required` is keyed by class name, not `id(model)`. Identity keys are only
  sound while every model stays reachable for the whole build, which happens to
  hold (each `_build_object` result is stored under a unique name straight away)
  but is an invariant nothing states and a refactor would silently break, handing
  a rebuilt model another model's `required` set.

- Fields copied out of a base no longer alias its `constraints` dict. `replace`
  copies shallowly, so the base and the subtype shared one mutable mapping;
  nothing mutates it today, which is exactly what makes the aliasing easy to
  miss later.

- Only names that actually received a runtime import are taken out of the
  `TYPE_CHECKING` block. `runtime_refs` is intersected with the layout plan up
  front, so a name outside it can no longer be dropped from the deferred block
  and left referenced but never imported at all.

Also verified alongside: `--optional omitted` narrows correctly through the new
required-tightening path (`v: str` over the base's `Omittable[str]`), which the
generation sweep had not been covering.
Seven of eleven findings held up; the rest are recorded below with why not.

- adaptix: a field re-declared without a default now emits `dataclasses.field()`.
  `dataclasses` resolves a default with `getattr(cls, name)`, which walks the MRO
  and finds the base's class attribute, so `class C(P): v: str | None` over
  `P.v = None` silently inherited the default -- `C(c='x')` constructed as
  `v=None`. That made the whole required-tightening path a no-op for one of the
  three serializers, and defeated every defaultless narrowing override besides.
  pydantic and msgspec build their fields from `__annotations__` and were never
  affected, which is why it takes a construction test rather than an IR one.

- The `# discriminator:` note is gated on the mapping alone. `base_model` is no
  proxy for "is a union base" -- a subclass is the opposite -- so a leaf subtype
  restating `discriminator: {propertyName: ...}` got announced as a tagged-union
  base with no mapping to show. `render_alias` gets the same gate: it was still
  emitting a contentless `# discriminator: petType` above a union alias.

- Dropping an override logs at warning only when the annotation genuinely changes.
  Relaxing an inherited field to nullable is something the README documents as
  routine and inherited, so warning on it was one line of unactionable noise per
  such property.

- `_merge_dropped_base` carries `additionalProperties` across as well, and replays
  the orphan's own `required` tightenings -- with its base edge gone,
  `_reconcile_inheritance` skips it, so nothing else would.

- `_literals_fit` accepts `Literal[True]` over an `int` base: `bool` is a subtype
  of `int`. The `not isinstance(v, bool)` guard is right for rendering a default,
  not for assignability.

- `_has_own_structure` composes with `_is_object` instead of restating its
  singleton-`allOf` guard, so the two cannot drift; `_own_required` is only
  populated in inheritance mode, the sole reader; and msgspec's unused
  `_default_repr` is gone rather than left with a signature that no longer
  matches adaptix's.

Not taken:

- Pinning the tag as `Literal[Kind.A]` instead of `Kind`. Tighter, and it would
  keep pydantic's tagged-union wiring, but msgspec refuses to build a struct from
  `Literal[<StrEnum member>]` and the IR is shared by all three serializers.
  Verified directly against msgspec before reverting.

- Synthesizing a tag for a subtype whose holder declares no properties. The
  default merge mode has always rendered that case without a tag, so this is not
  a regression, and inventing a field no schema declares would change the output
  of every existing merge-mode user.

Verified: 241 tests, and 72 generated packages (6 specs x 3 serializers x 2
layouts x 2 modes) pass `ruff check --isolated` and `mypy --strict`.
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (83590bd) to head (9ca381d).

Additional details and impacted files
@@            Coverage Diff             @@
##              main        #5    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           29        29            
  Lines         2256      2557   +301     
==========================================
+ Hits          2256      2557   +301     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

`main` is at 100%, so the branch had been leaving 20 statements uncovered.
Closing them turned up two pieces of code that no spec can reach.

- `_resolve_base_model`'s `if not isinstance(base_type, RefType): return None` was
  unreachable: `_convert_ref` names every target it resolves and always returns a
  `RefType`. Replaced with an assert, which states the invariant instead of
  pretending there is a case to degrade on.

- `_merge_dropped_base` no longer replays the model's own `required`. The
  previous commit added it on the reasoning that `_reconcile_inheritance` skips a
  cycle-broken model, but `_flatten_object` unions `required` across every `allOf`
  member and a cycle makes that flow both ways, so both ends already built the
  shared property as required. The test written for it passed for that reason
  rather than the intended one. `additionalProperties` genuinely does need
  carrying over -- an inherited member is the one thing `_flatten_object` skips --
  and keeps its test.

New tests cover the rest: `_is_narrowing` across the type shapes that reach each
branch (including the deliberate false no for a `Literal` over a union base),
`_literals_fit` per primitive, `_declares_model` against every arm of the
`_build_named` dispatch it mirrors, `_inherited_fields` stopping at a non-model
base, `inherited_field_names` stopping at an undeclared one, the three
non-enum shapes `_retype_discriminator_tag` must leave alone, and a fieldless
pydantic subclass needing an explicit `pass`.

Verified: 266 tests and 100.00% coverage on Python 3.12, 3.13 and 3.14; ruff,
ruff format --check and mypy clean; 72 synthetic and 36 example-spec packages
still pass `ruff check --isolated` and `mypy --strict`.
@goduni
goduni merged commit 374a4ff into goduni:main Aug 7, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants