Add explicit variants to Error

[Plecra]

We're currently using strings as impromptu error variants throughout the codebase. This lacks visibility for users, and risks the individual strings being treated as match cases.

We need to replace each line that constructs an InvalidArchive/UnsupportedArchive and add it to the enum declaration. Maybe we could add an unsupported method to check whose responsibility the error is?

[Plecra]

In response to @BenjaminRi in #232 (comment)

if you try to unzip an encrypzed ZIP file, but you supply no password, it returns a ZipError::UnsupportedArchive

Ah yes, removing this API was unfortunate as we wanted to maintain compatibility within 0.5.x. I should've implemented the API I mentioned in #197 (comment) at the time. Sorry!

I completely agree that error variants should be in the type system, and will be removing strings from the error type entirely when I can. When I was last actively maintaining the crate, I was looking at transitioning to 0.6 so this will hopefully be soon.

My expectation is that the error type will be updated along these lines, though it's not complete. Let me know what you think can be improved!

use std::{error, fmt, io};

struct Error(());
// We don't necessarily need to use these names, but the familiarity is nice.
/// Mirrors relevant variants of io::ErrorKind
enum ErrorKind {
    /// The archive is malformed
    InvalidData,
    /// Password not supplied for encrypted file/archive
    PermissionDenied,
    /// The archive uses features not implemented in this library
    // This might also be better as `Unsupported(Missing)` where `Missing` is an opaque type which can be used for details.
    Unsupported,
}
impl From<ErrorKind> for io::ErrorKind {}
impl fmt::Debug for Error {}
impl fmt::Display for Error {}
impl error::Error for Error {}
impl Error {
    fn kind(&self) -> Option<ErrorKind>;
    fn is_io_error(&self) -> bool;
    /// Represent the error as an [`io::Error`].
    fn into_io_error(self) -> io::Error;
}
    

[angusholder]

When you make the breaking change to the error enum, might be worth adding #[non_exhaustive] to it? That way you can add more error cases in future without it being another breaking change

[BenjaminRi]

I believe at least the following errors are necessary:

/// Error type for Zip
#[derive(Debug, Error)]
pub enum ZipError {
    /// An Error caused by I/O
    #[error(transparent)]
    Io(#[from] io::Error),

    /// This file is probably not a zip archive
    #[error("invalid Zip archive")]
    InvalidArchive(&'static str),

    /// This archive is not supported
    #[error("unsupported Zip archive")]
    UnsupportedArchive(&'static str),

    /// The requested file could not be found in the archive
    #[error("specified file not found in archive")]
    FileNotFound,

    /// No password was given but the data is encrypted
    #[error("missing password, file in archive is encrypted")]
    PasswordRequired,

    /// The given password is wrong
    #[error("invalid password for file in archive")]
    InvalidPassword,
}

In particular, we need to notify the user when he specifies no password at all, but the file is encrypted. We also need to notify him when he specifies the wrong password. These are two different cases. It doesn't really matter in what enum they are, but I believe that the API should be able to communicate these errors precisely.

In addition to that, I agree with @angusholder, we need to be careful with enum extensions. These are breaking changes for all our users downstream unless we specify the non_exhaustive clause.

[Plecra]

Absolutely agree on non_exhaustive. The variants for the user to match on will use it for the time being.

I'm sorry I haven't been clear on the password errors: there is no question in my mind that they should be simple to distinguish for the user. We will have individual errors to indicate whether a password is either missing or incorrect.

I am not going to use a thiserror-style enum, as they do not suit the extra metadata we need to describe zip errors, or the variety of ways zip archives can misbehave.

[Plecra]

I'll pop this alternative here too. It builds on io::Error, making the success path more straightforward.

pub struct InvalidArchive(());
pub struct Unsupported(());

pub struct InvalidPassword(());
pub struct PasswordRequired(());

impl TryFrom<io::Error> for InvalidArchive {}
impl TryFrom<io::Error> for Unsupported {}

impl From<InvalidPassword> for io::Error {}
impl From<PasswordRequired> for io::Error {}

impl error::Error for InvalidArchive {}
impl error::Error for Unsupported {}
impl error::Error for InvalidPassword {}
impl error::Error for MissingPassword {}

pub struct Archive<S>(());
pub struct File<S>(());
pub struct FileContents<S>(());
pub struct Reader<S>(());

impl<S: io::Read + io::Seek> Archive<S> {
    pub fn open(source: S) -> io::Result<Self>;
}
impl<S> File<S> {
    /// ```rust
    /// fn jar_metadata(archive: &mut Archive) -> io::Result<Metadata> {
    ///     let meta_file = archive.iter_mut().find(|file| file.name() == "_METADATA")
    ///        .ok_or_else(|| io::Error::new("metadata missing", io::ErrorKind::NotFound)?;
    ///     Metadata::parse(meta_file.reader()?.unencrypted()?)
    /// }
    /// ```
    pub fn reader(self) -> io::Result<FileContents<S>>;
}
impl FileContents<S> {
    pub fn unencrypted(self) -> Result<Reader<S>, PasswordRequired>;
    pub fn decrypt(self, password: &[u8]) -> Result<Reader<S>, InvalidPassword>;
}