Transposition must refuse an extreme interval, not panic on it
Audit finding 1, reproduced and fixed. Pitch::transposed did its arithmetic in
i32 while the interval's own components are i32, so intermediates overflowed:
diatonic_steps = i32::MAX panicked at `12 * new_octave`, chromatic_steps =
i32::MAX at `semitone + c`. TranspositionInterval::inverse negated i32::MIN.
The comment above the arithmetic even said "widen before arithmetic" -- it
widened i8 to i32, which is exactly not wide enough. All of it now widens to
i64, where the largest intermediate is bounded by ~3.7e9.
Refusing is the contract. Panicking on a value the public type admits is not.
I checked whether this was worse than a panic. The workspace sets
overflow-checks = true in release, but epiphany-core is a library and a
consumer's default release profile has them off, where these expressions wrap.
A 10.5M-case sweep of wrapping-vs-exact arithmetic (175 base pitches x 60225
interval pairs, edges plus random) found ZERO inputs where wrapping produced a
wrong Ok rather than a refusal. So this was a panic, not silent corruption, and
the audit's characterisation was exactly right.
inverse() now returns Option: -i32::MIN is not an i32. 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. Both regressions mutation-verified by
restoring the i32 arithmetic and the bare negation.
Also in this commit, two documentation corrections:
- The reducer's test-harness comment claimed both old transpose_* tests "now
reduce ONTO a base and assert the pitch value". They do not, and should not;
DECISIONS.md already said so. The comment now matches.
- The catalog's undo semantics for BOTH transpose kinds claimed value-restoring
undo recovers the pre-transpose pitch from the write chain. Neither kind
records into that chain, so this was false. The frozen Transpose reverts to
its honest pre-Push-4a statement -- undo does not negate it (P11-C8) -- and,
per the freeze doctrine, that is now pinned: making it record would change
what a stored {Transpose, UndoTransaction} history replays to. TransposeInterval's
paragraph states the requirement the next commit implements.
Gate: clippy 0, 30 targets / 985 passed / 0 failed, docs 0 under -D warnings,
conformance 8/8, zero golden churn, catalog rebuilds clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
582b0ca234
commit
4228320f88
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<Self> {
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Binary file not shown.
|
|
@ -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}
|
||||
|
|
|
|||
Loading…
Reference in New Issue