Skip to content

Commit 48df0ab

Browse files
committed
fix(codegen): render list[NewType] as list<NewType>
list[PhoneNumber] rendered as "PhoneNumber (list)" — implying PhoneNumber itself is a list type. The root cause: format_type couldn't distinguish list layers outside a NewType from list layers inside one. Add newtype_outer_list_depth to TypeInfo, snapshotted from list_depth when the type analyzer enters the first NewType. The renderer uses this to choose list<X> syntax (list wraps the NewType) vs a (list) qualifier (NewType wraps a list internally). Non-NewType identities (enums, models) continue using list<X>.
1 parent 4a1dc06 commit 48df0ab

File tree

6 files changed

+166
-47
lines changed

6 files changed

+166
-47
lines changed

packages/overture-schema-codegen/docs/design.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,10 @@ layers in a fixed order, accumulating information into an `_UnwrapState`:
125125

126126
The result is `TypeInfo` -- a flat dataclass that fully describes the unwrapped type:
127127
classification (`TypeKind`), optional/dict flags, `list_depth` (count of `list[...]`
128-
layers), accumulated constraints with provenance, NewType names, source type, literal
129-
values, and (for UNION kind) the tuple of concrete `BaseModel` member types. Dict types carry recursively analyzed `TypeInfo`
130-
for their key and value types.
128+
layers), `newtype_outer_list_depth` (list layers outside the outermost NewType boundary),
129+
accumulated constraints with provenance, NewType names, source type, literal values, and
130+
(for UNION kind) the tuple of concrete `BaseModel` member types. Dict types carry
131+
recursively analyzed `TypeInfo` for their key and value types.
131132

132133
Multi-depth `Annotated` layers (common in practice, since NewTypes wrap `Annotated`
133134
types that wrap further NewTypes) are handled naturally by the loop -- each iteration

packages/overture-schema-codegen/docs/walkthrough.md

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,11 @@ The function runs a single `while True` loop that peels layers in fixed order. E
122122
iteration handles one wrapper:
123123

124124
**NewType** records names at two levels. The first NewType encountered becomes
125-
`outermost_newtype_name` (the user-facing identity, e.g. "FeatureVersion"). Subsequent
126-
NewTypes update `last_newtype_name` (the innermost, used for constraint provenance and
127-
as the terminal `base_type`). The loop unwraps via `__supertype__` and continues.
125+
`outermost_newtype_name` (the user-facing identity, e.g. "FeatureVersion") and snapshots
126+
the current `list_depth` into `newtype_outer_list_depth` -- capturing how many list
127+
layers appeared before the NewType boundary. Subsequent NewTypes update
128+
`last_newtype_name` (the innermost, used for constraint provenance and as the terminal
129+
`base_type`). The loop unwraps via `__supertype__` and continues.
128130

129131
**Annotated** collects every metadata object as a `ConstraintSource`, tagging each with
130132
whichever NewType was most recently entered. This is how constraint provenance survives:
@@ -168,7 +170,8 @@ member separately.
168170
int32)` where `int32 = NewType("int32", Annotated[int, Field(ge=0, le=2147483647)])`.
169171

170172
Iteration 1 sees `FeatureVersion`. It's a NewType -- record
171-
`outermost_newtype_name="FeatureVersion"`, unwrap to `int32`, continue. Iteration 2 sees
173+
`outermost_newtype_name="FeatureVersion"`, snapshot `newtype_outer_list_depth=0` (no list
174+
layers yet), unwrap to `int32`, continue. Iteration 2 sees
172175
`int32`. Also a NewType -- update `last_newtype_name="int32"`, unwrap to `Annotated[int,
173176
Field(ge=0, ...)]`, continue. Iteration 3 sees `Annotated`. Collect
174177
`ConstraintSource(source="int32", constraint=<Field metadata>)`, unwrap to `int`. The
@@ -185,10 +188,10 @@ rendering.
185188
### _UnwrapState
186189

187190
The accumulator dataclass carries state across iterations: optional/dict flags,
188-
`list_depth` (incremented per `list[...]` layer), the constraint list, both NewType name
189-
slots, and the captured description. Its
190-
`build_type_info` method assembles the final `TypeInfo` from accumulated state, freezing
191-
the constraint list into a tuple.
191+
`list_depth` (incremented per `list[...]` layer), `newtype_outer_list_depth` (snapshotted
192+
from `list_depth` when the first NewType is entered), the constraint list, both NewType
193+
name slots, and the captured description. Its `build_type_info` method assembles the
194+
final `TypeInfo` from accumulated state, freezing the constraint list into a tuple.
192195

193196
### walk_type_info
194197

@@ -506,10 +509,16 @@ provenance rather than direct field reference.
506509
`format_type` handles the full range of field types. Single-value Literals render as
507510
`"value"` in backticks. Semantic NewTypes and enums/models get markdown links via
508511
`_resolve_type_link`, which checks the `LinkContext` registry and falls back to plain
509-
code spans. Lists wrap `list_depth` times. Linked inner types use broken-backtick syntax
510-
(`` `list<` `` ... `` `>` ``) built as a single wrapper to avoid adjacent backticks that
511-
CommonMark would interpret as multi-backtick code span delimiters. Dict types render as `` `map<K, V>` ``. Qualifiers
512-
(optional, list, map) append in parentheses.
512+
code spans. For types with a linked identity (semantic NewTypes, enums, models), list
513+
rendering depends on where the list layers sit relative to the NewType boundary.
514+
`newtype_outer_list_depth > 0` means the list wraps the NewType (`list[PhoneNumber]`) and
515+
renders as `list<PhoneNumber>`. `is_list` with `newtype_name` set means the NewType
516+
wraps a list internally (`Sources` wrapping `list[SourceItem]`) and renders with a
517+
`(list)` qualifier. Non-NewType identities (enums, models) use `list<X>` syntax. Linked
518+
inner types use broken-backtick syntax (`` `list<` `` ... `` `>` ``) built as a single
519+
wrapper to avoid adjacent backticks that CommonMark would interpret as multi-backtick
520+
code span delimiters. Dict types render as `` `map<K, V>` ``. Qualifiers (optional, list,
521+
map) append in parentheses.
513522

514523
Union members format independently -- each gets its own link resolution, joined with
515524
pipe separators escaped for table-cell safety.

packages/overture-schema-codegen/src/overture/schema/codegen/markdown_type_format.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,11 +143,21 @@ def format_type(
143143
display = f"`{format_dict_type(ti)}`"
144144
elif identity:
145145
display = resolve_type_link(identity, ctx)
146-
if ti.is_list and identity.name == ti.newtype_name:
146+
# List layers outside a NewType wrap with list<> syntax (e.g., list[PhoneNumber]
147+
# renders as list<PhoneNumber>). List layers inside a NewType use a (list)
148+
# qualifier instead (e.g., Sources wrapping list[SourceItem] renders as
149+
# Sources (list)), since the list-ness is an implementation detail of the type.
150+
if ti.newtype_outer_list_depth > 0:
151+
assert ti.is_list # outer list layers are a subset of total list layers
152+
display = _wrap_list_n(display, ti.newtype_outer_list_depth)
153+
elif ti.is_list and ti.newtype_name is not None: # list is inside the NewType
147154
qualifiers.append("list")
148155
elif ti.is_list:
149156
display = _wrap_list_n(display, ti.list_depth)
150157
else:
158+
# Fallback: types without a linked identity. Registered primitives (int32,
159+
# Geometry) and Pydantic types (HttpUrl) may still link to aggregate pages
160+
# via the placement registry. Unregistered primitives render as plain code.
151161
base = resolve_type_name(ti, "markdown")
152162
link = _try_primitive_link(ti, base, ctx)
153163
if link and ti.is_list:

packages/overture-schema-codegen/src/overture/schema/codegen/type_analyzer.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ class TypeInfo:
5757
kind: TypeKind
5858
is_optional: bool = False
5959
list_depth: int = 0
60+
newtype_outer_list_depth: int = 0
6061
is_dict: bool = False
6162
dict_key_type: TypeInfo | None = None
6263
dict_value_type: TypeInfo | None = None
@@ -113,10 +114,13 @@ class _UnwrapState:
113114
as the resolved ``base_type`` for the terminal type.
114115
- ``last_newtype_ref``: the most recently entered NewType callable,
115116
used as constraint provenance (which NewType contributed each constraint).
117+
- ``newtype_outer_list_depth``: list layers accumulated before entering
118+
the outermost NewType boundary.
116119
"""
117120

118121
is_optional: bool = False
119122
list_depth: int = 0
123+
newtype_outer_list_depth: int = 0
120124
is_dict: bool = False
121125
dict_key_type: TypeInfo | None = None
122126
dict_value_type: TypeInfo | None = None
@@ -146,6 +150,7 @@ def build_type_info(
146150
kind=kind,
147151
is_optional=self.is_optional,
148152
list_depth=self.list_depth,
153+
newtype_outer_list_depth=self.newtype_outer_list_depth,
149154
is_dict=self.is_dict,
150155
dict_key_type=self.dict_key_type,
151156
dict_value_type=self.dict_value_type,
@@ -176,6 +181,7 @@ def analyze_type(annotation: object) -> TypeInfo:
176181
state.last_newtype_name = name
177182
state.last_newtype_ref = annotation
178183
if state.outermost_newtype_name is None:
184+
state.newtype_outer_list_depth = state.list_depth
179185
state.outermost_newtype_name = name
180186
state.outermost_newtype_ref = annotation
181187
annotation = annotation.__supertype__ # type: ignore[attr-defined]

packages/overture-schema-codegen/tests/test_markdown_type_format.py

Lines changed: 77 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -29,38 +29,33 @@ class TestFormatType:
2929

3030
def test_plain_str_renders_as_string(self) -> None:
3131
ti = analyze_type(str)
32-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
33-
assert format_type(field) == "`string`"
32+
assert format_type(_make_field(ti)) == "`string`"
3433

3534
def test_optional_adds_qualifier(self) -> None:
3635
ti = analyze_type(str | None)
37-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=False)
38-
assert format_type(field) == "`string` (optional)"
36+
assert format_type(_make_field(ti, is_required=False)) == "`string` (optional)"
3937

4038
def test_literal_renders_as_quoted_value(self) -> None:
4139
ti = analyze_type(Literal["places"])
42-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
43-
assert format_type(field) == '`"places"`'
40+
assert format_type(_make_field(ti)) == '`"places"`'
4441

4542
def test_multi_value_literal_renders_comma_separated(self) -> None:
4643
ti = analyze_type(Literal["a", "b", "c"])
47-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
48-
assert format_type(field) == '`"a"` \\| `"b"` \\| `"c"`'
44+
assert format_type(_make_field(ti)) == '`"a"` \\| `"b"` \\| `"c"`'
4945

5046
def test_enum_without_context_renders_as_code(self) -> None:
5147
class Color(str, Enum):
5248
RED = "red"
5349

5450
ti = analyze_type(Color)
55-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
56-
assert format_type(field) == "`Color`"
51+
assert format_type(_make_field(ti)) == "`Color`"
5752

5853
def test_enum_with_link_context(self) -> None:
5954
class Color(str, Enum):
6055
RED = "red"
6156

6257
ti = analyze_type(Color)
63-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
58+
field = _make_field(ti)
6459
ctx = LinkContext(
6560
page_path=PurePosixPath("buildings/building/building.md"),
6661
registry={
@@ -71,18 +66,15 @@ class Color(str, Enum):
7166

7267
def test_list_of_primitives(self) -> None:
7368
ti = analyze_type(list[str])
74-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
75-
assert format_type(field) == "`list<string>`"
69+
assert format_type(_make_field(ti)) == "`list<string>`"
7670

7771
def test_nested_list_of_primitives(self) -> None:
7872
ti = analyze_type(list[list[str]])
79-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
80-
assert format_type(field) == "`list<list<string>>`"
73+
assert format_type(_make_field(ti)) == "`list<list<string>>`"
8174

8275
def test_registered_primitive_not_linked(self) -> None:
8376
ti = analyze_type(int32)
84-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
85-
result = format_type(field)
77+
result = format_type(_make_field(ti))
8678
assert result == "`int32`"
8779
assert "](int32.md)" not in result
8880

@@ -102,17 +94,19 @@ def test_dict_with_newtype_shows_semantic_name(self) -> None:
10294
assert result == "map<MyKey, int64>"
10395

10496

105-
def _make_union_field(ti: TypeInfo, *, is_required: bool = True) -> FieldSpec:
106-
"""Build a FieldSpec wrapping a union TypeInfo for test convenience."""
107-
return FieldSpec(name="x", type_info=ti, description=None, is_required=is_required)
97+
def _make_field(
98+
ti: TypeInfo, *, name: str = "x", is_required: bool = True
99+
) -> FieldSpec:
100+
"""Build a FieldSpec for test convenience."""
101+
return FieldSpec(name=name, type_info=ti, description=None, is_required=is_required)
108102

109103

110104
class TestFormatUnionType:
111105
"""Tests for UNION-kind TypeInfo in format_type."""
112106

113107
def test_union_renders_all_members(self) -> None:
114108
ti = analyze_type(_ModelA | _ModelB)
115-
result = format_type(_make_union_field(ti))
109+
result = format_type(_make_field(ti))
116110
assert "`_ModelA`" in result
117111
assert "`_ModelB`" in result
118112
# Pipe separator escaped for table cells
@@ -131,13 +125,13 @@ def test_union_with_link_context_links_each_member(self) -> None:
131125
),
132126
},
133127
)
134-
result = format_type(_make_union_field(ti), ctx)
128+
result = format_type(_make_field(ti), ctx)
135129
assert "[`_ModelA`](types/model_a.md)" in result
136130
assert "[`_ModelB`](types/model_b.md)" in result
137131

138132
def test_optional_union_adds_qualifier(self) -> None:
139133
ti = analyze_type(_ModelA | _ModelB | None)
140-
result = format_type(_make_union_field(ti, is_required=False))
134+
result = format_type(_make_field(ti, is_required=False))
141135
assert "(optional)" in result
142136
assert "`_ModelA`" in result
143137
assert "`_ModelB`" in result
@@ -149,14 +143,14 @@ def test_list_of_union_adds_qualifier(self) -> None:
149143
list_depth=1,
150144
union_members=(_ModelA, _ModelB),
151145
)
152-
result = format_type(_make_union_field(ti))
146+
result = format_type(_make_field(ti))
153147
assert "(list)" in result
154148
assert "`_ModelA`" in result
155149
assert "`_ModelB`" in result
156150

157151
def test_union_members_unlinked_without_context(self) -> None:
158152
ti = analyze_type(_ModelA | _ModelB)
159-
result = format_type(_make_union_field(ti))
153+
result = format_type(_make_field(ti))
160154
# No markdown links without context
161155
assert "]()" not in result
162156
assert "[`" not in result
@@ -172,7 +166,7 @@ def test_union_partial_links(self) -> None:
172166
)
173167
},
174168
)
175-
result = format_type(_make_union_field(ti), ctx)
169+
result = format_type(_make_field(ti), ctx)
176170
assert "[`_ModelA`](types/model_a.md)" in result
177171
assert "`_ModelB`" in result
178172
# _ModelB should NOT be linked
@@ -184,7 +178,7 @@ class TestPydanticTypeLinking:
184178

185179
def test_pydantic_type_linked_when_in_registry(self) -> None:
186180
ti = analyze_type(HttpUrl)
187-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
181+
field = _make_field(ti)
188182
ctx = LinkContext(
189183
page_path=PurePosixPath("places/place/place.md"),
190184
registry={
@@ -199,7 +193,7 @@ def test_pydantic_type_linked_when_in_registry(self) -> None:
199193

200194
def test_pydantic_type_unlinked_without_registry_entry(self) -> None:
201195
ti = analyze_type(HttpUrl)
202-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
196+
field = _make_field(ti)
203197
ctx = LinkContext(
204198
page_path=PurePosixPath("places/place/place.md"),
205199
registry={},
@@ -210,7 +204,7 @@ def test_pydantic_type_unlinked_without_registry_entry(self) -> None:
210204

211205
def test_list_of_pydantic_type_linked(self) -> None:
212206
ti = analyze_type(list[HttpUrl])
213-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
207+
field = _make_field(ti)
214208
ctx = LinkContext(
215209
page_path=PurePosixPath("places/place/place.md"),
216210
registry={
@@ -226,7 +220,7 @@ def test_list_of_pydantic_type_linked(self) -> None:
226220
def test_registered_primitive_links_to_aggregate_page(self) -> None:
227221
"""int32 links to the primitives aggregate page when in registry."""
228222
ti = analyze_type(int32)
229-
field = FieldSpec(name="x", type_info=ti, description=None, is_required=True)
223+
field = _make_field(ti)
230224
ctx = LinkContext(
231225
page_path=PurePosixPath("places/place/place.md"),
232226
registry={
@@ -240,6 +234,59 @@ def test_registered_primitive_links_to_aggregate_page(self) -> None:
240234
assert "system/primitive/primitives.md" in result
241235

242236

237+
class TestListOfSemanticNewtype:
238+
"""Tests for list[SemanticNewType] rendering.
239+
240+
When a scalar NewType appears inside list[], the type renders as
241+
list<NewTypeName> rather than NewTypeName (list). The (list) qualifier
242+
is reserved for NewTypes that internally wrap a list.
243+
"""
244+
245+
def test_list_of_scalar_newtype_renders_list_syntax(self) -> None:
246+
"""list[ScalarNewType] renders as list<Name>, not Name (list)."""
247+
ScalarNT = NewType("ScalarNT", str)
248+
ti = analyze_type(list[ScalarNT])
249+
result = format_type(_make_field(ti))
250+
assert "list<" in result
251+
assert "ScalarNT" in result
252+
assert "(list)" not in result
253+
254+
def test_newtype_wrapping_list_renders_qualifier(self) -> None:
255+
"""NewType wrapping list[X] renders as Name (list)."""
256+
ListNT = NewType("ListNT", list[str])
257+
ti = analyze_type(ListNT)
258+
result = format_type(_make_field(ti))
259+
assert "(list)" in result
260+
assert "ListNT" in result
261+
262+
def test_list_of_scalar_newtype_with_link(self) -> None:
263+
"""list[ScalarNewType] with link context renders linked list<Name>."""
264+
ScalarNT = NewType("ScalarNT", str)
265+
ti = analyze_type(list[ScalarNT])
266+
field = _make_field(ti)
267+
ctx = LinkContext(
268+
page_path=PurePosixPath("places/place/place.md"),
269+
registry={
270+
TypeIdentity(ScalarNT, "ScalarNT"): PurePosixPath("system/scalar_nt.md")
271+
},
272+
)
273+
result = format_type(field, ctx)
274+
assert "list<" in result
275+
assert "ScalarNT" in result
276+
assert "system/scalar_nt.md" in result
277+
assert "(list)" not in result
278+
279+
def test_nested_list_of_scalar_newtype_renders_nested_list_syntax(self) -> None:
280+
"""list[list[ScalarNewType]] renders as list<list<Name>>."""
281+
ScalarNT = NewType("ScalarNT", str)
282+
ti = analyze_type(list[list[ScalarNT]])
283+
result = format_type(_make_field(ti))
284+
assert "list<" in result
285+
assert "list<`" in result or "`list<list<" in result
286+
assert "ScalarNT" in result
287+
assert "(list)" not in result
288+
289+
243290
class TestFormatUnderlyingUnionType:
244291
"""Tests for UNION-kind TypeInfo in format_underlying_type."""
245292

0 commit comments

Comments
 (0)