From f00eed6f9a209722402f7bdd16fb6be4e2ed6d60 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 27 Sep 2026 16:54:32 -0400 Subject: [PATCH 1/3] Add native Python writer Render annotations and deterministic imports in memory. Verify emitted modules and resolved annotations. --- designs/codegen/index.md | 1 + designs/codegen/writer.md | 98 +++++ .../smithy-python-feature-writer.json | 4 + .../smithy-python/src/smithy_python/writer.py | 197 +++++++++++ .../smithy-python/tests/unit/test_writer.py | 334 ++++++++++++++++++ 5 files changed, 634 insertions(+) create mode 100644 designs/codegen/writer.md create mode 100644 packages/smithy-python/.changes/next-release/smithy-python-feature-writer.json create mode 100644 packages/smithy-python/src/smithy_python/writer.py create mode 100644 packages/smithy-python/tests/unit/test_writer.py diff --git a/designs/codegen/index.md b/designs/codegen/index.md index eaf0fe73d..1cc06e48e 100644 --- a/designs/codegen/index.md +++ b/designs/codegen/index.md @@ -63,3 +63,4 @@ behavior of generated packages. * [Code Generator CLI](cli.md) * [Service and Data-Shape Selection](selection.md) * [Native Python Symbols](symbols.md) +* [Native Python Writer](writer.md) diff --git a/designs/codegen/writer.md b/designs/codegen/writer.md new file mode 100644 index 000000000..c93bb41c0 --- /dev/null +++ b/designs/codegen/writer.md @@ -0,0 +1,98 @@ +# Native Python writer + +`PythonWriter` turns lines of Python text and `TypeReference` values into the +source of one module. Pass references from `SymbolProvider.type_reference()` +or construct them directly. The writer chooses annotation spellings and imports. +The caller decides which declarations, fields and defaults to write. + +```python +from smithy_python.symbols import TypeReference +from smithy_python.writer import PythonWriter + +node = TypeReference("Node", "example.models", nullable=True) +writer = PythonWriter( + "example.models", declarations={"Node"}, local_names={"children", "amount"} +) +writer.line("class Node:") +with writer.indent(): + writer.line("children: ", TypeReference("list", "builtins", (node,))) + writer.line("amount: ", TypeReference("Decimal", "decimal")) +source = writer.render() +``` + +The result is: + +```python +from __future__ import annotations + +from decimal import Decimal + + +class Node: + children: list[Node | None] + amount: Decimal +``` + +## Writing a module + +`PythonWriter(module, *, declarations=(), local_names=())` takes the destination +module name and the names the caller will use. Supply valid Python names. +`declarations` contains module-level names. Duplicates raise `CodegenError`. +`local_names` contains field and local names from across the module. Repeated +local names are allowed because different classes can have the same field name. +The symbol provider already checks for duplicate fields within a declaration. + +* `line(*parts)` joins strings and type references without separators at the + current indentation. `line()` writes a blank line without spaces. +* `indent()` adds four spaces inside a context manager and restores indentation + even when the block raises an exception. +* `render()` returns source ending in a newline, with a future-annotations + header, sorted imports and the body in written order. It does not modify the + writer. Adding another reference can change aliases in the next render. + +Raw text is trusted Python, not a template. The writer does not parse it to +find missing name reservations. Callers supply blank lines between declarations. + +## Annotations and imports + +Nested arguments retain their order. `nullable=True` adds `| None` only at that +level: `list[str | None]` differs from `list[str] | None`. The `None` literal +stays `None`, including when marked nullable. Only that literal can omit its +module. Other module-less references raise `CodegenError`. + +Generated source targets Python 3.12+. The future import permits references to +classes defined later, including recursive types. These are annotations, not +expressions to evaluate while defining classes. + +* Builtins normally use `str`, `int`, `list[T]` and `dict[K, V]`. +* Types in the current module use their bare names without imports. +* Other types normally use `from module import Name`. + +Imports are deduplicated by module and name, then sorted by module, name and +alias. Referenced packages don't have to be installed in the generator's environment. +For example, rendering a reference to `smithy_core.documents.Document` does not +import `smithy_core`. + +## Names that collide + +An import matching a declaration, local name, builtin, the future import's +`annotations` name, or another imported type uses a module-derived alias. +For example, `decimal.Decimal` becomes `_decimal_Decimal`. Dots in a module +path become underscores. All imports sharing a short name receive aliases, +regardless of the order they were written. + +A field named `list` makes builtin list references use `_builtins.list[T]`, with +`import builtins as _builtins`. An alias that still collides raises +`CodegenError`. The writer never adds numbered suffixes or renames declarations. +The builtin-name list is fixed to Python 3.12, independent of the host version. + +Comparisons follow Python's treatment of identifier spellings. For example, +`K` and `K` bind the same name and cannot be separate declarations. Original +spellings, including `_2HTTPServer`, are retained in emitted source. + +A same-module reference also listed in `local_names` raises `CodegenError`. +This first version does not add self-imports or aliases for local declarations. +Listing it only in `declarations` is normal. + +The writer returns source in memory. File output, documentation conversion and +actual declaration generation remain separate follow-ups. diff --git a/packages/smithy-python/.changes/next-release/smithy-python-feature-writer.json b/packages/smithy-python/.changes/next-release/smithy-python-feature-writer.json new file mode 100644 index 000000000..77f0e4e09 --- /dev/null +++ b/packages/smithy-python/.changes/next-release/smithy-python-feature-writer.json @@ -0,0 +1,4 @@ +{ + "type": "feature", + "description": "Added a native Python writer with structured annotations, deterministic collision-safe imports, indentation contexts, and forward-reference support." +} diff --git a/packages/smithy-python/src/smithy_python/writer.py b/packages/smithy-python/src/smithy_python/writer.py new file mode 100644 index 000000000..2d4aca3fa --- /dev/null +++ b/packages/smithy-python/src/smithy_python/writer.py @@ -0,0 +1,197 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Structured annotation writing for a single Python module.""" + +from __future__ import annotations + +from collections import Counter +from collections.abc import Generator, Iterable +from contextlib import contextmanager +from unicodedata import normalize + +from .exceptions import CodegenError +from .symbols import TypeReference + +# Python 3.12 builtins, fixed so planning does not depend on the generator host. +_BUILTINS = frozenset( + "ArithmeticError AssertionError AttributeError BaseException BaseExceptionGroup " + "BlockingIOError BrokenPipeError BufferError BytesWarning ChildProcessError " + "ConnectionAbortedError ConnectionError ConnectionRefusedError ConnectionResetError " + "DeprecationWarning EOFError Ellipsis EncodingWarning EnvironmentError Exception " + "ExceptionGroup False FileExistsError FileNotFoundError FloatingPointError " + "FutureWarning GeneratorExit IOError ImportError ImportWarning IndentationError " + "IndexError InterruptedError IsADirectoryError KeyError KeyboardInterrupt " + "LookupError MemoryError ModuleNotFoundError NameError None NotADirectoryError " + "NotImplemented NotImplementedError OSError OverflowError PendingDeprecationWarning " + "PermissionError ProcessLookupError RecursionError ReferenceError ResourceWarning " + "RuntimeError RuntimeWarning StopAsyncIteration StopIteration SyntaxError " + "SyntaxWarning SystemError SystemExit TabError TimeoutError True TypeError " + "UnboundLocalError UnicodeDecodeError UnicodeEncodeError UnicodeError " + "UnicodeTranslateError UnicodeWarning UserWarning ValueError Warning WindowsError " + "ZeroDivisionError __build_class__ __debug__ __doc__ __import__ __loader__ " + "__name__ __package__ __spec__ abs aiter all anext any ascii bin bool breakpoint " + "bytearray bytes callable chr classmethod compile complex copyright credits " + "delattr dict dir divmod enumerate eval exec exit filter float format frozenset " + "getattr globals hasattr hash help hex id input int isinstance issubclass iter " + "len license list locals map max memoryview min next object oct open ord pow " + "print property quit range repr reversed round set setattr slice sorted " + "staticmethod str sum super tuple type vars zip".split() +) + + +def _binding(name: str) -> str: + """Compare names as Python binds them, without changing emitted spelling.""" + return normalize("NFKC", name) + + +class PythonWriter: + """Write trusted Python lines, retaining type references until rendering.""" + + def __init__( + self, + module: str, + *, + declarations: Iterable[str] = (), + local_names: Iterable[str] = (), + ) -> None: + self._module = module + self._declarations: set[str] = set() + for name in declarations: + binding = _binding(name) + if binding in self._declarations: + raise CodegenError(f"Duplicate generated declaration: {name!r}") + self._declarations.add(binding) + self._local_names = frozenset(_binding(name) for name in local_names) + self._depth = 0 + self._lines: list[tuple[int, tuple[str | TypeReference, ...]]] = [] + + def line(self, *parts: str | TypeReference) -> None: + """Append one line, concatenating its parts without separators.""" + self._lines.append((self._depth, parts)) + + @contextmanager + def indent(self) -> Generator[None]: + """Indent by four spaces for the duration of the block.""" + self._depth += 1 + try: + yield + finally: + self._depth -= 1 + + def _plan_imports(self) -> tuple[dict[tuple[str | None, str], str], list[str]]: + identities: set[tuple[str | None, str]] = set() + pending = [ + part + for _, parts in self._lines + for part in parts + if isinstance(part, TypeReference) + ] + while pending: + ref = pending.pop() + if ref.module is None and (ref.name != "None" or ref.arguments): + raise CodegenError( + f"Only the None literal may omit its module: {ref.name!r}" + ) + identities.add((ref.module, ref.name)) + pending.extend(ref.arguments) + current_names = { + _binding(name) for module, name in identities if module == self._module + } + for name in sorted(current_names & self._local_names): + raise CodegenError( + f"Same-module reference {self._module}.{name} conflicts with local name {name!r}" + ) + reserved = self._declarations | self._local_names | current_names + external = sorted( + (module, name) + for module, name in identities + if module is not None and module not in ("builtins", self._module) + ) + counts = Counter(_binding(name) for _, name in external) + names: dict[tuple[str | None, str], str] = {} + imports: list[tuple[str, str, str, str]] = [] + for module, name in external: + alias = ( + f"_{module.replace('.', '_')}_{name}" + if _binding(name) in reserved + or _binding(name) in _BUILTINS + or _binding(name) == "annotations" + or counts[_binding(name)] > 1 + else name + ) + names[(module, name)] = alias + suffix = f" as {alias}" if alias != name else "" + imports.append( + (module, name, alias, f"from {module} import {name}{suffix}") + ) + qualify_builtins = False + for module, name in identities: + if module is None: + names[(module, name)] = "None" + elif module == "builtins": + shadowed = _binding(name) in reserved + names[(module, name)] = f"_builtins.{name}" if shadowed else name + qualify_builtins |= shadowed + elif module == self._module: + names[(module, name)] = name + if qualify_builtins: + imports.append( + ("builtins", "", "_builtins", "import builtins as _builtins") + ) + + # Check the complete plan, including unaliased imports. No binding gets + # priority merely because its reference was encountered first. + bindings = dict.fromkeys(reserved, "a generated declaration or local name") + imports.sort() + for module, name, alias, _ in imports: + owner = f"{module}.{name}" if name else module + binding = _binding(alias) + if binding in bindings or binding in _BUILTINS: + conflict = bindings.get(binding, "a builtin") + raise CodegenError( + f"Import binding {alias!r} for {owner} conflicts with {conflict}" + ) + bindings[binding] = owner + return names, [statement for _, _, _, statement in imports] + + @staticmethod + def _annotation( + reference: TypeReference, names: dict[tuple[str | None, str], str] + ) -> str: + # Emit tokens rather than recursing through potentially deep collections. + pending: list[str | TypeReference] = [reference] + result: list[str] = [] + while pending: + part = pending.pop() + if isinstance(part, str): + result.append(part) + continue + if part.module is None: + result.append("None") + continue + result.append(names[(part.module, part.name)]) + if part.nullable: + pending.append(" | None") + if part.arguments: + pending.append("]") + for index in range(len(part.arguments) - 1, -1, -1): + pending.append(part.arguments[index]) + if index: + pending.append(", ") + pending.append("[") + return "".join(result) + + def render(self) -> str: + """Return complete source without changing the recorded lines.""" + names, imports = self._plan_imports() + header = "from __future__ import annotations\n" + if imports: + header += "\n" + "\n".join(imports) + "\n" + body: list[str] = [] + for depth, parts in self._lines: + text = "".join( + part if isinstance(part, str) else self._annotation(part, names) + for part in parts + ) + body.append(" " * depth + text if text else "") + return header + ("\n\n" + "\n".join(body) + "\n" if body else "") diff --git a/packages/smithy-python/tests/unit/test_writer.py b/packages/smithy-python/tests/unit/test_writer.py new file mode 100644 index 000000000..57d41df66 --- /dev/null +++ b/packages/smithy-python/tests/unit/test_writer.py @@ -0,0 +1,334 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +import importlib.util +import sys +from decimal import Decimal +from pathlib import Path +from types import ModuleType +from typing import get_type_hints + +import pytest +from smithy_python.symbols import TypeReference +from smithy_python.writer import PythonWriter + + +def load_written_module( + writer: PythonWriter, directory: Path, monkeypatch: pytest.MonkeyPatch +) -> ModuleType: + path = directory / "written_models.py" + path.write_text(writer.render(), encoding="utf-8") + spec = importlib.util.spec_from_file_location("written_models", path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + monkeypatch.setitem(sys.modules, spec.name, module) + spec.loader.exec_module(module) + return module + + +def test_nested_forward_and_mutual_annotations( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + writer = PythonWriter("written_models", declarations=("Node", "_2HTTPServer")) + writer.line("class Node:") + with writer.indent(): + writer.line( + "children: ", + TypeReference( + "dict", + "builtins", + ( + TypeReference("str", "builtins"), + TypeReference( + "list", + "builtins", + (TypeReference("Node", "written_models", nullable=True),), + nullable=True, + ), + ), + nullable=True, + ), + ) + writer.line("server: ", TypeReference("_2HTTPServer", "written_models")) + writer.line("amount: ", TypeReference("Decimal", "decimal")) + writer.line("nothing: ", TypeReference("None")) + writer.line() + writer.line("class _2HTTPServer:") + with writer.indent(): + writer.line("node: ", TypeReference("Node", "written_models")) + source = writer.render() + assert source.startswith("from __future__ import annotations\n") + assert "from written_models" not in source + assert "dict[str, list[Node | None] | None] | None" in source + assert source == writer.render() + assert source.endswith("\n") + module = load_written_module(writer, tmp_path, monkeypatch) + node = module.Node + server = module._2HTTPServer + assert get_type_hints(node) == { + "children": dict[str, list[node | None] | None] | None, + "server": server, + "amount": Decimal, + "nothing": type(None), + } + assert get_type_hints(server) == {"node": node} + + +def test_indentation_restored_after_exception() -> None: + writer = PythonWriter("models") + with pytest.raises(RuntimeError), writer.indent(): + writer.line("first") + with writer.indent(): + writer.line("second") + writer.line() + raise RuntimeError + writer.line("last") + assert writer.render().endswith(" first\n second\n\nlast\n") + + +@pytest.mark.parametrize("scope", ["declaration", "local"]) +@pytest.mark.parametrize( + "reserved", + [ + ("Decimal", "list", "int"), + ( + "\uff24\uff45\uff43\uff49\uff4d\uff41\uff4c", + "\uff4c\uff49\uff53\uff54", + "\uff49\uff4e\uff54", + ), + ], +) +def test_shadowed_references_load( + scope: str, + reserved: tuple[str, ...], + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + writer = PythonWriter( + "written_models", + declarations=reserved if scope == "declaration" else ("Node",), + local_names=reserved if scope == "local" else (), + ) + if scope == "declaration": + for name in reserved: + writer.line(name, " = 'shadow'") + writer.line("class Node:") + with writer.indent(): + if scope == "local": + for name in reserved: + writer.line(name, " = 'shadow'") + writer.line("amount: ", TypeReference("Decimal", "decimal")) + writer.line( + "items: ", + TypeReference("list", "builtins", (TypeReference("int", "builtins"),)), + ) + source = writer.render() + assert "import builtins as _builtins" in source + assert "from decimal import Decimal as _decimal_Decimal" in source + module = load_written_module(writer, tmp_path, monkeypatch) + node: type = module.Node + assert get_type_hints(node, vars(module), dict(vars(node))) == { + "amount": Decimal, + "items": list[int], + } + + +def test_all_colliding_imports_are_aliased() -> None: + for modules in (("alpha.models", "beta.models"), ("beta.models", "alpha.models")): + writer = PythonWriter("models") + for module in modules: + writer.line("value: ", TypeReference("Thing", module)) + source = writer.render() + assert "from alpha.models import Thing as _alpha_models_Thing" in source + assert "from beta.models import Thing as _beta_models_Thing" in source + assert source.index("from alpha") < source.index("from beta") + + +@pytest.mark.parametrize("name", ["str", "list", "open", "Exception"]) +def test_external_builtin_name_is_aliased(name: str) -> None: + writer = PythonWriter("models") + writer.line("value: ", TypeReference(name, "external")) + assert f"from external import {name} as _external_{name}" in writer.render() + + +@pytest.mark.parametrize( + "declarations,locals_,refs,conflict", + [ + ( + ("Decimal", "_decimal_Decimal"), + (), + (TypeReference("Decimal", "decimal"),), + "_decimal_Decimal", + ), + ((), ("list", "_builtins"), (TypeReference("list", "builtins"),), "_builtins"), + (("Node",), ("Node",), (TypeReference("Node", "models"),), "Node"), + ( + (), + (), + (TypeReference("Thing", "a.b"), TypeReference("Thing", "a_b")), + "_a_b_Thing", + ), + ( + ("Decimal",), + (), + ( + TypeReference("Decimal", "decimal"), + TypeReference("_decimal_Decimal", "other"), + ), + "_decimal_Decimal", + ), + ( + (), + ("list",), + (TypeReference("list", "builtins"), TypeReference("_builtins", "other")), + "_builtins", + ), + (("Node", "Node"), (), (), "Node"), + (("K", "\u212a"), (), (), "\u212a"), + ( + ("Decimal", "_\uff44\uff45\uff43\uff49\uff4d\uff41\uff4c_Decimal"), + (), + (TypeReference("Decimal", "decimal"),), + "_decimal_Decimal", + ), + (("K",), ("\u212a",), (TypeReference("K", "models"),), "K"), + ], +) +def test_binding_conflicts( + declarations: tuple[str, ...], + locals_: tuple[str, ...], + refs: tuple[TypeReference, ...], + conflict: str, +) -> None: + from smithy_python.exceptions import CodegenError + + with pytest.raises(CodegenError, match=conflict): + writer = PythonWriter("models", declarations=declarations, local_names=locals_) + for ref in refs: + writer.line("value: ", ref) + writer.render() + + +def test_equivalent_import_names_are_aliased() -> None: + writer = PythonWriter("models") + writer.line("first: ", TypeReference("K", "alpha")) + writer.line("second: ", TypeReference("\u212a", "beta")) + writer.line("text: ", TypeReference("\uff53\uff54\uff52", "external")) + source = writer.render() + assert "from alpha import K as _alpha_K" in source + assert "from beta import \u212a as _beta_\u212a" in source + assert ( + "from external import \uff53\uff54\uff52 as _external_\uff53\uff54\uff52" + in source + ) + + +def test_current_module_reference_reserves_its_binding() -> None: + writer = PythonWriter("models") + writer.line("first: ", TypeReference("Decimal", "models")) + writer.line("second: ", TypeReference("Decimal", "decimal")) + assert "from decimal import Decimal as _decimal_Decimal" in writer.render() + + +def test_nullable_none_is_still_literal( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + writer = PythonWriter("written_models") + writer.line("value: ", TypeReference("None", nullable=True)) + module = load_written_module(writer, tmp_path, monkeypatch) + assert get_type_hints(module) == {"value": type(None)} + + +def test_render_replans_without_changing_previous_lines() -> None: + writer = PythonWriter("models") + writer.line("first: ", TypeReference("Thing", "alpha")) + assert "first: Thing" in writer.render() + writer.line("second: ", TypeReference("Thing", "beta")) + assert "first: _alpha_Thing" in writer.render() + assert "second: _beta_Thing" in writer.render() + assert writer.render() == writer.render() + + +def test_deep_collection_rendering() -> None: + ref = TypeReference("str", "builtins") + for _ in range(1200): + ref = TypeReference("list", "builtins", (ref,)) + writer = PythonWriter("models") + writer.line("value: ", ref) + assert writer.render().endswith( + "value: " + "list[" * 1200 + "str" + "]" * 1200 + "\n" + ) + + +def test_future_import_name_is_reserved() -> None: + writer = PythonWriter("models") + writer.line("value: ", TypeReference("annotations", "external")) + assert ( + "from external import annotations as _external_annotations" in writer.render() + ) + + +@pytest.mark.parametrize( + "reference", + [ + TypeReference("Nope"), + TypeReference("None", arguments=(TypeReference("str", "builtins"),)), + ], +) +def test_only_none_literal_can_omit_module(reference: TypeReference) -> None: + from smithy_python.exceptions import CodegenError + + writer = PythonWriter("models") + writer.line("value: ", reference) + with pytest.raises(CodegenError, match="module"): + writer.render() + + +def test_generation_without_runtime_packages() -> None: + import subprocess + + source_path = Path(__file__).resolve().parents[2] / "src" + program = ( + "import sys, importlib.util\n" + f"sys.path.insert(0, {str(source_path)!r})\n" + "assert importlib.util.find_spec('smithy_core') is None\n" + "from smithy_python.writer import PythonWriter\n" + "from smithy_python.symbols import TypeReference\n" + "writer = PythonWriter('models')\n" + "writer.line('document: ', TypeReference('Document', 'smithy_core.documents'))\n" + "assert 'from smithy_core.documents import Document' in writer.render()\n" + "assert 'smithy_core' not in sys.modules\n" + ) + result = subprocess.run( + [sys.executable, "-I", "-S", "-c", program], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + + +def test_empty_writer_and_trusted_text() -> None: + writer = PythonWriter("models", local_names=("value", "value")) + assert writer.render() == "from __future__ import annotations\n" + writer.line("value = '$T {not_a_template}'") + assert writer.render().endswith("value = '$T {not_a_template}'\n") + + +def test_import_identity_and_order() -> None: + refs = ( + TypeReference("Decimal", "decimal"), + TypeReference("datetime", "datetime"), + TypeReference("Decimal", "decimal", nullable=True), + TypeReference("list", "builtins", (TypeReference("Decimal", "decimal"),)), + ) + prefixes: list[str] = [] + for order in (refs, tuple(reversed(refs))): + writer = PythonWriter("models") + for ref in order: + writer.line("value: ", ref) + prefix = writer.render().split("value:")[0] + assert prefix.count("from decimal import Decimal") == 1 + assert prefix.index("from datetime") < prefix.index("from decimal") + prefixes.append(prefix) + assert prefixes[0] == prefixes[1] From 2d7a5a063920797e8e4972d1f8bdeacaefcf19e9 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 27 Sep 2026 17:44:02 -0400 Subject: [PATCH 2/3] Reserve builtins across supported Python versions Cover Python 3.12 through 3.15 with a fixed builtin-name set. Add regression tests and clarify local-name reservations. --- designs/codegen/writer.md | 10 +++++--- .../smithy-python/src/smithy_python/writer.py | 13 +++++----- .../smithy-python/tests/unit/test_writer.py | 25 +++++++++++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/designs/codegen/writer.md b/designs/codegen/writer.md index c93bb41c0..2f9cfa778 100644 --- a/designs/codegen/writer.md +++ b/designs/codegen/writer.md @@ -38,8 +38,10 @@ class Node: `PythonWriter(module, *, declarations=(), local_names=())` takes the destination module name and the names the caller will use. Supply valid Python names. `declarations` contains module-level names. Duplicates raise `CodegenError`. -`local_names` contains field and local names from across the module. Repeated -local names are allowed because different classes can have the same field name. +`local_names` contains field and local names that can shadow annotations across +the module. Do not automatically include enum constants or other class members +that are not in an annotation's scope. Repeated local names are allowed because +different classes can have the same field name. The symbol provider already checks for duplicate fields within a declaration. * `line(*parts)` joins strings and type references without separators at the @@ -84,7 +86,9 @@ regardless of the order they were written. A field named `list` makes builtin list references use `_builtins.list[T]`, with `import builtins as _builtins`. An alias that still collides raises `CodegenError`. The writer never adds numbered suffixes or renames declarations. -The builtin-name list is fixed to Python 3.12, independent of the host version. +The builtin-name list combines Python 3.12 through 3.15, including +platform-specific names, so imports do not depend on the generator host. +Update the list when adding support for another Python version. Comparisons follow Python's treatment of identifier spellings. For example, `K` and `K` bind the same name and cannot be separate declarations. Original diff --git a/packages/smithy-python/src/smithy_python/writer.py b/packages/smithy-python/src/smithy_python/writer.py index 2d4aca3fa..e7afef4c8 100644 --- a/packages/smithy-python/src/smithy_python/writer.py +++ b/packages/smithy-python/src/smithy_python/writer.py @@ -12,29 +12,30 @@ from .exceptions import CodegenError from .symbols import TypeReference -# Python 3.12 builtins, fixed so planning does not depend on the generator host. +# Union of Python 3.12-3.15 builtins, including platform-specific names. +# Keep this fixed so import planning does not depend on the generator host. _BUILTINS = frozenset( "ArithmeticError AssertionError AttributeError BaseException BaseExceptionGroup " "BlockingIOError BrokenPipeError BufferError BytesWarning ChildProcessError " "ConnectionAbortedError ConnectionError ConnectionRefusedError ConnectionResetError " "DeprecationWarning EOFError Ellipsis EncodingWarning EnvironmentError Exception " "ExceptionGroup False FileExistsError FileNotFoundError FloatingPointError " - "FutureWarning GeneratorExit IOError ImportError ImportWarning IndentationError " + "FutureWarning GeneratorExit IOError ImportCycleError ImportError ImportWarning IndentationError " "IndexError InterruptedError IsADirectoryError KeyError KeyboardInterrupt " "LookupError MemoryError ModuleNotFoundError NameError None NotADirectoryError " "NotImplemented NotImplementedError OSError OverflowError PendingDeprecationWarning " - "PermissionError ProcessLookupError RecursionError ReferenceError ResourceWarning " + "PermissionError ProcessLookupError PythonFinalizationError RecursionError ReferenceError ResourceWarning " "RuntimeError RuntimeWarning StopAsyncIteration StopIteration SyntaxError " "SyntaxWarning SystemError SystemExit TabError TimeoutError True TypeError " "UnboundLocalError UnicodeDecodeError UnicodeEncodeError UnicodeError " "UnicodeTranslateError UnicodeWarning UserWarning ValueError Warning WindowsError " - "ZeroDivisionError __build_class__ __debug__ __doc__ __import__ __loader__ " + "ZeroDivisionError _IncompleteInputError __build_class__ __debug__ __doc__ __import__ __lazy_import__ __loader__ " "__name__ __package__ __spec__ abs aiter all anext any ascii bin bool breakpoint " "bytearray bytes callable chr classmethod compile complex copyright credits " - "delattr dict dir divmod enumerate eval exec exit filter float format frozenset " + "delattr dict dir divmod enumerate eval exec exit filter float format frozendict frozenset " "getattr globals hasattr hash help hex id input int isinstance issubclass iter " "len license list locals map max memoryview min next object oct open ord pow " - "print property quit range repr reversed round set setattr slice sorted " + "print property quit range repr reversed round sentinel set setattr slice sorted " "staticmethod str sum super tuple type vars zip".split() ) diff --git a/packages/smithy-python/tests/unit/test_writer.py b/packages/smithy-python/tests/unit/test_writer.py index 57d41df66..a1006f5e3 100644 --- a/packages/smithy-python/tests/unit/test_writer.py +++ b/packages/smithy-python/tests/unit/test_writer.py @@ -74,6 +74,31 @@ def test_nested_forward_and_mutual_annotations( assert get_type_hints(server) == {"node": node} +@pytest.mark.parametrize( + "name", + [ + "PythonFinalizationError", # Added in Python 3.13. + "_IncompleteInputError", # Added in Python 3.13. + "ImportCycleError", # Added in Python 3.15. + "__lazy_import__", # Added in Python 3.15. + "frozendict", # Added in Python 3.15. + "sentinel", # Added in Python 3.15. + "WindowsError", # Windows-only, present throughout Python 3.12-3.15. + ], +) +def test_imports_reserve_builtins_across_supported_versions( + name: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + external = ModuleType("external") + setattr(external, name, str) + monkeypatch.setitem(sys.modules, "external", external) + writer = PythonWriter("written_models") + writer.line("value: ", TypeReference(name, "external")) + assert f"from external import {name} as _external_{name}" in writer.render() + module = load_written_module(writer, tmp_path, monkeypatch) + assert get_type_hints(module) == {"value": str} + + def test_indentation_restored_after_exception() -> None: writer = PythonWriter("models") with pytest.raises(RuntimeError), writer.indent(): From cd657b94a68fb940fcc68e9c56e3823488755855 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 27 Sep 2026 20:31:05 -0400 Subject: [PATCH 3/3] Handle equivalent Python import names Avoid self-imports and duplicate imports for equivalent Unicode spellings. Add runtime collision and builtin coverage tests. --- designs/codegen/writer.md | 5 +- .../smithy-python/src/smithy_python/writer.py | 33 +++-- .../smithy-python/tests/unit/test_writer.py | 130 +++++++++++++++--- 3 files changed, 140 insertions(+), 28 deletions(-) diff --git a/designs/codegen/writer.md b/designs/codegen/writer.md index 2f9cfa778..e88e7be88 100644 --- a/designs/codegen/writer.md +++ b/designs/codegen/writer.md @@ -92,7 +92,10 @@ Update the list when adding support for another Python version. Comparisons follow Python's treatment of identifier spellings. For example, `K` and `K` bind the same name and cannot be separate declarations. Original -spellings, including `_2HTTPServer`, are retained in emitted source. +spellings, including `_2HTTPServer`, are retained in emitted source. Module names +follow the same comparison rules, so equivalent spellings do not cause self-imports. +Equivalent references share one import, using the lexicographically smallest +supplied module/name pair so the choice does not depend on reference order. A same-module reference also listed in `local_names` raises `CodegenError`. This first version does not add self-imports or aliases for local declarations. diff --git a/packages/smithy-python/src/smithy_python/writer.py b/packages/smithy-python/src/smithy_python/writer.py index e7afef4c8..1710c8cc2 100644 --- a/packages/smithy-python/src/smithy_python/writer.py +++ b/packages/smithy-python/src/smithy_python/writer.py @@ -55,7 +55,7 @@ def __init__( declarations: Iterable[str] = (), local_names: Iterable[str] = (), ) -> None: - self._module = module + self._module = _binding(module) self._declarations: set[str] = set() for name in declarations: binding = _binding(name) @@ -95,8 +95,19 @@ def _plan_imports(self) -> tuple[dict[tuple[str | None, str], str], list[str]]: ) identities.add((ref.module, ref.name)) pending.extend(ref.arguments) + # Equivalent Python identifiers share one import. Pick a supplied spelling + # deterministically, without rewriting the caller's source names. + representatives: dict[tuple[str | None, str], tuple[str | None, str]] = {} + for module, name in sorted( + identities, key=lambda item: (item[0] or "", item[1]) + ): + identity = ( + _binding(module) if module is not None else None, + _binding(name), + ) + representatives.setdefault(identity, (module, name)) current_names = { - _binding(name) for module, name in identities if module == self._module + name for module, name in representatives if module == self._module } for name in sorted(current_names & self._local_names): raise CodegenError( @@ -105,8 +116,8 @@ def _plan_imports(self) -> tuple[dict[tuple[str | None, str], str], list[str]]: reserved = self._declarations | self._local_names | current_names external = sorted( (module, name) - for module, name in identities - if module is not None and module not in ("builtins", self._module) + for (module_binding, _), (module, name) in representatives.items() + if module is not None and module_binding not in ("builtins", self._module) ) counts = Counter(_binding(name) for _, name in external) names: dict[tuple[str | None, str], str] = {} @@ -126,14 +137,14 @@ def _plan_imports(self) -> tuple[dict[tuple[str | None, str], str], list[str]]: (module, name, alias, f"from {module} import {name}{suffix}") ) qualify_builtins = False - for module, name in identities: + for (module_binding, name_binding), (module, name) in representatives.items(): if module is None: names[(module, name)] = "None" - elif module == "builtins": - shadowed = _binding(name) in reserved + elif module_binding == "builtins": + shadowed = name_binding in reserved names[(module, name)] = f"_builtins.{name}" if shadowed else name qualify_builtins |= shadowed - elif module == self._module: + elif module_binding == self._module: names[(module, name)] = name if qualify_builtins: imports.append( @@ -153,6 +164,12 @@ def _plan_imports(self) -> tuple[dict[tuple[str | None, str], str], list[str]]: f"Import binding {alias!r} for {owner} conflicts with {conflict}" ) bindings[binding] = owner + for module, name in identities: + identity = ( + _binding(module) if module is not None else None, + _binding(name), + ) + names[(module, name)] = names[representatives[identity]] return names, [statement for _, _, _, statement in imports] @staticmethod diff --git a/packages/smithy-python/tests/unit/test_writer.py b/packages/smithy-python/tests/unit/test_writer.py index a1006f5e3..6dcf49e8d 100644 --- a/packages/smithy-python/tests/unit/test_writer.py +++ b/packages/smithy-python/tests/unit/test_writer.py @@ -1,7 +1,9 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 +import builtins import importlib.util +import re import sys from decimal import Decimal from pathlib import Path @@ -18,6 +20,8 @@ def load_written_module( ) -> ModuleType: path = directory / "written_models.py" path.write_text(writer.render(), encoding="utf-8") + # Repeated renders can have the same size and filesystem timestamp. + Path(importlib.util.cache_from_source(str(path))).unlink(missing_ok=True) spec = importlib.util.spec_from_file_location("written_models", path) assert spec is not None and spec.loader is not None module = importlib.util.module_from_spec(spec) @@ -158,15 +162,28 @@ def test_shadowed_references_load( } -def test_all_colliding_imports_are_aliased() -> None: +def test_all_colliding_imports_are_aliased( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + alpha = ModuleType("alpha.models") + beta = ModuleType("beta.models") + setattr(alpha, "Thing", str) + setattr(beta, "Thing", int) + monkeypatch.setitem(sys.modules, "alpha.models", alpha) + monkeypatch.setitem(sys.modules, "beta.models", beta) for modules in (("alpha.models", "beta.models"), ("beta.models", "alpha.models")): writer = PythonWriter("models") - for module in modules: - writer.line("value: ", TypeReference("Thing", module)) + for index, module in enumerate(modules): + writer.line(f"value_{index}: ", TypeReference("Thing", module)) source = writer.render() assert "from alpha.models import Thing as _alpha_models_Thing" in source assert "from beta.models import Thing as _beta_models_Thing" in source assert source.index("from alpha") < source.index("from beta") + result = load_written_module(writer, tmp_path, monkeypatch) + assert get_type_hints(result) == { + f"value_{index}": str if module == "alpha.models" else int + for index, module in enumerate(modules) + } @pytest.mark.parametrize("name", ["str", "list", "open", "Exception"]) @@ -177,21 +194,31 @@ def test_external_builtin_name_is_aliased(name: str) -> None: @pytest.mark.parametrize( - "declarations,locals_,refs,conflict", + "declarations,locals_,refs,prefix", [ ( ("Decimal", "_decimal_Decimal"), (), (TypeReference("Decimal", "decimal"),), - "_decimal_Decimal", + "Import binding '_decimal_Decimal' for", + ), + ( + (), + ("list", "_builtins"), + (TypeReference("list", "builtins"),), + "Import binding '_builtins' for", + ), + ( + ("Node",), + ("Node",), + (TypeReference("Node", "models"),), + "Same-module reference models.Node conflicts with local name", ), - ((), ("list", "_builtins"), (TypeReference("list", "builtins"),), "_builtins"), - (("Node",), ("Node",), (TypeReference("Node", "models"),), "Node"), ( (), (), (TypeReference("Thing", "a.b"), TypeReference("Thing", "a_b")), - "_a_b_Thing", + "Import binding '_a_b_Thing' for", ), ( ("Decimal",), @@ -200,37 +227,40 @@ def test_external_builtin_name_is_aliased(name: str) -> None: TypeReference("Decimal", "decimal"), TypeReference("_decimal_Decimal", "other"), ), - "_decimal_Decimal", + "Import binding '_decimal_Decimal' for", ), ( (), ("list",), (TypeReference("list", "builtins"), TypeReference("_builtins", "other")), - "_builtins", + "Import binding '_builtins' for", ), - (("Node", "Node"), (), (), "Node"), - (("K", "\u212a"), (), (), "\u212a"), ( ("Decimal", "_\uff44\uff45\uff43\uff49\uff4d\uff41\uff4c_Decimal"), (), (TypeReference("Decimal", "decimal"),), - "_decimal_Decimal", + "Import binding '_decimal_Decimal' for", + ), + ( + ("K",), + ("\u212a",), + (TypeReference("K", "models"),), + "Same-module reference models.K conflicts with local name", ), - (("K",), ("\u212a",), (TypeReference("K", "models"),), "K"), ], ) def test_binding_conflicts( declarations: tuple[str, ...], locals_: tuple[str, ...], refs: tuple[TypeReference, ...], - conflict: str, + prefix: str, ) -> None: from smithy_python.exceptions import CodegenError - with pytest.raises(CodegenError, match=conflict): - writer = PythonWriter("models", declarations=declarations, local_names=locals_) - for ref in refs: - writer.line("value: ", ref) + writer = PythonWriter("models", declarations=declarations, local_names=locals_) + for ref in refs: + writer.line("value: ", ref) + with pytest.raises(CodegenError, match="^" + re.escape(prefix)): writer.render() @@ -248,6 +278,68 @@ def test_equivalent_import_names_are_aliased() -> None: ) +@pytest.mark.parametrize("names", [("Node", "Node"), ("K", "\u212a")]) +def test_duplicate_declarations_fail_at_construction(names: tuple[str, str]) -> None: + from smithy_python.exceptions import CodegenError + + message = f"Duplicate generated declaration: {names[1]!r}" + with pytest.raises(CodegenError, match="^" + re.escape(message) + "$"): + PythonWriter("models", declarations=names) + + +def test_builtin_names_cover_running_interpreter() -> None: + writer = PythonWriter("models") + for name in dir(builtins): + writer.line("value: ", TypeReference(name, "external")) + source = writer.render() + for name in dir(builtins): + assert f"from external import {name} as _external_{name}\n" in source + + +@pytest.mark.parametrize("module", ["written_models", "\uff57ritten_models"]) +def test_equivalent_current_module_names( + module: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + writer = PythonWriter(module, declarations=("Node",)) + writer.line("class Node:") + with writer.indent(): + writer.line( + "child: ", TypeReference("Node", "\uff57ritten_models", nullable=True) + ) + result = load_written_module(writer, tmp_path, monkeypatch) + assert get_type_hints(result.Node) == {"child": result.Node | None} + + +def test_equivalent_external_identities_are_deduplicated( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + refs = ( + TypeReference("Decimal", "decimal"), + TypeReference("\uff24ecimal", "\uff44ecimal"), + ) + outputs: list[str] = [] + for order in (refs, tuple(reversed(refs))): + writer = PythonWriter("written_models") + for index, ref in enumerate(order): + writer.line(f"value_{index}: ", ref) + source = writer.render() + assert source.count("from decimal import Decimal") == 1 + outputs.append(source) + module = load_written_module(writer, tmp_path, monkeypatch) + assert get_type_hints(module) == {"value_0": Decimal, "value_1": Decimal} + assert outputs[0] == outputs[1] + + +def test_equivalent_builtin_module_name( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + writer = PythonWriter("written_models") + writer.line("value: ", TypeReference("str", "\uff42uiltins")) + assert "import str" not in writer.render() + module = load_written_module(writer, tmp_path, monkeypatch) + assert get_type_hints(module) == {"value": str} + + def test_current_module_reference_reserves_its_binding() -> None: writer = PythonWriter("models") writer.line("first: ", TypeReference("Decimal", "models"))