forms: carry int64-sized bounds as exact strings, so the client gate actually closes - #259
Open
Yaraslaut wants to merge 3 commits into
Open
forms: carry int64-sized bounds as exact strings, so the client gate actually closes#259Yaraslaut wants to merge 3 commits into
Yaraslaut wants to merge 3 commits into
Conversation
…actually closes DynamicForm's numeric gate compared a typed value against `minimum`/`maximum` read straight from the parsed schema. Those are JSON numbers, and every shipped app hands the renderer its schema via `JSON.parse(controller.schemasJson)`, so a bound above 2^53 is already an IEEE-754 double by the time the gate sees it: INT64_MAX arrives as 9223372036854775808. Comparing INT64_MAX + 1 against that judges it "not greater", so the renderer's own validation admitted a value the schema excludes. Nothing was corrupted -- the payload keeps exact digits and the server rejects it with parse_number_failure -- but the client called a value valid that never was. `mergeSchemaExtras` already reads the schema in u64 number mode precisely so these bounds are not rounded on the C++ side; the comment saying so is right there. The renderer half was never done, because the rounding happens after C++ hands the text over. So schemaJson<A>() now also emits the bound as an exact decimal *string* -- `x-exactMinimum`/`x-exactMaximum` -- which JSON.parse cannot round, and the integer gate prefers it, comparing digits directly since no JS number can hold the bound. Both keys are additive: `minimum`/`maximum` are untouched, so a renderer ignoring them behaves exactly as before. Emitted only above 2^53. An int32_t field, a Ranged slider, a hand-written `maximum: 10` -- none loses anything to a double, and none of their schemas change by a byte. Only $defs/int64_t and $defs/uint64_t pick up the companion. Named x-exactMinimum, not x-minimumText, because of what the first name broke: `x-min` and `x-max` already exist as *slider* bounds, and test_widget_hints.cpp's PlainFieldsEmitNoWidgetHint asserts their absence with a bare substring check -- which `x-minimumText` satisfied, failing that test with a message about slider bounds. The prefix collision would have been just as confusing to a human reader. That assertion is tightened here to match JSON keys (`"x-min":`) rather than substrings, so the next key sharing a prefix does not fail a test that has nothing to do with it. Verification. Mutation-checked in both halves independently: removing the C++ emitter fails 3 of the 4 emitter cases, and making the gate ignore the exact bound fails test_int64_max_plus_one_is_rejected. Per the issue's invariant-7 note, no existing fixture went near INT64_MAX -- one with small bounds passes whether or not the bug exists -- so every new QML fixture sits at the boundary: INT64_MAX/INT64_MIN accepted and exact, +1/-1 past each rejected, leading zeros handled, and the numeric fallback still gating where no companion is emitted. morph_tests 20632 assertions / 1158 cases; QML 125/0; Doxygen clean. Closes #213
CI runs clang-tidy-diff with -warnings-as-errors=* over changed lines, which #258 tripped; reproducing it locally against the same clang 22 the workflow pins turned up the same class of findings here, before CI got to them. Fixed rather than suppressed where the check was right: - modernize-use-integer-sign-comparison: the two bounds arrive in different signednesses, so the casts were exactly the sign-mismatch the comparison exists to get right. Now std::cmp_greater/std::cmp_less, with a signed twin of the 2^53 limit for the negative side. - The fixture models gain real state, so execute() genuinely needs `this` and is neither static-able nor const-able -- rather than NOLINTing readability-convert-member-functions-to-static. - readability-container-contains: C++23 std::string::contains. Suppressed only where the pattern is forced: - misc-use-internal-linkage on the fixtures -- glaze's reflection takes the address of an `extern const T`, so the anonymous namespace the check asks for is exactly what they cannot have. Same suppression tests/ test_shared_instances.cpp already uses. - cert-err58-cpp / bugprone-throwing-static-initialization / misc-const-correctness across the BRIDGE_REGISTER_* expansions: every test that registers a model has this shape. - cppcoreguidelines-pro-bounds-avoid-unchecked-container-access on the glaze DOM, the same NOLINT the surrounding header already carries. - misc-no-recursion on annotateExactNumericBounds -- walking a JSON tree is inherently recursive; that is the function. No behaviour change: 11 assertions / 4 cases unchanged, framework and QML suites green.
The original run was cancelled by the supersede-obsolete-runs concurrency rule (#257) shortly after it merged, and re-running the cancelled workflows produced attempts that were themselves cancelled within minutes. An empty commit gives the PR a fresh head so its checks run from a clean slate. No content change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #213.
The defect
DynamicForm's integer gate compared the typed value againstminimum/maximumread straight from the parsed schema. Those are JSON numbers, and every shipped
app hands the renderer its schema via
JSON.parse(controller.schemasJson)— so abound above 2^53 is already an IEEE-754 double by the time the gate sees it:
Comparing
INT64_MAX + 1against that judges it equal, not greater, so therenderer's own validation admitted a value the schema excludes. Nothing is
corrupted — the payload keeps exact digits and the server rejects it with
parse_number_failure— but the client called a value valid that never was.mergeSchemaExtrasalready reads the schema in u64 number mode precisely sothese bounds are not rounded on the C++ side, with a comment saying so. The
renderer half was never done, because the rounding happens after C++ hands the
text over.
The fix
schemaJson<A>()also emits each oversized bound as an exact decimal string—
x-exactMinimum/x-exactMaximum— whichJSON.parsecannot round. Theinteger gate prefers it and compares digits directly, since no JS number can hold
the bound.
Additive and narrow.
minimum/maximumare untouched, so a renderer thatignores the new keys behaves exactly as before (per the spec's versioning
stance). And they are emitted only above 2^53 — an
int32_tfield, aRangedslider, a hand-writtenmaximum: 10lose nothing to a double, and theirschemas do not change by a byte. Only
$defs/int64_tand$defs/uint64_tpickup companions.
Why they aren't called
x-minimumTextThat was the first name, and it broke a test — usefully.
x-minandx-maxalready exist as slider bounds, and
test_widget_hints.cpp'sPlainFieldsEmitNoWidgetHintasserts their absence with a bare substringcheck, which
x-minimumTextsatisfied. The failure pointed at slider bounds fora change that had nothing to do with them — and the prefix collision would have
been just as confusing to a human reader.
Renamed, and that assertion is tightened to match JSON keys (
"x-min":) ratherthan substrings, so the next key sharing a prefix doesn't fail an unrelated test.
Verification
The issue flags invariant 7 and it is the crux: no existing fixture went near
INT64_MAX, and one with small bounds passes whether or not the bug exists. Soevery new QML fixture sits at the boundary —
INT64_MAX/INT64_MINacceptedand exact, one past each rejected, leading zeros handled, and the numeric
fallback still gating where no companion is emitted.
Mutation-checked in both halves, independently:
test_int64_max_plus_one_is_rejectedfailsmorph_testsWARN_AS_ERROR)scripts/check_spec_citations.shInteraction with #253
#253 (fix for #189) makes
resolvePropresolve throughanyOf, so a barestd::optional<std::int64_t>becomes a typed integer field and inherits$defs/int64_t's bounds — and would inherit this defect too. I measured that onthe #253 branch and recorded it on #213. Because both paths converge in
resolveProp, this fix covers theanyOfpath automatically once #253 lands;nothing further is needed. The two branches touch nearby lines in
DynamicForm.qml, so whichever merges second may need a trivial rebase.