From ea76bd0a6854822faaec3b2896664b88374ef47a Mon Sep 17 00:00:00 2001 From: K1rL3s Date: Fri, 14 Aug 2026 19:44:07 +0300 Subject: [PATCH 1/3] fix: preserve explicit inherited fields --- README.md | 11 ++- src/unihttp_openapi_generator/ir/builder.py | 87 ++++--------------- src/unihttp_openapi_generator/ir/models.py | 2 + .../render/serializers/adaptix.py | 5 +- .../render/serializers/msgspec.py | 3 +- .../render/serializers/pydantic.py | 3 +- tests/test_builder.py | 73 +++++++++------- tests/test_render_models.py | 36 ++++++++ 8 files changed, 108 insertions(+), 112 deletions(-) diff --git a/README.md b/README.md index 5c4c327..2774afa 100644 --- a/README.md +++ b/README.md @@ -445,12 +445,11 @@ What to do with `allOf: [{$ref: Base}, ...]`. - Constructors become keyword-only **for the models in a hierarchy** — a subclass may pin an inherited field to a default while adding required fields of its own, which positional ordering cannot express. Models outside every hierarchy are untouched. - - A subtype that restates an inherited property just to attach prose, or to relax it - to nullable, simply **inherits** it: re-declaring `v: str | None` over the base's - `v: str` is rejected by `mypy --strict`. Genuine narrowings are kept — including a - `Literal` tag over a `str` base, but not over a base of a different scalar type - (`Literal['one', 'two']` does not narrow an `int`). So is a restatement that changes - the `default`, tightens the `constraints`, or makes the field `required`. + - A property explicitly declared by a subtype stays on that subtype, even when it is + identical to the inherited property. Compatible overrides render normally. An + incompatible override, such as `v: str | None` over `v: str`, gets a local + `# type: ignore[assignment]` so the generated model preserves the schema and still + passes `mypy --strict`. - Naming an inherited property in the subtype's `required` **without** restating the property still tightens it: the subtype re-declares it with the base's annotation and no default, so the constructor demands it. diff --git a/src/unihttp_openapi_generator/ir/builder.py b/src/unihttp_openapi_generator/ir/builder.py index 712a892..b1616ed 100644 --- a/src/unihttp_openapi_generator/ir/builder.py +++ b/src/unihttp_openapi_generator/ir/builder.py @@ -263,7 +263,7 @@ def _reconcile_inheritance(self, declarations: list[Declaration]) -> None: inherited = self._inherited_fields(by_name, decl.base_model) self._restore_required_narrowing(decl, inherited) self._retype_discriminator_tag(decl, inherited, by_name) - self._drop_unsafe_overrides(decl, inherited) + self._mark_unsafe_overrides(decl, inherited) self._rename_shadowed_fields(decl, inherited) self._align_override_names(decl, inherited) @@ -280,7 +280,7 @@ def _restore_required_narrowing(self, model: IRModel, inherited: dict[str, IRFie an optional field is rendered ``T | None`` whether it is optional or genuinely nullable, and the IR no longer distinguishes the two, so narrowing to ``T`` could reject a null the spec allows. Same annotation is always a legal - override, and ``_drop_unsafe_overrides`` keeps it because requiredness differs. + override, and ``_mark_unsafe_overrides`` keeps it because the type is compatible. """ own = {f.wire_name for f in model.fields} for wire in sorted(self._own_required.get(model.name, set())): @@ -938,79 +938,26 @@ def _build_object(self, name: str, schema: dict[str, Any], base_uri: str) -> IRM model.additional_properties = ANY return model - def _drop_unsafe_overrides(self, model: IRModel, inherited: dict[str, IRField]) -> None: - """Remove re-declared inherited fields that would not type-check as overrides. - - Specs routinely restate a base property in a subtype just to attach prose, or to - relax it to nullable. Re-emitting those produces ``class Sub(Base)`` with an - attribute whose type is not a subtype of the base's, which ``mypy --strict`` - rejects outright (``Incompatible types in assignment``). The base's declaration - already covers the field, so anything that is not a genuine narrowing is - dropped and simply inherited. - - This also covers the pinned discriminator tag: ``_apply_discriminator_tag`` - adds it after the model is built, and a base that types the tag property as an - enum (or as its own narrower ``Literal``) does not admit a subtype's - ``Literal`` over it. - """ - kept: list[IRField] = [] + def _mark_unsafe_overrides(self, model: IRModel, inherited: dict[str, IRField]) -> None: + """Mark incompatible inherited overrides for a local mypy ignore""" for f in model.fields: base_field = inherited.get(f.wire_name) - if base_field is None or self._is_narrowing(f.type, base_field.type): - kept.append(f) + if base_field is None: continue - # Same annotation is always a legal override, so a restatement survives - # whenever it carries something the base does not. - if f.type.annotation() == base_field.type.annotation() and self._refines(f, base_field): - kept.append(f) + if f.type.annotation() == base_field.type.annotation() or self._is_narrowing( + f.type, base_field.type + ): continue - widened = isinstance(f.type, OptionalType) and not isinstance( - base_field.type, OptionalType + f.ignore_assignment = True + logger.warning( + "%s.%s re-declares %s incompatibly (%s vs %s); preserving with " + "type: ignore[assignment]", + model.name, + f.name, + f.wire_name, + f.type.annotation(), + base_field.type.annotation(), ) - if f.type.annotation() == base_field.type.annotation() or widened: - # A verbatim restatement (spec prose, usually): the base's declaration - # already says everything this one does, so nothing is lost by - # inheriting it and there is nothing for the user to act on. - # Restating a property to attach prose, or to relax it to nullable, is - # something specs do routinely and the README documents as inherited -- - # so it is not something the user can or should act on. - logger.debug( - "%s.%s restates inherited %s (%s); inheriting instead", - model.name, - f.name, - f.wire_name, - f.type.annotation(), - ) - else: - # This one *does* lose information -- the subtype asked for a type the - # base does not admit, and the generated client will use the base's. - logger.warning( - "%s.%s re-declares %s incompatibly (%s vs %s); inheriting instead", - model.name, - f.name, - f.wire_name, - f.type.annotation(), - base_field.type.annotation(), - ) - model.fields = kept - - @staticmethod - def _refines(sub: IRField, base: IRField) -> bool: - """Whether a same-annotation restatement carries something the base lacks. - - An identical annotation is always a legal override, so the only question is - whether re-emitting it changes anything: - - - a different ``default`` -- dropping it hands the subtype the base's value; - - different ``constraints`` -- ``maxLength``/``pattern``/``minimum`` are - enforced by every serializer, so losing them makes the generated client - accept payloads the API will reject; - - stricter requiredness -- see ``_restore_required_narrowing``. - """ - changes_default = sub.has_default and (not base.has_default or sub.default != base.default) - return ( - changes_default or sub.constraints != base.constraints or sub.required != base.required - ) @staticmethod def _rename_shadowed_fields(model: IRModel, inherited: dict[str, IRField]) -> None: diff --git a/src/unihttp_openapi_generator/ir/models.py b/src/unihttp_openapi_generator/ir/models.py index 9731d94..70f1eb5 100644 --- a/src/unihttp_openapi_generator/ir/models.py +++ b/src/unihttp_openapi_generator/ir/models.py @@ -40,6 +40,8 @@ class is reported by ``runtime_refs`` so the module imports it at runtime. read_only: bool = False write_only: bool = False constraints: dict[str, Any] = field(default_factory=dict) + ignore_assignment: bool = False + """Whether an incompatible inherited override needs a local mypy ignore""" @property def needs_alias(self) -> bool: diff --git a/src/unihttp_openapi_generator/render/serializers/adaptix.py b/src/unihttp_openapi_generator/render/serializers/adaptix.py index a41eb14..9e15473 100644 --- a/src/unihttp_openapi_generator/render/serializers/adaptix.py +++ b/src/unihttp_openapi_generator/render/serializers/adaptix.py @@ -62,7 +62,10 @@ def render_model(self, model: IRModel) -> str: lines.append(" pass") inherited = self.inherited_field_names(model) for f in fields: - lines.append(" " + self._field_line(f.name, f.type.annotation(), f, inherited)) + ignore = " # type: ignore[assignment]" if f.ignore_assignment else "" + lines.append( + " " + self._field_line(f.name, f.type.annotation(), f, inherited) + ignore + ) return "\n".join(lines) @staticmethod diff --git a/src/unihttp_openapi_generator/render/serializers/msgspec.py b/src/unihttp_openapi_generator/render/serializers/msgspec.py index 26073ec..f6bbad2 100644 --- a/src/unihttp_openapi_generator/render/serializers/msgspec.py +++ b/src/unihttp_openapi_generator/render/serializers/msgspec.py @@ -74,7 +74,8 @@ def render_model(self, model: IRModel) -> str: lines.append(doc.rstrip("\n")) fields = sorted(model.fields, key=self._sort_key) for f in fields: - lines.append(" " + self._field_line(f)) + ignore = " # type: ignore[assignment]" if f.ignore_assignment else "" + lines.append(" " + self._field_line(f) + ignore) if not fields and not doc: lines.append(" pass") return "\n".join(lines) diff --git a/src/unihttp_openapi_generator/render/serializers/pydantic.py b/src/unihttp_openapi_generator/render/serializers/pydantic.py index 89faa54..1e467ed 100644 --- a/src/unihttp_openapi_generator/render/serializers/pydantic.py +++ b/src/unihttp_openapi_generator/render/serializers/pydantic.py @@ -161,7 +161,8 @@ def render_model(self, model: IRModel) -> str: # ``model_config`` used to keep the body non-empty for free. lines.append(" pass") for f in model.fields: - lines.append(" " + self._field_line(f)) + ignore = " # type: ignore[assignment]" if f.ignore_assignment else "" + lines.append(" " + self._field_line(f) + ignore) return "\n".join(lines) def _field_line(self, f: IRField) -> str: diff --git a/tests/test_builder.py b/tests/test_builder.py index 51516ad..081d304 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -715,7 +715,11 @@ def test_strip_prefix_auto() -> None: "Button": { "type": "object", "required": ["type", "text"], - "properties": {"type": {"type": "string"}, "text": {"type": "string"}}, + "properties": { + "type": {"type": "string"}, + "text": {"type": "string"}, + "label": {"type": "string"}, + }, "discriminator": { "propertyName": "type", "mapping": { @@ -732,6 +736,8 @@ def test_strip_prefix_auto() -> None: "properties": { # restated only to add prose: it must stay required "text": {"type": "string", "description": "Visible label."}, + # byte-for-byte restatement: explicit presence still matters + "label": {"type": "string"}, "payload": {"type": "string"}, }, }, @@ -771,35 +777,28 @@ def test_inheritance_keeps_parent_fields_on_parent(inherited: IRDocument) -> Non base = _decl(inherited, "Button") assert isinstance(base, IRModel) assert base.base_model is None - assert [f.name for f in base.fields] == ["type", "text"] + assert [f.name for f in base.fields] == ["type", "text", "label"] sub = _decl(inherited, "CallbackButton") assert sub.base_model == "Button" - # own fields only: the new ``payload`` and the pinned tag. ``text`` is restated by - # the spec purely to attach prose, so it is inherited rather than re-declared. - assert {f.name for f in sub.fields} == {"type", "payload"} + # Explicit child fields remain declarations even when they repeat a base field + assert {f.name for f in sub.fields} == {"type", "text", "label", "payload"} -def test_inheritance_drops_redundant_restatement(inherited: IRDocument) -> None: - # ``CallbackButton`` restates ``text`` only to add a description. Re-emitting it - # would put ``text: str`` on the subclass shadowing an identical base attribute -- - # noise at best, and a mypy ``[assignment]`` error as soon as the restatement - # differs at all (see ``test_inheritance_drops_widening_restatement``). +def test_inheritance_keeps_explicit_restatement(inherited: IRDocument) -> None: sub = _decl(inherited, "CallbackButton") - assert "text" not in {f.name for f in sub.fields} - base = _decl(inherited, "Button") - assert isinstance(base, IRModel) - text = next(f for f in base.fields if f.name == "text") + text = next(f for f in sub.fields if f.name == "text") + assert text.required is True assert text.type.annotation() == "str" + assert text.description == "Visible label." + assert text.ignore_assignment is False + label = next(f for f in sub.fields if f.name == "label") + assert label.description is None + assert label.ignore_assignment is False -def test_inheritance_drops_widening_restatement() -> None: - """A subtype relaxing an inherited field must not emit an unsound override. - - ``class C(P)`` with ``v: str | None`` over ``v: str`` is rejected by - ``mypy --strict``, so the subtype inherits the base's declaration instead. - """ +def test_inheritance_keeps_widening_restatement() -> None: spec: dict[str, Any] = { "openapi": "3.1.0", "info": {"title": "S", "version": "1.0.0"}, @@ -820,7 +819,8 @@ def test_inheritance_drops_widening_restatement() -> None: sub = _decl(ir, "C") assert isinstance(sub, IRModel) assert sub.base_model == "P" - assert sub.fields == [] + assert [field.name for field in sub.fields] == ["v"] + assert sub.fields[0].ignore_assignment is True def test_inheritance_keeps_narrowing_restatement() -> None: @@ -901,7 +901,7 @@ def test_inheritance_multiple_refs_still_merge(inherited: IRDocument) -> None: # Two `$ref`s give no single parent to pick, so the merge behaviour is kept. mixed = _decl(inherited, "Mixed") assert mixed.base_model is None - assert {f.name for f in mixed.fields} == {"id", "type", "text"} + assert {f.name for f in mixed.fields} == {"id", "type", "text", "label"} def test_without_inheritance_parent_fields_are_merged() -> None: @@ -910,7 +910,7 @@ def test_without_inheritance_parent_fields_are_merged() -> None: sub = _decl(ir, "CallbackButton") assert isinstance(sub, IRModel) assert sub.base_model is None - assert {f.name for f in sub.fields} == {"type", "text", "payload"} + assert {f.name for f in sub.fields} == {"type", "text", "label", "payload"} # the discriminated base collapses into a union alias, as before assert isinstance(_decl(ir, "Button"), IRAlias) @@ -1111,8 +1111,8 @@ def test_inheritance_tag_is_pinned_as_the_enum_member_the_base_declares() -> Non def test_inheritance_checks_the_whole_base_chain() -> None: """A subclass carries only its own fields, so a grandparent's are one hop further. - ``C`` re-declares ``v`` as an integer over ``A``'s string. Looking only at the direct - parent ``B`` finds nothing and lets the unsound override through. + ``C`` re-declares ``v`` as an integer over ``A``'s string. Looking only at the + direct parent ``B`` finds nothing and misses the required assignment ignore """ spec: dict[str, Any] = { "openapi": "3.1.0", @@ -1137,7 +1137,10 @@ def test_inheritance_checks_the_whole_base_chain() -> None: }, } ir = build_ir(spec, RefResolver(spec), inheritance=True) - assert [f.name for f in _decl(ir, "C").fields] == ["c"] + fields = _decl(ir, "C").fields + + assert [field.name for field in fields] == ["v", "c"] + assert fields[0].ignore_assignment is True def test_inheritance_renames_a_field_shadowing_an_inherited_identifier() -> None: @@ -1311,7 +1314,8 @@ def test_inheritance_literal_over_a_mismatched_primitive_is_not_a_narrowing() -> ) sub = _decl(build_ir(spec, RefResolver(spec), inheritance=True), "C") assert sub.base_model == "P" - assert sub.fields == [] # dropped as unsound; the int declaration is inherited + assert [field.name for field in sub.fields] == ["k"] + assert sub.fields[0].ignore_assignment is True # the same restatement over a ``str`` base *is* a genuine narrowing and stays spec["components"]["schemas"]["P"]["properties"]["k"] = {"type": "string"} @@ -1591,9 +1595,11 @@ def test_retype_discriminator_tag_leaves_non_enum_bases_alone() -> None: assert str_tag.type.annotation() == "Literal['one']" assert str_tag.default_expr is None - # a model-typed tag admits no member to pin, so the unsound Literal is dropped and - # the subtype inherits the base's declaration - assert [f.wire_name for f in _decl(ir, "ObjSub").fields] == ["b"] + # A model-typed tag admits no member to pin, so the explicit tag is kept with an + # assignment ignore + obj_fields = _decl(ir, "ObjSub").fields + assert [field.wire_name for field in obj_fields] == ["kind", "b"] + assert obj_fields[0].ignore_assignment is True own = next(f for f in _decl(ir, "OwnLiteral").fields if f.wire_name == "mode") assert own.default_expr is None @@ -1641,8 +1647,8 @@ def test_retype_discriminator_tag_needs_the_value_to_be_an_enum_member() -> None """A mapping key outside the base's enum leaves the tag alone. The spec is inconsistent -- it tags a subtype with a value the enum it typed the - property as does not admit -- so there is no member to pin and the unsound - ``Literal`` is dropped rather than guessed at. + property as does not admit - so there is no member to pin and the explicit + ``Literal`` is kept with an assignment ignore """ spec = _hier( { @@ -1666,4 +1672,5 @@ def test_retype_discriminator_tag_needs_the_value_to_be_an_enum_member() -> None ) sub = _decl(build_ir(spec, RefResolver(spec), inheritance=True), "Sub") assert sub.base_model == "Base" - assert [f.wire_name for f in sub.fields] == ["a"] + assert [field.wire_name for field in sub.fields] == ["kind", "a"] + assert sub.fields[0].ignore_assignment is True diff --git a/tests/test_render_models.py b/tests/test_render_models.py index 3ed86bd..a830381 100644 --- a/tests/test_render_models.py +++ b/tests/test_render_models.py @@ -427,6 +427,42 @@ def test_models_without_inheritance_keep_positional_dataclasses() -> None: assert "@dataclass(kw_only=True)" not in source +@pytest.mark.parametrize("serializer", list(Serializer)) +def test_incompatible_explicit_override_gets_local_ignore( + serializer: Serializer, tmp_path: Path +) -> None: + spec: dict[str, Any] = { + "openapi": "3.1.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "Parent": { + "type": "object", + "required": ["value"], + "properties": {"value": {"type": "string"}}, + }, + "Child": { + "allOf": [ + {"$ref": "#/components/schemas/Parent"}, + { + "properties": { + "value": {"type": ["string", "null"]}, + }, + }, + ], + }, + }, + }, + } + ir = build_ir(spec, RefResolver(spec), inheritance=True) + source = render_models_module(ir, get_strategy(serializer)) + line = next(line for line in source.splitlines() if "value: str | None" in line) + + assert line.endswith(" # type: ignore[assignment]") + _load(source, tmp_path, f"incompatible_override_{serializer.value}") + + def test_discriminated_base_class_keeps_its_mapping_visible() -> None: """A base kept as a class must not swallow the discriminator it declares. From 20ddfc2097b549bfe0ebbf95744d97a5cb0c102f Mon Sep 17 00:00:00 2001 From: K1rL3s Date: Fri, 14 Aug 2026 20:20:57 +0300 Subject: [PATCH 2/3] fix: avoid unused inheritance assignment ignores --- src/unihttp_openapi_generator/ir/builder.py | 3 +++ tests/test_builder.py | 30 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/unihttp_openapi_generator/ir/builder.py b/src/unihttp_openapi_generator/ir/builder.py index b1616ed..bf227ff 100644 --- a/src/unihttp_openapi_generator/ir/builder.py +++ b/src/unihttp_openapi_generator/ir/builder.py @@ -942,6 +942,7 @@ def _mark_unsafe_overrides(self, model: IRModel, inherited: dict[str, IRField]) """Mark incompatible inherited overrides for a local mypy ignore""" for f in model.fields: base_field = inherited.get(f.wire_name) + f.ignore_assignment = False if base_field is None: continue if f.type.annotation() == base_field.type.annotation() or self._is_narrowing( @@ -1012,6 +1013,8 @@ def _is_narrowing(cls, sub: IRType, base: IRType) -> bool: return False # a pure restatement: nothing to gain, just inherit it if isinstance(base, PrimitiveType) and base.py == "Any": return True + if isinstance(sub, PrimitiveType) and isinstance(base, PrimitiveType): + return sub.py == "int" and base.py == "float" if isinstance(sub, OptionalType) and isinstance(base, OptionalType): # ``T | None`` narrows ``U | None`` exactly when ``T`` narrows ``U``. return cls._is_narrowing(sub.inner, base.inner) diff --git a/tests/test_builder.py b/tests/test_builder.py index 081d304..74389df 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -12,6 +12,7 @@ from unihttp_openapi_generator.ir.operations import BodyKind, ParamLocation from unihttp_openapi_generator.ir.types import ( BOOL, + FLOAT, INT, STR, ListType, @@ -1484,6 +1485,8 @@ def test_allof_cycle_merges_the_dropped_base_instead_of_losing_its_fields() -> N # both optional: compare the inners (OptionalType(LiteralType(("a",))), OptionalType(STR), True), (OptionalType(STR), OptionalType(INT), False), + # mypy's numeric tower admits an integer wherever a float is expected + (INT, FLOAT, True), ], ) def test_is_narrowing_type_shapes(sub: Any, base: Any, expected: bool) -> None: @@ -1674,3 +1677,30 @@ def test_retype_discriminator_tag_needs_the_value_to_be_an_enum_member() -> None assert sub.base_model == "Base" assert [field.wire_name for field in sub.fields] == ["kind", "a"] assert sub.fields[0].ignore_assignment is True + + +def test_required_narrowing_recomputes_inherited_assignment_ignore() -> None: + spec = _hier( + { + "A": {"type": "object", "properties": {"v": {"type": "string"}}}, + "B": { + "allOf": [ + {"$ref": "#/components/schemas/A"}, + {"properties": {"v": {"type": "integer"}}}, + ] + }, + "C": { + "allOf": [ + {"$ref": "#/components/schemas/B"}, + {"required": ["v"]}, + ] + }, + } + ) + ir = build_ir(spec, RefResolver(spec), inheritance=True) + b_field = next(field for field in _decl(ir, "B").fields if field.wire_name == "v") + c_field = next(field for field in _decl(ir, "C").fields if field.wire_name == "v") + + assert b_field.ignore_assignment is True + assert c_field.required is True + assert c_field.ignore_assignment is False From b2f715c14a70eb7c2513394f04a88fd4c9a235dd Mon Sep 17 00:00:00 2001 From: goduni <37146584+goduni@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:30:13 +0000 Subject: [PATCH 3/3] fix: keep incompatible-override suppression from failing mypy --strict Preserving every explicit child declaration made the narrowing predicate load-bearing in a way it was not written for. It is deliberately conservative because a false "no" used to mean only a slightly less precise inherited type; now a false "no" emits `# type: ignore[assignment]` on a line mypy is happy with, and `--strict` reports the unused ignore as an error of its own. Two ordinary spec shapes hit this and made `--inheritance --check` fail: - a subtype narrowing a `$ref` to a schema that inherits from the base's (`companion: Pet` over `companion: Creature`) -- nothing in either annotation says the two are related; - `boolean` over `integer`, which mypy accepts via the numeric tower. Both are now recognised as narrowings: `_is_narrowing` consults the declaration map for `$ref` overrides and a promotion table for primitives. The suppression comment additionally lists `unused-ignore`, so the remaining gap between the IR's view of a type and mypy's costs a redundant comment rather than a failed build. Both shapes join the hierarchy fixture, so the compile gate holds them under `mypy --strict` for every serializer and layout. The flag itself moves to `IRField.incompatible_override`: the IR records the fact about the spec, and the serializer strategies decide what to emit for it in one shared place instead of three copies. An incompatible override is a spec defect -- the subtype is not substitutable for its base -- so the warning now says that, and points at the schema and property to fix upstream. --- README.md | 12 +- src/unihttp_openapi_generator/ir/builder.py | 91 ++++++++++++--- src/unihttp_openapi_generator/ir/models.py | 9 +- .../render/serializers/adaptix.py | 6 +- .../render/serializers/base.py | 17 +++ .../render/serializers/msgspec.py | 3 +- .../render/serializers/pydantic.py | 3 +- tests/conftest.py | 41 ++++++- tests/test_builder.py | 109 ++++++++++++++++-- tests/test_render_models.py | 4 +- 10 files changed, 249 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 2774afa..c2973df 100644 --- a/README.md +++ b/README.md @@ -446,10 +446,14 @@ What to do with `allOf: [{$ref: Base}, ...]`. pin an inherited field to a default while adding required fields of its own, which positional ordering cannot express. Models outside every hierarchy are untouched. - A property explicitly declared by a subtype stays on that subtype, even when it is - identical to the inherited property. Compatible overrides render normally. An - incompatible override, such as `v: str | None` over `v: str`, gets a local - `# type: ignore[assignment]` so the generated model preserves the schema and still - passes `mypy --strict`. + identical to the inherited property. Compatible overrides render normally — that + includes narrowing a `$ref` to a schema that inherits from the base's + (`companion: Pet` over `companion: Creature`) and `integer` over `number`. An + override the base cannot admit, such as `v: str | None` over `v: str`, means the + subtype is not substitutable for its base: that is a defect in the spec and worth + fixing there, so it is reported as a warning. The generated model stays faithful to + the schema and carries a local `# type: ignore[assignment, unused-ignore]`, which + keeps it clean under `mypy --strict` either way. - Naming an inherited property in the subtype's `required` **without** restating the property still tightens it: the subtype re-declares it with the base's annotation and no default, so the constructor demands it. diff --git a/src/unihttp_openapi_generator/ir/builder.py b/src/unihttp_openapi_generator/ir/builder.py index bf227ff..4eb4006 100644 --- a/src/unihttp_openapi_generator/ir/builder.py +++ b/src/unihttp_openapi_generator/ir/builder.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +from collections.abc import Mapping from dataclasses import replace from typing import Any from urllib.parse import urlsplit @@ -60,6 +61,11 @@ _HTTP_METHODS = ("get", "put", "post", "delete", "patch", "head", "options", "trace") +# Python's numeric tower as mypy applies it: an ``int`` is accepted wherever a ``float`` +# is expected, and a ``bool`` wherever an ``int`` is. A subtype re-typing ``number`` as +# ``integer`` is an ordinary OpenAPI narrowing, so it must not read as incompatible. +_PROMOTIONS = frozenset({("int", "float"), ("bool", "int"), ("bool", "float")}) + # Identifiers imported into generated modules; a model/method class must never reuse one # or it would shadow the import (markers, unihttp core, serializer bases, common types). _RESERVED_NAMES = frozenset( @@ -263,7 +269,7 @@ def _reconcile_inheritance(self, declarations: list[Declaration]) -> None: inherited = self._inherited_fields(by_name, decl.base_model) self._restore_required_narrowing(decl, inherited) self._retype_discriminator_tag(decl, inherited, by_name) - self._mark_unsafe_overrides(decl, inherited) + self._mark_unsafe_overrides(decl, inherited, by_name) self._rename_shadowed_fields(decl, inherited) self._align_override_names(decl, inherited) @@ -280,7 +286,7 @@ def _restore_required_narrowing(self, model: IRModel, inherited: dict[str, IRFie an optional field is rendered ``T | None`` whether it is optional or genuinely nullable, and the IR no longer distinguishes the two, so narrowing to ``T`` could reject a null the spec allows. Same annotation is always a legal - override, and ``_mark_unsafe_overrides`` keeps it because the type is compatible. + override, so ``_mark_unsafe_overrides`` leaves the re-declaration unflagged. """ own = {f.wire_name for f in model.fields} for wire in sorted(self._own_required.get(model.name, set())): @@ -295,6 +301,9 @@ def _restore_required_narrowing(self, model: IRModel, inherited: dict[str, IRFie has_default=False, omittable=False, constraints=dict(base_field.constraints), + # Judged against *this* model's base chain, not the one the copied + # field was flagged for; ``_mark_unsafe_overrides`` runs next. + incompatible_override=False, ) ) @@ -938,23 +947,39 @@ def _build_object(self, name: str, schema: dict[str, Any], base_uri: str) -> IRM model.additional_properties = ANY return model - def _mark_unsafe_overrides(self, model: IRModel, inherited: dict[str, IRField]) -> None: - """Mark incompatible inherited overrides for a local mypy ignore""" + def _mark_unsafe_overrides( + self, + model: IRModel, + inherited: dict[str, IRField], + by_name: dict[str, Declaration], + ) -> None: + """Flag re-declared inherited fields whose type the base does not admit. + + Every property a subtype declares stays on the subtype, so the flag records + only that the spec asked for something ``class Sub(Base)`` cannot express + soundly; the serializers decide what to emit for it. + + A spec that does this is inconsistent -- the subtype is not substitutable for + its base -- and that is worth fixing in the spec rather than in its clients, so + every occurrence is logged with the schema and property it comes from. + """ for f in model.fields: base_field = inherited.get(f.wire_name) - f.ignore_assignment = False + # Always assigned, never only set: ``_restore_required_narrowing`` copies + # fields down from the base chain, flag and all, and the copy is judged + # against a different base than the original was. + f.incompatible_override = False if base_field is None: continue if f.type.annotation() == base_field.type.annotation() or self._is_narrowing( - f.type, base_field.type + f.type, base_field.type, by_name ): continue - f.ignore_assignment = True + f.incompatible_override = True logger.warning( - "%s.%s re-declares %s incompatibly (%s vs %s); preserving with " - "type: ignore[assignment]", + "%s re-declares %r as %s, which the inherited %s does not admit; " + "the subtype is not substitutable for its base -- fix the schema", model.name, - f.name, f.wire_name, f.type.annotation(), base_field.type.annotation(), @@ -1002,25 +1027,36 @@ def _align_override_names(model: IRModel, inherited: dict[str, IRField]) -> None f.name = base_field.name @classmethod - def _is_narrowing(cls, sub: IRType, base: IRType) -> bool: + def _is_narrowing( + cls, sub: IRType, base: IRType, by_name: Mapping[str, Declaration] | None = None + ) -> bool: """Whether ``sub`` is safe to re-declare over an inherited ``base`` annotation. - Deliberately conservative: it only says yes for the shapes that are provably - assignable, because a false yes emits code that fails ``mypy --strict`` while a - false no merely inherits a slightly less precise type. + Only says yes for shapes that are provably assignable, and a no is answered by + emitting the declaration under a suppression comment rather than by dropping + it -- so a false no costs a line of noise in the output, not a lost property. + The suppression carries ``unused-ignore`` for exactly that reason. + + ``by_name`` resolves ``$ref`` narrowings: ``Dog`` over ``Animal`` is assignable + only if the generated ``Dog`` really does subclass ``Animal``, which nothing in + either annotation says. Omitting it just makes those answer no. """ if sub.annotation() == base.annotation(): return False # a pure restatement: nothing to gain, just inherit it if isinstance(base, PrimitiveType) and base.py == "Any": return True if isinstance(sub, PrimitiveType) and isinstance(base, PrimitiveType): - return sub.py == "int" and base.py == "float" + return (sub.py, base.py) in _PROMOTIONS + if isinstance(sub, RefType) and isinstance(base, RefType): + return cls._extends(sub.name, base.name, by_name) if isinstance(sub, OptionalType) and isinstance(base, OptionalType): # ``T | None`` narrows ``U | None`` exactly when ``T`` narrows ``U``. - return cls._is_narrowing(sub.inner, base.inner) + return cls._is_narrowing(sub.inner, base.inner, by_name) if isinstance(base, OptionalType): # ``T | None`` admits ``T`` and anything that narrows ``T``. - return sub.annotation() == base.inner.annotation() or cls._is_narrowing(sub, base.inner) + return sub.annotation() == base.inner.annotation() or cls._is_narrowing( + sub, base.inner, by_name + ) if isinstance(sub, OptionalType): return False # adding None to a non-optional base widens it if isinstance(sub, LiteralType): @@ -1034,9 +1070,28 @@ def _is_narrowing(cls, sub: IRType, base: IRType) -> bool: return isinstance(base, PrimitiveType) and cls._literals_fit(sub.values, base.py) if isinstance(base, UnionType): return any( - cls._is_narrowing(sub, m) or sub.annotation() == m.annotation() + cls._is_narrowing(sub, m, by_name) or sub.annotation() == m.annotation() for m in base.members ) + # Containers land here: ``list``/``dict`` are invariant, so a narrowed element + # type is not a narrowed container. + return False + + @staticmethod + def _extends(sub: str, base: str, by_name: Mapping[str, Declaration] | None) -> bool: + """Whether the model named ``sub`` inherits from ``base``, however deep.""" + if by_name is None: + return False + seen: set[str] = set() + current: str | None = sub + while current is not None and current not in seen: + seen.add(current) + decl = by_name.get(current) + if not isinstance(decl, IRModel): + return False + if decl.base_model == base: + return True + current = decl.base_model return False @staticmethod diff --git a/src/unihttp_openapi_generator/ir/models.py b/src/unihttp_openapi_generator/ir/models.py index 70f1eb5..205805c 100644 --- a/src/unihttp_openapi_generator/ir/models.py +++ b/src/unihttp_openapi_generator/ir/models.py @@ -40,8 +40,13 @@ class is reported by ``runtime_refs`` so the module imports it at runtime. read_only: bool = False write_only: bool = False constraints: dict[str, Any] = field(default_factory=dict) - ignore_assignment: bool = False - """Whether an incompatible inherited override needs a local mypy ignore""" + incompatible_override: bool = False + """Whether this field re-declares an inherited one with a type the base does not admit. + + A fact about the spec, not a rendering decision: the subtype asked for something + ``class Sub(Base)`` cannot express soundly. What to do about it belongs to the + serializer strategies (see ``override_suppression``). + """ @property def needs_alias(self) -> bool: diff --git a/src/unihttp_openapi_generator/render/serializers/adaptix.py b/src/unihttp_openapi_generator/render/serializers/adaptix.py index 9e15473..c580b96 100644 --- a/src/unihttp_openapi_generator/render/serializers/adaptix.py +++ b/src/unihttp_openapi_generator/render/serializers/adaptix.py @@ -62,10 +62,8 @@ def render_model(self, model: IRModel) -> str: lines.append(" pass") inherited = self.inherited_field_names(model) for f in fields: - ignore = " # type: ignore[assignment]" if f.ignore_assignment else "" - lines.append( - " " + self._field_line(f.name, f.type.annotation(), f, inherited) + ignore - ) + line = self._field_line(f.name, f.type.annotation(), f, inherited) + lines.append(" " + line + self.override_suppression(f)) return "\n".join(lines) @staticmethod diff --git a/src/unihttp_openapi_generator/render/serializers/base.py b/src/unihttp_openapi_generator/render/serializers/base.py index ba63e01..bc3c3d6 100644 --- a/src/unihttp_openapi_generator/render/serializers/base.py +++ b/src/unihttp_openapi_generator/render/serializers/base.py @@ -137,6 +137,23 @@ def inherited_field_names(self, model: IRModel) -> set[str]: current = parent.base_model return names + @staticmethod + def override_suppression(f: IRField) -> str: + """Trailing comment for a field the subtype re-declares incompatibly. + + ``unused-ignore`` is listed alongside ``assignment`` on purpose: the builder + decides whether an override is compatible from the IR alone, and the IR is a + coarser view of the type than mypy's. It has no notion of the numeric tower + beyond ``int``/``float``, and a ``$ref`` narrowed to a schema that is itself a + subtype resolves to a plain ``RefType`` whose relation to the base's is not + visible in the annotation. Whenever the builder is stricter than mypy the bare + ``[assignment]`` ignore would be unused, which ``--strict`` reports as an error + of its own -- so the comment silences its own redundancy. + """ + if not f.incompatible_override: + return "" + return " # type: ignore[assignment, unused-ignore]" + # -- imports --------------------------------------------------------------- @abstractmethod diff --git a/src/unihttp_openapi_generator/render/serializers/msgspec.py b/src/unihttp_openapi_generator/render/serializers/msgspec.py index f6bbad2..91c6f5e 100644 --- a/src/unihttp_openapi_generator/render/serializers/msgspec.py +++ b/src/unihttp_openapi_generator/render/serializers/msgspec.py @@ -74,8 +74,7 @@ def render_model(self, model: IRModel) -> str: lines.append(doc.rstrip("\n")) fields = sorted(model.fields, key=self._sort_key) for f in fields: - ignore = " # type: ignore[assignment]" if f.ignore_assignment else "" - lines.append(" " + self._field_line(f) + ignore) + lines.append(" " + self._field_line(f) + self.override_suppression(f)) if not fields and not doc: lines.append(" pass") return "\n".join(lines) diff --git a/src/unihttp_openapi_generator/render/serializers/pydantic.py b/src/unihttp_openapi_generator/render/serializers/pydantic.py index 1e467ed..905414c 100644 --- a/src/unihttp_openapi_generator/render/serializers/pydantic.py +++ b/src/unihttp_openapi_generator/render/serializers/pydantic.py @@ -161,8 +161,7 @@ def render_model(self, model: IRModel) -> str: # ``model_config`` used to keep the body non-empty for free. lines.append(" pass") for f in model.fields: - ignore = " # type: ignore[assignment]" if f.ignore_assignment else "" - lines.append(" " + self._field_line(f) + ignore) + lines.append(" " + self._field_line(f) + self.override_suppression(f)) return "\n".join(lines) def _field_line(self, f: IRField) -> str: diff --git a/tests/conftest.py b/tests/conftest.py index 810e1f6..d1435ca 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -178,8 +178,9 @@ def hierarchy_spec() -> dict[str, Any]: real defect at some point: a discriminated base that keeps its own properties, a tag the base types as an enum (so the subtype's ``Literal`` is not assignable), restatements that only add prose / relax to nullable / change a default, wire names - that snake-case onto an inherited identifier, a three-level chain, and a base whose - own body refers back to its subtype. + that snake-case onto an inherited identifier, a three-level chain, a base whose own + body refers back to its subtype, and narrowings only the base chain or the numeric + tower can justify. """ return { "openapi": "3.1.0", @@ -339,6 +340,42 @@ def hierarchy_spec() -> dict[str, Any]: {"properties": {"value": {"type": "string"}}}, ] }, + # A subtype narrowing a property to a schema that subclasses the base's: + # nothing in either annotation says ``Pet`` is a ``Creature``, so the + # override reads as incompatible unless the base chain is consulted. + "Creature": { + "type": "object", + "required": ["name"], + "properties": {"name": {"type": "string"}}, + }, + "Pet": { + "allOf": [ + {"$ref": "#/components/schemas/Creature"}, + {"properties": {"tame": {"type": "boolean"}}}, + ] + }, + "Owner": { + "type": "object", + "required": ["companion", "score"], + "properties": { + "companion": {"$ref": "#/components/schemas/Creature"}, + "score": {"type": "number"}, + }, + }, + "PetOwner": { + "allOf": [ + {"$ref": "#/components/schemas/Owner"}, + { + "type": "object", + "required": ["companion", "score"], + "properties": { + "companion": {"$ref": "#/components/schemas/Pet"}, + # the numeric tower: ``integer`` narrows ``number`` + "score": {"type": "integer"}, + }, + }, + ] + }, } }, } diff --git a/tests/test_builder.py b/tests/test_builder.py index 74389df..66444a1 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -793,10 +793,10 @@ def test_inheritance_keeps_explicit_restatement(inherited: IRDocument) -> None: assert text.required is True assert text.type.annotation() == "str" assert text.description == "Visible label." - assert text.ignore_assignment is False + assert text.incompatible_override is False label = next(f for f in sub.fields if f.name == "label") assert label.description is None - assert label.ignore_assignment is False + assert label.incompatible_override is False def test_inheritance_keeps_widening_restatement() -> None: @@ -821,7 +821,7 @@ def test_inheritance_keeps_widening_restatement() -> None: assert isinstance(sub, IRModel) assert sub.base_model == "P" assert [field.name for field in sub.fields] == ["v"] - assert sub.fields[0].ignore_assignment is True + assert sub.fields[0].incompatible_override is True def test_inheritance_keeps_narrowing_restatement() -> None: @@ -1141,7 +1141,7 @@ def test_inheritance_checks_the_whole_base_chain() -> None: fields = _decl(ir, "C").fields assert [field.name for field in fields] == ["v", "c"] - assert fields[0].ignore_assignment is True + assert fields[0].incompatible_override is True def test_inheritance_renames_a_field_shadowing_an_inherited_identifier() -> None: @@ -1316,7 +1316,7 @@ def test_inheritance_literal_over_a_mismatched_primitive_is_not_a_narrowing() -> sub = _decl(build_ir(spec, RefResolver(spec), inheritance=True), "C") assert sub.base_model == "P" assert [field.name for field in sub.fields] == ["k"] - assert sub.fields[0].ignore_assignment is True + assert sub.fields[0].incompatible_override is True # the same restatement over a ``str`` base *is* a genuine narrowing and stays spec["components"]["schemas"]["P"]["properties"]["k"] = {"type": "string"} @@ -1476,7 +1476,9 @@ def test_allof_cycle_merges_the_dropped_base_instead_of_losing_its_fields() -> N (LiteralType(("c",)), LiteralType(("a", "b")), False), # union base: narrowing any one member is enough (STR, UnionType((STR, INT)), True), - (BOOL, UnionType((STR, INT)), False), + # via the ``int`` member, since a bool is an int as far as mypy is concerned + (BOOL, UnionType((STR, INT)), True), + (FLOAT, UnionType((STR, INT)), False), # a Literal is answered against the base's *own* shape before the union branch # is reached, so this is a deliberate false no: the subtype inherits the wider # annotation instead of narrowing it. Safe, since a false yes emits code that @@ -1485,8 +1487,15 @@ def test_allof_cycle_merges_the_dropped_base_instead_of_losing_its_fields() -> N # both optional: compare the inners (OptionalType(LiteralType(("a",))), OptionalType(STR), True), (OptionalType(STR), OptionalType(INT), False), - # mypy's numeric tower admits an integer wherever a float is expected + # mypy's numeric tower admits an integer wherever a float is expected, and a + # bool wherever an int is (INT, FLOAT, True), + (BOOL, INT, True), + (FLOAT, INT, False), + # containers are invariant: a narrowed element is not a narrowed container + (ListType(INT), ListType(FLOAT), False), + # a $ref answers no without a declaration map to check the base chain in + (RefType("Dog"), RefType("Animal"), False), ], ) def test_is_narrowing_type_shapes(sub: Any, base: Any, expected: bool) -> None: @@ -1602,7 +1611,7 @@ def test_retype_discriminator_tag_leaves_non_enum_bases_alone() -> None: # assignment ignore obj_fields = _decl(ir, "ObjSub").fields assert [field.wire_name for field in obj_fields] == ["kind", "b"] - assert obj_fields[0].ignore_assignment is True + assert obj_fields[0].incompatible_override is True own = next(f for f in _decl(ir, "OwnLiteral").fields if f.wire_name == "mode") assert own.default_expr is None @@ -1676,7 +1685,7 @@ def test_retype_discriminator_tag_needs_the_value_to_be_an_enum_member() -> None sub = _decl(build_ir(spec, RefResolver(spec), inheritance=True), "Sub") assert sub.base_model == "Base" assert [field.wire_name for field in sub.fields] == ["kind", "a"] - assert sub.fields[0].ignore_assignment is True + assert sub.fields[0].incompatible_override is True def test_required_narrowing_recomputes_inherited_assignment_ignore() -> None: @@ -1701,6 +1710,84 @@ def test_required_narrowing_recomputes_inherited_assignment_ignore() -> None: b_field = next(field for field in _decl(ir, "B").fields if field.wire_name == "v") c_field = next(field for field in _decl(ir, "C").fields if field.wire_name == "v") - assert b_field.ignore_assignment is True + assert b_field.incompatible_override is True assert c_field.required is True - assert c_field.ignore_assignment is False + assert c_field.incompatible_override is False + + +def test_inheritance_ref_narrowed_to_a_subclass_is_compatible() -> None: + """``Dog`` over ``Animal`` is a legal override once the base chain is consulted. + + Both sides are a bare ``RefType``; only the declaration map says one subclasses the + other. Flagging it would put a suppression comment on a line mypy is happy with. + """ + spec = _hier( + { + "Animal": {"type": "object", "properties": {"name": {"type": "string"}}}, + "Dog": { + "allOf": [ + {"$ref": "#/components/schemas/Animal"}, + {"properties": {"breed": {"type": "string"}}}, + ] + }, + "Keeper": { + "type": "object", + "required": ["pet"], + "properties": {"pet": {"$ref": "#/components/schemas/Animal"}}, + }, + "DogKeeper": { + "allOf": [ + {"$ref": "#/components/schemas/Keeper"}, + { + "required": ["pet"], + "properties": {"pet": {"$ref": "#/components/schemas/Dog"}}, + }, + ] + }, + } + ) + ir = build_ir(spec, RefResolver(spec), inheritance=True) + pet = next(field for field in _decl(ir, "DogKeeper").fields if field.wire_name == "pet") + + assert pet.type.annotation() == "Dog" + assert pet.incompatible_override is False + + # the same override the other way round *is* incompatible + schemas = spec["components"]["schemas"] + schemas["Keeper"]["properties"]["pet"] = {"$ref": "#/components/schemas/Dog"} + schemas["DogKeeper"]["allOf"][1]["properties"]["pet"] = {"$ref": "#/components/schemas/Animal"} + ir = build_ir(spec, RefResolver(spec), inheritance=True) + pet = next(field for field in _decl(ir, "DogKeeper").fields if field.wire_name == "pet") + + assert pet.incompatible_override is True + + # and a ``$ref`` to something that is not a class at all has no chain to walk + schemas["Kind"] = {"type": "string", "enum": ["dog", "cat"]} + schemas["DogKeeper"]["allOf"][1]["properties"]["pet"] = {"$ref": "#/components/schemas/Kind"} + ir = build_ir(spec, RefResolver(spec), inheritance=True) + pet = next(field for field in _decl(ir, "DogKeeper").fields if field.wire_name == "pet") + + assert pet.incompatible_override is True + + +def test_inheritance_integer_over_number_is_compatible() -> None: + spec = _hier( + { + "P": { + "type": "object", + "required": ["v"], + "properties": {"v": {"type": "number"}}, + }, + "C": { + "allOf": [ + {"$ref": "#/components/schemas/P"}, + {"required": ["v"], "properties": {"v": {"type": "integer"}}}, + ] + }, + } + ) + ir = build_ir(spec, RefResolver(spec), inheritance=True) + v = next(field for field in _decl(ir, "C").fields if field.wire_name == "v") + + assert v.type.annotation() == "int" + assert v.incompatible_override is False diff --git a/tests/test_render_models.py b/tests/test_render_models.py index a830381..3935dea 100644 --- a/tests/test_render_models.py +++ b/tests/test_render_models.py @@ -459,7 +459,9 @@ def test_incompatible_explicit_override_gets_local_ignore( source = render_models_module(ir, get_strategy(serializer)) line = next(line for line in source.splitlines() if "value: str | None" in line) - assert line.endswith(" # type: ignore[assignment]") + # ``unused-ignore`` rides along so the comment stays clean under ``mypy --strict`` + # wherever mypy is more permissive than the IR's view of the two types. + assert line.endswith(" # type: ignore[assignment, unused-ignore]") _load(source, tmp_path, f"incompatible_override_{serializer.value}")