Add Decimal type with 14 built-in operations#340
Conversation
14 host-import operations for exact decimal arithmetic: decimal_from_int, decimal_from_float, decimal_from_string, decimal_to_string, decimal_to_float, decimal_add, decimal_sub, decimal_mul, decimal_div, decimal_neg, decimal_compare, decimal_eq, decimal_round, decimal_abs. Same opaque i32 handle pattern as Map/Set. Python runtime uses decimal.Decimal; browser runtime uses string-based arithmetic. New conformance test ch09_decimal (59 programs). 17 unit tests. Spec §9.7.2 updated with full documentation. Tick #62 Map in ROADMAP diagram. Bump version to 0.0.97. Closes #333 Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an opaque Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (compile)
participant Compiler as Vera Compiler
participant WASM as Generated WASM Module
participant Host as Host Runtime (browser JS)
Dev->>Compiler: compile Vera program using Decimal ops
Compiler->>Compiler: infer types, mark decimal_ops_used & register imports
Compiler->>WASM: emit module with import "vera".decimal_* entries
WASM->>Host: call host `decimal_*` function at runtime
Host->>Host: map handle ↔ value, perform arithmetic/compare, allocate Option/Ordering ADTs
Host-->>WASM: return i32 handles / pair results / ADT pointers
WASM-->>Dev: program runs with Decimal semantics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Decimal completes the standard library collections from issue #62, Map (v0.0.94), Set (v0.0.95), and Decimal (v0.0.97). All three use the same host-import pattern: Python/JS runtimes maintain native data structures behind opaque i32 handles. The WASM module never touches the actual values; all operations are delegated to the host via function calls. |
Co-Authored-By: Claude <noreply@anthropic.invalid>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
==========================================
- Coverage 95.43% 95.37% -0.06%
==========================================
Files 45 45
Lines 15892 16141 +249
==========================================
+ Hits 15167 15395 +228
- Misses 725 746 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude <noreply@anthropic.invalid>
…, compare branches) Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vera/checker/resolution.py (1)
80-85:⚠️ Potential issue | 🟠 MajorReject type arguments on
Decimalto preserve type-system correctness.This branch currently accepts parameterised
Decimalforms by constructingAdtType("Decimal", args). For an opaque non-generic Decimal type,Decimal<...>should be rejected with a diagnostic.🔧 Proposed fix
- # Array, Tuple, Map, Set, Decimal (built-in non-primitive types) - if name in ("Array", "Tuple", "Map", "Set", "Decimal"): + # Decimal (built-in opaque, non-parameterised type) + if name == "Decimal": + if te.type_args: + self._error( + te, + "Decimal does not take type arguments.", + rationale="Decimal is an opaque built-in type.", + fix="Use 'Decimal' without type parameters.", + ) + return UnknownType() + return AdtType(name, ()) + + # Array, Tuple, Map, Set (built-in non-primitive types) + if name in ("Array", "Tuple", "Map", "Set"): if te.type_args: args = tuple(self._resolve_type(a) for a in te.type_args) return AdtType(name, args) return AdtType(name, ())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/checker/resolution.py` around lines 80 - 85, In _resolve_type, treat Decimal as an opaque non-generic ADT: when name == "Decimal" and te.type_args is non-empty, emit a diagnostic using the checker’s existing reporting mechanism (e.g. self.diagnostic / self.report_error) indicating that Decimal does not accept type arguments, and return the non-parameterised form (AdtType("Decimal", ())) instead of constructing AdtType with args; otherwise continue returning AdtType("Decimal", ()) as before.SKILL.md (1)
1576-1576:⚠️ Potential issue | 🟡 MinorUpdate conformance suite count in documentation.
Line 1576 states the conformance directory contains 39 programmes, but it currently contains 59. Update to reflect the actual count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SKILL.md` at line 1576, Update the documentation sentence in SKILL.md that currently reads "The `tests/conformance/` directory contains 39 small, self-contained programs..." to reflect the current count by replacing "39" with "59" so it reads "contains 59 small, self-contained programs..."; locate the exact sentence mentioning `tests/conformance/` and change only the numeric count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 78: The roadmap line about Decimal’s monomorphization gaps is incomplete:
update the sentence referencing Option<Decimal> to state that Option<Decimal>
returned from decimal_div/decimal_from_string and Ordering from decimal_compare
cannot currently be used with match or option_unwrap_or due to monomorphization
gaps; specifically mention Option<Decimal>, decimal_div, decimal_from_string,
decimal_compare, Ordering, match, and option_unwrap_or so readers know both
match and option_unwrap_or are affected.
In `@scripts/check_skill_examples.py`:
- Around line 46-47: The ALLOWLIST dict contains duplicate numeric keys (e.g.,
974, 1274, 1364 and the groups around 99/119-127) so later entries silently
override earlier reasons; fix by collapsing duplicates into a single entry per
key (merge the reasons into one list/string) or rename keys so each line number
is unique, and update any references that expect the original entries (search
for the ALLOWLIST definition and entries named 974, 1274, 1364 and the
99/119-127 groups to perform the merge/rename).
In `@SKILL.md`:
- Around line 556-561: Update SKILL.md examples to document the codegen
limitation for decimal_compare: after the existing line that shows
"decimal_compare(`@Decimal.0`, `@Decimal.1`) -- returns Ordering", add a brief note
that pattern matching (match) on the Ordering result is not yet supported by
codegen (reference Ordering and match) and advise users to avoid match on
decimal_compare outputs; also add explicit example signatures for
decimal_from_string and decimal_div (showing their parameter lists and that they
return Option<Decimal>) alongside the existing warning about Option<Decimal>
usage and mention that Option<Decimal> cannot be used with option_unwrap_or or
match due to monomorphization limitations (reference decimal_from_string,
decimal_div, and option_unwrap_or) so readers see both the functions and their
current restrictions.
In `@spec/09-standard-library.md`:
- Line 1561: Update the note in §9.7.2 to also mention that `Ordering` values
produced by `decimal_compare` suffer the same monomorphization gap as
`Option<Decimal>` (i.e., cannot be used with `option_unwrap_or`/`match` because
the monomorphizer does not produce `option_unwrap_or$Decimal` or the equivalent
for `Ordering`); specifically add `Ordering`/`decimal_compare` to the known
limitation sentence alongside `Option<Decimal>` and the functions `decimal_div`
and `decimal_from_string` so the limitation is complete and accurate.
In `@TESTING.md`:
- Line 36: Update the outdated conformance program counts in the README: change
the phrase "58 small, focused programs" found at the sentence referenced by line
80 to "59 small, focused programs" and similarly update the other two
occurrences (the references currently at lines 307 and 349) to read 59 so all
counts match the actual number of `.vera` files in tests/conformance; ensure any
adjacent text that references the count (e.g., summaries, totals or parentheses)
is updated to use 59 as well.
In `@tests/test_checker.py`:
- Around line 1710-1767: The TestDecimalChecker currently covers only some
Decimal built-ins; add tests in tests/test_checker.py under the
TestDecimalChecker class for the remaining functions (create a pair of tests for
each symbol): decimal_from_float, decimal_from_string, decimal_sub, decimal_mul,
decimal_div, decimal_neg, decimal_compare, decimal_round, and decimal_abs. For
each built-in add one _check_ok case exercising correct types (e.g., calling
decimal_sub(decimal_from_int(2), decimal_from_int(1)) or
decimal_from_float(1.23)) and one _check_err case passing a wrong arg type
(e.g., plain Int or String) asserting the checker returns a "type" (or "expects"
for arity errors) message; name tests descriptively like test_decimal_sub and
test_decimal_sub_wrong_type to match the existing pattern and use the same
_check_ok/_check_err helpers and docstring style so they integrate with the
existing TestDecimalChecker.
In `@tests/test_codegen.py`:
- Around line 8177-8187: The test only checks negation of 42 and misses the -0
path which the browser runtime produces as "-0" string; update tests to include
the zero edge-case by adding an assertion that decimal_neg(decimal_from_int(0))
compares equal to decimal_from_int(0) (via decimal_eq) or add a separate test
function (e.g., test_decimal_neg_zero) that calls
decimal_neg(decimal_from_int(0)) and asserts equality using decimal_eq so the
browser runtime's "-0" is exercised and fails if not normalized.
- Around line 8214-8221: The test test_decimal_to_float is too weak (uses floor)
and should assert exact conversion results: change the assertion to compare
decimal_to_float(decimal_from_int(42)) exactly to 42.0 (or the expected float
representation) and add an exact-string assertion for
decimal_to_string(decimal_from_int(42)) == "42"; also add extra cases for zero
and a negative value (e.g., decimal_from_int(0) -> 0.0 and decimal_from_int(-7)
-> -7.0) to cover edge behavior and composition with decimal_to_string so broken
implementations like 42.9 or "24" fail; locate and edit the test methods
test_decimal_to_float and the related test around decimal_to_string to implement
these exact equality checks.
In `@vera/browser/runtime.mjs`:
- Around line 1373-1463: The decimal helpers currently convert decimal strings
to JS Number in functions like decimal_add, decimal_sub, decimal_mul,
decimal_div, decimal_round, decimal_to_float and decimal_compare which
reintroduces IEEE-754 errors and can produce unnormalized "-0"; instead
implement all arithmetic and rounding using string-based or BigInt-safe routines
(use or expand decStrAdd, decStrSub, decStrMul, decStrDiv and a string-based
round) so operations on values stored in decimalStore never pass through Number;
also normalize results (e.g., convert "-0" -> "0", trim leading zeros, and a
canonical sign/scale) and update decimal_eq and decimal_to_string to
compare/return normalized strings; keep decimal_div returning allocOptionNone()
on exact division-by-zero as before.
In `@vera/codegen/api.py`:
- Around line 1347-1368: The host function host_decimal_from_string duplicates
raw memory access; replace the manual memory reads (the bytes/mem.data_ptr and
raw.decode steps) with the existing _read_wasm_string helper to get the UTF-8
string. In other words, inside host_decimal_from_string (the
wasmtime.Caller-based function handling ptr and length) call s =
_read_wasm_string(caller, ptr, length) and keep the try/except InvalidOperation,
_decimal_alloc(d), and the _alloc_option_some_i32/_alloc_option_none behavior
unchanged; remove the direct mem access lines so the function uses the helper
for consistent WASM memory reads.
- Around line 1497-1511: The host_decimal_round handler can raise
decimal.InvalidOperation when places is extreme; update the host_decimal_round
implementation (registered via linker.define_func for "vera","decimal_round") to
validate or clamp the places parameter and/or wrap the quantize call in a
try/except: fetch d from _decimal_store[h], compute a safe clamped_places (e.g.
limit to a reasonable min/max), build q from that clamped value, call
d.quantize(q) inside try/except catching decimal.InvalidOperation and return a
safe fallback (for example the original _decimal_alloc(d) or an error sentinel)
using the existing _decimal_alloc logic so WASM traps are prevented.
In `@vera/wasm/calls.py`:
- Around line 7305-7314: The Decimal host-import translators that return
heap-backed data (e.g., _translate_decimal_from_string and the other translators
that call _register_decimal_import for Decimal functions returning
Option/Ordering pointers or String pairs) must set self.needs_alloc = True so
the allocator is enabled; after calling _register_decimal_import (the call that
produces wasm_name) and before returning the instruction list, set
self.needs_alloc = True and then return arg_instrs + [f"call {wasm_name}"].
Ensure this change is applied to each translator that registers Decimal imports
which return heap-backed values.
In `@vera/wasm/inference.py`:
- Around line 857-858: _infer_fncall_wasm_type currently doesn't return types
for Decimal builtins; add explicit cases mapping builtin names to the correct
WASM return types: return "i32" for decimal_from_int, decimal_add, decimal_sub,
decimal_mul, decimal_neg, decimal_round, decimal_abs, decimal_from_string,
decimal_div; return "f64" for decimal_to_float; return "i32" for decimal_eq and
decimal_compare; and return "i32_pair" for decimal_to_string. Place these checks
alongside the other builtin handlers inside _infer_fncall_wasm_type (use the
builtin name strings to locate the switch/if chain). Also simplify the
host-import handle check that now does multiple startswith calls (the one
checking Map<, Set<, Decimal) by using a single startswith with a tuple or an
any(...) pattern to make it clearer and consistent.
---
Outside diff comments:
In `@SKILL.md`:
- Line 1576: Update the documentation sentence in SKILL.md that currently reads
"The `tests/conformance/` directory contains 39 small, self-contained
programs..." to reflect the current count by replacing "39" with "59" so it
reads "contains 59 small, self-contained programs..."; locate the exact sentence
mentioning `tests/conformance/` and change only the numeric count.
In `@vera/checker/resolution.py`:
- Around line 80-85: In _resolve_type, treat Decimal as an opaque non-generic
ADT: when name == "Decimal" and te.type_args is non-empty, emit a diagnostic
using the checker’s existing reporting mechanism (e.g. self.diagnostic /
self.report_error) indicating that Decimal does not accept type arguments, and
return the non-parameterised form (AdtType("Decimal", ())) instead of
constructing AdtType with args; otherwise continue returning AdtType("Decimal",
()) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5889a9f-f312-4289-9f8e-c18686d159fb
⛔ Files ignored due to path filters (5)
docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**tests/conformance/ch09_decimal.verais excluded by!**/*.vera
📒 Files selected for processing (25)
CHANGELOG.mdCLAUDE.mdROADMAP.mdSKILL.mdTESTING.mdpyproject.tomlscripts/check_skill_examples.pyscripts/check_spec_examples.pyspec/09-standard-library.mdtests/conformance/manifest.jsontests/test_checker.pytests/test_codegen.pyvera/__init__.pyvera/browser/runtime.mjsvera/checker/resolution.pyvera/codegen/api.pyvera/codegen/assembly.pyvera/codegen/compilability.pyvera/codegen/core.pyvera/codegen/functions.pyvera/codegen/modules.pyvera/environment.pyvera/wasm/calls.pyvera/wasm/context.pyvera/wasm/inference.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
515-515:⚠️ Potential issue | 🟡 MinorUpdate version number to v0.0.97.
The PR summary explicitly states "Version bumped to v0.0.97", but this line still shows v0.0.96. This creates a factual inconsistency between the documented version and the actual release version.
📝 Proposed fix
-Vera is in **active development** at v0.0.96. The reference compiler — parser, AST, type checker, contract verifier (Z3), WASM code generator, module system, browser runtime, and runtime contract insertion — is working. Programs compile to WebAssembly and execute via wasmtime or in the browser. +Vera is in **active development** at v0.0.97. The reference compiler — parser, AST, type checker, contract verifier (Z3), WASM code generator, module system, browser runtime, and runtime contract insertion — is working. Programs compile to WebAssembly and execute via wasmtime or in the browser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 515, Update the documented release version string from "v0.0.96" to "v0.0.97" in the README entry that currently reads "Vera is in **active development** at v0.0.96." so the text matches the PR summary; locate and replace the exact token v0.0.96 with v0.0.97 in README.md (the human-readable release/version line).
♻️ Duplicate comments (3)
TESTING.md (1)
11-11:⚠️ Potential issue | 🟡 MinorConformance count is still inconsistent in this document.
These updated lines use 59, but Line 80, Line 307, and Line 349 still say 58. Please align all references to 59 to keep TESTING.md internally consistent.
As per coding guidelines, "
**/*.md: Review Markdown files for factual accuracy against the codebase, broken links, and outdated information."Also applies to: 36-36, 117-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TESTING.md` at line 11, Update the inconsistent conformance program count in TESTING.md so all mentions read "59" instead of "58": search for occurrences of the phrase "Conformance programs" and the numeric "58" (notably the lines referenced around Line 80, Line 307, Line 349 and the ranges around 36 and 117-125) and replace them with "59" so the table row "| **Conformance programs** | 59 programs across 9 spec chapters, validating every language feature |" and all standalone counts are consistent across the file.tests/test_codegen.py (2)
8177-8187:⚠️ Potential issue | 🟠 MajorAdd the zero-edge regression for
decimal_neg.Line 8177 only validates
neg(42), but it misses the zero path (-0) where host/runtime normalisation bugs usually surface. Please add an explicitdecimal_neg(decimal_from_int(0))equality assertion against zero.As per coding guidelines, "Add codegen/runtime tests in
tests/test_codegen.pywhen adding or modifying built-in functions — cover normal cases, edge cases (empty inputs, zero values), and composition with other built-ins".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8177 - 8187, The test test_decimal_neg only checks neg(42) but omits the zero edge-case where -0 can surface normalization bugs; update the test_decimal_neg source to also compute decimal_neg(decimal_from_int(0)) and assert via decimal_eq that it equals decimal_from_int(0) (e.g., include a separate equality check or combine into the existing if) so the call to decimal_neg and decimal_eq covers both 42 and 0 cases when invoking _run.
8214-8221:⚠️ Potential issue | 🟠 MajorStrengthen conversion assertions to exact-value checks.
Line 8219 and Line 8237 currently allow incorrect implementations through (
floor(...)and length-only checks). These should assert exact conversion values/strings, including zero/negative edge cases.Suggested tightening
- def test_decimal_to_float(self) -> None: - """Convert decimal 42 to float and check via floor.""" + def test_decimal_to_float(self) -> None: + """decimal_to_float preserves exact numeric value for integer decimals.""" source = """ -public fn main(-> `@Int`) +public fn main(-> `@Float64`) requires(true) ensures(true) effects(pure) -{ floor(decimal_to_float(decimal_from_int(42))) } +{ decimal_to_float(decimal_from_int(42)) } """ - assert _run(source) == 42 + assert _run_float(source) == 42.0 - def test_decimal_to_string_length(self) -> None: - """string_length(decimal_to_string(decimal_from_int(42))) > 0.""" - source = """ -public fn main(-> `@Int`) - requires(true) ensures(true) effects(pure) -{ string_length(decimal_to_string(decimal_from_int(42))) } -""" - assert _run(source) == 2 + def test_decimal_to_string(self) -> None: + """decimal_to_string renders exact decimal text.""" + source = _IO_PRELUDE + """ +public fn main(`@Unit` -> `@Unit`) + requires(true) ensures(true) effects(<IO>) +{ IO.print(decimal_to_string(decimal_from_int(42))) } +""" + assert _run_io(source, fn="main") == "42"As per coding guidelines, "DO flag: Tests that don't assert anything meaningful" and "Add codegen/runtime tests in
tests/test_codegen.pywhen adding or modifying built-in functions — cover normal cases, edge cases (empty inputs, zero values), and composition with other built-ins".Also applies to: 8232-8239, 8241-8249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8214 - 8221, The test_decimal_to_float currently asserts via floor(decimal_to_float(decimal_from_int(42))) which allows incorrect implementations; update test_decimal_to_float to assert the exact output value from _run (use direct equality to 42.0 or appropriate language literal) instead of wrapping with floor, and add sibling tests that call decimal_to_float/decimal_from_int for zero and negative cases (e.g., decimal_from_int(0) -> 0.0, decimal_from_int(-1) -> -1.0) to cover edge cases; locate and modify the test function named test_decimal_to_float and any similar tests around lines referencing decimal_to_float, decimal_from_int, floor, and _run to replace length/floor-style checks with exact-value assertions for positive, zero, and negative inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_codegen.py`:
- Around line 8277-8289: The test test_decimal_compare_less currently only calls
decimal_eq and doesn't exercise decimal_compare; update the test source so it
constructs two decimals via decimal_from_int (as already done) but calls
decimal_compare(`@Decimal.0`, `@Decimal.1`) and checks the returned ordering
(expecting a negative/“less than” result) instead of using decimal_eq; ensure
the assertion uses _run(...) to verify the decimal_compare return value (e.g.,
expecting -1 or whatever the host import returns for less-than) so the test
actually validates ordering and host import wiring for decimal_compare.
- Around line 8263-8275: The test function test_decimal_div_nonzero currently
performs decimal_add/decimal_round and never calls decimal_div; change the test
body so it actually invokes decimal_div (e.g., create `@Decimal` from 10 and 2,
call decimal_div(`@Decimal.0`, `@Decimal.1`) and then check decimal_round(..., 0) ==
decimal_from_int(5)) so the test calls decimal_div and still returns 1 on
success (update the comparison literal from 12 to 5); reference the test
function name test_decimal_div_nonzero, the helper _run, and the functions
decimal_from_int, decimal_div, decimal_round in your edit.
- Around line 8250-8262: The test test_decimal_from_string_valid is wrong
because it never calls decimal_from_string; update the test to invoke
decimal_from_string (not decimal_from_int) with at least one valid string (e.g.,
"0" and "42" or a single valid decimal string) and assert the result is the
expected Some/tag=1 value; use the existing decimal_eq helper (or compare .1/.0
as the pattern in the test) to verify equality and keep the test name and intent
unchanged so it actually exercises the decimal_from_string API.
---
Outside diff comments:
In `@README.md`:
- Line 515: Update the documented release version string from "v0.0.96" to
"v0.0.97" in the README entry that currently reads "Vera is in **active
development** at v0.0.96." so the text matches the PR summary; locate and
replace the exact token v0.0.96 with v0.0.97 in README.md (the human-readable
release/version line).
---
Duplicate comments:
In `@TESTING.md`:
- Line 11: Update the inconsistent conformance program count in TESTING.md so
all mentions read "59" instead of "58": search for occurrences of the phrase
"Conformance programs" and the numeric "58" (notably the lines referenced around
Line 80, Line 307, Line 349 and the ranges around 36 and 117-125) and replace
them with "59" so the table row "| **Conformance programs** | 59 programs across
9 spec chapters, validating every language feature |" and all standalone counts
are consistent across the file.
In `@tests/test_codegen.py`:
- Around line 8177-8187: The test test_decimal_neg only checks neg(42) but omits
the zero edge-case where -0 can surface normalization bugs; update the
test_decimal_neg source to also compute decimal_neg(decimal_from_int(0)) and
assert via decimal_eq that it equals decimal_from_int(0) (e.g., include a
separate equality check or combine into the existing if) so the call to
decimal_neg and decimal_eq covers both 42 and 0 cases when invoking _run.
- Around line 8214-8221: The test_decimal_to_float currently asserts via
floor(decimal_to_float(decimal_from_int(42))) which allows incorrect
implementations; update test_decimal_to_float to assert the exact output value
from _run (use direct equality to 42.0 or appropriate language literal) instead
of wrapping with floor, and add sibling tests that call
decimal_to_float/decimal_from_int for zero and negative cases (e.g.,
decimal_from_int(0) -> 0.0, decimal_from_int(-1) -> -1.0) to cover edge cases;
locate and modify the test function named test_decimal_to_float and any similar
tests around lines referencing decimal_to_float, decimal_from_int, floor, and
_run to replace length/floor-style checks with exact-value assertions for
positive, zero, and negative inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a949c16-ac9f-4931-9727-0b9a8317d8c2
📒 Files selected for processing (6)
README.mdTESTING.mdscripts/check_readme_examples.pytests/test_codegen.pytests/test_readme.pyvera/README.md
|
Critical (🔴):
Major (🟠): Minor (🟡): Trivial (🔵): Outside diff: |
- Fix 3 duplicate ALLOWLIST keys in check_skill_examples.py - Set needs_alloc for Decimal ops returning heap values (calls.py) - Add precision-loss comment to browser runtime decimal arithmetic - Fix decimal_neg to handle "-0" canonically - Add option_unwrap_or to ROADMAP/spec/SKILL limitation notes - Add Ordering/decimal_compare to limitation documentation - Separate Decimal from parameterised types in resolver - Use _read_wasm_string helper in host_decimal_from_string - Wrap decimal_round quantize in try/except for edge cases - Add 6 new checker tests + 2 codegen tests - Add Decimal entries to _infer_fncall_wasm_type - Update SKILL.md conformance count 39→59 - Update TESTING.md counts (2863→2871) - Regenerate site assets Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai Addressed both outside-diff items:
|
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
…rtions - 12 new checker tests (happy-path + wrong-type for all Decimal ops) - Replace test_decimal_to_float with exact 3.5 round-trip via _run_float - Replace test_decimal_to_string_length with exact IO assertion - Rename misleading tests that did not call their claimed function - Update doc counts (2883 tests) Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
tests/test_codegen.py (4)
8276-8288:⚠️ Potential issue | 🟠 Major
test_decimal_from_string_validnever exercisesdecimal_from_string.Line 8282-Line 8284 only compares two
decimal_from_intvalues, so parser/import regressions fordecimal_from_stringwill not be caught.💡 Proposed fix
def test_decimal_from_string_valid(self) -> None: - """decimal_from_string with valid input returns Some (tag=1).""" + """decimal_from_string host call succeeds for valid input.""" source = """ public fn main(-> `@Int`) requires(true) ensures(true) effects(pure) { - let `@Decimal` = decimal_from_int(0); - let `@Decimal` = decimal_from_int(42); - if decimal_eq(`@Decimal.1`, `@Decimal.0`) then { 0 } else { 1 } + let `@Option`<Decimal> = decimal_from_string("42"); + 1 } """ assert _run(source) == 1As per coding guidelines, "Add codegen/runtime tests in
tests/test_codegen.pywhen adding or modifying built-in functions — cover normal cases, edge cases (empty inputs, zero values), and composition with other built-ins".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8276 - 8288, The test named test_decimal_from_string_valid currently never calls decimal_from_string (it only uses decimal_from_int), so add a call to decimal_from_string in the test source and assert the expected result to exercise parsing/import paths; update the in-test source string inside test_decimal_from_string_valid to invoke decimal_from_string with a valid string (e.g., "0" and "42") instead of/in addition to decimal_from_int, then run via _run and adjust the final assertion to verify the returned tag/value from decimal_from_string (keep function name test_decimal_from_string_valid, helper _run, and comparisons to decimal_eq as appropriate).
8303-8315:⚠️ Potential issue | 🟠 Major
test_decimal_compare_lessnever callsdecimal_compare.Line 8311 only checks
decimal_eqon two different values, so ordering behaviour is untested.💡 Proposed fix
def test_decimal_compare_less(self) -> None: - """decimal_compare(1, 2) — test indirectly via decimal_eq.""" + """decimal_compare host call executes for less-than operands.""" source = """ public fn main(-> `@Int`) requires(true) ensures(true) effects(pure) { - let `@Decimal` = decimal_from_int(1); - let `@Decimal` = decimal_from_int(2); - if decimal_eq(`@Decimal.1`, `@Decimal.0`) then { 0 } else { 1 } + let `@Ordering` = decimal_compare(decimal_from_int(1), decimal_from_int(2)); + 1 } """ assert _run(source) == 1As per coding guidelines, "Add codegen/runtime tests in
tests/test_codegen.pywhen adding or modifying built-in functions — cover normal cases, edge cases (empty inputs, zero values), and composition with other built-ins".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8303 - 8315, The test test_decimal_compare_less never invokes decimal_compare but instead only calls decimal_eq; update the test source so it constructs two decimals with decimal_from_int (as currently done) and calls decimal_compare on them (e.g., decimal_compare(`@Decimal.0`, `@Decimal.1`)) and assert the runtime result equals the expected ordering value (e.g., -1 for 1 < 2). Ensure the test uses the decimal_compare call in the function body and that the final assert in test_decimal_compare_less checks the returned comparison integer.
8289-8301:⚠️ Potential issue | 🟠 Major
test_decimal_div_nonzerodoes not calldecimal_div.Line 8297-Line 8298 performs
decimal_add+decimal_round; division logic is not under test.💡 Proposed fix
def test_decimal_div_nonzero(self) -> None: - """decimal_div by nonzero works (test via decimal_eq on result).""" + """decimal_div host call executes for non-zero divisor.""" source = """ public fn main(-> `@Int`) requires(true) ensures(true) effects(pure) { - let `@Decimal` = decimal_from_int(10); - let `@Decimal` = decimal_from_int(2); - let `@Decimal` = decimal_add(`@Decimal.1`, `@Decimal.0`); - if decimal_eq(decimal_round(`@Decimal.0`, 0), decimal_from_int(12)) then { 1 } else { 0 } + let `@Option`<Decimal> = decimal_div(decimal_from_int(10), decimal_from_int(2)); + 1 } """ assert _run(source) == 1As per coding guidelines, "Add codegen/runtime tests in
tests/test_codegen.pywhen adding or modifying built-in functions — cover normal cases, edge cases (empty inputs, zero values), and composition with other built-ins".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8289 - 8301, The test test_decimal_div_nonzero is supposed to exercise decimal_div but currently uses decimal_add/decimal_round; update the inline Wig source in test_decimal_div_nonzero to call decimal_div on two nonzero decimals (use decimal_from_int(10) and decimal_from_int(2)), then assert the result via decimal_eq (or decimal_round + decimal_eq) to the expected decimal (e.g., decimal_from_int(5)). Locate the test by name and replace the decimal_add line with a call to decimal_div and adjust the final decimal_eq check to compare against the correct expected value.
8226-8247:⚠️ Potential issue | 🟠 MajorStrengthen decimal conversion assertions so broken implementations cannot pass.
Line 8231 (
floor(...)), Line 8242-Line 8244 (range-only), and Line 8263 (string_length(...)) are too weak and can pass with incorrect conversion/formatting behaviour.💡 Proposed fix
- def test_decimal_to_float(self) -> None: - """Convert decimal 42 to float and check via floor.""" + def test_decimal_to_float(self) -> None: + """decimal_to_float(decimal_from_int(42)) == 42.0 exactly.""" source = """ -public fn main(-> `@Int`) +public fn main(-> `@Float64`) requires(true) ensures(true) effects(pure) -{ floor(decimal_to_float(decimal_from_int(42))) } +{ decimal_to_float(decimal_from_int(42)) } """ - assert _run(source) == 42 + assert _run_float(source) == 42.0 def test_decimal_to_float_non_integer(self) -> None: - """decimal_to_float(decimal_from_float(3.14)) round-trips correctly.""" + """decimal_to_float preserves fractional component.""" source = """ -public fn main(-> `@Int`) +public fn main(-> `@Float64`) requires(true) ensures(true) effects(pure) { - let `@Float64` = decimal_to_float(decimal_from_float(3.14)); - if `@Float64.0` > 3.0 then { - if `@Float64.0` < 4.0 then { 1 } else { 0 } - } else { 0 } + decimal_to_float(decimal_from_float(3.14)) } """ - assert _run(source) == 1 + assert abs(_run_float(source) - 3.14) < 1e-12 - def test_decimal_to_string_length(self) -> None: - """string_length(decimal_to_string(decimal_from_int(42))) > 0.""" - source = """ -public fn main(-> `@Int`) - requires(true) ensures(true) effects(pure) -{ string_length(decimal_to_string(decimal_from_int(42))) } -""" - assert _run(source) == 2 + def test_decimal_to_string(self) -> None: + """decimal_to_string(decimal_from_int(42)) renders exactly '42'.""" + source = _IO_PRELUDE + """ +public fn main(`@Unit` -> `@Unit`) + requires(true) ensures(true) effects(<IO>) +{ IO.print(decimal_to_string(decimal_from_int(42))) } +""" + assert _run_io(source, fn="main") == "42"As per coding guidelines, "DO flag: Tests that don't assert anything meaningful" and "Add codegen/runtime tests in
tests/test_codegen.pywhen adding or modifying built-in functions — cover normal cases, edge cases (empty inputs, zero values), and composition with other built-ins".Also applies to: 8258-8266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8226 - 8247, The tests for decimal conversion (test_decimal_to_float and test_decimal_to_float_non_integer) are too weak—replace the loose floor/range checks with stricter assertions that validate exact or tightly-bounded round-trip behavior for decimal_to_float/decimal_from_int and decimal_to_float/decimal_from_float: assert exact float values where appropriate (e.g. decimal_from_int(42) -> 42.0), test that decimal_from_float(x) -> decimal_to_float returns a value within machine epsilon of the original (or assert exact repr for known literals like 3.14), and add edge-case cases (zero, negative, large, fractional extremes); similarly tighten any string/format checks (string_length usage) to assert exact formatted output and length for known inputs so broken conversions/formatting cannot pass; update test names and bodies (test_decimal_to_float, test_decimal_to_float_non_integer and the string-length-related tests) accordingly.vera/browser/runtime.mjs (2)
1452-1456:⚠️ Potential issue | 🟠 Major
decimal_eqis testing formatting rather than numeric value.Two Decimal handles storing
"1"and"1.0"should compare equal, but this branch returnsfalsebecause it compares the raw stored strings. Canonicalise on insert or compare normalised decimal values instead.Based on learnings: in
vera/browser/runtime.mjs,decimal_eqcompares raw stored string representations (e.g."1.0"and"1"are NOT equal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/browser/runtime.mjs` around lines 1452 - 1456, The decimal_eq implementation currently compares raw strings from decimalStore and thus treats "1" and "1.0" as different; update the comparison in imports.vera.decimal_eq (or normalize on insert into decimalStore) to compare canonicalised numeric values instead of raw formatting: either store a normalized representation when values are inserted into decimalStore (e.g., using a Decimal library or a normalization function that strips insignificant trailing zeros and normalizes scale) or make decimal_eq fetch both entries and compare their normalized forms (e.g., Decimal.normalize(a) === Decimal.normalize(b) or apply the same trimming/scale normalization to both strings) so numerically-equal decimals evaluate as equal.
1388-1391:⚠️ Potential issue | 🔴 CriticalBrowser Decimal operations are still lossy and target-divergent.
decStrAdd/Sub/Mul/Div,decimal_compare, anddecimal_roundstill route throughNumber/Math.round, so the browser target does not preserve exact-decimal semantics. That also letsdecimal_divtreat very small non-zero divisors such as"1e-1000"as zero after underflow, and halfway rounding now follows JS rounding rather than Decimal semantics.Based on learnings: arithmetic ops (
decimal_add,decimal_sub,decimal_mul,decimal_div,decimal_round,decimal_compare) all convert through JSNumber()(IEEE 754 binary float), which loses precision for large or high-precision values.Also applies to: 1427-1450, 1458-1464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/browser/runtime.mjs` around lines 1388 - 1391, The browser implementations of decStrAdd, decStrSub, decStrMul, decStrDiv (and related decimal_compare, decimal_round, decimal_div) incorrectly convert decimals via Number/Math.round causing precision loss and wrong rounding/underflow behavior; replace the Number-based approach by using a proper arbitrary-precision decimal routine (e.g., integrate a small decimal library like decimal.js/decimal.js-light or implement string/BigInt scaled arithmetic) to perform addition, subtraction, multiplication, division, comparison, and rounding directly on decimal strings; ensure decimal_div treats tiny non-zero divisors (e.g., "1e-1000") as non-zero by using the chosen decimal implementation’s division and implement Decimal-standard "half" rounding rules in decimal_round and decimal_compare instead of Math.round/Number conversions.vera/codegen/api.py (1)
1494-1505:⚠️ Potential issue | 🟠 MajorGuard quantiser construction as well as
quantize().Line 1500 is evaluated outside the
try, so extremeplacesvalues can still raise while buildingqand escape as a WASM trap. Move the power expression into the guarded section (or clamp first) so the fallback covers the whole rounding path.♻️ Proposed fix
if "decimal_round" in result.decimal_ops_used: def host_decimal_round( _caller: wasmtime.Caller, h: int, places: int, ) -> int: d = _decimal_store[h] - # Use quantize for precise rounding - q = PyDecimal(10) ** -places try: + # Use quantize for precise rounding + q = PyDecimal(10) ** -places return _decimal_alloc(d.quantize(q)) except InvalidOperation: # Extreme exponent — return original value unchanged return _decimal_alloc(d)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/codegen/api.py` around lines 1494 - 1505, The quantiser creation (q = PyDecimal(10) ** -places) can raise outside the try and cause a WASM trap; update host_decimal_round so the power expression is performed inside the try (or clamp/validate places before) so both constructing q and calling d.quantize(q) are guarded; modify the function around _decimal_store, PyDecimal, _decimal_alloc and InvalidOperation to ensure any exception during quantiser creation falls through to the same fallback that returns _decimal_alloc(d).tests/test_checker.py (1)
1710-1814:⚠️ Potential issue | 🟠 MajorDecimal checker coverage is still incomplete.
This block still misses
_check_okcases fordecimal_from_float,decimal_sub, anddecimal_mul, and nearly all of the new Decimal built-ins still lack a wrong-type assertion. It also does not lock in the non-parameterisedDecimalrule with a rejection case such as@Decimal<Int>, so both the builtin signatures and the resolver edge case can regress quietly.As per coding guidelines: "
tests/test_checker.py: Add type checker tests intests/test_checker.pywhen adding or modifying built-in functions — at minimum, one test with correct types and one with a wrong argument type" and "DO flag: Missing edge cases for new compiler features."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_checker.py` around lines 1710 - 1814, Add missing test cases in the TestDecimalChecker suite: include _check_ok cases for decimal_from_float, decimal_sub, and decimal_mul, and add corresponding _check_err cases that pass wrong-typed arguments (e.g., Int where Decimal is expected) for each Decimal builtin (decimal_from_float, decimal_from_int, decimal_add, decimal_sub, decimal_mul, decimal_div, decimal_eq, decimal_compare, decimal_round, decimal_neg, decimal_abs, decimal_to_string, decimal_to_float) using the same pattern as existing tests (_check_ok/_check_err with "type" or "expects" assertions); also add a rejection test asserting that a parameterized Decimal like `@Decimal`<Int> is invalid to lock in the non-parameterised Decimal rule — place these tests in the TestDecimalChecker class alongside the existing methods to exercise functions such as decimal_from_float, decimal_sub, decimal_mul and the resolver edge case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/09-standard-library.md`:
- Line 1513: The spec currently contradicts itself: the "9.7.2 Decimal" section
is documented as implemented but §9.1 still lists Decimal under "Future types"
and §9.7 is titled "Built-in Types (Future)"; update the headings and lists to
be consistent by removing the "Future" marker where Decimal is implemented or
moving Decimal out of the §9.1 "Future types" list and renaming §9.7 from
"Built-in Types (Future)" to "Built-in Types" (or vice versa if Decimal should
remain future), and ensure all references to Decimal across §9.1 and §9.7 use
the same status label so the document is not contradictory.
- Around line 1515-1546: The spec currently claims Decimal provides exact
decimal arithmetic but does not document the browser runtime limitation where
arithmetic operations (decimal_add, decimal_sub, decimal_mul, decimal_div,
decimal_round, decimal_compare) route through Number() and lose precision;
update the spec text around the Decimal description to either (a) scope the
“exact arithmetic” guarantee to the Python/host runtime only, or (b) add a clear
note describing the browser MVP limitation (mention that decimal equality uses
raw string comparison but arithmetic is lossy), list the affected functions
(decimal_add, decimal_sub, decimal_mul, decimal_div, decimal_round,
decimal_compare), and reference the browser runtime migration to an
arbitrary-precision decimal library as the planned fix.
In `@vera/checker/resolution.py`:
- Around line 80-83: The Decimal branch currently drops any type arguments
(returning AdtType("Decimal", ())) instead of rejecting them; update the
resolver in resolution.py so that when encountering name == "Decimal" it checks
te.type_args and raises a resolution/type error if any type arguments are
present (mirroring how Array/Tuple/Map/Set are validated), otherwise return
AdtType("Decimal", ()). Refer to the existing handling of te.type_args in the
Array/Tuple code paths and use the same error-reporting mechanism used elsewhere
in the resolver to produce a clear rejection for parameterised Decimal.
In `@vera/wasm/calls.py`:
- Around line 257-293: These WASM builtin handlers (the call.name checks for
"decimal_from_int", "decimal_from_float", "decimal_from_string",
"decimal_to_string", "decimal_to_float", "decimal_add", "decimal_sub",
"decimal_mul", "decimal_div", "decimal_neg", "decimal_compare", "decimal_eq",
"decimal_round", "decimal_abs") must be gated for non-browser targets; update
the dispatch in vera/wasm/calls.py to detect the target/runtime (use the
existing env/target flag or runtime check) and only return the decimal
translation paths (_translate_decimal_unary, _translate_decimal_binary,
_translate_decimal_div, _translate_decimal_compare, _translate_decimal_eq,
_translate_decimal_round, _translate_decimal_from_string,
_translate_decimal_to_string) when the backend can guarantee exact decimals,
otherwise skip/raise or fall back to non-exact behavior for browser builds so
browser/WASM runs do not expose precision-dependent Decimal builtins.
---
Duplicate comments:
In `@tests/test_checker.py`:
- Around line 1710-1814: Add missing test cases in the TestDecimalChecker suite:
include _check_ok cases for decimal_from_float, decimal_sub, and decimal_mul,
and add corresponding _check_err cases that pass wrong-typed arguments (e.g.,
Int where Decimal is expected) for each Decimal builtin (decimal_from_float,
decimal_from_int, decimal_add, decimal_sub, decimal_mul, decimal_div,
decimal_eq, decimal_compare, decimal_round, decimal_neg, decimal_abs,
decimal_to_string, decimal_to_float) using the same pattern as existing tests
(_check_ok/_check_err with "type" or "expects" assertions); also add a rejection
test asserting that a parameterized Decimal like `@Decimal`<Int> is invalid to
lock in the non-parameterised Decimal rule — place these tests in the
TestDecimalChecker class alongside the existing methods to exercise functions
such as decimal_from_float, decimal_sub, decimal_mul and the resolver edge case.
In `@tests/test_codegen.py`:
- Around line 8276-8288: The test named test_decimal_from_string_valid currently
never calls decimal_from_string (it only uses decimal_from_int), so add a call
to decimal_from_string in the test source and assert the expected result to
exercise parsing/import paths; update the in-test source string inside
test_decimal_from_string_valid to invoke decimal_from_string with a valid string
(e.g., "0" and "42") instead of/in addition to decimal_from_int, then run via
_run and adjust the final assertion to verify the returned tag/value from
decimal_from_string (keep function name test_decimal_from_string_valid, helper
_run, and comparisons to decimal_eq as appropriate).
- Around line 8303-8315: The test test_decimal_compare_less never invokes
decimal_compare but instead only calls decimal_eq; update the test source so it
constructs two decimals with decimal_from_int (as currently done) and calls
decimal_compare on them (e.g., decimal_compare(`@Decimal.0`, `@Decimal.1`)) and
assert the runtime result equals the expected ordering value (e.g., -1 for 1 <
2). Ensure the test uses the decimal_compare call in the function body and that
the final assert in test_decimal_compare_less checks the returned comparison
integer.
- Around line 8289-8301: The test test_decimal_div_nonzero is supposed to
exercise decimal_div but currently uses decimal_add/decimal_round; update the
inline Wig source in test_decimal_div_nonzero to call decimal_div on two nonzero
decimals (use decimal_from_int(10) and decimal_from_int(2)), then assert the
result via decimal_eq (or decimal_round + decimal_eq) to the expected decimal
(e.g., decimal_from_int(5)). Locate the test by name and replace the decimal_add
line with a call to decimal_div and adjust the final decimal_eq check to compare
against the correct expected value.
- Around line 8226-8247: The tests for decimal conversion (test_decimal_to_float
and test_decimal_to_float_non_integer) are too weak—replace the loose
floor/range checks with stricter assertions that validate exact or
tightly-bounded round-trip behavior for decimal_to_float/decimal_from_int and
decimal_to_float/decimal_from_float: assert exact float values where appropriate
(e.g. decimal_from_int(42) -> 42.0), test that decimal_from_float(x) ->
decimal_to_float returns a value within machine epsilon of the original (or
assert exact repr for known literals like 3.14), and add edge-case cases (zero,
negative, large, fractional extremes); similarly tighten any string/format
checks (string_length usage) to assert exact formatted output and length for
known inputs so broken conversions/formatting cannot pass; update test names and
bodies (test_decimal_to_float, test_decimal_to_float_non_integer and the
string-length-related tests) accordingly.
In `@vera/browser/runtime.mjs`:
- Around line 1452-1456: The decimal_eq implementation currently compares raw
strings from decimalStore and thus treats "1" and "1.0" as different; update the
comparison in imports.vera.decimal_eq (or normalize on insert into decimalStore)
to compare canonicalised numeric values instead of raw formatting: either store
a normalized representation when values are inserted into decimalStore (e.g.,
using a Decimal library or a normalization function that strips insignificant
trailing zeros and normalizes scale) or make decimal_eq fetch both entries and
compare their normalized forms (e.g., Decimal.normalize(a) ===
Decimal.normalize(b) or apply the same trimming/scale normalization to both
strings) so numerically-equal decimals evaluate as equal.
- Around line 1388-1391: The browser implementations of decStrAdd, decStrSub,
decStrMul, decStrDiv (and related decimal_compare, decimal_round, decimal_div)
incorrectly convert decimals via Number/Math.round causing precision loss and
wrong rounding/underflow behavior; replace the Number-based approach by using a
proper arbitrary-precision decimal routine (e.g., integrate a small decimal
library like decimal.js/decimal.js-light or implement string/BigInt scaled
arithmetic) to perform addition, subtraction, multiplication, division,
comparison, and rounding directly on decimal strings; ensure decimal_div treats
tiny non-zero divisors (e.g., "1e-1000") as non-zero by using the chosen decimal
implementation’s division and implement Decimal-standard "half" rounding rules
in decimal_round and decimal_compare instead of Math.round/Number conversions.
In `@vera/codegen/api.py`:
- Around line 1494-1505: The quantiser creation (q = PyDecimal(10) ** -places)
can raise outside the try and cause a WASM trap; update host_decimal_round so
the power expression is performed inside the try (or clamp/validate places
before) so both constructing q and calling d.quantize(q) are guarded; modify the
function around _decimal_store, PyDecimal, _decimal_alloc and InvalidOperation
to ensure any exception during quantiser creation falls through to the same
fallback that returns _decimal_alloc(d).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6b6cdd7-3535-4dfe-92c9-6f43c7265c27
⛔ Files ignored due to path filters (1)
docs/llms-full.txtis excluded by!docs/**
📒 Files selected for processing (12)
ROADMAP.mdSKILL.mdTESTING.mdscripts/check_skill_examples.pyspec/09-standard-library.mdtests/test_checker.pytests/test_codegen.pyvera/browser/runtime.mjsvera/checker/resolution.pyvera/codegen/api.pyvera/wasm/calls.pyvera/wasm/inference.py
| # Decimal builtins | ||
| if call.name == "decimal_from_int" and len(call.args) == 1: | ||
| return self._translate_decimal_unary( | ||
| call, env, "decimal_from_int", "i64", "i32") | ||
| if call.name == "decimal_from_float" and len(call.args) == 1: | ||
| return self._translate_decimal_unary( | ||
| call, env, "decimal_from_float", "f64", "i32") | ||
| if call.name == "decimal_from_string" and len(call.args) == 1: | ||
| return self._translate_decimal_from_string(call, env) | ||
| if call.name == "decimal_to_string" and len(call.args) == 1: | ||
| return self._translate_decimal_to_string(call, env) | ||
| if call.name == "decimal_to_float" and len(call.args) == 1: | ||
| return self._translate_decimal_unary( | ||
| call, env, "decimal_to_float", "i32", "f64") | ||
| if call.name == "decimal_add" and len(call.args) == 2: | ||
| return self._translate_decimal_binary( | ||
| call, env, "decimal_add") | ||
| if call.name == "decimal_sub" and len(call.args) == 2: | ||
| return self._translate_decimal_binary( | ||
| call, env, "decimal_sub") | ||
| if call.name == "decimal_mul" and len(call.args) == 2: | ||
| return self._translate_decimal_binary( | ||
| call, env, "decimal_mul") | ||
| if call.name == "decimal_div" and len(call.args) == 2: | ||
| return self._translate_decimal_div(call, env) | ||
| if call.name == "decimal_neg" and len(call.args) == 1: | ||
| return self._translate_decimal_unary( | ||
| call, env, "decimal_neg", "i32", "i32") | ||
| if call.name == "decimal_compare" and len(call.args) == 2: | ||
| return self._translate_decimal_compare(call, env) | ||
| if call.name == "decimal_eq" and len(call.args) == 2: | ||
| return self._translate_decimal_eq(call, env) | ||
| if call.name == "decimal_round" and len(call.args) == 2: | ||
| return self._translate_decimal_round(call, env) | ||
| if call.name == "decimal_abs" and len(call.args) == 1: | ||
| return self._translate_decimal_unary( | ||
| call, env, "decimal_abs", "i32", "i32") |
There was a problem hiding this comment.
Do not expose Decimal as exact on the browser/WASM path yet.
These built-ins are now reachable from WASM programs, but the browser host still implements Decimal arithmetic through JS Number, so browser builds cannot uphold the exact-decimal contract from the PR objective. That turns decimal_add/decimal_sub/decimal_mul/decimal_div/decimal_round/decimal_compare/decimal_eq into target-dependent behaviour for a type that is supposed to be precision-safe. Please either gate these entries for browser targets or land an exact browser backend before release.
Based on learnings: in vera/browser/runtime.mjs, decimal_eq compares raw stored strings and the arithmetic ops convert through JS Number(), which loses precision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vera/wasm/calls.py` around lines 257 - 293, These WASM builtin handlers (the
call.name checks for "decimal_from_int", "decimal_from_float",
"decimal_from_string", "decimal_to_string", "decimal_to_float", "decimal_add",
"decimal_sub", "decimal_mul", "decimal_div", "decimal_neg", "decimal_compare",
"decimal_eq", "decimal_round", "decimal_abs") must be gated for non-browser
targets; update the dispatch in vera/wasm/calls.py to detect the target/runtime
(use the existing env/target flag or runtime check) and only return the decimal
translation paths (_translate_decimal_unary, _translate_decimal_binary,
_translate_decimal_div, _translate_decimal_compare, _translate_decimal_eq,
_translate_decimal_round, _translate_decimal_from_string,
_translate_decimal_to_string) when the backend can guarantee exact decimals,
otherwise skip/raise or fall back to non-exact behavior for browser builds so
browser/WASM runs do not expose precision-dependent Decimal builtins.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/test_checker.py (1)
1712-1750:⚠️ Potential issue | 🟠 MajorAdd the missing wrong-type checker cases for four Decimal built-ins.
Coverage is still incomplete for
decimal_from_int,decimal_eq,decimal_to_string, anddecimal_to_float: each has a happy-path test but no explicit wrong-argument-type test. Please add one_check_err(..., "type")case per built-in to complete the checker contract.Proposed patch
@@ def test_decimal_to_float(self) -> None: """decimal_to_float(`@Decimal` -> `@Float64`) type checks OK.""" _check_ok(""" private fn foo(`@Unit` -> `@Float64`) requires(true) ensures(true) effects(pure) { decimal_to_float(decimal_from_int(42)) } """) + + def test_decimal_from_int_wrong_type(self) -> None: + """decimal_from_int with Float64 arg produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@Decimal`) + requires(true) ensures(true) effects(pure) +{ decimal_from_int(3.14) } +""", "type") + + def test_decimal_eq_wrong_type(self) -> None: + """decimal_eq with Int args produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@Bool`) + requires(true) ensures(true) effects(pure) +{ decimal_eq(1, 1) } +""", "type") + + def test_decimal_to_string_wrong_type(self) -> None: + """decimal_to_string with Int arg produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@String`) + requires(true) ensures(true) effects(pure) +{ decimal_to_string(42) } +""", "type") + + def test_decimal_to_float_wrong_type(self) -> None: + """decimal_to_float with Int arg produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@Float64`) + requires(true) ensures(true) effects(pure) +{ decimal_to_float(42) } +""", "type")As per coding guidelines: "
tests/test_checker.py: Add type checker tests intests/test_checker.pywhen adding or modifying built-in functions — at minimum, one test with correct types and one with a wrong argument type".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_checker.py` around lines 1712 - 1750, Add missing wrong-type checker tests: for each existing happy-path test functions (test_decimal_from_int, test_decimal_eq, test_decimal_to_string, test_decimal_to_float) add a corresponding _check_err(...) case that calls the same built-in (decimal_from_int, decimal_eq, decimal_to_string, decimal_to_float) with an incorrect argument type and asserts a "type" error; place each new _check_err next to its _check_ok sibling and ensure the expected error string is "type".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TESTING.md`:
- Line 73: Update the stale pytest count in the Markdown so both references
match the actual total for test_conformance.py: change the occurrence that
currently reads "290 tests" to "295 tests" so it aligns with the table row for
`test_conformance.py` stating 295 conformance tests.
In `@tests/test_codegen.py`:
- Around line 8271-8310: The two tests test_decimal_from_int_different_values
and test_decimal_inequality duplicate the same check that decimal_eq returns
false for two different decimal_from_int values; keep one as-is (e.g.,
test_decimal_from_int_different_values) and change the other
(test_decimal_inequality) to exercise a non-overlapping edge case: for example,
use decimal_from_int with a negative value, a large value, or compare
decimal_round behavior (use decimal_from_int, decimal_eq, decimal_round or
decimal_add to form the new assertion). Update test_decimal_inequality to
construct that distinct scenario so it no longer duplicates
decimal_eq(`@Decimal.1`, `@Decimal.0`) on two small positive ints.
- Around line 8262-8269: The test test_decimal_from_float uses floor(...) == 3
which is too weak; change the assertion to compare the actual float produced by
decimal_to_float(decimal_from_float(3.14)) against the exact representable float
value (i.e., assert decimal_to_float(decimal_from_float(3.14)) ==
<expected_float_literal>), or capture the result in a variable and assert exact
equality to the language's nearest float representation for 3.14, rather than
using floor.
- Around line 8311-8386: The tests (test_decimal_div_host_called,
test_decimal_from_string_host_called, test_decimal_compare_host_called, and the
related decimal_* tests) only return constants so they don't validate that the
host functions are actually wired; update each test to assert the compiled WAT
or codegen output contains the expected import/call for the host symbol (e.g.,
"decimal_div", "decimal_from_string", "decimal_compare") or the generated
callsite (call/import names used by your codegen), by invoking the test helper
that returns WAT (or adding a helper to compile to WAT) and asserting the
presence of the host import/call pattern for that specific function instead of
just returning a constant; keep the existing runtime assertions for coverage but
add the WAT/assertion check to ensure the host wiring is exercised.
---
Duplicate comments:
In `@tests/test_checker.py`:
- Around line 1712-1750: Add missing wrong-type checker tests: for each existing
happy-path test functions (test_decimal_from_int, test_decimal_eq,
test_decimal_to_string, test_decimal_to_float) add a corresponding
_check_err(...) case that calls the same built-in (decimal_from_int, decimal_eq,
decimal_to_string, decimal_to_float) with an incorrect argument type and asserts
a "type" error; place each new _check_err next to its _check_ok sibling and
ensure the expected error string is "type".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4ee7fe4-66cd-4941-9f75-05996ae636c0
📒 Files selected for processing (3)
TESTING.mdtests/test_checker.pytests/test_codegen.py
| def test_decimal_from_float(self) -> None: | ||
| """decimal_from_float round-trips through decimal_to_float.""" | ||
| source = """ | ||
| public fn main(-> @Int) | ||
| requires(true) ensures(true) effects(pure) | ||
| { floor(decimal_to_float(decimal_from_float(3.14))) } | ||
| """ | ||
| assert _run(source) == 3 |
There was a problem hiding this comment.
test_decimal_from_float uses a weak oracle.
At Line [8267], floor(...) == 3 can still pass with incorrect fractional conversion (for example any 3.x). Please assert an exact representable float value instead.
Proposed tightening
- def test_decimal_from_float(self) -> None:
- """decimal_from_float round-trips through decimal_to_float."""
- source = """
-public fn main(-> `@Int`)
- requires(true) ensures(true) effects(pure)
-{ floor(decimal_to_float(decimal_from_float(3.14))) }
-"""
- assert _run(source) == 3
+ def test_decimal_from_float(self) -> None:
+ """decimal_from_float preserves an exactly representable fractional value."""
+ source = """
+public fn main(-> `@Float64`)
+ requires(true) ensures(true) effects(pure)
+{ decimal_to_float(decimal_from_float(-7.5)) }
+"""
+ assert _run_float(source) == -7.5As per coding guidelines, "DO flag: Tests that don't assert anything meaningful".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_codegen.py` around lines 8262 - 8269, The test
test_decimal_from_float uses floor(...) == 3 which is too weak; change the
assertion to compare the actual float produced by
decimal_to_float(decimal_from_float(3.14)) against the exact representable float
value (i.e., assert decimal_to_float(decimal_from_float(3.14)) ==
<expected_float_literal>), or capture the result in a variable and assert exact
equality to the language's nearest float representation for 3.14, rather than
using floor.
- Remove Decimal from spec Future types list, add as implemented type - Rename §9.7 from 'Built-in Types (Future)' to 'Built-in Types' - Document browser runtime precision limitation in spec §9.7.2 - Reject Decimal<T> with error in resolver (Decimal is not parameterised) - Add test_decimal_rejects_type_args checker test - Fix 66 shifted spec allowlist line numbers - Update doc counts (2884 tests) Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai Addressed all 4 items from the latest review:
|
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/test_checker.py (1)
1712-1913:⚠️ Potential issue | 🟠 MajorAdd wrong-type negative tests for four remaining Decimal built-ins.
From Line 1712 onwards,
decimal_from_int,decimal_eq,decimal_to_string, anddecimal_to_floatcurrently have happy-path checks only. Please add one wrong-argument-type case for each to keep checker signature enforcement fully covered.Suggested additions
+ def test_decimal_from_int_wrong_type(self) -> None: + """decimal_from_int with String arg produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@Decimal`) + requires(true) ensures(true) effects(pure) +{ decimal_from_int("42") } +""", "type") + + def test_decimal_eq_wrong_type(self) -> None: + """decimal_eq with Int args produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@Bool`) + requires(true) ensures(true) effects(pure) +{ decimal_eq(1, 1) } +""", "type") + + def test_decimal_to_string_wrong_type(self) -> None: + """decimal_to_string with Int arg produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@String`) + requires(true) ensures(true) effects(pure) +{ decimal_to_string(42) } +""", "type") + + def test_decimal_to_float_wrong_type(self) -> None: + """decimal_to_float with Int arg produces error.""" + _check_err(""" +private fn foo(`@Unit` -> `@Float64`) + requires(true) ensures(true) effects(pure) +{ decimal_to_float(42) } +""", "type")As per coding guidelines: "
tests/test_checker.py: Add type checker tests intests/test_checker.pywhen adding or modifying built-in functions — at minimum, one test with correct types and one with a wrong argument type".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_checker.py` around lines 1712 - 1913, Add negative wrong-type tests for the four Decimal built-ins that currently only have happy-path tests: decimal_from_int, decimal_eq, decimal_to_string, and decimal_to_float. For each corresponding test function (e.g. add test_decimal_from_int_wrong_type, test_decimal_eq_wrong_type, test_decimal_to_string_wrong_type, test_decimal_to_float_wrong_type) add a _check_err call that invokes the built-in with an incorrect argument type (e.g. pass an Int where a Decimal or String/Float64 is expected) and assert a "type" error; place them alongside the existing test_* functions in tests/test_checker.py so signature enforcement is covered.TESTING.md (1)
149-149:⚠️ Potential issue | 🟡 MinorUpdate the stale pytest count to match the current total.
Line 149 still references "290 tests", but line 73 correctly states 295 conformance tests (59 programs × 5 checks). This discrepancy was flagged in a previous review and remains unaddressed.
Suggested fix
-# Via pytest (parametrized — 290 tests) +# Via pytest (parametrized — 295 tests) pytest tests/test_conformance.py -vAs per coding guidelines,
**/*.md: review Markdown files for factual accuracy against the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TESTING.md` at line 149, Update the stale pytest test count in the TESTING.md header that currently reads "# Via pytest (parametrized — 290 tests)" to the correct total (295 tests) to match the conformance count stated earlier ("295 conformance tests (59 programs × 5 checks)"); simply replace "290" with "295" so both references are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/09-standard-library.md`:
- Line 1566: Update the spec sentence to clarify that browser `decimal_eq`
performs strict representation equality (string comparison) rather than numeric
equivalence: mention that `decimal_eq` uses exact string comparison of decimal
representations so `"1.0"` ≠ `"1"`, and contrast this with the Python runtime’s
numeric equivalence; reference the browser runtime operations (`decimal_add`,
`decimal_sub`, `decimal_mul`, `decimal_div`, `decimal_round`, `decimal_compare`,
`decimal_eq`) and state that only `decimal_eq` is representation-based in the
browser MVP.
In `@TESTING.md`:
- Line 9: Update the summary row in TESTING.md that currently reads the tests
total as "~32,100 lines of test code" (the table row starting with "| **Tests**
| 2,884 across 26 files") to "~33,100 lines of test code" so the overview
matches the sum of the "Lines" column in the test file table.
---
Duplicate comments:
In `@TESTING.md`:
- Line 149: Update the stale pytest test count in the TESTING.md header that
currently reads "# Via pytest (parametrized — 290 tests)" to the correct total
(295 tests) to match the conformance count stated earlier ("295 conformance
tests (59 programs × 5 checks)"); simply replace "290" with "295" so both
references are consistent.
In `@tests/test_checker.py`:
- Around line 1712-1913: Add negative wrong-type tests for the four Decimal
built-ins that currently only have happy-path tests: decimal_from_int,
decimal_eq, decimal_to_string, and decimal_to_float. For each corresponding test
function (e.g. add test_decimal_from_int_wrong_type, test_decimal_eq_wrong_type,
test_decimal_to_string_wrong_type, test_decimal_to_float_wrong_type) add a
_check_err call that invokes the built-in with an incorrect argument type (e.g.
pass an Int where a Decimal or String/Float64 is expected) and assert a "type"
error; place them alongside the existing test_* functions in
tests/test_checker.py so signature enforcement is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4d22123-b917-4f7a-9305-3c14e6bd2bdf
📒 Files selected for processing (5)
TESTING.mdscripts/check_spec_examples.pyspec/09-standard-library.mdtests/test_checker.pyvera/checker/resolution.py
- Fix TESTING.md stale '290 tests' to 295, line count to ~33,100 - Replace duplicate test_decimal_inequality with test_decimal_neg_large_value - Add WAT import assertions to host-called tests (div, from_string, compare) - Document browser decimal_eq string-representation semantics in spec - test_decimal_from_float already uses _run_float(3.5) (no change needed) - Browser gating: declined — builtins work on both targets, precision limitation is documented in spec not gated in codegen Co-Authored-By: Claude <noreply@anthropic.invalid>
4 new codegen tests exercising WASM type inference paths for Decimal: chained arithmetic, to_float, to_string, eq. Fix TESTING.md counts (2888 tests). Fix stale conformance count (290→295). Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai Responses to two review-body items:
|
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
|
Merging with codecov/patch at 90.11% (target 95.43%). The gap is structural: |
|
This is a common pattern with compiler coverage: defensive error branches after the type checker are unreachable by construction — if the code passes type checking, those branches can't fire. You'd need to bypass the type checker to test them, which would require test infrastructure changes (injecting malformed AST nodes directly into codegen). |
Summary
decimal_from_int,decimal_from_float,decimal_from_string,decimal_to_string,decimal_to_float,decimal_add,decimal_sub,decimal_mul,decimal_div,decimal_neg,decimal_compare,decimal_eq,decimal_round,decimal_absdecimal.Decimal; browser uses string-based arithmeticch09_decimal(59 programs)Known limitations
Option<Decimal>fromdecimal_div/decimal_from_stringcannot be used withmatchoroption_unwrap_ordue to monomorphization gaps (Decimal is not recognized as a monomorphizable type parameter)Orderingfromdecimal_comparecannot be used withmatchfor the same reasonCloses #333
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores