Skip to content

Commit

Permalink
Structify errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@adam-fowler
adam-fowler committed May 14, 2024
1 parent 9ed9826 commit 576003f
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 7 deletions.
13 changes: 11 additions & 2 deletions Sources/ExtrasBase64/Base32.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,17 @@ public enum Base32 {
public static let allowNullCharacters = DecodingOptions(rawValue: UInt(1 << 0))
}

public enum DecodingError: Swift.Error, Equatable {
case invalidCharacter
public struct DecodingError: Swift.Error, Equatable {
enum _Internal {
case invalidCharacter
}

fileprivate let value: _Internal
init(_ value: _Internal) {
self.value = value
}

public static var invalidCharacter: Self { .init(.invalidCharacter) }
}

/// Base32 Encode a buffer to an array of bytes
Expand Down
22 changes: 17 additions & 5 deletions Sources/ExtrasBase64/Base64.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,11 +334,23 @@ extension Base64 {
public static let omitPaddingCharacter = DecodingOptions(rawValue: UInt(1 << 1))
}

public enum DecodingError: Error, Equatable {
case invalidLength
case invalidCharacter(UInt8)
case unexpectedPaddingCharacter
case unexpectedEnd
public struct DecodingError: Error, Equatable {
fileprivate enum _Internal: Error, Equatable {
case invalidLength
case invalidCharacter(UInt8)
case unexpectedPaddingCharacter
case unexpectedEnd
}

fileprivate let value: _Internal
fileprivate init(_ value: _Internal) {
self.value = value
}

public static var invalidLength: Self { .init(.invalidLength) }
public static func invalidCharacter(_ character: UInt8) -> Self { .init(.invalidCharacter(character)) }
public static var unexpectedPaddingCharacter: Self { .init(.unexpectedPaddingCharacter) }
public static var unexpectedEnd: Self { .init(.unexpectedEnd) }
}

@inlinable
Expand Down

5 comments on commit 576003f

@justinpawela
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adam-fowler Can I ask what the motivation was for this change?

It doesn't seem make any difference internally, and in my code which uses this library, it just makes the errors harder to catch.

Unless I'm missing something? I haven't seen any best-practices or anything swimming around the Swift forums, but maybe I've missed it.

@adam-fowler
Copy link
Collaborator Author

@adam-fowler adam-fowler commented on 576003f Jul 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justinpawela the reason for the change is so the error type can be extended without generating a breaking change that requires a major version release.

Extending an enum ie adding a case is considered a breaking change. By changing the error to a struct we can add new errors without causing a breaking change.

@adam-fowler
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some context here is a pitch for extensible enums that never made it https://forums.swift.org/t/extensible-enumerations-for-non-resilient-libraries/35900

@justinpawela
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adam-fowler Thanks for your time responding and for providing the link to the forums discussion. I understand what you mean about the breaking change. However...

Major Caveat: This feedback is not really directed at you or this commit; it would probably be better placed in the forums. I know it is a little philosophical.

To me, this feels like a circumvention of one of the benefits of Swift enums in control flow. To be able to exhaustively handle (or catch) all DecodingErrors is a feature of Swift pattern matching, not a language hole to be worked around. If I, as a library user, care deeply enough about the specific case of DecodingError to try to handle (or catch) the cases individually, I want my code not to compile if the library adds a new case. If I didn't care, I wouldn't be examining the cases individually, and any new cases wouldn't affect my build anyway.

I do realize this creates more burden on library users, but it should be a welcome burden. I also realize it creates more burden on library maintainers, but strong typing, type safety, and pattern matching exhaustivity are all core advantages of Swift, and working around them to avoid having to do a major version bump feels, to me, a bit like pushing the burden of uncertainty onto library users. With this change, if I care about catching specific cases of DecodingError, I will always have to check the release notes/source code when this package is updated, because I won't be able to rely on switch/catch exhaustivity checks.

From my point of view, moving to a 1.0 release should be a time when we feel more comfortable with the group of error cases we expect to be throwing. Not a time to be adding an escape hatch to be able to add more error cases without doing major version bumps.

Also, a more concrete concern: I used to be able catch (or switch on) any .invalidCharacter error like this:

    do {
        return try Base64.decode(bytes: data, options: opts)
    } catch Base64.DecodingError.invalidCharacter(let char) {
        doSomething(with: char)
    }

or:

    // let error: Base64.DecodingError
    switch error {
    case .invalidCharacter(let char):
        doSomething(with: char)
    // ... other cases ...
    }

But now I cannot really handle or catch an .invalidCharacter error at all, because it is a function, not a value. Or I have to match specifically on each of the 191 remaining character codes. I can't even do ranges or anything because the storage is hidden away in the private _Internal enum.

Anyway, thanks for your time discussing this, for the contributions you've made to this package, and for your contributions to Swift server in general. You've made a big impact on the ecosystem.

@adam-fowler
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately extendable error types are a reality in Swift library development.

  • You can't place the onus on the user to fix up their code if you add a new case to an enum because the user might be another library which is then used by someone else who is then dependent on the maintainer (who may have gone AWOL) of the library to update their code if they want a compiling project with latest code.
  • In a perfect world we would know ahead of time all the errors a library might need to throw, but systems get extended and it is perfectly possible to extend a library without requiring a major update of a library. If those updates require new errors it should be possible to add those as well without requiring a major update.

But given all that I may have made a mistake in this case. The pattern of replacing enum Errors with structs only really works for enums that don't have associated values. We have a number of options here.

  1. Deprecate Base64.DecodingError and replace with original enum
  2. Deprecate invalidCharacter(UInt8) and replace with a new case that doesn't include the character
  3. Deprecate invalidCharacter(UInt8) and replace with a new Error type

Please sign in to comment.