Skip to content

fix(types): resolve Literal[None] instead of raising TypeError - #1946

Open
sarathfrancis90 wants to merge 1 commit into
dottxt-ai:mainfrom
sarathfrancis90:fix/literal-none-value
Open

sarathfrancis90 wants to merge 1 commit into
dottxt-ai:mainfrom
sarathfrancis90:fix/literal-none-value

Conversation

@sarathfrancis90

Copy link
Copy Markdown
Contributor

While using Literal output types I hit a crash whenever None was one of the values. python_types_to_terms treats a bare None as a type and recurses into it, so any of these raise TypeError: Type None is currently not supported:

from typing import Literal
from outlines.types.dsl import python_types_to_terms

python_types_to_terms(Literal[None])
python_types_to_terms(Literal["active", None])
python_types_to_terms(list[Literal["yes", None]])

This is inconsistent with the rest of the DSL: Optional[...]/Union[..., None] already render their None member as the bare None keyword (via _handle_union), and Literal[True]/Literal[False] were previously fixed the same way. Only the None literal was left out.

Fix

Map a None value to Regex("None") in python_types_to_terms, so it resolves to the bare None keyword and stays unquoted when nested inside a container type - exactly what the union path already does. Literal[None] now becomes (None), matching Optional.

Testing

Added unit and end-to-end tests in tests/types/test_dsl.py covering Literal[None], Literal[..., None], a bare None value, and list[Literal[..., None]] (the None branch stays a bare keyword and is not JSON-quoted). Ran pytest tests/types (all green, coverage of dsl.py stays at 100%) and pre-commit run --all-files.

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.
@github-actions

Copy link
Copy Markdown

📚 Documentation preview: https://dottxt-ai.github.io/outlines/pr-preview/pr-1946/

Preview updates automatically with each commit.

Comment thread tests/types/test_dsl.py
assert not _re.fullmatch(standalone, '"None"')

list_pattern = to_regex(python_types_to_terms(list[Literal["yes", None]]))
assert _re.fullmatch(list_pattern, "[None]")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ErenAta16 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (like True/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.

Comment thread src/outlines/types/dsl.py
return types.datetime

# Basic type instances
if ptype is None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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 ErenAta16 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

This branch has not been deployed

No deployments
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.

3 participants