Separate ErrorKinds by callsite

[LastExceed]

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

[edwloef]

I'm pretty sure that's basically what we had before #1167

[LastExceed]

Thanks for linking. For comparison, this is what was there before that PR:

pub struct BackendSpecificError {
    pub description: String,
}

pub enum DeviceIdError {
    BackendSpecific { err: BackendSpecificError },
    UnsupportedPlatform,
}

pub enum DeviceNameError {
    BackendSpecific { err: BackendSpecificError },
}

pub enum SupportedStreamConfigsError {
    DeviceNotAvailable,
    DeviceBusy,
    InvalidArgument,
    BackendSpecific { err: BackendSpecificError },
}

pub enum DefaultStreamConfigError {
    DeviceNotAvailable,
    DeviceBusy,
    StreamTypeNotSupported,
    BackendSpecific { err: BackendSpecificError },
}

pub enum BuildStreamError {
    DeviceNotAvailable,
    DeviceBusy,
    StreamConfigNotSupported,
    InvalidArgument,
    StreamIdOverflow,
    BackendSpecific { err: BackendSpecificError },
}

pub enum PlayStreamError {
    DeviceNotAvailable,
    BackendSpecific { err: BackendSpecificError },
}

pub enum PauseStreamError {
    DeviceNotAvailable,
    BackendSpecific { err: BackendSpecificError },
}

pub enum StreamError {
    DeviceNotAvailable,
    StreamInvalidated,
    BufferUnderrun,
    BackendSpecific { err: BackendSpecificError },
}

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]

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]

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 have e.msg(), and e.kind(), which would match PlayStreamErr, so one unified error type, but say play() -> 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]

it was a bit more granular

I would actually argue that it wasn't. For instance, PlayStreamError and PauseStreamError being 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.

DeviceBusy can occur in many error kinds

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]

Nope, don't worry about your table on my account, (I think was thinking of DeviceNotAvailable not DeviceBusy, 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 found

[Marco-Farruggio]

just noticed the dividers on your table, maybe a reasonable mid-ground would be splitting into Host/Device/Stream error?

[Marco-Farruggio]

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]

I'm pretty sure that's basically what we had before #1167

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 std regarding io::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 for io::Error then it should be enough for cpal.

[Marco-Farruggio]

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]

As far as I'm aware io::Error is "that way" because it's basically a 1:1 mirror of C's errno, 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]

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:

1. What we had before feat(error): unified errors with precise kinds, messages, and no panics #1167 - 9 enums with many duplicate variants that somehow still lack a significant amount of specificity
2. What we currently have - 1 single flat enum for everything, with more specificity than no.1
3. What we could have - 3 or 4 enums + a nested one, with just as much specificity as no.2

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.

could we clearly define the problem we want to solve?

Currently, the ErrorKind type suggests a lot more error scenarios than are actually possible, causing error handling to be more inconvenient than it needs to be.

when it’s good enough for io::Error then it should be enough for cpal

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]

could we clearly define the problem we want to solve?

Currently, the ErrorKind type suggests a lot more error scenarios than are actually possible, causing error handling to be more inconvenient than it needs to be.

We need to weigh the costs of API breakage versus solution heft. Cross-posted from #1334:

In the presence of BackendSpecific catch-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.

when it’s good enough for io::Error then it should be enough for cpal

I don't really see that as an active counterweight.

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:

• established
• well-known
• analogous (audio I/O <> file I/O)

[LastExceed]

you cannot guarantee exhaustiveness or determinism, plus they the guarantee is no stronger or weaker than an enum that's tagged with #[non_exhaustive].

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)