Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add impl<E: std::error::Error> From<E> for savvy::Error #324

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

yutannihilation
Copy link
Owner

Third attempt. Close #305

This pull request makes conversion from other error to savvy::Error easily, just like anyhow. This is a breaking change, so this bumps the version.

Good things

To put simply, such .map_err() will no longer be needed.

Before

fn foo() -> savvy::Result<()> {
    let value = mutex
        .lock()
        .map_err(|e| crate::Error::new(&e.to_string()))?;
    ...
}

After

fn foo() -> savvy::Result<()> {
    let value = mutex
        .lock()?;
    ...
}

Breaking changes

No From<&str> conversion

On the other hand, savvy::Error loses From<String> and From<&str>. If savvy has these impl, it conflicts with the error conversion. I for got what this error means, but, considering the fact that anyhow::Error doesn't have From<String> as well, this is probably unavoidable.

error[E0119]: conflicting implementations of trait `std::convert::From<std::string::String>` for type `error::Error`
  --> src\error.rs:75:1
   |
55 |   impl From<String> for Error {
   |   --------------------------- first implementation here
...
75 | / impl<E> From<E> for Error
76 | | where
77 | |     E: std::error::Error + 'static,
   | |___________________________________^ conflicting implementation for `error::Error`
   |
   = note: upstream crates may add a new impl of trait `std::error::Error` for type `std::string::String` in future versions

As a workaround, savvy provides savvy_err!() macro, which is a shorthand of savvy::Error::new(format!(...)). This follows the idea of anyhow!() macro.

So, if the current implementation uses .into() conversion from a string, it needs to replaced with savvy_err!().

Before

    Err("Not a known ALTREP".into())
}

After

    Err(savvy_err!("Not a known ALTREP"))
}

Conflicts with custom error conversion

impl<E: std::error::Error> From<E> for savvy::Error will conflict if the R package uses its own error and the conversion to savvy::Error. In order to avoid this, this pull request adds use-custom-error feature.

   error[E0119]: conflicting implementations of trait `From<RPolarsErr>` for type `savvy::Error`
     --> src\error.rs:27:1
      |
   27 | impl From<RPolarsErr> for savvy::Error {
      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
      = note: conflicting implementation in crate `savvy`:
              - impl<E> From<E> for savvy::Error
                where E: StdError, E: 'static;

Minor technical note

anyhow::Error requires Send and Sync (so, the above code of Mutex doesn't work for anyhow::Error). If I understand correctly, this is because

On the other hand, savvy creates a string immediately on conversion. Because only a string can be propagated to R session anyway. A string is Send and Sync no matter whether the original error is Send and/or Sync or not. So, savvy's conversion doesn't require Send and Sync.

@yutannihilation
Copy link
Owner Author

I don't see any problems. Let's merge and go ahead.

@yutannihilation yutannihilation merged commit 6ccd05e into main Oct 31, 2024
15 checks passed
@yutannihilation yutannihilation deleted the feat/error-conversion2 branch October 31, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

anyhow
1 participant