Fix DSL booleans in JSON containers - #1947
cyphercodes wants to merge 2 commits into
Conversation
| return String(f'"{term.value}"') | ||
| if isinstance(term, Regex): | ||
| if term.pattern == "(True|False)": | ||
| return Regex("(true|false)") |
There was a problem hiding this comment.
This helper is also used for dict keys, so dict[bool, int] will now emit {true: ...}; that is still invalid JSON because object keys must be strings. Could the key path turn these into "true"/"false" instead?
ErenAta16
left a comment
There was a problem hiding this comment.
The bug is real and I confirmed it by running both sides rather than reading the diff. Clone of main at be2cd15 against 75dcd31:
sample main this PR valid JSON
list[bool] [true] False True yes
list[bool] [True] True False no
dict[str, bool] {"k":true} False True yes
dict[str, bool] {"k":True} True False no
So main generates [True], which no JSON parser accepts, and this branch fixes it. The standalone case stays Python-style in both, which is the boundary that matters:
bool standalone 'True' -> True 'true' -> False
Recursion is covered too, which I was not sure about from the diff alone. list[Optional[bool]] and list[Literal[True]] both reach [true] on this branch and neither did before, so the Alternatives path is fine.
Where I would push back is on how the boolean term is recognised. Matching on term.pattern means any user term that happens to share the spelling gets rewritten:
from outlines.types.dsl import Regex, _ensure_json_quoted
_ensure_json_quoted(Regex("True")).pattern
# main: 'True'
# this PR: 'true'Same for Regex("False") and Regex("(True|False)"). Someone writing Regex("True") to match the literal word in prose, inside a list[...], silently gets a pattern that no longer matches their input. Regex.__eq__ is pattern-based, so there is nothing distinguishing the library's boolean from a user's identical one once you are looking at the pattern string.
This is worth reconciling with #1961, which is open against this exact function and goes the other way deliberately. It matches the temporal terms by identity and says why in a comment:
Matched by identity, since
Regexequality is pattern-based and would also catch a user's own term that happens to share the pattern.
Two changes landing in _ensure_json_quoted with opposite conventions for the same problem would be an awkward place to end up, and they will conflict textually as well since both rewrite the same lines.
The complication, and I think this is why you reached for the pattern check, is that identity alone does not cover Literal[True]. types.boolean is a module-level singleton so term is types.boolean works, but a bool inside a Literal comes from a different place:
# python_types_to_terms, basic type instances
if isinstance(ptype, bool):
return Regex(str(ptype))That builds a fresh Regex("True") with no way to tell it apart downstream, which is exactly the term your third branch is catching. So the two cases have genuinely different origins and the pattern check is the only thing that unifies them after the fact.
The cleaner split is to handle each where the intent is still known: match types.boolean by identity here, and deal with the Literal bool at the branch above, where isinstance(ptype, bool) has already told you it is a boolean and not a user pattern that reads like one. That keeps the JSON spelling decision attached to something that actually knows it is looking at a bool, and it means a user's Regex("True") is never a candidate. It also lines this up with how #1961 handles the same shape of problem, so whichever lands second does not have to undo the other's convention.
Smaller note: the branches return Regex("true") as a new term rather than transforming the existing one, so anything a caller might have attached to the original instance is dropped. Not an issue with the current call sites, but it is the same identity-erasing move that makes the false positive above possible, so it is worth being deliberate about.
Tests look fine and test_literal_bool_in_container_uses_json_literals asserting not _re.fullmatch(pattern, "[True]") is the right negative to have. If you keep any form of pattern matching, a test pinning that a user's own Regex("True") survives untouched would be the one guarding the case above.
…ool-json-1942 # Conflicts: # src/outlines/types/dsl.py
ErenAta16
left a comment
There was a problem hiding this comment.
Checked that case on the branch and it doesn't fire — dict[bool, int] already quotes the key.
Dict[bool, int] -> \{("((true|false))":([+-]?(0|[1-9][0-9]*))(,\ "((true|false))":...
'{true:1}' matches=False (invalid JSON)
'{"true":1}' matches=True (valid JSON)
'{True:1}' matches=False (invalid JSON)
_handle_dict passes the key through _ensure_json_quoted(..., quote_regex=True) while the value gets the plain call, with the comment right above saying why:
# JSON object keys must always be quoted strings, so quote the key term
# even when the Python key type isn't `str` (e.g. `Dict[int, str]`).
key_type = _ensure_json_quoted(python_types_to_terms(args[0], ...), quote_regex=True)
value_type = _ensure_json_quoted(python_types_to_terms(args[1], ...))So the key path was already covered before this PR, and the change to the boolean term flows into it as "true"/"false" rather than bare true/false. Same for Dict[int, str], which is what that branch was originally written for.
The two paths this PR does change come out valid:
Dict[str, bool] '{"k":true}' matches=True valid JSON
List[bool] '[true]' matches=True valid JSON
Summary
Fixes #1942.
This updates DSL container handling so boolean values use JSON literals (
true/false) when nested in list, tuple, or dict types, while preserving the existing standalonetypes.booleanbehavior.Tests
pytest tests/types/test_dsl.py -qlist[bool]anddict[str, bool]Notes
I used AI assistance to prepare this focused bug fix.