Accept negative numbers in multipleOf (#222) - #357
Conversation
RegexAst::MultipleOf only matches the unsigned digit sequence, so And-ing it with the (signed) int/number range regex rejected every negative value, including valid negative multiples (e.g. -6 for multipleOf 3). Wrap it as Concat(["-?", MultipleOf]) so the sign is allowed; divisibility is independent of sign. Adds negative integer cases and re-enables the negative non-integer (3.5) cases. Partially fixes guidance-ai#222 (sign handling). The decimal-point handling of integer-valued multipleOf on `number` (e.g. 3.0) and scientific notation (guidance-ai#216) are left as follow-ups.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates JSON Schema multipleOf handling to accept negative numeric literals and expands tests to cover negative cases.
Changes:
- Extend integer
multipleOftests to include negative values. - Enable negative test cases for
numbermultipleOf(non-integer multiples). - Update the JSON compiler to allow an optional leading
-when applyingRegexAst::MultipleOf.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| parser/tests/test_json_primitives.rs | Expands multipleOf test matrices to include negative inputs and clarifies an issue reference. |
| parser/src/json/compiler.rs | Adjusts regex AST construction so multipleOf validation accepts negative numeric strings via optional leading -. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // RegexAst::MultipleOf only matches the unsigned digit sequence, so | ||
| // allow an optional leading '-' to accept negative multiples (e.g. | ||
| // -6 for multipleOf 3); divisibility is independent of sign. | ||
| // https://github.com/guidance-ai/llguidance/issues/222 | ||
| let multiple_of = RegexAst::Concat(vec![ | ||
| RegexAst::Regex("-?".to_string()), | ||
| RegexAst::MultipleOf(d.coef, d.exp), | ||
| ]); | ||
| ast = RegexAst::And(vec![ast, multiple_of]); |
There was a problem hiding this comment.
Extracted a signed_multiple_of_ast(coef, exp) helper and used it in both json_int and json_number, so the construction is no longer duplicated.
| // RegexAst::MultipleOf only matches the unsigned digit sequence, so | ||
| // allow an optional leading '-' to accept negative multiples (e.g. | ||
| // -6 for multipleOf 3); divisibility is independent of sign. |
There was a problem hiding this comment.
Reworded it. RegexAst::MultipleOf matches the unsigned numeric literal (the digits, plus a decimal point for non-integer multipleOf), not just a digit sequence. Updated the comment to say that.
Dedupe the optional-sign multipleOf construction across json_int and json_number into a helper; reword the comment to note MultipleOf matches the unsigned numeric literal (incl. decimal point), not just digits.
|
@riedgar-ms the two red checks are baseline mismatches from this fix improving results (MaskBench reports 0 errors, 2 improvements), so the expected_maskbench.json and expected_json_schema_test_suite.json snapshots need regenerating rather than any logic change. |
Makes sense - can you update the JSON so that the tests pass? It's a manual process unfortunately (and the update might be to 'newly revealed test failure' not 'test now passes') |
|
@riedgar-ms updated |
riedgar-ms
left a comment
riedgar-ms
left a comment
There was a problem hiding this comment.
I'm OK with this. Thoughts @hudson-ai ?
RegexAst::MultipleOfonly matches unsigned digits, so And-ing it with the signed range regex rejected all negative values — including valid negative multiples like-6formultipleOf 3. This wraps it asConcat(["-?", MultipleOf]); divisibility is sign-independent and-?matches empty for positives (no regression). Adds negative integer cases and re-enables the maintainer-marked negative3.5cases.Scope: this fixes the sign facet of #222. Two related facets are left as follow-ups and noted in the tests — (1) integer-valued
multipleOf(e.g.3.0) onnumberstill chokes on the decimal point, and (2) scientific notation (3.5e22) is #216. Full suite + fmt + clippy clean.