fix: guard against IndexError/ValueError when dict annotation has no type args - #3405
fix: guard against IndexError/ValueError when dict annotation has no type args#3405adhavan18 wants to merge 4 commits into
Conversation
…type args `_transform_recursive` and its async counterpart (in `_utils/_transform.py`) unconditionally did `get_args(stripped_type)[1]` for any `origin == dict` branch. When the annotation is a bare, unparameterised `dict` (no `[K, V]` type arguments), `get_args(dict)` returns an empty tuple, so the index access raises `IndexError: tuple index out of range`. `construct_type` in `_models.py` had the same assumption, unpacking `get_args(type_)` into exactly two targets with `_, items_type = ...`, which raises `ValueError: not enough values to unpack (expected 2, got 0)` for a bare `dict`. Fix: in both sites, check `len(args) >= 2` before indexing. A bare `dict` annotation carries no value-type information, so returning the mapping unchanged (rather than trying to recurse) is the correct behaviour. Adds four regression tests. Closes openai#3338, openai#3341
|
Gentle ping, this has been approved for 9 days. Happy to rebase or make any changes if needed. |
ruff was failing the lint job with F401: the test imports Dict inside the function, but typing.Dict is already imported at module scope, so the local one shadows it for no reason. The annotation still resolves and both dict tests pass.
The bare `dict` is the whole point of TestBareDict, so mypy's type-arg error and pyright's reportMissingTypeArgument are both expected here. Suppressed on that line with a comment, rather than parameterising the annotation and losing what the test covers.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed both coercion and request-transform paths, including sync/async behavior and typing variants. Returning the mapping unchanged when bare built-in dict or typing.Dict provides no value type is the correct fallback; parameterized Dict/dict aliases continue to recurse through their value type, and Annotated aliases still apply outer field metadata. The changes are confined to handwritten internals/tests and are compatible with the Python 3.10 floor. Validation: exact-head diff check and compileall pass, Pyright strict reports no issues, and direct probes cover built-in dict, typing.Dict, Annotated bare dict, synchronous transform, asynchronous transform, and construct_type. No GitHub CI checks are currently attached to this head.
|
this has three approvals now (@nomiveritas and @jbeckwith-oai) and has been sitting at blocked since 3 aug. is there something still outstanding on my side, or is it just waiting on someone with merge rights? happy to rebase, split it, or adjust anything if there's a concern that hasn't been written down. just want to make sure it isn't stuck purely by accident. |
|
From my side, there’s nothing else needed. I’ve approved the PR, and @jbeckwith-oai also gave a thorough approval with no outstanding technical concerns. It looks like the remaining step is on the maintainer/merge side. |
|
Thank you for the fix, tests, and follow-ups, and sorry for the wait. The missing type arguments for bare Closing this PR because the merged change resolves the same crash. Please use a release containing #3760 when available; the fix is not included in v3.11.0. |
4 participants
Fixes #3338 and #3341.
Problem
get_args(dict)returns an empty tuple when the annotation is a bare, unparameteriseddict(no[K, V]). Two places assumed there are always two args:_transform.py:get_args(stripped_type)[1]→IndexError_models.py:_, items_type = get_args(type_)→ValueErrorMinimal reproducer:
Fix
Check
len(args) >= 2before indexing; return early without transformation when the annotation is unparameterised.Test
All existing tests pass. New test added for bare
dictannotation.