From cb0ba0daef631975f3b80d4d86863743fe5486ba Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 6 Oct 2026 13:18:59 +0000 Subject: [PATCH 1/4] fix(types): declare the canonical list overrides precisely for every checker Since #1392 made the canonical models real subclasses, ten overriding list fields in `canonical_creative` were annotated `SchemaVariant[...]`. That marker is for adopter overrides and only `adcp.types.mypy_plugin` understands it: with the plugin the fields were `Any`, so `GetProductsResponse.products` lost its `list[Product] | None`; without it, and under pyright and Pylance, it was not a valid type at all, so `for p in response.products` was an error. CI missed it because the repository enables the plugin and did not run pyright. The fields now carry their precise `list[...]` types with a per-line `type: ignore[assignment]` for the intentional invariant override. Both mypy and pyright honour that comment, so the suppression is one line and the read-side type is the real one under either checker, with or without the plugin. `ListCreativesResponse.creatives` needs no suppression at all: the generated field is a covariant `Sequence`. `UpdateMediaBuyRequest.new_packages` regains the generated `min_length=1` the retyped redeclaration had dropped. A strict adopter fixture iterates each of the ten fields and reads an element attribute; `tests/test_canonical_override_field_types.py` grades it with mypy (plugin on and off) and pyright, derives the override set from the runtime models so a new retyped list cannot go uncovered, and refuses `SchemaVariant` in the module. `scripts/run_pyright_type_checks.py` runs pyright over the adopter fixtures as `make typecheck-all` and the CI type-check job now do; eleven fixtures pyright rejects for unrelated, pre-existing reasons are listed there with the diagnostic each one hits, and the list may only shrink. Closes #1416 Co-Authored-By: Claude Fable 5.1 --- .github/workflows/ci.yml | 4 + Makefile | 5 +- docs/types-9-migration.md | 6 + scripts/run_pyright_type_checks.py | 100 ++++++++++++ src/adcp/types/canonical_creative.py | 23 +-- src/adcp/types/variants.py | 7 + tests/test_canonical_override_field_types.py | 145 ++++++++++++++++++ .../canonical_override_field_types.py | 88 +++++++++++ tests/type_checks/creative_asset_binding.py | 12 +- 9 files changed, 373 insertions(+), 17 deletions(-) create mode 100755 scripts/run_pyright_type_checks.py create mode 100644 tests/test_canonical_override_field_types.py create mode 100644 tests/type_checks/canonical_override_field_types.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7c8cf645a..3479be7d2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -141,6 +141,10 @@ jobs: if: matrix.python-version == '3.12' run: mypy --strict tests/type_checks/ examples/reporting_webhook_activity.py examples/reporting_status_notifications.py examples/reporting_destination_writer.py examples/reporting_durable_materializer.py examples/reporting_receipt_ingress.py examples/reporting_production.py + - name: Run adopter type-check suite under pyright + if: matrix.python-version == '3.12' + run: python scripts/run_pyright_type_checks.py + - name: Check versioned typing generation if: matrix.python-version == '3.12' run: | diff --git a/Makefile b/Makefile index 52cbc9cfb..64bf08d2f 100644 --- a/Makefile +++ b/Makefile @@ -44,7 +44,7 @@ typecheck: ## Run type checker (mypy) on source code $(MYPY) src/adcp/ @echo "✓ Type checking passed" -typecheck-all: typecheck test-type-checks check-type-ignore-contract ## Run all type-check contracts +typecheck-all: typecheck test-type-checks test-type-checks-pyright check-type-ignore-contract ## Run all type-check contracts @echo "✓ All type-check contracts passed" test: ## Run test suite with coverage @@ -59,6 +59,9 @@ test-type-checks: ## Run adopter-pattern type-check suite (mypy --strict, zero t $(MYPY) --strict tests/type_checks/ @echo "✓ Adopter type-checks passed" +test-type-checks-pyright: ## Run the adopter-pattern type-check suite under pyright (see scripts/run_pyright_type_checks.py) + $(PYTHON) scripts/run_pyright_type_checks.py + check-type-ignore-contract: ## Fail if adopter type-check fixtures use type: ignore suppressions $(PYTHON) scripts/check_type_ignore_contract.py @echo "✓ Adopter type-check fixtures contain no type: ignore suppressions" diff --git a/docs/types-9-migration.md b/docs/types-9-migration.md index cfc61bd00..96890b2b0 100644 --- a/docs/types-9-migration.md +++ b/docs/types-9-migration.md @@ -285,6 +285,12 @@ deleted and the generated models go back to agreeing with their schema. a `TYPE_CHECKING` block with `init=False`, so a type checker refuses the keyword the runtime refuses instead of offering a constructor argument that raises `ValidationError`. Read them and you get `None`. +* The canonical models' overriding list fields — `GetProductsResponse.products`, + `CreateMediaBuyResponse1.packages`, `ListCreativesResponse.creatives`, + `Product.format_options` and the rest — are declared with their precise + element types (#1416). They read as `list[Product] | None` and so on under + mypy with or without `adcp.types.mypy_plugin`, and under pyright and + Pylance, where the earlier `SchemaVariant[...]` spelling was an error. * A format reference's `agent_url` is carried as the wire string, validated as a URL (#1384). `ref.agent_url` is a `str`, not an `AnyUrl`, so `migrated_…` option IDs derived from a model match those derived from the diff --git a/scripts/run_pyright_type_checks.py b/scripts/run_pyright_type_checks.py new file mode 100755 index 000000000..13545bc94 --- /dev/null +++ b/scripts/run_pyright_type_checks.py @@ -0,0 +1,100 @@ +#!/usr/bin/env python3 +"""Grade the adopter type-check fixtures under pyright as well as mypy (#1416). + +``tests/type_checks/`` is what an adopter's own strict type-check sees of this +SDK. CI graded it with mypy only, and mypy ran with ``adcp.types.mypy_plugin``, +so an annotation only that plugin understands passed CI while every pyright +and Pylance user saw errors. This runs pyright over the same fixtures. + +A fixture pyright cannot grade yet is listed in ``EXCLUDED`` with the reason; +the list may only shrink. Pass ``--all`` to run the excluded fixtures too and +see what is left to fix. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +TYPE_CHECK_DIR = ROOT / "tests" / "type_checks" + +#: Fixtures pyright rejects today for reasons unrelated to what they grade. +#: Each entry names the pyright-only diagnostic so the fix is findable. +EXCLUDED: dict[str, str] = { + "composing_root_subclassing.py": ( + "reportIncompatibleVariableOverride: pyright treats a covariant override " + "of a mutable field as incompatible; mypy admits it" + ), + "cross_class_override_with_schema_variant.py": ( + "SchemaVariant is a mypy-plugin marker with no pyright equivalent " + "(see adcp/types/variants.py)" + ), + "extend_response_with_sequence.py": ( + "reportIncompatibleVariableOverride on the covariant Sequence override" + ), + "media_buy_actions.py": ( + "reportAttributeAccessIssue: pyright does not resolve DurationUnit through " + "the lazy adcp.types facade" + ), + "reporting_buyer_submission_intents.py": ( + "reportArgumentType: ADCPClient against the ReportingReceiptSubmissionClient protocol" + ), + "reporting_durable_materializer.py": ( + "reportAssignmentType: store classes against the reporting store protocols " + "(Coroutine vs CoroutineType return types)" + ), + "reporting_frozen_feed.py": ( + "reportAssignmentType: store classes against the reporting store protocols" + ), + "reporting_inline_storage.py": ( + "reportAssignmentType: store classes against the reporting store protocols" + ), + "reporting_receipt_ingress.py": ( + "reportAssignmentType: store classes against the reporting store protocols" + ), + "targeting_input_field_types.py": ( + "reportAssertTypeFailure: pyright reads GeoCountry where mypy reads str" + ), + "versioned_types.py": ("reportTypedDictNotRequiredAccess on NotRequired TypedDict keys"), +} + + +def fixtures(*, include_excluded: bool) -> list[Path]: + return sorted( + path + for path in TYPE_CHECK_DIR.glob("*.py") + if include_excluded or path.name not in EXCLUDED + ) + + +def main(argv: list[str] | None = None) -> int: + args = sys.argv[1:] if argv is None else argv + include_excluded = "--all" in args + + stale = sorted(name for name in EXCLUDED if not (TYPE_CHECK_DIR / name).exists()) + if stale: + print("run_pyright_type_checks: EXCLUDED names fixtures that no longer exist:") + for name in stale: + print(f" {name}") + return 1 + + selected = fixtures(include_excluded=include_excluded) + command = [ + sys.executable, + "-m", + "pyright", + "--pythonpath", + sys.executable, + *(str(path) for path in selected), + ] + result = subprocess.run(command, cwd=ROOT) + if result.returncode == 0: + skipped = "" if include_excluded else f", {len(EXCLUDED)} excluded" + print(f"✓ pyright passed on {len(selected)} adopter type-check fixtures{skipped}") + return result.returncode + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/adcp/types/canonical_creative.py b/src/adcp/types/canonical_creative.py index 4a6c51d38..0a28211aa 100644 --- a/src/adcp/types/canonical_creative.py +++ b/src/adcp/types/canonical_creative.py @@ -169,7 +169,6 @@ MEDIA_BUY_LEGACY_STATUS_VALUES, unwrap_enum_value, ) -from adcp.types.variants import SchemaVariant def is_canonical_format_kind( @@ -689,7 +688,7 @@ class Placement(_LegacyPlacement, CanonicalBoundaryModel): if TYPE_CHECKING: # the removed field, hidden from the constructor too format_ids: _RemovedFormatIdSequence = Field(default=None, init=False) - format_options: SchemaVariant[list[Format] | None] = Field(default=None, min_length=1) + format_options: list[Format] | None = Field(default=None, min_length=1) # type: ignore[assignment] class Product(_LegacyProduct, CanonicalBoundaryModel): @@ -698,10 +697,10 @@ class Product(_LegacyProduct, CanonicalBoundaryModel): if TYPE_CHECKING: # the removed field, hidden from the constructor too format_ids: _RemovedFormatIds = Field(default=None, init=False) - format_options: SchemaVariant[list[Format]] = Field( + format_options: list[Format] = Field( # type: ignore[assignment] min_length=1, description="Canonical creative formats accepted by this product." ) - placements: SchemaVariant[list[Placement] | None] = Field(default=None, min_length=1) + placements: list[Placement] | None = Field(default=None, min_length=1) # type: ignore[assignment] pricing_options: list[CanonicalPricingOption] = Field(min_length=1) @@ -842,7 +841,7 @@ class DeliveryCreative(_LegacyDeliveryCreative, CanonicalBoundaryModel): if TYPE_CHECKING: # the removed field, hidden from the constructor too format_id: _RemovedFormatId = Field(default=None, init=False) - variants: SchemaVariant[list[_DeliveryCreativeVariant]] = _inherit( + variants: list[_DeliveryCreativeVariant] = _inherit( # type: ignore[assignment] _LegacyDeliveryCreative, "variants" ) @@ -882,7 +881,7 @@ def _validate_format_params(self) -> PackageRequest: class PackageUpdate(_LegacyPackageUpdate, CanonicalBoundaryModel): """Canonical package update; creatives are canonical assets.""" - creatives: SchemaVariant[list[CreativeAsset] | None] = Field(default=None, min_length=1) + creatives: list[CreativeAsset] | None = Field(default=None, min_length=1) # type: ignore[assignment] class Package(_LegacyPackage, CanonicalBoundaryModel): @@ -914,7 +913,7 @@ def _reject_legacy_fields(cls, value: Any) -> Any: class GetProductsResponse(_LegacyGetProductsResponse, CanonicalBoundaryModel): """Canonical discovery response; products are canonical products.""" - products: SchemaVariant[list[Product] | None] = None + products: list[Product] | None = None # type: ignore[assignment] class CreateMediaBuyRequest(_LegacyCreateMediaBuyRequest, CanonicalBoundaryModel): @@ -927,13 +926,15 @@ class UpdateMediaBuyRequest(_LegacyUpdateMediaBuyRequest, CanonicalBoundaryModel """Canonical update request; both package lists are canonical.""" packages: list[PackageUpdate] | None = None - new_packages: SchemaVariant[list[PackageRequest] | None] = None + new_packages: list[PackageRequest] | None = Field( # type: ignore[assignment] + default=None, min_length=1 + ) class CreateMediaBuyResponse1(_LegacyCreateMediaBuyResponse1, CanonicalBoundaryModel): """Canonical create response preserving the 3.x legacy-status normalizer.""" - packages: SchemaVariant[list[Package]] + packages: list[Package] # type: ignore[assignment] @model_validator(mode="before") @classmethod @@ -997,7 +998,7 @@ class UpdateMediaBuyResponse3(_LegacyUpdateMediaBuyResponse3, CanonicalBoundaryM class SyncCreativesRequest(_LegacySyncCreativesRequest, CanonicalBoundaryModel): """Canonical creative sync request; creatives are canonical assets.""" - creatives: SchemaVariant[list[CreativeAsset]] = Field(min_length=1) + creatives: list[CreativeAsset] = Field(min_length=1) # type: ignore[assignment] class ListCreativesRequest(_LegacyListCreativesRequest, CanonicalBoundaryModel): @@ -1020,7 +1021,7 @@ def _reject_legacy_fields(cls, value: Any) -> Any: class ListCreativesResponse(_LegacyListCreativesResponse, CanonicalBoundaryModel): """Canonical creative listing; rows are canonical listed creatives.""" - creatives: SchemaVariant[list[Creative]] + creatives: list[Creative] class MediaBuyPackage(_LegacyMediaBuyPackage, CanonicalBoundaryModel): diff --git a/src/adcp/types/variants.py b/src/adcp/types/variants.py index 5df44264f..16773a90a 100644 --- a/src/adcp/types/variants.py +++ b/src/adcp/types/variants.py @@ -21,6 +21,13 @@ class GetMediaBuyDeliveryResponse(LibraryGetMediaBuyDeliveryResponse): type-check via ``Sequence[T]`` covariance (PR #635) and the marker would obscure that the override is sub-typing, not substitution. +The SDK's own models never use it either. ``adcp.types.canonical_creative`` +retypes generated ``list`` fields to their canonical element classes with +the precise ``list[...]`` annotation and a per-line ``type: ignore``, which +both mypy and pyright honour, so an adopter reads ``list[Product] | None`` +from ``GetProductsResponse.products`` under either checker, with or +without this plugin (#1416). + **Pyright / Pylance**: the bundled mypy plugin doesn't affect pyright. Adopters using Pylance in VSCode will still see the LSP override flagged on ``SchemaVariant[T]`` fields even though their mypy CI diff --git a/tests/test_canonical_override_field_types.py b/tests/test_canonical_override_field_types.py new file mode 100644 index 000000000..ae21a93a7 --- /dev/null +++ b/tests/test_canonical_override_field_types.py @@ -0,0 +1,145 @@ +"""The canonical override fields are precise under every checker (#1416). + +``tests/type_checks/canonical_override_field_types.py`` iterates each canonical +list override and reads an element attribute. CI grades the fixture directory +with mypy and ``adcp.types.mypy_plugin`` on, which is the one configuration +under which the old ``SchemaVariant[...]`` annotations did not error — they +read as ``Any``. These tests grade the fixture under the other two +configurations an adopter actually runs, and pin that every list override in +``canonical_creative`` is one the fixture covers. +""" + +from __future__ import annotations + +import ast +import os +import subprocess +import sys +import types +from pathlib import Path +from typing import Annotated, Union, get_args, get_origin + +import pytest +from pydantic import BaseModel + +from adcp.types import canonical_creative + +ROOT = Path(__file__).resolve().parents[1] +FIXTURE = ROOT / "tests" / "type_checks" / "canonical_override_field_types.py" + + +def _run(command: list[str]) -> subprocess.CompletedProcess[str]: + environment = dict(os.environ, MYPYPATH=str(ROOT / "src")) + return subprocess.run( + command, cwd=ROOT, env=environment, capture_output=True, text=True, timeout=600 + ) + + +def test_fixture_passes_mypy_strict_with_the_plugin() -> None: + result = _run([sys.executable, "-m", "mypy", "--strict", str(FIXTURE)]) + assert result.returncode == 0, result.stdout + result.stderr + + +def test_fixture_passes_mypy_strict_without_the_plugin(tmp_path: Path) -> None: + """An adopter who does not install ``adcp.types.mypy_plugin`` sees real types.""" + config = tmp_path / "mypy.ini" + config.write_text("[mypy]\nstrict = True\n") + result = _run( + [sys.executable, "-m", "mypy", "--config-file", str(config), "--strict", str(FIXTURE)] + ) + assert result.returncode == 0, result.stdout + result.stderr + + +def test_fixture_passes_pyright() -> None: + result = _run([sys.executable, "-m", "pyright", "--pythonpath", sys.executable, str(FIXTURE)]) + assert result.returncode == 0, result.stdout + result.stderr + + +def _list_element(annotation: object) -> object | None: + """The ``T`` of a ``list[T]`` or ``list[T] | None`` annotation, else ``None``. + + ``Annotated`` metadata on ``T`` is stripped so a discriminator re-spelling of + the same union does not read as a retyped element. + """ + if get_origin(annotation) in (Union, types.UnionType): + arms = [arm for arm in get_args(annotation) if arm is not type(None)] + if len(arms) != 1: + return None + annotation = arms[0] + if get_origin(annotation) is not list: + return None + element = get_args(annotation)[0] + while get_origin(element) is Annotated: + element = get_args(element)[0] + return element + + +def _list_overrides() -> set[tuple[str, str]]: + """Every ``(model, field)`` where a canonical model retypes an inherited list.""" + overrides: set[tuple[str, str]] = set() + for name, model in vars(canonical_creative).items(): + if not (isinstance(model, type) and issubclass(model, BaseModel)): + continue + if not issubclass(model, canonical_creative.CanonicalBoundaryModel): + continue + for field in vars(model).get("__annotations__", {}): + if field not in model.model_fields: + continue + base = next( + (b for b in model.__mro__[1:] if field in getattr(b, "model_fields", {})), + None, + ) + if base is None: + continue + own = _list_element(model.model_fields[field].annotation) + inherited = _list_element(base.model_fields[field].annotation) + if own is not None and inherited is not None and own != inherited: + overrides.add((name, field)) + return overrides + + +def _fixture_coverage() -> set[tuple[str, str]]: + """The ``(parameter type, attribute)`` pairs the fixture's ``assert_type`` calls read.""" + covered: set[tuple[str, str]] = set() + for node in ast.parse(FIXTURE.read_text(encoding="utf-8")).body: + if not isinstance(node, ast.FunctionDef): + continue + parameter = node.args.args[0] + assert parameter.annotation is not None + model = ast.unparse(parameter.annotation) + for call in ast.walk(node): + if ( + isinstance(call, ast.Call) + and isinstance(call.func, ast.Name) + and call.func.id == "assert_type" + and isinstance(call.args[0], ast.Attribute) + ): + covered.add((model, call.args[0].attr)) + return covered + + +def test_fixture_covers_every_list_override() -> None: + overrides = _list_overrides() + assert overrides, "no canonical list overrides discovered" + missing = overrides - _fixture_coverage() + assert not missing, f"add these fields to {FIXTURE.name}: {sorted(missing)}" + + +def test_no_canonical_field_is_annotated_schema_variant() -> None: + """``SchemaVariant`` is for adopter overrides; the library declares real types.""" + source = (ROOT / "src" / "adcp" / "types" / "canonical_creative.py").read_text("utf-8") + assert "SchemaVariant" not in source + + +@pytest.mark.parametrize( + ("model", "field"), sorted(_list_overrides()), ids=lambda value: str(value) +) +def test_override_keeps_the_generated_constraints(model: str, field: str) -> None: + """Retyping the element must not silently drop ``min_length`` and friends.""" + canonical = getattr(canonical_creative, model) + base = next(b for b in canonical.__mro__[1:] if field in getattr(b, "model_fields", {})) + inherited = base.model_fields[field] + own = canonical.model_fields[field] + inherited_min = [m for m in inherited.metadata if type(m).__name__ == "MinLen"] + own_min = [m for m in own.metadata if type(m).__name__ == "MinLen"] + assert own_min == inherited_min or (own_min and not inherited_min), (model, field) diff --git a/tests/type_checks/canonical_override_field_types.py b/tests/type_checks/canonical_override_field_types.py new file mode 100644 index 000000000..4703ae264 --- /dev/null +++ b/tests/type_checks/canonical_override_field_types.py @@ -0,0 +1,88 @@ +"""The canonical models' overriding list fields read as precise lists (#1416). + +Each canonical model below redeclares a generated ``list[]`` field +as ``list[]``. Those overrides used to be annotated +``SchemaVariant[...]``, a marker only ``adcp.types.mypy_plugin`` understands: +with the plugin the fields were ``Any``, without it (and under pyright) they +were not valid types at all, so ``for p in response.products`` was an error. + +This fixture iterates every one of those fields and reads an attribute of the +element. It is graded three ways by ``tests/test_canonical_override_field_types.py``: +``mypy --strict`` with the repository configuration (plugin on), ``mypy --strict`` +with a bare configuration (plugin off), and pyright. +""" + +from __future__ import annotations + +from typing_extensions import assert_type + +from adcp.types import ( + CreateMediaBuyResponse1, + Creative, + CreativeAsset, + Format, + GetProductsResponse, + ListCreativesResponse, + Package, + PackageRequest, + PackageUpdate, + Placement, + Product, + SyncCreativesRequest, + UpdateMediaBuyRequest, +) + +# ``adcp.types.DeliveryCreative`` is the generated wire row; the canonical served +# creative that retypes ``variants`` is reached through ``GetCreativeDeliveryResponse`` +# and spelled from its module here, as ``creative_asset_binding.py`` does. +from adcp.types.canonical_creative import DeliveryCreative, _DeliveryCreativeVariant + + +def placement_format_options(placement: Placement) -> list[str]: + assert_type(placement.format_options, list[Format] | None) + return [str(option.format_kind) for option in placement.format_options or []] + + +def product_format_options(product: Product) -> list[str]: + assert_type(product.format_options, list[Format]) + return [str(option.format_kind) for option in product.format_options] + + +def product_placements(product: Product) -> list[str]: + assert_type(product.placements, list[Placement] | None) + return [placement.placement_id for placement in product.placements or []] + + +def delivery_creative_variants(creative: DeliveryCreative) -> list[str]: + assert_type(creative.variants, list[_DeliveryCreativeVariant]) + return [variant.variant_id for variant in creative.variants] + + +def package_update_creatives(update: PackageUpdate) -> list[str]: + assert_type(update.creatives, list[CreativeAsset] | None) + return [asset.creative_id for asset in update.creatives or []] + + +def get_products_response_products(response: GetProductsResponse) -> list[str]: + assert_type(response.products, list[Product] | None) + return [product.product_id for product in response.products or []] + + +def update_media_buy_request_new_packages(request: UpdateMediaBuyRequest) -> list[str]: + assert_type(request.new_packages, list[PackageRequest] | None) + return [package.product_id for package in request.new_packages or []] + + +def create_media_buy_response1_packages(response: CreateMediaBuyResponse1) -> list[str]: + assert_type(response.packages, list[Package]) + return [package.package_id for package in response.packages] + + +def sync_creatives_request_creatives(request: SyncCreativesRequest) -> list[str]: + assert_type(request.creatives, list[CreativeAsset]) + return [asset.creative_id for asset in request.creatives] + + +def list_creatives_response_creatives(response: ListCreativesResponse) -> list[str]: + assert_type(response.creatives, list[Creative]) + return [creative.creative_id for creative in response.creatives] diff --git a/tests/type_checks/creative_asset_binding.py b/tests/type_checks/creative_asset_binding.py index d10c0419a..e1dc541ad 100644 --- a/tests/type_checks/creative_asset_binding.py +++ b/tests/type_checks/creative_asset_binding.py @@ -63,11 +63,13 @@ def check_served_manifest(response: GetCreativeDeliveryResponse) -> None: for creative in response.creatives: # ``variants`` narrows the generated ``list[CreativeVariant]`` to the # tolerant delivery row, which pydantic accepts and mypy's invariant - # list rule does not — so the field is declared ``SchemaVariant`` and - # reads as ``Any``. Name the element type to keep the assertion below - # grading something: the row is module-private because it is a readback - # shape no adopter constructs, which is why the type is spelled here - # rather than imported from ``adcp.types``. + # list rule does not — the library declares the precise list and + # suppresses the override on its side (#1416), so the field reads as + # ``list[_DeliveryCreativeVariant]`` here under mypy and pyright alike. + # The row is module-private because it is a readback shape no adopter + # constructs, which is why the type is spelled here rather than + # imported from ``adcp.types``. + assert_type(creative.variants, list[_DeliveryCreativeVariant]) variants: Sequence[_DeliveryCreativeVariant] = creative.variants for variant in variants: if variant.manifest is not None: From 924aa902b43536fed43bfdbfa1b2a655a2517ba0 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 6 Oct 2026 14:34:06 +0000 Subject: [PATCH 2/4] test(types): pass str as the parametrize id function directly Co-Authored-By: Claude Fable 5.1 --- tests/test_canonical_override_field_types.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_canonical_override_field_types.py b/tests/test_canonical_override_field_types.py index ae21a93a7..940f7a423 100644 --- a/tests/test_canonical_override_field_types.py +++ b/tests/test_canonical_override_field_types.py @@ -131,9 +131,7 @@ def test_no_canonical_field_is_annotated_schema_variant() -> None: assert "SchemaVariant" not in source -@pytest.mark.parametrize( - ("model", "field"), sorted(_list_overrides()), ids=lambda value: str(value) -) +@pytest.mark.parametrize(("model", "field"), sorted(_list_overrides()), ids=str) def test_override_keeps_the_generated_constraints(model: str, field: str) -> None: """Retyping the element must not silently drop ``min_length`` and friends.""" canonical = getattr(canonical_creative, model) From 63142fa87010e9caa8cc23d520229f8af62797b7 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 6 Oct 2026 16:15:52 +0000 Subject: [PATCH 3/4] fix(ci): skip pyright fixtures whose optional dependency is not installed CI's type-check lane installs the dev extra only, so the fixtures that import psycopg or psycopg_pool failed the new pyright gate with reportMissingImports. mypy tolerates exactly those modules through ignore_missing_imports; pyright has no per-module equivalent, so the runner now skips a fixture whose top-level imports the interpreter cannot find and names it in the output. With the [pg] extra installed, as locally, every fixture is graded. Co-Authored-By: Claude Fable 5.1 --- scripts/run_pyright_type_checks.py | 38 +++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/scripts/run_pyright_type_checks.py b/scripts/run_pyright_type_checks.py index 13545bc94..c8e6e0ef6 100755 --- a/scripts/run_pyright_type_checks.py +++ b/scripts/run_pyright_type_checks.py @@ -9,10 +9,18 @@ A fixture pyright cannot grade yet is listed in ``EXCLUDED`` with the reason; the list may only shrink. Pass ``--all`` to run the excluded fixtures too and see what is left to fix. + +A fixture that imports an optional dependency the running interpreter does not +have (the ``[pg]`` extra's ``psycopg``, in CI's type-check lane) is skipped and +named in the output. That is the same tolerance ``pyproject.toml`` gives mypy +through ``ignore_missing_imports`` for those modules; pyright has no per-module +equivalent, and an unresolved import is an error, not an ``Any``. """ from __future__ import annotations +import ast +import importlib.util import subprocess import sys from pathlib import Path @@ -69,6 +77,25 @@ def fixtures(*, include_excluded: bool) -> list[Path]: ) +def missing_imports(path: Path) -> list[str]: + """Top-level packages ``path`` imports that this interpreter cannot find.""" + roots: set[str] = set() + for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"))): + if isinstance(node, ast.Import): + roots.update(alias.name.partition(".")[0] for alias in node.names) + elif isinstance(node, ast.ImportFrom) and node.module and not node.level: + roots.add(node.module.partition(".")[0]) + missing: list[str] = [] + for root in sorted(roots): + try: + found = importlib.util.find_spec(root) is not None + except (ImportError, ValueError): + found = False + if not found: + missing.append(root) + return missing + + def main(argv: list[str] | None = None) -> int: args = sys.argv[1:] if argv is None else argv include_excluded = "--all" in args @@ -80,7 +107,16 @@ def main(argv: list[str] | None = None) -> int: print(f" {name}") return 1 - selected = fixtures(include_excluded=include_excluded) + selected: list[Path] = [] + for path in fixtures(include_excluded=include_excluded): + missing = missing_imports(path) + if missing: + print( + f"run_pyright_type_checks: skipping {path.name}: optional dependency " + f"not installed ({', '.join(missing)})" + ) + continue + selected.append(path) command = [ sys.executable, "-m", From a838d05eb34ec87c20f51315207ea79b3ef7fa3a Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 6 Oct 2026 17:20:52 +0000 Subject: [PATCH 4/4] fix(ci): install the pg extra for the pyright gate instead of skipping fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skipping a fixture whose optional dependency is absent was broader than the mypy tolerance it cited: ignore_missing_imports still grades the rest of the file, while the skip dropped six psycopg fixtures from pyright in the dev-only CI type-check lane, so a pyright-only regression there would ship green. The CI step now installs the pg extra before running pyright, and the runner fails up front — naming the module and the extra — when any selected fixture imports something the interpreter cannot find, so no fixture is ever silently dropped. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/ci.yml | 6 ++++- scripts/run_pyright_type_checks.py | 40 ++++++++++++++++++------------ 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3479be7d2..cbce93d69 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -143,7 +143,11 @@ jobs: - name: Run adopter type-check suite under pyright if: matrix.python-version == '3.12' - run: python scripts/run_pyright_type_checks.py + # The fixtures that import psycopg need the pg extra, which this job's + # dev-only install does not bring; pyright has no ignore_missing_imports. + run: | + uv pip install --system -e ".[pg]" + python scripts/run_pyright_type_checks.py - name: Check versioned typing generation if: matrix.python-version == '3.12' diff --git a/scripts/run_pyright_type_checks.py b/scripts/run_pyright_type_checks.py index c8e6e0ef6..a15268399 100755 --- a/scripts/run_pyright_type_checks.py +++ b/scripts/run_pyright_type_checks.py @@ -10,11 +10,12 @@ the list may only shrink. Pass ``--all`` to run the excluded fixtures too and see what is left to fix. -A fixture that imports an optional dependency the running interpreter does not -have (the ``[pg]`` extra's ``psycopg``, in CI's type-check lane) is skipped and -named in the output. That is the same tolerance ``pyproject.toml`` gives mypy -through ``ignore_missing_imports`` for those modules; pyright has no per-module -equivalent, and an unresolved import is an error, not an ``Any``. +Several fixtures import the ``[pg]`` extra's ``psycopg``. mypy tolerates those +modules being absent through ``ignore_missing_imports``; pyright has no +per-module equivalent, and an unresolved import is an error, not an ``Any``. +So the gate requires the extra rather than skipping the fixture: a fixture +whose top-level imports this interpreter cannot find fails the run up front, +naming the module and the extra to install, and the CI step installs it. """ from __future__ import annotations @@ -78,7 +79,12 @@ def fixtures(*, include_excluded: bool) -> list[Path]: def missing_imports(path: Path) -> list[str]: - """Top-level packages ``path`` imports that this interpreter cannot find.""" + """Top-level packages ``path`` imports that this interpreter cannot find. + + Checked before pyright runs so a missing optional dependency is reported as + what it is — an incomplete environment — instead of as type errors in the + fixture, and so a fixture is never silently dropped from the gate. + """ roots: set[str] = set() for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"))): if isinstance(node, ast.Import): @@ -107,16 +113,18 @@ def main(argv: list[str] | None = None) -> int: print(f" {name}") return 1 - selected: list[Path] = [] - for path in fixtures(include_excluded=include_excluded): - missing = missing_imports(path) - if missing: - print( - f"run_pyright_type_checks: skipping {path.name}: optional dependency " - f"not installed ({', '.join(missing)})" - ) - continue - selected.append(path) + selected = fixtures(include_excluded=include_excluded) + unavailable = {path.name: missing_imports(path) for path in selected} + unavailable = {name: roots for name, roots in unavailable.items() if roots} + if unavailable: + print( + "run_pyright_type_checks: these fixtures import modules this interpreter " + "cannot find; install the optional extras (e.g. `uv sync --all-extras` or " + "`pip install -e '.[dev,pg]'`) so pyright grades every fixture:" + ) + for name, roots in sorted(unavailable.items()): + print(f" {name}: {', '.join(roots)}") + return 1 command = [ sys.executable, "-m",