Narrowing down error types

[amomchilov]

(continued from this comment)

Right now, this library (and SecureXPC) model their errors as enums whose cases are (roughly) 1-to-1 with the enums/constants of the underlying library. I see two usability difficulties with this:

1. Not every error case is applicable to every throwing call. For example, bless(label:authorization) looks like it can throw jobMustBeEnabled (because that value exists in the look up table the error path uses), but it doesn't (or does it? I have no way to tell without digging into it, which is kind of the point).

• As a result, api user have to do their own research into which cases are actually possible, and which ones to toss into a default: fatalError("this error case can't happen in this context"). Each user would have to rediscover that for themselves.

2. Some times Apple's error constants have dual meaning that depends on their context. Reading the those tea leaves, in order to pin down the problem and figure out how to fix it, requires diving into internal details of this framework (and Apples'). It makes it a leaky abstraction.

I think we have an opportunity to do the research and pool it together, and codify it into use-case specific error enums. E.g. BlessError, which only contains cases that can be thrown from the bless operation, and whose descriptions translates the general messages/comments of the error codes into concrete issues.

What are your thoughts, Josh?

[jakaplan]

Not every error case is applicable to every throwing call. For example, bless(label:authorization) looks like it can throw jobMustBeEnabled (because that value exists in the look up table the error path uses), but it doesn't (or does it? I have no way to tell without digging into it, which is kind of the point).

My understanding is this is standard for Swift frameworks. As an example CryptoKit includes CryptoKitError which is presumably thrown in many different places by the framework. As an example (I pretty much chose this at random), this initializer throws. Presumably it throws an instance of CryptoKitError, although that's not documented.

As a result, api user have to do their own research into which cases are actually possible, and which ones to toss into a default: fatalError("this error case can't happen in this context"). Each user would have to rediscover that for themselves.

Well whether they want to use a fatalError(...) is of course up to them, but yes that appears to be normal for Swift frameworks? At least it is in the Apple ones I've seen.

Some times Apple's error constants have dual meaning that depends on their context. Reading the those tea leaves, in order to pin down the problem and figure out how to fix it, requires diving into internal details of this framework (and Apples'). It makes it a leaky abstraction.

I'm open to figuring out how to improve this, although I could see it significantly increasing the complexity of error generation. Because Apple's documentation is often seriously lacking (SMJobBless(...) is a great example), this could be a large endeavor to do well.

[amomchilov]

For now I'd be happy to accept PRs that provide improved documentation on specifically what errors are thrown by each function.

Heh, I'm already working on this :)

here isn't any guarantee that currently observed behavior will match future behavior.

I think that's where -library-evolution-mode comes in. These enums would become "unfrozen", and callers would have to add a @unknown default: case to their switch statements.

(You might ask what the difference is at that point. The difference would be that we don't have to have every kSMError* constant as a possible case of a hypothetical BlessError, on the off chance that it gets returned under a set of circumstances we didn't consider. We can add cases for all the failure modes that we know can happen for sure (no more // todo: can this every happen? in client code). If we miss any, we can add them in the future, only once we're sure that such an error can actually ever happen.)

This allows us to cover all the known cases today, while having an escape hatch to handle unknown ones in the future. Alternatively, we can keep an unknown case as there is today.

[jakaplan]

Perhaps I misunderstand how -library-evolution-mode works (I only skimmed the proposal doc you linked to), but it's not apparent to me how this helps with needing an unknown case. Although it seems like it's very helpful when there's the potential to add more cases in the future. Since we can't be certain what OSError or CFError values are going to be returned by the underlying framework API calls, we always need some Swift Error enum case to map those to and that's what unknown is for. Am I misunderstanding?

[amomchilov]

An unfrozen enum (caused by importing from C/ObjC, or using -library-evolution-mode) is similar to having an case unknown(OSStatus), but it's more broad. When you add your own case unknown(OSStatus), you're locking yourself out from the ability to model an unknown CFError (for example), or a SomeOtherFailureCodeType. Added new cases (such as case unknownCFError(CFError)) becomes a breaking API change, because even though your clients were forced to handle the existing unknown case, they haven't written anything to handle a new unknownCFError case.

In library evolution mode, you don't need to add an explicit unknown case (although you can, if you want), but you get the ability to add arbitrary new case, with arbitrary "shapes" (you can add whatever associated values are appropriate for modelling your newly-discovered failure modes). For an unfrozen enum, this is a non-breaking API change, because all the clients were already forced to handle @unknown default.

In practice, this means that you can be conservative in defining your Error's cases, sticking to only the ones you're certain can be thrown. The door is open to adding new cases in the future, as you discover new failure modes. As a result, callers are never put in a position to guess "did the library author include the foo case because foo can actually be thrown by this, or only because they weren't sure and wanted to be thorough?".

Idk if I explained that very well. LMK if you have questions.

[jakaplan]

What you wrote matches my understanding from skimming the -library-evolution-mode proposal doc.

Let's say we turn on that mode, it seems like we'd still need (one or more) unknown cases. For example if we're calling a C API which returns an OSError value, we might expect it's going to return one of three values that we know how to map to enum cases we've defined. However, at runtime it's always possible some other value will be returned. We need to handle that value somehow, and it seems like having a unknown case would be the best way to do so?

[amomchilov]

I think it’s a matter of how high or low level you want your wrapper to be.

In my opinion, I think it’s worth putting in effort to maximally isolate the caller from OSStatus codes, CFError, errno, etc. the issue is that the errors spit out by these underlying frameworks are so vague, and require so much interpretation effort, that it’s unreasonable to expect much of the callers.

However, at runtime it's always possible some other value will be returned. We need to handle that value somehow, and it seems like having a unknown case would be the best way to do so?

I think that if we get an OSError that does not map directly to one of the existing “high level” error cases, then that should be considered a defect of the framework that should be remedied by a new case being added in a new release. We should do the research into the failure mode, what causes it, and automate a check that spits out a nicer diagnostic. E.g. “you forgot to widget your gadget” instead of “auth denied”.

I don’t think having a case unknown(OSStatus) in that context is useful, because I don’t think the enclosed OSStatus is that useful (unless you already have an in depth understanding of the underlying apple frameworks, which is kind of what we’re trying to avoid, right?)

[jakaplan]

See #14 which is in the spirit of this discussion.