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

Crate isn't really no-std compatible #47

Closed
tomaka opened this issue Sep 12, 2023 · 8 comments · Fixed by #50
Closed

Crate isn't really no-std compatible #47

tomaka opened this issue Sep 12, 2023 · 8 comments · Fixed by #50

Comments

@tomaka
Copy link

tomaka commented Sep 12, 2023

#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.

@KillingSpark
Copy link
Owner

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
Copy link
Author

tomaka commented Sep 12, 2023

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.

@KillingSpark
Copy link
Owner

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?

@KillingSpark KillingSpark mentioned this issue Mar 31, 2023
2 tasks
@tamird
Copy link
Contributor

tamird commented Oct 9, 2023

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

@KillingSpark
Copy link
Owner

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

@tamird
Copy link
Contributor

tamird commented Oct 13, 2023

Nice. Sent #49 to address existing lint errors.

@tomaka
Copy link
Author

tomaka commented Oct 14, 2023

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
Copy link
Author

tomaka commented Oct 23, 2023

Thank you 🙏

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 a pull request may close this issue.

3 participants