fix(types): resolve Literal[None] instead of raising TypeError - #1946
sarathfrancis90 wants to merge 1 commit into
Conversation
python_types_to_terms treated a bare None value as a type and recursed
into it, so Literal[None] (and any Literal that includes None, e.g.
Literal["active", None]) crashed with "Type None is currently not
supported". This is inconsistent with Optional/Union, whose None member
is already rendered as the bare None keyword, and with Literal[True] /
Literal[False].
Map a None value to Regex("None") so it resolves to the bare None
keyword, staying unquoted when nested inside a container type.
|
📚 Documentation preview: https://dottxt-ai.github.io/outlines/pr-preview/pr-1946/ Preview updates automatically with each commit. |
| assert not _re.fullmatch(standalone, '"None"') | ||
|
|
||
| list_pattern = to_regex(python_types_to_terms(list[Literal["yes", None]])) | ||
| assert _re.fullmatch(list_pattern, "[None]") |
There was a problem hiding this comment.
list[Literal[None]] is still generating [None], which json.loads rejects; the JSON spelling here is null. Should _ensure_json_quoted translate this branch in container contexts, the same way #1947 handles True/False, while keeping standalone Literal[None] as None?
ErenAta16
left a comment
There was a problem hiding this comment.
Confirmed the crash and the fix by running both sides, clone of main at be2cd15 against 9cad2ba.
All three of the reported forms raise on main:
Literal[None] TypeError: Type None is currently not supported
Literal["active", None] TypeError: Type None is currently not supported
list[Literal["yes", None]] TypeError: Type None is currently not supported
and all three resolve on this branch:
Literal[None] ((None))
Literal["active", None] (active|(None))
list[Literal["yes", None]] \[("yes"|(None))(,\ ("yes"|(None)))*\]
Plain literals are untouched, which is the thing I wanted to be sure a change in python_types_to_terms had not disturbed:
Literal["a", "b"] standalone a=match "a"=no match
list[Literal["a", "b"]] ["a"]=match [a]=no match
Literal[1, 2] 1, 2 match, 3 does not
Optional[int] unchanged
So the ptype is None branch is in the right place and the guard is narrow enough not to catch anything else.
One consequence worth putting on record before this lands. The fix is correct and a crash is clearly worse than what follows, so this is not a reason to hold it. But list[Literal["yes", None]] was previously unreachable, and what it now produces is not valid JSON:
list[Literal["yes", None]] ["yes"] matches [None] matches [null] rejected
Dict[str, Literal[None]] {"k":None} matches {"k":null} rejected
[None] is not something json.loads will take, and [null], the spelling it should be emitting, is the one the pattern forbids. So the change swaps a loud failure for a quiet one on the container path. I opened #1971 for that, since it is not specific to Literal and Optional[int] inside a container has the same problem today.
The reason I am raising it here rather than only in the issue is the comment on the new branch:
Kept as a
Regex(likeTrue/False) so it stays unquoted when the value ends up nested inside a container type.
That is the same reasoning as the existing _handle_union comment, and it is right about the quoting. Keeping it a Regex avoids "None" coming out as a quoted string, which would be a different wrong answer. It is just that True/False is the precedent #1942 already identified as wrong, so this now anchors to it in a second place.
Concretely, Regex("None") is constructed in two spots after this merges, here and at _handle_union. Whoever fixes #1971 has to find both and keep them in step. Pulling it out into one module-level term that both branches return would make this a one-line change later instead of a two-site one, and it would also let the container-context check match it by identity, which is the approach #1961 settled on for the temporal terms after the same question came up there.
Tests are the right shape. result_none.terms == [Regex("None")] pins the structure rather than the rendered regex, which is what you want, and covering the mixed Literal["active", None] case catches the interaction with String members. If you take the shared-term suggestion the assertions should keep working as they are.
| return types.datetime | ||
|
|
||
| # Basic type instances | ||
| if ptype is None: |
There was a problem hiding this comment.
_handle_union peels off both type(None) and None because typing normalizes the two, but this only catches the bare value: get_args(List[None]) is (NoneType,), so List[None] / Dict[str, None] / Tuple[None, ...] still recurse into NoneType and hit the "Type NoneType is currently not supported" raise. Worth widening to if ptype is None or ptype is type(None) and adding a container case to the test?
ErenAta16
left a comment
There was a problem hiding this comment.
@Sanjays2402 both of your points hold, and measuring them together turns up the reason the second one is subtler than it looks.
On the container spellings. Confirmed, Python 3.12.10:
List[None] get_args -> (<class 'NoneType'>,)
Dict[str, None] get_args -> (<class 'str'>, <class 'NoneType'>)
Tuple[None, ...] get_args -> (<class 'NoneType'>, Ellipsis)
Optional[int] get_args -> (<class 'int'>, <class 'NoneType'>)
typing normalizes the bare None to NoneType when it goes through a subscription, so ptype is None is False for every one of those and the recursion lands on the NoneType raise. Widening to ptype is None or ptype is type(None) is the fix.
But Literal[None] is the exception, and it is the reason the predicate needs both halves rather than just the type(None) one:
Literal[None] get_args -> (None,) # the value, not the type
Literal preserves the object it was given, so that branch yields bare None where every container yields NoneType. So:
ptype = None -> is None: True is type(None): False
ptype = type(None) -> is None: False is type(None): True
Neither test alone covers both spellings. ptype is None or ptype is type(None) is exactly right rather than merely broader, and it is worth a comment saying so, because a later reader will look at the two clauses, decide typing normalizes them anyway, and drop one. Which one they drop determines whether Literal[None] or List[None] breaks.
For the test, the container case you suggest is the right shape, and I would parametrize it over all three spellings plus Literal[None] so the asymmetry is pinned rather than implied.
On your first question, list[Literal[None]] rendering [None]: I think that is a separate mechanism from this PR and belongs with the work in #1974, which makes Optional/Union None render as JSON null inside containers while keeping the bare Python keyword standalone. Literal[None] needs the same treatment for the same reason, since json.loads("[None]") fails either way. Doing it here would mean this PR owns both the type-resolution fix and a rendering change, and the rendering one already has a home with tests around it.
Nothing here blocks the change as it stands; resolving Literal[None] instead of raising is right on its own.
While using
Literaloutput types I hit a crash wheneverNonewas one of the values.python_types_to_termstreats a bareNoneas a type and recurses into it, so any of these raiseTypeError: Type None is currently not supported:This is inconsistent with the rest of the DSL:
Optional[...]/Union[..., None]already render theirNonemember as the bareNonekeyword (via_handle_union), andLiteral[True]/Literal[False]were previously fixed the same way. Only theNoneliteral was left out.Fix
Map a
Nonevalue toRegex("None")inpython_types_to_terms, so it resolves to the bareNonekeyword and stays unquoted when nested inside a container type - exactly what the union path already does.Literal[None]now becomes(None), matchingOptional.Testing
Added unit and end-to-end tests in
tests/types/test_dsl.pycoveringLiteral[None],Literal[..., None], a bareNonevalue, andlist[Literal[..., None]](theNonebranch stays a bare keyword and is not JSON-quoted). Ranpytest tests/types(all green, coverage ofdsl.pystays at 100%) andpre-commit run --all-files.