KillingSpark/zstd-rs

Crate isn't really no-std compatible

tomaka opened this issue ยท 8 comments

tomaka commented

#39 supposedly added support for no-std.

However I strongly disagree with this point:

Uses of std::error::Error are conditionally replaced with core::error::Error. This is currently unstable, so nightly is required for no_std usage for the time being. I believe it is still not possible to build a no_std binary without nightly, so this should be an acceptable requirement.

You can't really consider a crate to be "no-std compatible" if it requires nightly.
And let's face it: at the rhythm where it's going (the first attempts at moving this trait were in 2021), it's going to take a very long time for core::error::Error to be stabilized.

My suggestion would be to not derive the Error trait at all, but only Debug and Display and possible Clone. This can be done by ditching the thiserror crate and using a different one.
After all, the Error trait isn't very useful. All it does is potentially provide a stack trace. It's not because it's named "error" that types that represent errors always have to derive it.

I am by no means an expert in the no-std field and the PR seemed to satisfy the person that wanted to use the crate in a no_std environment

I believe it is still not possible to build a no_std binary without nightly, so this should be an acceptable requirement.

Is this no longer true? If it isn't it changes the trade-offs. If it is still true I don't see the point you are trying to make here: You can't really consider a crate to be "no-std compatible" if it requires nightly.

My suggestion would be to not derive the Error trait at all, but only Debug and Display and possible Clone. This can be done by ditching the thiserror crate and using a different one.

thiserror is not only useful for deriving the error crate but it makes the display messages really comfortable to format within the #derive tags.

After all, the Error trait isn't very useful. All it does is potentially provide a stack trace.

The stacktrace is a very valuable source of information if anything goes wrong and I have to debug it. I guess not having a stacktrace in no_std is fine, but completly ditching it seems weird to me, but it is maybe be worth it?

Would you have a suggestion for a better crate to make display messages as easy to create as with thiserror that is no_std compatible?

tomaka commented

Is this no longer true?

I think that since rust-lang/rust#66741 it is possible to create no-std binaries. This issue was the last blocker if I'm not mistaken.
Regardless, there are many use cases that do not involve compiling a binary, for example embedding a Rust library within another program.

thiserror is not only useful for deriving the error crate but it makes the display messages really comfortable to format within the #derive tags.

I don't care so much about which error crate is used, but I'm personally using derive_more and it provides the same ease of formatting except that it derives Display rather than Error.

The stacktrace is a very valuable source of information if anything goes wrong and I have to debug it.

Is it actually? If you have several different places return the same error enum variant then a stack trace can help you know which of these places has actually triggered it. However, while I don't know the source code of zstd-rs, I feel like this is rarely the case. Most often than not each enum variant can only get generated from a single line of code in a single file, which itself only gets called from a single line of code in a single file, etc. making the stack trace not useful at all.

Providing some context to the error (e.g. the values of the inputs) is generally very valuable, but that's not something that the Error trait does.

Regardless, there are many use cases that do not involve compiling a binary, for example embedding a Rust library within another program.

Does that not involve building a binary just in the static_lib/dylib format? Again I am not very familiar with the whole no_std environment. I'd imagine, that this involves the same restrictions as "normal" no_std compilation, right?

tamird commented

I think #48 resolves this by lifting the nightly requirement.

Started work on this branch. Looks promising. https://github.com/KillingSpark/zstd-rs/tree/remove_thiserror

tamird commented

Nice. Sent #49 to address existing lint errors.

tomaka commented

You might also be able to do something like #[cfg_attr(std, derive(derive_more::Error))], so that the Error implementations remain if the std feature is enabled.

tomaka commented

Thank you ๐Ÿ™