Add Send + Sync to common traits

[mkatychev]

Various error libraries have datastructures such as anyhow::Error that required the errors they hold to be Send + Sync in addition to being std::error::Error.

The common traits below have had their ::Error associated type further constraints to include Send + Sync:

• sophia_api::dataset::Dataset
• sophia_api::graph::Graph
• sophia_api::source::Source
• sophia_api::term::TryFromTerm
• sophia_inmem::index::TermIndex

[mkatychev]

Knowing that this is a breaking change, I held off on explicitly feature flagging unless explicitly requested. This can easily be modified:

Suggested change

	type Error: Error + 'static + Send + Sync;
	#[cfg(not(feature = "threadsafe_errs"))]
	type Error: Error + 'static;
	#[cfg(feature = "threadsafe_errs")]
	type Error: Error + 'static + Send + Sync;

[pchampin]

Thanks for this suggestion, it makes sense. However, I would like to check first whether it breaks badly some known implementations. I will check with mine. @KonradHoeffner (for hdt), @damooo (for manas) and @vemonet (for nanopub), could you please check whether this change would be a problem for you?

[KonradHoeffner]

@pchampin: Thanks for the heads-up! In HDT I have type Error = Infallible, which implements Send and Sync so there wouldn't be a problem.

[damooo]

Works for manas. For graphs and datasets, I too use Error to be Infallible, as there will be no external io. In remaining cases I explicitly mentioned Send + Sync + 'static bounds in where clauses.

[vemonet]

Thanks for letting us know!

I have implemented my errors following the official rust by examples docs given at https://doc.rust-lang.org/rust-by-example/error/multiple_error_types/define_error_type.html

It seems to be the recommended way to implement custom errors cleanly, and I am not a big fan of adding a whole dependency just for something as basic as handling errors (especially that the standard way to handle errors is quite nice and makes it clear which errors might be raised by the lib)

I have tried to add impl Send for TermError {} but it makes the compiler unhappy, it want me to make it unsafe impl Send for TermError {}

the trait std::marker::Send enforces invariants that the compiler can't check. Review the trait documentation and make sure this implementation upholds those invariants before adding the unsafe keyword

Do you have an idea of how we could add Send + Sync to the recommended way to implement errors in rust without introducing unsafe code?

[mkatychev]

@vemonet I was thinking of making this a feature flag in sophia_iri(the "root" crate):

#[cfg(not(feature = "threadsafe_err"))]
pub use std::error::Error;
#[cfg(feature = "threadsafe_err")]
pub use ThreadSafeError as Error;

pub trait ThreadSafeError: std::error::Error + Send + Sync + 'static {}
impl<E> ThreadSafeError for E where E: std::error::Error + Send + Sync + 'static {}

then replace any std::error::Error instances with the sophia_iri::Error (wherever it's actually reexported in) above, this should make the change backwards compatible by toggling the threadsafe_err feature flag.

[mkatychev]

I'm moving this PR to draft to implement the threadsafe_err feature flag discussed above

[mkatychev]

@pchampin I've implemented the threadsafe_err flag and fixed some with the new 1.80.0 stable that triggered some clippy errors. This PR is ready for review again

[pchampin]

@vemonet

Do you have an idea of how we could add Send + Sync to the recommended way to implement errors in rust without introducing unsafe code?

Send and Sync are special traits, which you don't need to implement explicitly. In fact, your TermError and NpError implement them, because they do not contain any field that is !Send or !Sync (e.g. Rc<T>). So my take is that this change would not break your code either.

@mkatychev
thanks a lot for the effort you put the feature flag, but in fact, I think I would rather include the change directly, without the feature flag... I'm sorry you wasted your time 😅. Although it is technically a breaking change, it will probably break no-one's code in practice, and the feature flag would induce a risk of splitting the ecosystem. Furthermore, we are still in 0.x versions, so breaking changes are acceptable.

Without the feature flag, I don't think that the alias ThreadSafeError is needed. I'd rather keep things explicit, using standard traits.

[mkatychev]

@pchampin I've reverted the flag changes, should be good to go