Skip to content

gamut-core: PixelFormat::ALL is a fixed-length array on a #[non_exhaustive] enum, so appending a format is a type change #629

Description

@justin13888

The shape

// crates/gamut-core/src/pixel.rs
/// Discriminants are explicit and permanent — they are C ABI values; new variants append.
#[non_exhaustive]
#[repr(u32)]
pub enum PixelFormat { Gray8 = 0, /* … */ GrayAlpha16 = 10 }

impl PixelFormat {
    pub const ALL: [PixelFormat; 11] = [ /* … */ ];
}

The enum is #[non_exhaustive] and its own docs say "new variants append". ALL is a
fixed-length array, so its type is [PixelFormat; 11] — appending a twelfth format changes the
constant's type, not merely its contents. Every caller who named the type, matched on the length,
or relied on array-ness (PixelFormat::ALL.map(..), PixelFormat::ALL.into_iter() by value,
const N: usize = PixelFormat::ALL.len() in a const generic position) breaks on an append that
#[non_exhaustive] exists to make non-breaking. That is the promise inverted: the attribute tells a
caller "appending is safe for you", and this constant makes it not.

It is the only one left in the workspace with that shape. Every other ALL is already a slice:

constant type enum
gamut_icc::KnownTag::ALL &'static [KnownTag]
gamut_exif::ExifTag::ALL &'static [ExifTag]
gamut_xmp::WellKnownNs::ALL &'static [WellKnownNs]
gamut_metadata::capability::Format::ALL &'static [Self] #[non_exhaustive]
gamut_metadata::capability::Carrier::ALL &'static [Self] #[non_exhaustive]
gamut_metadata::capability::Direction::ALL [Self; 2] exhaustive — correct as an array
gamut_core::PixelFormat::ALL [PixelFormat; 11] #[non_exhaustive]

Why this is filed rather than fixed

PR #509 introduced capability::Format and capability::Carrier with exactly this defect and fixed
it before the constants shipped, in the same pull request that added them (its decision 19). The
contrast is the useful part here: that fix cost nothing, because nobody could yet have named the
array type. This one is not the same case.

gamut-core is at 2.0.1 and published, and PixelFormat::ALL is released API that every other
crate in the workspace sits on top of. Converting it to &'static [PixelFormat] is itself a
breaking change today — the very break the conversion is meant to prevent later — so it costs a
major bump of the workspace's root crate and a sweep of the callers. That is a price somebody has to
choose to pay; it is not a repair that can be slipped in.

Concretely, whoever picks this up is choosing between:

  1. Convert at the next gamut-core major. One break, taken deliberately, and the shape then
    matches every other ALL in the workspace. In-repo callers are crates/gamut-core/src/pixel.rs,
    crates/gamut-core/src/invariants.rs and crates/gamut-core/src/convert.rs; note also that
    crates/gamut-ffi/DESIGN.md documents enumerating gamut_core::PixelFormat::ALL × codecs to
    generate the C surface, so the C-ABI generation reads this constant.
  2. Add a slice-typed sibling and deprecate the array. Non-breaking now, at the cost of two names
    for one list until the next major removes the array.
  3. Leave it and accept the break at the next append. Defensible only if no twelfth pixel format
    is expected — but the enum is #[non_exhaustive] and documented as appendable precisely because
    one is.

What was not done

Nothing was changed. This was found while closing the review of #509 and is outside that branch's
manifest (gamut-core is not in it), and — unlike the constants that branch owns — it is already
released, so a repair there would have been an unreviewed breaking change to the root crate smuggled
into a documentation round.

Refs #420.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions