Repository navigation
Separate ErrorKinds by callsite #1342
Description
Gonna briefly go on a tangent here, but not without reason:
Rust lacks enum variant types. This is a language limitation. If you've used TypeScript before, then you might be familiar with the concept of type unions. If enum variants had dedicated types, then the same variant could be part of multiple enums. The common way to emulate this in Rust is basically:
enum MyEnum {
Variant1(Variant1),
Variant2(Variant2),
}
struct Variant1 {
// ...
}
struct Variant2 {
// ...
}Which is painfully verbose, but again: It is a workaround for a language limitation.
Going back to cpal now: If Rust enums were type unions, then our (current) variant types for the ErrorKind enum would be unit structs. But the point here is that having the same variant in 2 enums shouldn't be thought of as having 2 distinct values representing the same thing, but rather as the same type being used in 2 places. The benefit is functional: It tells you which errors can occur at a given callsite. The drawback is an implementation detail. It introduces a lot of boilerplate code because Rust lacks the ability to express type unions concisely.
To get a better feeling for how to weight this tradeoff, I've tried to roughly sketch out the current error landscape:
I probably missed a few cases (do let me know), but the general idea would be something like:
Before:
struct Error {
kind: ErrorKind,
message: Option<Cow<'static, str>>,
}
enum ErrorKind {
DeviceBusy,
DeviceChanged,
DeviceNotAvailable,
HostUnavailable,
InvalidInput,
PermissionDenied,
RealtimeDenied,
ResourceExhausted,
StreamInvalidated,
UnsupportedConfig,
UnsupportedOperation,
BackendError,
Other
}After:
struct Error<Kind> {
kind: Kind,
message: Option<Cow<'static, str>>,
}
enum HostErrorKind {
HostUnavailable,
General(General)
}
enum DeviceErrorKind {
DeviceNotAvailable,
UnsupportedOperation,
General(General)
}
enum BuildStreamErrorKind {
DeviceBusy,
InvalidInput,
UnsupportedConfig,
Device(DeviceErrorKind)
}
enum StreamErrorKind {
StreamInvalidated,
RealtimeDenied,
DeviceChanged,
General(General)
}
enum General {
PermissionDenied,
ResourceExhausted,
BackendError,
Other
}Naming TBD
Activity
edwloef commentedon Aug 24, 2026
I'm pretty sure that's basically what we had before #1167
LastExceed commentedon Aug 24, 2026
Thanks for linking. For comparison, this is what was there before that PR:
To be clear, I think getting rid of ^that was the right call, and I wouldn't want to go back there. But I also think that my proposal is quite a bit different from this.
edwloef commentedon Aug 24, 2026
Yeah it was a bit more granular, but separate enums for spots that can have different error kinds (i.e. not
io::Error).Marco-Farruggio commentedon Aug 24, 2026
im actually quite torn, I think id prefer the more granular approach, but I cant quite decide what kind of an API id like, I did just have one idea, because i personally don't like littering my code base with unreachable!(), so maybe we could have
cpal::Error<PlayStreamErr>, then we could havee.msg(), ande.kind(), which would match PlayStreamErr, so one unified error type, but sayplay() -> Result<(), Error<PlayStreamErr>I'm still unsure about how to define all these enums, for example, DeviceBusy can occur in many error kinds, should that be duplicated? or flattened with some Device(...) varient, maybe we could have some macro generate them all?
LastExceed commentedon Aug 24, 2026
I would actually argue that it wasn't. For instance,
PlayStreamErrorandPauseStreamErrorbeing separate types does not actually provide any informational benefit (that I can think of), and at the same time they're missing dedicated variants for errors that actually CAN occur (e.g.RealtimeDenied,StreamInvalidated, etc), so really it was really just worse in every way.Is my table that incorrect? In fairness I only filled it out based on gut feeling, but I intended my proposal to not sacrifice any specificity.
Marco-Farruggio commentedon Aug 24, 2026
Nope, don't worry about your table on my account, (I think was thinking of
DeviceNotAvailablenotDeviceBusy, being returned at some call sites), but anyways independent of the error type in question, my point was something needs to be sorted out regarding duplication/varient General(General)/flatten, so that a reasonable balance of clarity and simplicity can be foundMarco-Farruggio commentedon Aug 24, 2026
just noticed the dividers on your table, maybe a reasonable mid-ground would be splitting into Host/Device/Stream error?
Marco-Farruggio commentedon Aug 24, 2026
infact, the only error kinds that host, device, and stream share, are shared between ALL of them, so we could have host/device/stream/general, which is 4 varients, (except realtime denied, which is annoying)
roderickvd commentedon Aug 24, 2026
I am aware of being biased because of this: could we clearly define the problem we want to solve?
Reverting to a place with lots of redundancy and little apparent value is undesirable.
I’m going to be a bit tough here: I’d also like to prevent redoing discussions that surely have been had on rust-lang with
stdregardingio::Error. These are engineering trade-offs with no single optimum. Which is why I chose to mirror theirs. Basic stance: when it’s good enough forio::Errorthen it should be enough for cpal.Marco-Farruggio commentedon Aug 24, 2026
good point, im more than happy to stick with the current error setup, as there's not too many variants to worry about anyways, and the deduplication is nice :)
edwloef commentedon Aug 24, 2026
As far as I'm aware
io::Erroris "that way" because it's basically a 1:1 mirror of C'serrno, which can basically have an arbitrary value after any operation. I don't think CPAL has to deal with that kind of API anywhere, so I personally prefer split error types.LastExceed commentedon Aug 24, 2026
I get the impression that we're talking past each other a bit here, so I'll try to recap:
There is THREE different configurations:
To reiterate, I do NOT want to go back to no.1 - good riddance to that mess. #1167 was definitely an improvement. no.3 is very different from no.1, please carefully re-read the code snippets I posted.
Currently, the
ErrorKindtype suggests a lot more error scenarios than are actually possible, causing error handling to be more inconvenient than it needs to be.I'd agree that there are probably more important things to work on, but I don't really see that as an active counterweight. I was half expecting for this issue to go straight into the backlog, I just wanted to provide a dedicated place to discuss this, since the thread in #1334 where this conversation started was straying a bit far from its original topic
roderickvd commentedon Sep 6, 2026
We need to weigh the costs of API breakage versus solution heft. Cross-posted from #1334:
In the presence of
BackendSpecificcatch-alls, you cannot guarantee exhaustiveness or determinism, plus they the guarantee is no stronger or weaker than an enum that's tagged with#[non_exhaustive].Rust idioms seem somewhat at odds to me here - please enlighten me if I'm missing something - by on the one hand recommending that "kind" enums are tagged with
#[non_exhaustive](not "safe"), while also recommending to strongly type errors.To clarify my reasoning:
For sure there are many ways leading to Rome. This issue proves it. There will be advantages and disadvantages to each with no single optimum. Rather than spending the time discussing, I chose to go with a pattern that is:
LastExceed commentedon Sep 6, 2026
True, but that is not the goal anyway. While we cannot prevent requiring catch-all branches, we can prevent encouraging certain branches to be handled specifically. E.g. if a user of cpal wants their error handling to be as thorough as possible, then they might add an individual branch for every variant of the enum, plus a catch-all, since it is
#[non_exhaustive]. There is nothing functionally bad about that, but it is effectively dead code, and we can spare users from potentially wasting their time on writing dead code by being more specific with our error types.Again, not very important, just wanted a dedicated issue in order to move the discussion out of #1334 (and perhaps for future reference in case the topic comes up again)