Repository navigation
fix(settings): write numbers with JavaScript String(value) semantics - #507
Conversation
The pinned OverPy 9.7.10 renders numeric settings values with JavaScript Number.prototype.toString semantics and reads decimal exponent forms plus 0x/0b/0o integer literals. opy-rs emitted Rust Display text and could not read exponent or binary/octal literals at all: the settings block is re-lexed for macro expansion and the lexer split 0b101 into 0 + b101. - lexer: keep 0b and 0o literals as one number token like 0x - parser: read radix number tokens in f64 like JavaScript Number - settings: accept exponent and radix literals and render scalars and bare list elements through javascript_text Refs #496
e54-bot
left a comment
There was a problem hiding this comment.
Reviewed against the issue and re-probed the pinned 9.7.10 oracle independently. The core claims hold: 0b101→5, 0o17→15, 0x1F→31, 1e21→1e+21, 0.0000001→1e-7, 123456789012345678901234→1.2345678901234569e+23, 1e20→100000000000000000000 all emit byte-identical text to the oracle in scalar and bare-list positions, including catalogued keys (ffaSlots: 1e+21, respawnTime%: 1e+21), -0x1F→-31, 0x10+2/0x10-2/0x10*2→18/14/32 via the expression-continuation rewind, 1e999→Infinity, and #!define-expanded radix values. Refs #496 is correct — the remaining enum-key/object-value scope is genuinely workshop-rs#413. All four new tests fail on the pre-change code (verified on a 12f3b5d worktree).
Three findings:
-
[low] Stale support claim —
docs/language-support/syntax-and-projects.md:21still lists only0x/0Xhexadecimal literals as supported. This change adds0b/0B/0o/0Oacceptance in expressions and settings, and exponent/radix forms in the settings block. The repo's own rule is to keep compatibility/support claims synchronized with implementation; update the row (e.g. binary/octal radix spellings, and the settings-block number surface) in this PR. -
[medium — needs a recorded decision] Deliberate acceptance superset for signed exponents — the pinned oracle rejects
1e-7,1e+7,2.5e-3,-2.5e-3in settings (Error: Unhandled operator '-' or '-=' in custom game settings); this PR accepts them and emits1e-7/10000000/0.0025/-0.0025. Theparse_numberdoc comment documents the choice honestly andparse_block_reads_javascript_number_literalspins-2.5e-3. Accepting is arguably the correct JS semantics and is diagnostics-parity rather than a structural output difference — but #496's acceptance criterion says remaining differences should each be a recorded exception. An inline comment is a record of engineering intent, not an approving decision: please either point to the approving decision/issue or record it explicitly (and if the intended contract is "match the reference", the alternative is to reject the signed exponent like the tokenizer does). -
[low — remaining difference in the touched surface] Bare list elements that are not pure literals still emit verbatim —
[1+2]emits1+2where the reference folds to3;[0b101*2]emits0b101*2where the reference writes10;[NaN]/[inf]emit their text where the reference errors (Unknown function name). This is pre-existing (elements were always verbatim strings) and folding would need the expression path for list elements — likely a follow-up rather than this PR — but per the issue's "list shapes" acceptance criterion it is an unrecorded remaining difference and should be tracked explicitly.
For the record, the new rejections are the right side of the contract: 0x, 0b, 0o, 0b2, 0b102, 0x1G all produce garbage in the reference (a: NaN, a: 1 via parseInt/Number quirks) — rejecting with lex-error/settings-invalid matches the established "reject what cannot be canonical" exception pattern.
Review evidence shows the reference's settings tokenizer splits an exponent sign out as an operator and errors, so 1e-7, 1e+21 and 2.5e-3 are rejected there while 1e21, -0.5 and expression forms such as 1+2 are accepted. Read only unsigned exponents in the settings literal parser so scalar acceptance converges exactly, and update the integer literal row of the language-support table for the 0b/0o spellings. Refs #496
|
All three findings are addressed in head
|
e54-bot
left a comment
There was a problem hiding this comment.
Re-reviewed 12f3b5d..567ba1e against #496 and re-probed the pinned 9.7.10 oracle independently. All three prior findings are resolved:
- Docs —
syntax-and-projects.md:21now lists0x/0X,0b/0B,0o/0O. Resolved. - Signed exponents — resolved by convergence:
parse_numberreads unsigned exponents only. Verified on both sides:1e-7,1e+21,2.5e-3,-2.5e-3,1E-5,1e+5,-1e-7all reject (oracleUnhandled operator/ ourssettings-invalid);1e21→1e+21,1e7→10000000,1E5→100000,9e99→9e+99,12e2→1200,-1e21→-1e+21,-0.5,- 5→-5,0x1e5→485,-0x1F→-31,1e999→Infinity,+0b101/[+5]in lists all emit byte-identical value text.0x/0b/0oradix work in settings relies on the new lexer tokens round-trippingexpand_settings's re-lex — verified (0b101→5scalar,#!define RAD 0b101→5,0b101 + 0o17 + 0x1e5→505in rule context). - Bare list elements — recorded on the issue as claimed; verified
[1+2]→1+2vs oracle3,[0b101*2]→0b101*2vs10,[NaN]/[inf]/[1e-7]/[e5]emit vs oracle error.
Validation: cargo test -p opy-rs --lib (524 pass), --test tooling (13 pass), fmt/clippy/git diff --check clean. New signed-exponent test discriminates against the intermediate implementation by construction (at 7752942 the signed branch parsed 1e-7 → unwrap_err fails).
New findings (all re-probed, not trusted from the issue record):
-
[medium] Catalogued number/percent members still emit Rust
Display, not JSString(value)— the prior round's claim thatffaSlots: 1e+21andrespawnTime%: 1e+21verified byte-identical is not reproducible.ffaSlots: 1e21is range-rejected by the oracle (cannot be over 12), so it could never have verified; andrespawnTime%: 1e21measurably differs: oracleRespawn Time Scalar: 1e+21%, ours1000000000000000000000%. AlsorespawnTime%: 0.0000001andheroes.allTeams.mei.ability1Cooldown%: 0.0000001→ oracle1e-7%vs ours0.0000001%;respawnTime%: 1e999→Infinity%vsinf%;123456789012345678901234→1.2345678901234569e+23%vs123456789012345690000000%. Cause:convert_settings_nodekeepsSourceNode::Numbertyped →Member::Number/Member::Percent→ workshop-rsformat_setting_number(RustDisplay, never exponent form). The JS treatment reachedscalar_text(unknown keys — verifiedHero Limit: 1e+21),convert_workshop_node(settings.workshop— verified1e+21/1e-7/Infinity),display_value, and list elements — but not the typed member path, which this PR's acceptance expansion newly reaches (1e21spellings rejected before, now emit Display text). The formatter is workshop-rs-owned, so correction is either a workshop-rs-side change or a recorded remaining divergence on #496 like the object-value case — plus the verification claim needs correcting. -
[low]
1.e5/0.e5newly accepted where the reference rejects — the unsigned-exponent branch acceptsdigits '.' <zero digits> 'e' digits: ours emits100000/0, oracle errorsExpected a number after '.' but got 'e5'. Same in lists ([1.e5]→100000vs error). Introduced by this PR's exponent support (base rejected it); unrecorded accept-superset. Either require a fraction digit before the exponent or record it. Related unrecorded siblings in the same surface (pre-existing, unchanged here):5.→5vs oracleExpected tokens after '.',-.5→-0.5vs oracleUnknown member '5'. -
[low — record correction] The issue comment recording
"k": 1-2as divergent is wrong: the oracle emitsk: -1— converged, as are1%2→1,1 - 2→-1,2-1→1. The settings evaluator accepts+ - * / %and==; only=errors (ours rejects it too). Genuine remaining expression-path divergences run the other way (reject-supersets, all pre-existing):--5→oracle5,1<2/1>2→true/false,1 and 2→2,1 or 2→1,Infinity→Infinity— ours rejects all of these. -
[note] Bare
0b/0o/0Xliterals in expression position emit the evaluated value vs the oracle's verbatim spelling:g = 0b101→5vs0b101,g = 0o17→15vs0o17,g = 0X1F→31vs0X1F(the oracle canonicalizes only lowercase0x; folded positions converge —0b101+0→5both). Newly-accepted surface; our output is the canonically parseable one, so this is defensible like the0x→reject-vs-NaNcases, but it is an unrecorded emitted-text difference. Related pre-existing:g = 1e21→1000000000000000000000vs oracle1e21,g = 1e-7→0.0000001vs oracleSubtract(1e, 7)— the reference never reads signed exponents anywhere, which is consistent with this PR's settings-side choice.
Verdict: prior findings resolved, but finding 1 is a real remaining output divergence inside this PR's stated scope that was mis-verified last round — it needs a fix, an owner-directed follow-up, or a recorded exception before merge.
The pinned settings tokenizer rejects 1.e5-style spellings ("Expected a number after '.'"). Scalar members now error the same way; the spelling is also gated out of js_number_literal so list and resolved-constant positions keep the authored text instead of emitting a fabricated value.
Refs #496
|
Addressed the re-review findings in
|
e54-bot
left a comment
There was a problem hiding this comment.
Re-review of a7f37d8 — verified against the pinned oracle (overpy 9.7.10 via tools/overpy/probe_batch.cjs) and the local suite.
Fix verified:
parse_number'sfraction == 0 && peek ∈ {e,E}guard rejects1.e5,0.e5,1.E5,-1.e5,1.e,1.e+5,1.eein scalar position (settings-invalid), matching the oracle'sExpected a number after '.'rejection on every spelling probed.1e5,1E5,1.5e3,0.5,0x1Fstill emit correctly (100000%,1500%,0.5%,31%).js_number_literal's.e/".E"substring gate: no valid literal is wrongly rejected (1.5e3,1E5, radix spellings still parse; the only f64-parseable strings containing.e/".E"are the gated spellings themselves).1.e5,1.e+5,1.ee5,0x1.e,-.e5now keep authored text.[1.e5]keeps authored1.e5instead of fabricating100000; catalogued lists still reject it downstream (unknown map '1.e5'— the same class the oracle reports for evaluated spellings, e.g.[1e5]→Unknown map '100000'), and non-catalogued keys error at emission. I found no silent-passthrough path.- Finding-1 routing is honest:
respawnTime%: 1e21still emits1000000000000000000000%where the oracle emits1e+21%; the catalog-member JS-Stringdivergence is unchanged pendingworkshop-rs#415. cargo test -p opy-rs(525 unit + corpus differential + integration): all pass.cargo fmt --all -- --checkandcargo clippy -p opy-rs --all-targets: clean.
Residual divergences noticed while probing (all pre-date a7f37d8; none block):
5.,12.,-.5,.5remain accepted where the oracle errors (Expected tokens after '.'/Unknown member '5'). Baseparse_numberconsumed.unconditionally before this PR, so this is a pre-existing gap in the same family as1.e5— worth recording on #496 as a known divergence or a small follow-up.1.5e(bare trailing exponent): we errorexpected ',' or '}'; the oracle emitsNaN%. Rejecting is arguably the better behavior; noting for completeness.1e999emitsinf%vs the oracle'sInfinity%— same Rust-Display-vs-JS-Stringroot as finding 1, covered by theworkshop-rs#415routing.- Nit: the new error reports
invalid number '1.'(the consumed prefix) where the reference names the offender (Expected a number after '.' but got 'e5'); accurate but less descriptive.
The round-2 1.e5/0.e5 regression is closed; no new findings against this commit.
Summary
Refs #496 — the settings-number half of the pinned-oracle divergence.
OverPy 9.7.10 renders numeric settings values with JavaScript
String(value)semantics (1e21→1e+21,0.0000001→1e-7,123456789012345678901234→1.2345678901234569e+23) and reads decimalexponent forms plus
0x/0b/0ointeger literals. opy-rs emitted RustDisplaytext and could not read exponent or binary/octal literals atall: the settings block is re-lexed for
#!defineexpansion and thelexer split
0b101into0+b101, which the re-render joined as0 101.lexer: keep0b/0oliterals as one number token like0x(also makes them valid in ordinary expressions, matching upstream's
JS tokenizer —
0b101 + 0o17folds to20)parser: evaluate radix number tokens inf64like JavaScriptNumber(nou64overflow cutoff for long digit runs)settings: accept exponent and radix literals in the JSONC cursorparser and render scalars plus bare list elements through the existing
compiler::number_format::javascript_text(nowpub(crate), withJavaScript spellings for non-finite values)
Verified byte-for-byte against the oracle: all 7 scalar probes and all
5 list elements emit identical text.
Remaining scope of #496
Catalogued settings keys whose authored value is an object/block still
fail (
settings key 'mapRotation' does not match its table kind).That is owned by
workshop-rs(canonical settings acceptance) andlands via
wrightkit/workshop-rs#413plus a releasedworkshop-rsversion bump here.
Test plan
cargo fmt --all -- --checkcargo clippy -p opy-rs --all-targets -- -D warningscargo test -p opy-rs --lib(523),--test tooling(13, incl.new
settings_numbers_render_like_the_pinned_oracle)cargo test -p opy-rs --test differential— oracle corpus agreespython3 -m unittest discover -s tools/overpy/tests(40)0b101→5,0o17→15,0x1F→31,1e21→1e+21,0.0000001→1e-7,123456789012345678901234→1.2345678901234569e+23,0.5e3→500,12e2→1200,1.0→1,1e20→100000000000000000000— all match