Skip to content

feat(values): Go infrastructure for numeric loss signals#753

Merged
aalpar merged 2 commits into
masterfrom
feat/values-sr-phase4-loss-signals-go
May 15, 2026
Merged

feat(values): Go infrastructure for numeric loss signals#753
aalpar merged 2 commits into
masterfrom
feat/values-sr-phase4-loss-signals-go

Conversation

@aalpar
Copy link
Copy Markdown
Owner

@aalpar aalpar commented May 15, 2026

Summary

Lays the Go-side foundation for the no-information-loss numeric pipeline
(design: plans/2026-05-14-numeric-loss-signals-design.md). This is PR 1 of 3.

  • New values/ public API: ToFloat64WithAccuracy, ToFloat64Lossless,
    ToComplex128WithAccuracy, ToComplex128Lossless, Complex128Result struct
  • New werr.ErrLossyConversion sentinel for precision and imaginary-drop errors
  • Accuracy symbols: SymbolAccuracy{Below,Exact,Above} + BigAccuracyToSymbol
  • Per-kind helpers for all 7 numeric types registered in NumericTypeSpec
  • BigFloat API hygiene: Float64()Float64Truncated() rename +
    new Float64WithAccuracy() method; 13 call-site migrations in extensions/, parser/, registry/

Key correctness fixes (caught by crosscheck)

Two direction-recovery bugs identified by post-implementation review:

  1. Rational overflow-to-Inf (values/rational.go): when a huge rational
    overflowed to ±Inf, (*big.Rat).SetFloat64(±Inf) returned nil and the
    function fell through to return f, big.Exact, true. Fixed: detect math.IsInf(f,0)
    before the round-trip and infer direction from sign.

  2. Integer MaxInt64 saturation (values/integer.go): float64(MaxInt64) = 2^63
    (rounds up), then int64(2^63) saturates back to MaxInt64 in Go, making
    the round-trip falsely compare equal. Fixed: early-exit with big.Above when
    f >= 9223372036854775808.0.

Test plan

  • 60 subtests in values/conversion_test.go covering all 7 kinds ×
    {exact, below, above, overflow, NaN} × {lossless-ok, lossless-error}
  • MaxInt64 and rational-overflow cases explicitly tested post-fix
  • make lint && make covercheck both pass (38 packages ≥ 80%)
  • go test ./... all green
  • Master CI green at time of push (run 25892419247)

🤖 Generated with Claude Code

Adds three-layer loss-signal infrastructure for the numeric tower so
precision loss from Go's math/big APIs is visible at the Scheme level.

## New public surface (values/)

- `ErrLossyConversion` sentinel in werr/ for precision or imaginary drop
- `SymbolAccuracy{Below,Exact,Above}` + `BigAccuracyToSymbol` in symbols_accuracy.go
- `Complex128Result{Value, RealAcc, ImagAcc}` struct (named fields prevent
  silent realAcc/imagAcc swap bugs where two adjacent big.Accuracy args would be
  type-identical)
- `ToFloat64WithAccuracy(n) (float64, big.Accuracy, bool, error)` — primary loss-signal
  helper; isReal=false iff input is Complex/BigComplex with nonzero imag
- `ToFloat64Lossless(n) (float64, error)` — errors with ErrLossyConversion on
  any rounding (acc≠Exact) or imaginary drop (!isReal)
- `ToComplex128WithAccuracy(n) (Complex128Result, error)` — per-component accuracy
- `ToComplex128Lossless(n) (complex128, error)` — errors if either component acc≠Exact

## Per-kind accuracy helpers (7 numeric types)

Each type's init() block registers toFloat64WithAccuracy and
toComplex128WithAccuracy in its NumericTypeSpec:

- Integer: int64 round-trip, with saturation guard for MaxInt64
  (float64(MaxInt64)=2^63 > MaxInt64; int64(2^63) saturates back to MaxInt64,
  making naive round-trip falsely report Exact → guard: f >= 9223372036854775808.0)
- BigInteger: big.Float round-trip via SetInt/Float64
- Float: identity — float64 IS the value, accuracy always Exact
- BigFloat: uses Float64WithAccuracy() — new method alongside renamed Float64Truncated()
  (former Float64()) to signal intentional truncation at call sites
- Rational: (*big.Rat).Float64() exact bool + round-trip Cmp, with overflow guard:
  when f is ±Inf, SetFloat64(±Inf) returns nil; infer direction from sign
  (+Inf > any positive rational → Above; -Inf < any negative → Below)
- Complex: real part always returned, isReal = (imag == 0)
- BigComplex: per-component dispatch through real/imag Number fields

## Correctness fixes for direction recovery

Two bugs caught by crosscheck:
1. Rational overflow-to-Inf: back=nil case was returning big.Exact; fixed to
   infer Above/Below from sign of the rational
2. Integer MaxInt64 saturation: int64(float64(MaxInt64)) = MaxInt64 due to Go's
   float→int saturation, making the round-trip falsely appear Exact; fixed with
   f >= 2^63 early-exit returning big.Above

## BigFloat API hygiene (13 call-site migrations)

Renamed (*BigFloat).Float64() → Float64Truncated() to signal that callers
accepting only a float64 are knowingly truncating. New Float64WithAccuracy()
returns (float64, big.Accuracy). 13 call sites in extensions/, internal/parser/,
registry/ migrated.

## Defensive improvement

BigAccuracyToSymbol default case now panics (werr.ErrInternal) instead of
silently returning SymbolAccuracyExact for unknown big.Accuracy values.

## Test coverage

values/conversion_test.go: 60 subtests across TestToFloat64WithAccuracy (29),
TestToFloat64Lossless (15), TestToComplex128WithAccuracy (13),
TestToComplex128Lossless (12), TestErrNotANumber (4). Verified numeric values:
- integer-overflow-positive: 2^53+1 → 2^53, big.Below (rounds toward zero)
- integer-maxint64: MaxInt64 → 2^63, big.Above (rounds away from zero)
- rational-onethird: 1/3 → big.Below; rational-above: 1/10 → big.Above
- rational-overflow-positive: 10^400 → +Inf, big.Above
- big-float-irrational-pi: 256-bit π → math.Pi, big.Below
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR lays the Go-side foundation for loss-signal-aware numeric conversions by threading big.Accuracy (and complex “isReal” signaling) through the values numeric registry and adding public conversion helpers (ToFloat64*, ToComplex128*) plus supporting sentinels/symbols. It also performs a BigFloat API hygiene rename (Float64()Float64Truncated()) and updates call sites/tests accordingly.

Changes:

  • Add ErrLossyConversion, accuracy singleton symbols, and new values public conversion helpers (accuracy + lossless wrappers).
  • Replace numeric-registry conversion hooks with To{Float64,Complex128}WithAccuracy and migrate internal call sites.
  • Rename BigFloat’s lossy Float64() to Float64Truncated() and introduce Float64WithAccuracy(), updating downstream call sites.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
werr/werr.go Adds ErrLossyConversion sentinel.
VERSION Bumps version string.
values/symbols_accuracy.go Introduces accuracy singleton symbols + mapper.
values/rational.go Implements rational float/complex conversion w/ accuracy recovery.
values/promotion.go Migrates Number→float/complex helpers to new registry API.
values/numeric_tower.go Updates registry lookup usage for simplify/exactness.
values/numeric_registry.go Replaces registry conversion hooks; renames LookupNumericSpecLookup.
values/numeric_registry_test.go Updates registry tests for new conversion APIs.
values/numeric_methods_coverage_test.go Migrates BigFloat call sites to Float64Truncated().
values/integer.go Adds integer conversion w/ accuracy (direction recovery).
values/float.go Adds float conversion w/ always-exact accuracy.
values/conversion.go Adds new public conversion helpers + Complex128Result.
values/conversion_test.go Adds table-driven tests for new conversion helpers.
values/complex.go Adds complex conversion w/ ok real-flag + accuracy.
values/big_number_test.go Migrates BigFloat test call sites to Float64Truncated().
values/big_integer.go Adds BigInteger conversion w/ big.Accuracy.
values/big_float.go Renames Float64()Float64Truncated(); adds Float64WithAccuracy().
values/big_complex.go Adds BigComplex conversion w/ per-component accuracy; migrates call sites.
values/big_complex_test.go Migrates BigFloat test call sites to Float64Truncated().
TODO.md Adds follow-up tracking section for loss-signals API evolution.
registry/helpers/value_conv.go Migrates BigComplex conversion to Float64Truncated().
plans/2026-05-14-numeric-loss-signals-impl.md Updates implementation plan to reflect current state/decisions.
plans/2026-05-14-numeric-loss-signals-design.md Updates design doc (hybrid return shape, BigFloat hygiene, etc.).
internal/parser/parser_test.go Migrates BigComplex float extraction to Float64Truncated().
internal/parser/parser_number.go Migrates BigComplex→inexact conversion to Float64Truncated().
internal/parser/parser_number_test.go Migrates BigComplex parse assertions to Float64Truncated().
internal/parser/big_number_test.go Migrates BigFloat test call sites to Float64Truncated().
extensions/math/prim_transcendental.go Migrates BigComplex transcendentals to Float64Truncated().
extensions/math/prim_rounding.go Migrates BigFloat rounding primitive to Float64Truncated().
extensions/math/prim_complex.go Migrates BigComplex magnitude/angle to Float64Truncated().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread VERSION Outdated
@@ -1 +1 @@
v1.15.131
v1.15.132
Comment thread values/integer.go Outdated
// direction-recovery. The comparison is in int64 to avoid float-comparison
// pitfalls (float comparison would itself suffer the rounding being measured).
//
// For |v| > 2^53 the float64 representation rounds toward zero, so:
Comment thread values/conversion_test.go
if math.IsNaN(tc.wantValue) {
c.Assert(math.IsNaN(f), qt.IsTrue)
} else {
c.Assert(f, qt.Equals, tc.wantValue)
Copilot findings (PR #753):
- integer.go:175 — correct comment: int64→float64 is round-to-nearest-even
  per IEEE 754, not round-toward-zero. Add 2^53+3 counter-example.
- conversion_test.go — detect +0/-0 regressions via math.Signbit;
  qt.Equals (==) treats +0 and -0 as equal so the existing test could
  not catch a sign-flip.
- VERSION bump: false positive — pre-commit hook auto-bumps every commit.

Post-PR-1 cleanup:
- Add isAlwaysReal flag + liftRealToComplex128 helper, collapsing 5
  redundant complex-lift closures across real-only kinds (Integer,
  BigInteger, Rational, Float, BigFloat).
- Revert LookupNumericSpec → Lookup rename; the 'Numeric' prefix is
  load-bearing at the package level.
- Rename NumberToComplex128 → NumberToComplex128Lossy per the loss-signal
  naming convention introduced in this PR.
- Codify the loss-signal suffix taxonomy (*WithAccuracy / *Truncated /
  *Lossy / *Lossless) in CODING_STYLE.md.
- Add TestBigAccuracyToSymbol covering the closed-domain mapping plus
  the ErrInternal panic for out-of-domain big.Accuracy values.
- Add 8 deferred follow-up entries to TODO.md, including 5 pre-existing
  BigComplex precision-loss bugs that the Float64Truncated rename made
  visible by name.
@aalpar
Copy link
Copy Markdown
Owner Author

aalpar commented May 15, 2026

Addressed Copilot review in 9d96a56:

1. values/integer.go:175 — comment correction
The original comment said "rounds toward zero" which is true for the specific 2^53+1 → 2^53 case but misgeneralizes. Go's int64→float64 conversion is round-to-nearest, ties to even per IEEE 754. Rewrote the comment with that rule plus a 2^53+3 → 2^53+4 counter-example to forestall the same simplification mistake.

2. values/conversion_test.go — negative-zero detection
Tightened the assertion path in TestToFloat64WithAccuracy: when tc.wantValue == 0, the test now calls math.Signbit on both got and want in addition to qt.Equals. The two test cases that exercise this — float-negative-zero and big-float-underflow-negative — now actually fail if the implementation returns +0 where -0 is expected.

3. VERSION bump
False positive — the pre-commit hook auto-bumps VERSION on every commit in this repo (see memory/feedback-copilot-version-false-positive.md). Reverting would just cause the next commit to bump it again. The release-cut is a separate signed commit; this PR is not it.

The same commit also folds in post-PR-1 cleanup: isAlwaysReal registry shortcut, the NumberToComplex128 → NumberToComplex128Lossy rename for naming-convention consistency, the *WithAccuracy / *Truncated / *Lossy / *Lossless suffix taxonomy in CODING_STYLE.md, and 8 deferred follow-ups in TODO.md — including 5 pre-existing BigComplex precision-loss bugs the Float64Truncated rename made visible.

Bench-gate against master at the cleanup HEAD: geomean -0.62% (HEAD slightly faster) at N=3, well within the per-run noise band (~±1.5%); 10/16 benchmarks favor HEAD. Hot-path arithmetic methods (Add/Subtract/Multiply/Divide on *Integer etc.) do not reference the new helpers — verified by grep.

@aalpar aalpar added the enhancement New feature or request label May 15, 2026
@aalpar aalpar merged commit 6127ab0 into master May 15, 2026
1 check passed
@aalpar aalpar deleted the feat/values-sr-phase4-loss-signals-go branch May 15, 2026 14:46
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants