Revisit Error handling

[thomasvl]

Since the library uses enums for errors, it means cases can't be added without breaking things; might be nice to revisit this model to see if something can be done to improve things.

[thomasvl]

#987 was tripped up by some of this.

[thomasvl]

I think some of the other Apple OSS packages (ArgumentParser?, docs?), dodged some of this by dropping the enums and using structs with statics (or something like that).

[Lukasa]

NIO has tried to adopt this as well. I'm in the process of trying to do some Thought Leadership(tm) about how to do this in the OSS ecosystem, but I wouldn't expect this to land in the near future. But yeah, structs with static lets work well.

[thomasvl]

@Lukasa any updates on guidance?

[Lukasa]

Yeah so our preferred pattern now looks like something like this:

struct ProtobufError: Error, Hashable, CustomStringConvertible {
    public var errorCode: ErrorCode

    public var sourceLocation: SourceLocation {
        return self.backingStorage.sourceLocation
    }

    public var errorDescription: String {
        return self.backingStorage.errorDescription
    }

    private let backing: BackingStorage

    init(errorCode: ErrorCode, description: String, file: String = #file, line: UInt = #line, function: String = #function) {
        self.errorCode = errorCode
        self.sourceLocation = SourceLocation(file: file, line: line, function: function)
        self.description = description
    }

    public static func ==(lhs: ProtobufError, rhs: ProtobufError) -> Bool {
        return lhs.errorCode == rhs.errorCode
    }

    public static func hash(into hasher: inout Hasher) {
        hasher.combine(errorCode)
    }

    public var description: String {
        return "ProtobufError(errorCode: \(errorCode), description: \(description), file: \(sourceLocation.file), line: \(sourceLocation.line), function: \(sourceLocation.function))"
    }
}

extension ProtobufError {
    public struct ErrorCode: Hashable, CustomStringConvertible {
        private enum Backing: Hashable {
            case errorOne  // your names go here
            case errorTwo
        }
        private var backing: Backing

        init(_ backing: Backing) {
            self.backing = backing
        }

        public static let errorOne = Self(.errorOne)
        public static let errorTwo = Self(.errorTwo)

        public var description: String {
            return String(describing: backing)
        }
    }
}

extension ProtobufError {
    public struct SourceLocation {
        public var file: String

        public var line: UInt

        public var function: String
    }
}

extension ProtobufError {
    final class BackingStorage {
        var sourceLocation: SourceLocation

        var errorDescription: String

        init(errorDescription: String, sourceLocation: SourceLocation) {
            self.sourceLocation = sourceLocation
            self.errorDescription = errorDescription
        }
    }
}

This is an annoyingly large amount of code but it maximises the utility of the error. The errorCode allows developers to programmatically understand the kind of error thrown, in case they want to handle it. The source location and error description allow us to both record the why of an error, and also to indicate from where in the code it originated. These two fields are stuffed into a class object to reduce the contiguous size of the Error, enabling it to be stored inline in an existential container instead of side-allocated. The errorCode is kept in the main error object as it's the most likely field to be interacted with by user code.

[thomasvl]

If we're going to do this, it seems logical to also look at #580 & #581 at the same time. So the meta question is of all of our different Error enums, which ones do we want to add the context like SourceLocation and description above?

The current TextFormat and JSON parsing probably does't even have complete offset tracking (because of creating nested objects in contexts) let along line/column info. Binary errors could attempt context via byte offsets.

And just about all of them might in theory be able to do something about a field number path to where the serialization/parsing failed, but that's potentially a lot of temp data to build/throw away in non error cases.

@Lukasa I guess there is no way to do something like associated data since things aren't enums any more? So things like unknown enum names (TextFormat/JSON) would just go into the description?

Or we just use 2.x to get away from enum Errors and don't bother with any of the additional context for the time being. I guess adding things once these are more structured is not a breaking change.

@Lukasa @tbkka @allevato @FranzBusch thoughts?

[Lukasa]

@Lukasa I guess there is no way to do something like associated data since things aren't enums any more? So things like unknown enum names (TextFormat/JSON) would just go into the description?

Broadly, yes. You can achieve some amount of this by having computed properties with optional return values where the return is only non-nil in the appropriate case.

[thomasvl]

This is happening via #1612

[gjcairo]

@thomasvl given we've decided to change the approach of #1612 slightly, do we still consider this done once that PR is merged? Or do we only consider it done once we've added back removed errors (i.e. once the cleanup for v2 happens: #1657)?

[thomasvl]

I guess since we have a new model, we can call it this done and track the migration in the other issue.