diff --git a/crates/epiphany-core/DECISIONS.md b/crates/epiphany-core/DECISIONS.md index f12f7f1..c8ecf0d 100644 --- a/crates/epiphany-core/DECISIONS.md +++ b/crates/epiphany-core/DECISIONS.md @@ -543,8 +543,19 @@ interval at the written/sounding boundary — nothing respells a written part into a sounding one — so `Instrument.transposition` stays advisory, for that reason and not the stated one. -**Refusal, not saturation.** `alteration` and `octave` are `i8`. A -transposition whose result does not fit refuses; so does one against a +**Refusal, not saturation, and never a panic.** All arithmetic widens to `i64` +first. `i32` is not wide enough to hold the intermediates of an `i32` interval: +the first version of `transposed` panicked on `diatonic_steps = i32::MAX` at +`12 * new_octave`, and `inverse()` panicked on `i32::MIN`. Refusing is the +contract; panicking on a value the public type admits is not. (Under +`overflow-checks = false` — any downstream release build — those expressions +wrapped instead. A 10.5M-case sweep of wrapping-vs-exact found *no* input where +wrapping produced a wrong `Ok` rather than a refusal, so the defect was a panic, +not silent corruption. `inverse()` now returns `Option`, because an interval +whose inverse is not representable is a fact about the type.) + +`alteration` and `octave` are `i8`. A transposition whose result does not fit +refuses; so does one against a non-`Cmn` position (no nominal to move) or an `AcousticRealization::AbsoluteHz` pitch (which overrides the tuning system, so moving the scale position moves the notehead without moving the sound). Saturation is the worst possible diff --git a/crates/epiphany-core/src/pitch.rs b/crates/epiphany-core/src/pitch.rs index 19170ee..fc922ba 100644 --- a/crates/epiphany-core/src/pitch.rs +++ b/crates/epiphany-core/src/pitch.rs @@ -217,13 +217,18 @@ pub struct TranspositionInterval { } impl TranspositionInterval { - /// The interval that undoes this one. Exact, because [`Pitch::transposed`] - /// never saturates — it refuses instead. - pub fn inverse(self) -> Self { - TranspositionInterval { - diatonic_steps: -self.diatonic_steps, - chromatic_steps: -self.chromatic_steps, - } + /// The interval that undoes this one, or `None` when it is not + /// representable: `i32::MIN` has no `i32` negation. Exact where it exists, + /// because [`Pitch::transposed`] never saturates — it refuses instead. + /// + /// The non-representable case is explicit rather than a panic or a wrap: + /// an interval whose inverse cannot be written down is a fact about the + /// type, and a caller composing undo out of inverses must see it. + pub fn inverse(self) -> Option { + Some(TranspositionInterval { + diatonic_steps: self.diatonic_steps.checked_neg()?, + chromatic_steps: self.chromatic_steps.checked_neg()?, + }) } } @@ -280,17 +285,25 @@ impl Pitch { return Err(TransposeRefusal::NonCmnPosition); }; - // Widen before arithmetic: the i8 bound is a *result* constraint, not - // an intermediate one, so an octave that overflows on the way to a - // value that fits would be a spurious refusal. - let n = nominal as i32; + // Widen to `i64` before any arithmetic. The `i8` bound is a *result* + // constraint, not an intermediate one, so an octave that overflows on + // the way to a value that fits would be a spurious refusal — but + // `i32` is not wide enough to hold the intermediates for an `i32` + // interval, and the previous version of this function panicked on + // `diatonic_steps = i32::MAX` at `12 * new_octave`. Refusing is the + // contract; panicking on a value the public type admits is not. + // + // `i64` is amply wide: `step` is bounded by `6 + 2^31`, so + // `new_octave` by `2^31/7 + 127`, and the largest intermediate + // `12 * new_octave` by roughly `3.7e9`. + let n = i64::from(nominal as u8); let semitone = - i32::from(nominal.chromatic()) + i32::from(alteration) + 12 * i32::from(octave); - let step = n + interval.diatonic_steps; - let new_nominal = CmnNominal::from_index(step.rem_euclid(7)); - let new_octave = i32::from(octave) + step.div_euclid(7); - let new_alteration = (semitone + interval.chromatic_steps) - - (i32::from(new_nominal.chromatic()) + 12 * new_octave); + i64::from(nominal.chromatic()) + i64::from(alteration) + 12 * i64::from(octave); + let step = n + i64::from(interval.diatonic_steps); + let new_nominal = CmnNominal::from_index(step.rem_euclid(7) as i32); + let new_octave = i64::from(octave) + step.div_euclid(7); + let new_alteration = (semitone + i64::from(interval.chromatic_steps)) + - (i64::from(new_nominal.chromatic()) + 12 * new_octave); let octave = i8::try_from(new_octave).map_err(|_| TransposeRefusal::OutOfRange)?; let alteration = i8::try_from(new_alteration).map_err(|_| TransposeRefusal::OutOfRange)?; @@ -1046,7 +1059,9 @@ mod tests { let start = cmn(CmnNominal::E, -1, 3); for interval in [iv(4, 7), iv(-2, -3), iv(7, 12), iv(0, 1), iv(5, 7)] { let there = start.transposed(interval).unwrap(); - let back = there.transposed(interval.inverse()).unwrap(); + let back = there + .transposed(interval.inverse().expect("these inverses exist")) + .unwrap(); assert_eq!( position(&back), position(&start), @@ -1078,6 +1093,45 @@ mod tests { assert_eq!(position(&high), (CmnNominal::C, 0, 127)); } + #[test] + fn an_extreme_interval_refuses_instead_of_panicking() { + // The public type admits any `i32`. Every one of these overflowed an + // `i32` intermediate and panicked (or, with overflow-checks off, wrapped + // through unspecified arithmetic) before the widening to `i64`. + let p = cmn(CmnNominal::C, 0, 4); + for (d, c) in [ + (i32::MAX, 0), + (i32::MIN, 0), + (0, i32::MAX), + (0, i32::MIN), + (i32::MAX, i32::MAX), + (i32::MIN, i32::MIN), + (i32::MAX, i32::MIN), + ] { + assert_eq!( + p.transposed(iv(d, c)), + Err(TransposeRefusal::OutOfRange), + "({d}, {c}) must refuse, not panic" + ); + } + // The boundary either side of a representable octave shift. + assert!(cmn(CmnNominal::C, 0, 126).transposed(iv(7, 12)).is_ok()); + assert_eq!( + cmn(CmnNominal::C, 0, 127).transposed(iv(7, 12)), + Err(TransposeRefusal::OutOfRange) + ); + } + + #[test] + fn the_inverse_of_the_unrepresentable_interval_is_none() { + // `-i32::MIN` is not an `i32`. Negating it panicked. + assert_eq!(iv(i32::MIN, 0).inverse(), None); + assert_eq!(iv(0, i32::MIN).inverse(), None); + assert_eq!(iv(i32::MIN, i32::MIN).inverse(), None); + assert_eq!(iv(i32::MIN + 1, 0).inverse(), Some(iv(i32::MAX, 0))); + assert_eq!(iv(4, 7).inverse(), Some(iv(-4, -7))); + } + #[test] fn a_transposition_refuses_a_non_cmn_position() { let mut p = cmn(CmnNominal::C, 0, 4); diff --git a/crates/epiphany-ops/src/reduce.rs b/crates/epiphany-ops/src/reduce.rs index a0a5e9e..4248383 100644 --- a/crates/epiphany-ops/src/reduce.rs +++ b/crates/epiphany-ops/src/reduce.rs @@ -8940,8 +8940,14 @@ mod tests { // The two `transpose_*` tests below were FALSE LOCKS until Push 4a: they // reduced base-free, where `graph` is `None` and `graph_transpose_pitch` // never runs, and they asserted only `OperationEffect`. Gutting the - // transpose entirely left both green. They now reduce ONTO a base and - // assert the pitch value, which is the thing the operation exists to change. + // transpose entirely left both green. + // + // They still reduce base-free, and should: what they assert — skip + // tombstoned, skip system-derived, refuse missing — *is* effect-log + // behaviour. Only the first one's NAME was a lie (it claimed to check the + // live target "shifts"). It was renamed. The shift itself is locked by the + // `the_frozen_transpose_*` tests, which reduce ONTO a base and assert the + // pitch value. /// Every `(event, pitch)` in canonical id order — `events.iter()` walks a /// slotmap, whose order is an implementation detail. diff --git a/spec/operation_catalog.pdf b/spec/operation_catalog.pdf index f3f3475..60ab587 100644 Binary files a/spec/operation_catalog.pdf and b/spec/operation_catalog.pdf differ diff --git a/spec/operation_catalog.tex b/spec/operation_catalog.tex index 15a0c29..f04c0dd 100644 --- a/spec/operation_catalog.tex +++ b/spec/operation_catalog.tex @@ -665,9 +665,20 @@ non-system target \emph{occurrence} in \texttt{targets}, the target's \texttt{Cm (a deterministic repair, not a conflict). \textbf{Undo semantics.} Transpose mints nothing, so the prototype's minted-object -undo (Section~\ref{sec:k0:undo}) does not negate it; value-restoring undo recovers -the pre-transpose pitch from the write chain rather than by applying an inverse -interval --- which, under saturation, would not exist. +undo (Section~\ref{sec:k0:undo}) does not negate it, and it records no write into +the pitch's value chain, so \emph{value-restoring undo does not restore it +either}: undoing a transaction containing a \texttt{Transpose} leaves the shifted +pitch shifted. An inverse-interval undo does not exist (under saturation the +inverse is not a function) and remains a deferred refinement (P11-C8). + +\textbf{This too is frozen.} Making \texttt{Transpose} record into the value +chain would not change its own reduction rule, but it \emph{would} change what a +stored history $\{\texttt{Transpose}, \texttt{UndoTransaction}\}$ replays to --- +from "the pitch stays shifted" to "the pitch returns" --- which is a change in +what an existing document means. \texttt{TransposeInterval} records its write and +\emph{is} undoable (Section~\ref{sec:k0:transpose-interval}); the frozen +operation is permanently not. That asymmetry is another reason never to author +it. \textbf{Re-anchoring.} Tombstoned targets are skipped (the transpose applies only to live pitches). \texttt{SYSTEM\_DERIVED}-namespace targets are likewise @@ -745,10 +756,18 @@ is transposed by \texttt{interval}. \textbf{Conflict cases.} None --- composition is deterministic in canonical order. -\textbf{Undo semantics.} As \texttt{Transpose}: nothing is minted, and -value-restoring undo recovers the pre-transpose pitch from the write chain. Here -an inverse interval does exist ($(-d, -c)$), because the reduction never -saturates; undo does not rely on it. +\textbf{Undo semantics.} Nothing is minted. \texttt{TransposeInterval} +\MUST{} record each transposed pitch's new value into that pitch's value chain, +and each spelling attachment it rewrote into the corresponding spelling chain, so +that value-restoring undo recovers the pre-transpose pitch \emph{and} its +pre-transpose spelling. Restoring the pitch alone would leave a notehead spelled +for a pitch that no longer exists. This is where \texttt{TransposeInterval} +departs from the frozen \texttt{Transpose}, which records nothing and is +therefore not undoable. + +An inverse interval usually exists ($(-d, -c)$), because the reduction never +saturates --- but not always: $\texttt{i32::MIN}$ has no negation. Undo does not +rely on it. \textbf{Re-anchoring.} As \texttt{Transpose}. @@ -1366,9 +1385,12 @@ undo) design, which subsumes this question. identifier derivation the ratified closed tag set does not yet include); and \texttt{Cascade}'s dependent-closure computation --- \texttt{Cascade} remains \texttt{StrictInverse} over the same set. \texttt{Transpose} inversion is no -longer listed: \texttt{TransposeInterval} has an exact inverse $(-d, -c)$ -because it never saturates (Push~4a, closing P12-K2), though value-restoring -undo does not need one. The frozen \texttt{Transpose} still has none. +longer listed: \texttt{TransposeInterval} is undone by value restoration, not by +applying an inverse interval (Push~4a, closing P12-K2). An inverse +$(-d, -c)$ does exist wherever both components are negatable --- the reduction +never saturates --- but $\texttt{i32::MIN}$ has none, so the inverse is partial +and undo does not depend on it. The frozen \texttt{Transpose} is undone by +neither. % =========================================================================== \chapter{v0 \texorpdfstring{$\rightarrow$}{->} v1 Payload Migration}