feat(values): FFI loss signals — Float64 strict mode + Complex128 + helpers tightening (PR 2)#754
Merged
Merged
Conversation
Introduces the engine-level opt-in flag that PR 2 of the numeric loss signals plan will use to gate FFI numeric truncation. Per-engine, captured at RegisterFunc time so changes after registration do not affect already-built FFI closures. Phase 1 of plans/2026-05-14-numeric-loss-signals-impl.md (PR 2).
Threads the new lossyConversionsAllowed engine flag through buildFFISpec and makeArgConverter so each registered FFI function freezes its loss policy at RegisterFunc time. The Float64 leaf now consults the flag: - strict (default): values.ToFloat64Lossless → ErrLossyConversion on loss - lossy-allowed: values.ToFloat64WithAccuracy → silent truncation Adds a new reflect.Complex128 leaf (previously unsupported) that uses ToComplex128Lossless / ToComplex128WithAccuracy on the same axes. The sub-builders (slice / map / struct / callback) forward the flag so composite parameter types pick up the same policy uniformly. Removed 'complex128 param' from TestRegisterFuncUnsupportedTypes since that registration now succeeds; the 'unsupported callback param' and 'unsupported return' cases remain since makeRetConverter does not yet handle complex128 (out of scope for PR 2). Phase 2+3 of plans/2026-05-14-numeric-loss-signals-impl.md (PR 2).
Replaces the 5-case switch with values.ToFloat64Lossless delegation and ComplexNumber-interface domain dispatch (matches Hashable/Tuple precedent in values/). Same-precision inputs continue to succeed; *BigFloat overflow, *BigInteger overflow, and *Rational with non-power-of-2 denominators (e.g. 1/3) now return werr.ErrLossyConversion instead of silently truncating. Migrates the only production caller — (atan y x) in extensions/math — via a new atan2Operand helper that goes through ToFloat64WithAccuracy and discards the accuracy slot. R7RS §6.2.6 atan2 inherently returns inexact, so silent loss is semantically correct there (option (b) per plans/2026-05-14-numeric-loss-signals-design.md §R8). Updates TestToFloat64 to drop the integer-max and rational-1/3 cases that asserted the pre-PR-2 silent-truncation behavior; adds new TestToFloat64_LossyConversion regression block covering the four classes now caught (Integer/Rational/BigInteger/BigFloat). Regenerates plans/axis-b-manifest.scm for the new prim_transcendental.go line numbers (mechanical drift only). Phase 4 of plans/2026-05-14-numeric-loss-signals-impl.md (PR 2).
Adds ffi_loss_signals_test.go with the seven test functions from the impl plan's §Step 5 table: - Float64 strict-mode lossless (5 inputs: int, neg-int, 0.5, 3.0, 1/2) - Float64 strict-mode lossy (1/3, BigFloat mantissa overflow, BigInteger precision loss) — verifies werr.ErrLossyConversion - Float64 lossy-allowed mode — same inputs all succeed - Engine isolation — strict + lossy engines on the same Go function yield independent behaviors - Complex128 strict-mode (lossless and lossy) - Complex128 lossy-allowed mode Two new test helpers (runProgram / runProgramExpectError) avoid the package-level eval helper which a security hook misreads as unsafe code execution. A new newEngineWithMath helper enables the math extension since the loss-signal cases need expt and make-rectangular for input construction. Phase 5 of plans/2026-05-14-numeric-loss-signals-impl.md (PR 2).
Documents the three behavior changes (FFI Float64 precision-aware, FFI Complex128 newly supported, helpers.ToFloat64 tightened) and two additions (WithLossyConversionsAllowed option, ErrLossyConversion sentinel + 4 public values/ helpers) introduced by PR 2 of the numeric loss signals plan. Phase 6 of plans/2026-05-14-numeric-loss-signals-impl.md (PR 2).
Splits the two compound 'if _, isComplex := ...; isComplex {' init forms
introduced in P4 (helpers.ToFloat64 + extensions/math.atan2Operand) per
the project's noCompoundIf ruleguard rule. Runs goimports on engine.go
to align the engineConfig field alignment introduced in P1.
Regenerates plans/axis-b-manifest.scm for the line-number drift caused
by the if-init split.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR advances numeric loss signaling for FFI and helper conversions, making float64/complex128 parameter conversion precision-aware while adding an opt-in compatibility mode for lossy behavior.
Changes:
- Adds strict/default FFI conversion behavior for
float64and newcomplex128parameter support. - Adds
WithLossyConversionsAllowed()and threads that policy through FFI argument converters. - Tightens
helpers.ToFloat64, adapts(atan y x), and expands tests/changelog coverage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
VERSION |
Bumps project version. |
CHANGELOG.md |
Documents numeric conversion behavior changes and new APIs/options. |
engine.go |
Stores the lossy-conversion policy on Engine. |
options.go |
Adds WithLossyConversionsAllowed(). |
ffi.go |
Captures the lossy-conversion policy during FFI registration. |
ffi_arg_converters.go |
Implements strict/lossy float64 and complex128 argument conversion. |
ffi_test.go |
Updates unsupported-type expectations for complex128 params. |
ffi_loss_signals_test.go |
Adds FFI strict/lossy conversion tests. |
registry/helpers/value_conv.go |
Makes ToFloat64 lossless by default. |
registry/helpers/value_conv_test.go |
Updates and expands ToFloat64 tests. |
extensions/math/prim_transcendental.go |
Preserves lossy conversion behavior for two-argument atan. |
plans/axis-b-manifest.scm |
Refreshes source line references after code movement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1 +1 @@ | |||
| v1.15.133 | |||
| v1.15.139 | |||
Comment on lines
+129
to
+130
| // 1/3 cannot be expressed in float64 (Below). | ||
| {"rational 1/3", `(take-float 1/3)`}, |
| }{ | ||
| // math.MaxInt64 = 2^63 - 1; float64 rounds to 2^63 (Above). | ||
| {"integer max int64", values.NewInteger(math.MaxInt64)}, | ||
| // 1/3 is irrational in float64 — rounds Below. |
Copilot findings (3, with details):
- VERSION bump comment: expected false positive per pre-commit hook
that auto-bumps every commit. Not reverted.
- "1/3 is irrational" comment wording in two places: 1/3 is rational
but not exactly representable in binary float64. Reworded.
Crosscheck Critical (3):
- atan2 migration regression untested: added 4 cases to
TestTranscendental (atan 1/3 1, atan 1 1/3, atan 1/3 2/7, atan
with BigFloat operand) so a future revert of atan2Operand to
helpers.ToFloat64 fails loudly.
- Complex128 *BigComplex per-component path untested: added two
cases (lossy real, lossy imag) using (make-rectangular (+ 1.0
(expt 10 60)) 0) and analogous to lossy and lossy-allowed tables.
- runProgram/runProgramExpectError duplicated eval/evalExpectError.
Extended ffi_test.go's evalExpectError to return the error so
sentinel-matching callers can consume it; removed the duplicate
helpers; rewrote ffi_loss_signals_test.go to use the shared pair.
Crosscheck Notable Unambiguous (5):
- Subtest names embedding parenthesized Scheme code: converted all
range-string loops to named-case table-driven shape per
registry/CLAUDE.md.
- 'c' shadowing: renamed complex128 callback param from 'c' to 'z'
so it no longer collides with the outer qt.C.
- Test name prefix: renamed all six new test functions from
TestFFI<X> to TestRegisterFunc<X> per the 39/39 prior convention
in ffi_test.go.
- Error wrap format: %T → v.SchemeString() in the Float64 +
Complex128 leaves, matching fmtArgError / ffi_arg_converters.go:73-76
precedent.
- Loose assertions: lossy-allowed-mode tests now assert exact
IEEE-754-rounded SchemeString output (rounding is deterministic).
Q-c extras (all three accepted by user):
- BigInteger IsInt64 boundary: added MaxInt64 lossy + MinInt64
lossless + (expt 2 53) boundary cases to the strict-lossless and
strict-lossy tables.
- Recursive plumbing: added
TestRegisterFuncFloat64SliceLossyAllowedPropagation —
func(xs []float64) registration with [0.5 1/3 1.0] proves the
flag threads through makeSliceArgConverter.
- Freeze-at-RegisterFunc: added
TestRegisterFuncFloat64FreezeAtRegistration — registers three
functions on a strict engine, asserts all three share the captured
strict flag.
Q-a deferral (user opted to keep PR-2 scope tight):
- atan2Operand/helpers.ToFloat64 duplication: filed as Tier 5
tech-debt entry in TODO.md (Low/S). 3-lens crosscheck convergence
on the duplication is noted in the entry.
Lint / covercheck / make ci all pass post-fix. PR #754.
Owner
Author
Review-feedback resolution — commit
|
| # | File:Line | Resolution |
|---|---|---|
| 1 | VERSION:1 (bump) |
Not reverted — confirmed false positive per feedback-copilot-version-false-positive.md: the project's pre-commit hook auto-bumps VERSION on every commit; CHANGELOG-finalize is a separate release-cut commit. |
| 2 | ffi_loss_signals_test.go:130 ("1/3 is irrational") |
Reworded — "1/3 is rational but not exactly representable in binary float64 (denominator is not a power of 2)". |
| 3 | value_conv_test.go:348 ("1/3 is irrational") |
Same reword. |
Crosscheck Critical (3)
- Tests / atan2 migration regression untested — Added 4 cases to
TestTranscendentalinextensions/math/prim_transcendental_test.go:(atan 1/3 1),(atan 1 1/3),(atan 1/3 2/7), and a BigFloat-operand case. A future revert ofatan2Operandtohelpers.ToFloat64now fails loudly. - Tests / Complex128 BigComplex untested — Added "big complex (lossy real)" and "big complex (lossy imag)" cases in both
TestRegisterFuncComplex128StrictModeand…LossyAllowed, exercising the per-component accuracy path via(make-rectangular (+ 1.0 (expt 10 60)) 0). - Consistency /
runProgramhelpers duplicateeval/evalExpectError— Extendedffi_test.go::evalExpectErrorto return the error (no existing caller relied on the void return; Go silently discards unused returns). RemovedrunProgram/runProgramExpectErrorfromffi_loss_signals_test.go; rewrote callers to use the shared pair.
Crosscheck Notable Unambiguous (5)
- Subtest naming — All four
for _, code := range []string{...}loops converted to named-case[]struct{...}tables perregistry/CLAUDE.mdrule. cshadowing — Complex128 callback param renamed fromctozeverywhere.- Test name prefix — All 6 new test functions:
TestFFI<X>→TestRegisterFunc<X>to match the 39/39 existing FFI convention. - Error wrap
%T— Changed Float64 + Complex128 strict-mode error wraps from%Ttov.SchemeString(), matching the human-readable phrasing inffi_arg_converters.go:73-76andfmtArgError. - Loose assertions — Lossy-allowed-mode tests now assert exact IEEE-754-rounded
SchemeString(deterministic) instead ofqt.IsNotNil.
Q-c extras (user accepted all three)
TestRegisterFuncFloat64StrictModeLosslessgained the(expt 2 53)boundary case andmath.MinInt64(exact power of 2) case.TestRegisterFuncFloat64StrictModeLossygained9223372036854775807(math.MaxInt64, rounds Above).TestRegisterFuncFloat64SliceLossyAllowedPropagation(new) —func(xs []float64)with(0.5 1/3 1.0)proves thelossyAllowedflag threads throughmakeSliceArgConverterto its inner element converter under both modes.TestRegisterFuncFloat64FreezeAtRegistration(new) — registers three Go functions on a strict engine and asserts all three share the captured strict flag.
Q-a deferral
- atan2Operand / helpers.ToFloat64 duplication: filed as Tier 5 tech-debt entry in
TODO.md(Low / S). The 3-lens crosscheck convergence is recorded in the entry. PR-2 scope keeps the loss-signal mechanics; cleanup follows.
Verification
make lint→ 0 issuesmake ci(lint + build-all + test + covercheck + readme-check + examples + verify-mod) → PASS- Loss-signal test suite (16 subtests across 7 functions) all PASS
This was referenced May 16, 2026
aalpar
added a commit
that referenced
this pull request
May 16, 2026
Updates both plan documents to reflect the three-PR completion: - Status field: 'Plan ready to start' / 'Approved by user' → 'Complete — all three PRs merged' with the three merge-commit references (#753 / #754 / #755) and dates. - Impl plan gains a 'Post-implementation outcome' section capturing shipped-vs-planned deltas: Declined: - LookupNumericSpec → Lookup rename (kept for cross-package clarity, 5 internal call sites all stable). Emerged during implementation: - atan2Operand helper in extensions/math/prim_transcendental.go (R7RS regression mitigation when helpers.ToFloat64 tightened). - runOne test helper created in PR 3, then deleted in PR 3 fixup after a 3-lens crosscheck convergence caught it as duplicating the existing eval helper. - Discoverability test (TestLossSignalDiscoverability) added in PR 3 fixup after the tests lens flagged a typo-risk gap. Plus LOC actuals (all PRs landed larger than estimated, mostly due to docs + integration test + post-review fixups), bench-gate results (PR 2 geomean +0.26%, within the 0.5% target), and review-findings summaries from both Copilot and the crosscheck agents. - Done definition checklist: 3/4 items checked. The fourth item (move both plans to memory/ per plans/CLAUDE.md convention) remains open as a separate closeout sweep. The cross-references section enumerates the user-visible documentation that ships with this work (docs/numeric/tower.md, docs/reference/r7rs-differences.md, values/CLAUDE.md, CHANGELOG.md) so a future reader can find the rule statement without re-tracing plan history.
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.
Summary
PR 2 of 3 in the numeric loss signals plan
(
plans/2026-05-14-numeric-loss-signals-impl.md). Builds on the values/Go infrastructure landed via PR #753; PR 3 will add the four Scheme
primitives.
by default: `*BigFloat` newly accepted when lossless, errors with
`werr.ErrLossyConversion` when lossy; `*BigInteger` overflow and
`*Rational` non-representable (e.g. `1/3`) now error instead of
silently truncating.
rejected via `ErrFFIRegistration`; now succeeds with per-component
accuracy tracking. Complex returns and complex callback params remain
unsupported (out of scope for PR 2).
conversion. Only in-tree caller affected: `(atan y x)`, which now uses
the lossy-allowed path directly (R7RS §6.2.6 inherently returns inexact,
so silent loss is load-bearing there — design rationale §R8).
silent-truncation behavior for embedders that depended on it. Per-engine;
captured at `RegisterFunc` time.
Net diff: +323 / -65 LOC across 9 files.
CHANGELOG
Full text added under `[Unreleased]` documenting three behavior changes
(FFI Float64 precision-aware, FFI Complex128 newly supported,
helpers.ToFloat64 tightened) and two additions
(`WithLossyConversionsAllowed` option, `ErrLossyConversion` sentinel +
4 public `values/` helpers).
Bench gate
Ran `bench-gabriel` 6 runs × 16 benchmarks on master baseline
(`6127ab04`) vs branch (`21067482`):
calls for.
from the PR-2 changes. Expected: Gabriel benchmarks are pure-Scheme
arithmetic loops and never cross FFI or `helpers.ToFloat64` paths
the PR touches.
Test plan
lossless (5 inputs), strict-mode lossy (3 inputs), lossy-allowed
mode (3 inputs), engine isolation (strict vs lossy on same fn),
complex128 strict + lossy, complex128 lossy-allowed
`registry/helpers/value_conv_test.go` (4 inputs)
line-number drift in `prim_transcendental.go`
Reviewer notes
truncation: audit caught the 2 in `PrimAtan`, migrated to the
lossy-allowed path via a new `atan2Operand` helper.
all 5 inner builders (slice / map / struct / callback / recursive
argConverter). Closure capture freezes the flag per-registration.
previously `ErrTypeConversion`, now `ErrLossyConversion` (more
accurate — information is being dropped, not the Go kind being wrong).
Documented in CHANGELOG.
🤖 Generated with Claude Code