Skip to content

Fix NotImplementedError on heterogeneous union model fields#747

Open
wbarnha wants to merge 1 commit into
masterfrom
claude/mode-pr-81-review-xe6yhe
Open

Fix NotImplementedError on heterogeneous union model fields#747
wbarnha wants to merge 1 commit into
masterfrom
claude/mode-pr-81-review-xe6yhe

Conversation

@wbarnha

@wbarnha wbarnha commented Jul 23, 2026

Copy link
Copy Markdown
Member

Description

Defining a faust.Record with a heterogeneous union field — one whose members are neither all Model subclasses nor all scalar "literal" types (str/bytes/float/int) — raised NotImplementedError: Union of types ... not supported at class-definition time. Minimal reproduction (from faust-streaming/mode#80):

from faust import Record

class SensitiveInfo(Record):
    data: str | list | dict | None
    meta: dict

Root cause

faust's type-expression compiler (faust/models/typing.py) dispatches unions to UnionNode. UnionNode._maybe_unroll_union collapses two union shapes to a single node — all-Model unions, and all-LITERAL_TYPES unions (which is why Union[str, int, float] already compiles to a bare pass-through). Every other union fell through to UnionNode.build, which unconditionally raised.

That path became reachable for X | Y annotations once mode-streaming began recognizing PEP 604 unions (mode 0.5.3): str | list | dict | None now registers as a union and reaches this node, whereas an unrecognized X | Y previously fell through to a literal pass-through. The exception is raised by faust's compiler, not by mode — this is the faust-side fix for the boundary characterized in faust-streaming/mode#81.

Fix

A heterogeneous union has no unambiguous coercion target, so UnionNode.build now emits a pass-through expression (the already-deserialized value is returned unchanged), mirroring the existing all-literal handling. Optional/None members keep their optional wrapping ((a if a is not None else None)). This restores the pre-0.5.3 behavior and is strictly more permissive than the previous hard failure — no existing test asserted the raise.

Tests

  • TypeExpression-level cases added to the parametrized CASES: Union[str, list, dict]a, and Union[str, list, dict, None](a if a is not None else None).
  • End-to-end Record regression tests for both the typing.Union and PEP 604 (|) spellings, asserting values (including None) round-trip unchanged.

All 108 tests/{functional,unit}/models tests pass locally; isort/black/flake8 clean.

Fixes the crash reported in faust-streaming/mode#80.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BnTZQEXqpedyrFAgwRnbsA


Generated by Claude Code

A Record field annotated with a heterogeneous union -- one whose members
are neither all Model subclasses nor all scalar "literal" types, e.g.
`str | list | dict | None` -- reached `UnionNode.build`, which
unconditionally raised `NotImplementedError` at class-definition time.

This surfaced after mode-streaming began recognizing PEP 604 (`X | Y`)
unions (see faust-streaming/mode#80): such annotations now register as
unions and reach this node, whereas previously an unrecognized `X | Y`
fell through to a bare pass-through. The exception is raised here by
faust's type-expression compiler, not by mode.

There is no unambiguous way to coerce a value into a heterogeneous union,
so build a pass-through expression instead -- mirroring how an all-literal
union such as `Union[str, int, float]` is already handled (it collapses to
a bare pass-through via `_maybe_unroll_union`). This restores the behavior
that existed before the union was recognized and lets these models be
defined again; `Optional`/`None` members keep their existing optional
wrapping.

Add regression tests covering both the `typing.Union` and PEP 604
spellings, at the `TypeExpression` level and end-to-end through `Record`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnTZQEXqpedyrFAgwRnbsA
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.06%. Comparing base (706b502) to head (fb21cf8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #747      +/-   ##
==========================================
+ Coverage   95.98%   96.06%   +0.07%     
==========================================
  Files         103      103              
  Lines       11069    11069              
  Branches     1191     1191              
==========================================
+ Hits        10625    10633       +8     
+ Misses        350      345       -5     
+ Partials       94       91       -3     

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant