Skip to content

feat(values): introduce NumericTypeSpec registry for cold-path dispatch#752

Merged
aalpar merged 3 commits into
masterfrom
feat/values-sr-phase3-numeric-registry
May 14, 2026
Merged

feat(values): introduce NumericTypeSpec registry for cold-path dispatch#752
aalpar merged 3 commits into
masterfrom
feat/values-sr-phase3-numeric-registry

Conversation

@aalpar
Copy link
Copy Markdown
Owner

@aalpar aalpar commented May 14, 2026

Summary

  • Adds [numKinds]NumericTypeSpec array populated via registerNumericSpec() in each type's init(), one record per NumericKind
  • Migrates Simplify, ExactnessOf, NumberToFloat64, NumberToComplex128 from hand-maintained type switches to registry lookup — collapses 4 of the 12 items from the ADDING-A-NEW-NUMERIC-TYPE guide into a single registration call
  • NumberToFloat64 now returns ErrNotAReal for Complex/BigComplex (previously silently extracted the real part), matching R7RS semantics (Q-i=C3)

Design notes

  • Fields unexported + 5 getter methods (SchemeName, SimplifyDown, ToFloat64, ToComplex128, IsAlwaysExact) — external callers cannot mutate specs
  • validateNumericSpecs exposed as an unexported function so tests can call it with crafted bad state (the live sync.Once is already consumed before tests run)
  • Registry is cold-path only — hot-path arithmetic dispatch tables (makeArithmeticDispatch et al.) are untouched

Tests

  • TestNumericRegistryAllKindsRegistered — all 7 kinds populated
  • TestNumericRegistrySmoke — per-kind getter correctness
  • TestEnsureNumericRegistryInitPanics — missing-kind and partial-fill panic paths
  • TestRegisterNumericSpecDuplicateRejected — duplicate registration rejected
  • Behavioral-equivalence golden tests: pre-migration switch logic captured inline, compared against registry path across 15 exemplars per function (TestSimplifyEquivalence, TestExactnessOfEquivalence, TestNumberToFloat64Equivalence, TestNumberToComplex128Equivalence)

Bench gate

Master and branch binaries run identically under current conditions (~0.89s for 1M-iteration loop). Gabriel suite was throttled during the run (E-cores, 30–60% spread); interleaved master/branch timing confirms no regression — cold-path changes do not appear in the Gabriel workload.

CI

make ci passes: lint (0 issues), all tests green, covercheck (38/38 packages ≥ 80%).

Adds a [numKinds]NumericTypeSpec array populated via registerNumericSpec()
in each type's init(). Migrates Simplify, ExactnessOf, NumberToFloat64, and
NumberToComplex128 from hand-maintained type switches to registry lookup,
collapsing 4 of the 12 items from the ADDING-A-NEW-NUMERIC-TYPE guide.

NumberToFloat64 now returns ErrNotAReal for Complex/BigComplex (previously
silently extracted the real part), matching R7RS semantics.

Includes behavioral-equivalence golden tests comparing pre-migration switch
logic against the registry path across 15 exemplars per function.
@aalpar aalpar added the enhancement New feature or request label May 14, 2026
@aalpar aalpar requested a review from Copilot May 14, 2026 23:23
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 introduces a numeric type-spec registry to centralize cold-path numeric behavior and reduce duplicated type-switch maintenance across the numeric tower.

Changes:

  • Adds NumericTypeSpec registration/lookup infrastructure and tests.
  • Migrates Simplify, ExactnessOf, NumberToFloat64, and NumberToComplex128 to registry-backed dispatch.
  • Registers per-kind simplify/conversion/exactness behavior in each numeric type file.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
werr/werr.go Adds a numeric registry sentinel error.
VERSION Bumps the project version.
values/* numeric files Register per-kind numeric specs and helper conversion/simplification functions.
values/numeric_registry.go Adds the numeric type-spec registry implementation.
values/numeric_registry_test.go Adds registry coverage and behavioral-equivalence tests.
values/numeric_dispatch_test.go Updates dispatch coverage for complex-to-float behavior.
values/numeric_tower.go Delegates simplification/exactness decisions to the registry.
values/promotion.go Delegates float/complex conversions to the registry.

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

Comment thread VERSION Outdated
@@ -1 +1 @@
v1.15.128
v1.15.129
Comment thread values/promotion.go Outdated
Comment on lines +327 to +329
f, err := Lookup(n.Kind()).ToFloat64(n)
if err != nil {
panic(werr.WrapForeignErrorf(werr.ErrNotANumber, "NumberToFloat64: cannot convert %T to float64", n))
Comment thread values/numeric_registry_test.go Outdated
NewRational(6, 2), // IsInteger → demotes
NewRational(7, 2), // non-integer — stays Rational
NewComplex(complex(3+0i, 0)),
NewComplex(complex(3.5+0i, 0)),
Comment thread values/numeric_registry.go Outdated
// Lookup returns the NumericTypeSpec for the given kind.
// Calls ensureNumericRegistryInit on every access (cold path only).
func Lookup(kind NumericKind) *NumericTypeSpec {
ensureNumericRegistryInit()
Crosscheck (5 lenses: code, errors, types, tests, consistency) surfaced
fixes across four areas. Architectural suggestions (methods-on-Number,
exactness-as-function, parallel-array collapse) deferred per user
direction; loss-signals plumbing for BigFloat/BigInteger/Rational → float64
deferred to the follow-up plan (plans/2026-05-14-numeric-loss-signals-*).

Behavioral fixes [code+errors|CONVENTION]:
- complexToFloat64/bigComplexToFloat64 now return the real component
  when imag == 0 (lossless), error only when imag != 0. Restores
  pre-migration behavior for the (make-rectangular (- 3+0i 0+0i) 5)
  call path that isRealNumber lets through. Aligns with the
  loss-signals design's "succeed silently when no information lost"
  principle.
- NumberToFloat64 panic now preserves the inner ErrNotAReal sentinel
  via werr.WrapForeignErrorWithCause, so errors.Is(panicVal,
  werr.ErrNotAReal) succeeds.
- Simplify(nil) returns nil; ExactnessOf(nil) panics with ErrNotANumber.
  Both pre-migration behaviors hid behind the type switch.

Registry refinements [code+consistency|CONVENTION]:
- Receiver rename s → p on all 5 NumericTypeSpec methods (matches
  package-wide convention; was 533 p vs 5 s after PR-1).
- LookupNumericSpec renamed from Lookup (descriptive in the broad
  values package).
- LookupNumericSpec bounds-checks the kind; out-of-range produces
  ErrNumericRegistry instead of Go runtime "index out of range".
- registryMu mutex removed (Go init phase is single-goroutine).
- Eager validation init() moved to validate_numeric_registry.go so
  it sorts after every per-type file (Go init order is lexical;
  rational.go's 'r' sorts after numeric_registry.go's 'n').
  Replaces sync.Once lazy validation.
- Duplicate-registration panic now includes both schemeNames.
- nil-field panic messages include the new spec's schemeName.

Test improvements [tests|CONVENTION]:
- TestNumericRegistryAllKindsRegistered uses reflection to verify
  every NumericTypeSpec function field is non-nil (mirrors
  TestAllDispatchEntriesPopulated discipline).
- TestNumericRegistrySmoke SimplifyDown line now asserts non-nil
  (was _ = ...). Added Complex(3+0i) and BigComplex(3,0) cases.
- TestTypeSwitchFunctionsHandleAllTypes recover block surfaces any
  panic as a typed test failure (was silently t.Errorf for any panic).
- New TestNumberToFloat64PanicsOnNonzeroImag locks the panic contract:
  errors.Is(panicVal, werr.ErrNotAReal) must succeed.
- equivalenceExemplars expanded with MinInt64, BigInteger > int64
  (both signs), Float negative zero, Rational(1/3), and an
  inexact-real-with-exact-zero-imag BigComplex.

Doc + style [consistency|CONVENTION]:
- ADDING A NEW NUMERIC TYPE guide updated: removes stale "update
  NumberToFloat64/NumberToComplex128" and "update Simplify/
  ExactnessOf" steps; adds registerNumericSpec step; renumbers.
- 21 new per-type helpers across 7 files gain godoc one-liners that
  note precision-loss status (lossless / silent-lossy / lossy-only-
  outside-mantissa).
- big_complex.go: helpers moved below the dispatch-table comment+var
  block so godoc attaches to the right declaration.
- SimplifyDown docstring corrected from "one step" to "multi-step
  descent inlined per-kind."
- numeric_registry_test.go gains Apache 2.0 copyright header.
@aalpar
Copy link
Copy Markdown
Owner Author

aalpar commented May 14, 2026

Crosscheck findings — resolution summary

5-lens /crosscheck:crosscheck all was run against the diff (code, errors, types, tests, consistency lenses, all on Opus). Findings classified into Critical / Notable / Architectural / Symptom buckets per the workflow in plans/CLAUDE.md.

Behavioral regression fixed

  • (make-rectangular (- (make-rectangular 3 0) (make-rectangular 0 0)) 5) now returns 3+5i again. The strict Q-i=C3 contract in PR-1 panicked here because isRealNumber in extensions/math/prim_complex.go:111-122 admits *Complex with imag == 0 as a real number. The new contract: complexToFloat64/bigComplexToFloat64 return the real component when imag == 0 (lossless), error only when imag != 0. This aligns with plans/2026-05-14-numeric-loss-signals-design.md's principle: "succeed silently when no information lost; error precisely when it would be." First crosscheck (3 lenses) had marked this safe; second crosscheck (code lens at Opus) caught the actual isRealNumber body. Test TestNumberToFloat64PanicsOnNonzeroImag locks the contract.

Other Critical fixes

  • Receiver sp on all 5 NumericTypeSpec methods (was 533 p vs 5 s after PR-1).
  • numeric_kind.go "ADDING A NEW NUMERIC TYPE" guide updated: removes stale NumberToFloat64/Simplify items, adds registerNumericSpec step, renumbers to 13 items.
  • NumberToFloat64 panic preserves ErrNotAReal as cause via werr.WrapForeignErrorWithCause.

Notable

  • LookupNumericSpec renamed from Lookup (descriptive in broad values package). Bounds-checks the kind.
  • registryMu removed (Go init is single-goroutine).
  • Eager init validation moved to validate_numeric_registry.go (sorts after all per-type files lexically; required because rational.go's init runs after numeric_registry.go's).
  • 21 new per-type helpers across 7 files gain godoc one-liners noting precision-loss status.
  • big_complex.go helpers moved below the dispatch-table comment+var block (godoc attachment).
  • Simplify(nil) returns nil; ExactnessOf(nil) panics with ErrNotANumber.
  • numeric_registry_test.go gains copyright header.

Test improvements

  • TestNumericRegistryAllKindsRegistered uses reflection to verify all function fields non-nil.
  • TestNumericRegistrySmoke SimplifyDown line is now meaningfully asserted; added Complex(3+0i) and BigComplex(3,0) cases.
  • TestTypeSwitchFunctionsHandleAllTypes recover block surfaces panics as typed failures.
  • equivalenceExemplars expanded with MinInt64, BigInteger > int64 (both signs), Float(-0.0), Rational(1/3), inexact-real-with-exact-zero-imag BigComplex.

Deferred

  • Loss-signals plumbing for BigFloat/BigInteger/Rationalfloat64 (silent truncation today): owned by plans/2026-05-14-numeric-loss-signals-impl.md, which is gated on this PR merging.
  • Types-lens architectural suggestions (methods-on-Number for SimplifyDown/ToFloat64/ToComplex128; exactness func(Number) Exactness field; collapse parallel arrays): real design improvements but a different design than the user-approved plan; deferred to a future structural-reduction wave.

CI: make ci passes (lint 0 issues, all tests green, 38/38 packages ≥ 80% coverage).

complex(3+0i, 0) compiles (untyped complex constant with zero imag
converts implicitly to float), but reads as if a complex value is
being passed as the real argument. NewComplex accepts complex128
directly, so the literal form (NewComplex(3+0i)) is both shorter
and unambiguous. Addresses Copilot's inline comment on PR #752.
@aalpar
Copy link
Copy Markdown
Owner Author

aalpar commented May 14, 2026

Copilot inline-comment resolutions

Four Copilot comments on the initial commit (8ab2ced1). Three were resolved by the crosscheck-fix commit (b68a8115); one resolved by the follow-up style commit (e91ed287); one is a known false positive.

1. VERSION bump — not actionable (known false positive)

A pre-commit hook auto-bumps VERSION on every commit. The hook fires on feature PRs too. Per memory feedback-copilot-version-false-positive.md: "pre-commit hook auto-bumps VERSION every commit; Copilot flags on every PR. Expected false positive; don't revert."

2. values/promotion.go ErrNotAReal cause-discard — resolved in b68a8115

NumberToFloat64 now uses werr.WrapForeignErrorWithCause(werr.ErrNotAReal, err, ...):

panic(werr.WrapForeignErrorWithCause(werr.ErrNotAReal, err,
    "NumberToFloat64: cannot convert %T to float64", n))

The inner ErrNotAReal from complexToFloat64/bigComplexToFloat64 is now the sentinel of the panicked error AND its cause. errors.Is(panicVal, werr.ErrNotAReal) succeeds. Test TestNumberToFloat64PanicsOnNonzeroImag locks this contract.

3. values/numeric_registry_test.go:329 complex(3+0i, 0) syntax — resolved in e91ed287

Copilot's "will not compile" assessment was inaccurate (Go converts untyped complex constants with zero imag implicitly to float, so the original compiled fine), but the style point was validcomplex(3+0i, 0) reads as if a complex value is being passed as the real argument. Cleaned up to use direct complex literals:

NewComplex(3 + 0i),
NewComplex(3.5 + 0i),
NewComplex(3 + 4i),

4. values/numeric_registry.go Lookup bounds check — resolved in b68a8115

Renamed LookupLookupNumericSpec (descriptive in the broad values package) and added a bounds check:

func LookupNumericSpec(kind NumericKind) *NumericTypeSpec {
    if int(kind) >= int(numKinds) {
        panic(werr.WrapForeignErrorf(werr.ErrNumericRegistry,
            "LookupNumericSpec: kind %d out of range [0,%d)", kind, numKinds))
    }
    return &numericRegistry[kind]
}

Out-of-range kinds now produce ErrNumericRegistry instead of a Go runtime index out of range panic.

@aalpar aalpar merged commit 082836d into master May 14, 2026
1 check passed
@aalpar aalpar deleted the feat/values-sr-phase3-numeric-registry branch May 14, 2026 23:54
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