-
-
Notifications
You must be signed in to change notification settings - Fork 135
feat: no-std support #455
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
base: main
Are you sure you want to change the base?
feat: no-std support #455
Conversation
This commit finishes full no_std support to miette.
…implementation - Add dedicated no-std CI job that builds for wasm32-unknown-unknown target - Fix Infallible StdError implementation for no-std environments The CI job validates: 1. Core no-std functionality (no default features) 2. fancy-no-syscall feature set compatible with no-std environments
| } | ||
| } | ||
|
|
||
| #[cfg(not(feature = "std"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curiosity: why not a different struct, instead of using one in two configs.
If the answer is "won't compile", then this is infectious / not additive, no?
src/eyreish/context.rs
Outdated
| } | ||
|
|
||
| impl Diag for Report { | ||
| #[cfg_attr(track_caller, track_caller)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is track_caller needed for the no_std implementation, or is that a separate change?
| use std::{error::Error, fmt::Display}; | ||
| extern crate alloc; | ||
|
|
||
| use crate::StdError as Error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why re-name StdError to Error here, but not elsewehre?
| // let error = $msg; | ||
| // (&error).miette_kind().new(error) | ||
|
|
||
| #[cfg(not(feature = "std"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can all these repeated declarations live in a single imported prelude instead, or nah?
| let hook = HOOK.get_or_init(|| Box::new(get_default_printer)).as_ref(); | ||
| pub(crate) fn capture_handler_with_location( | ||
| error: &(dyn Diagnostic + 'static), | ||
| ) -> Box<dyn ReportHandler> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the Once? can't it just be OnceLock?
I'm having a hard time following what this change is achieving, which means more comments might be needed here. I don't see why two different once's are needed.
| fn source(&self) -> Option<&(dyn StdError + 'static)> { | ||
| self.0.source() | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change, no?
| boxed::Box, | ||
| string::{String, ToString}, | ||
| }; | ||
| #[cfg(feature = "fancy")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't many of these still be alloc:: qualified types instead, in case someone wants to make fancy no-std too in future?
src/handler.rs
Outdated
| if #[cfg(feature = "fancy-no-backtrace")] { | ||
| supports_color::on(supports_color::Stream::Stderr).is_some() | ||
| } else if #[cfg(feature = "fancy-no-syscall")] { | ||
| // In no-std environment without color support, default to no color support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment not quite true. fancy-no-syscall can be enabled w/o no-std, right?
src/handler.rs
Outdated
| pub(super) fn supports_color() -> bool { | ||
| cfg_if! { | ||
| if #[cfg(feature = "fancy-no-syscall")] { | ||
| if #[cfg(feature = "fancy-no-backtrace")] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this reordering seems like it might be a behavior change.
Shouldn't it be something like:
if fancy-no-syscall, false
maybe else if no-std, false
else if fancy-no-backtrace, check
maybe else, false?
These negated features are so hard to think through. Feels like some of them should be flipped next time there's a major version?
| if #[cfg(feature = "fancy-no-syscall")] { | ||
| None | ||
| } else { | ||
| if #[cfg(feature = "fancy-no-backtrace")] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, seems like this may change behavior in with std scenarios?
| match std::env::var("NO_COLOR") { | ||
| _ if !std::io::stdout().is_terminal() || !std::io::stderr().is_terminal() => { | ||
| Self::none() | ||
| #[cfg(feature = "fancy-no-syscall")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems ok, though again, the negated features are really tricky...
| Self(Arc::new(SyntectHighlighter::default())) | ||
| #[cfg(all(feature = "syntect-highlighter", not(feature = "fancy-no-syscall")))] | ||
| { | ||
| use std::io::IsTerminal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's some deduplication that should be done vs src/handlers/theme.rs
It's already pretty complicated and presumably the two should be kept in sync.
Maybe a helper function? Ideally that memo-izes it to avoid many redundant syscalls if necessary
Note: the double or triple negation in the old version was making my head hurt, so I made it exhaustive instead
enum StyleDetection
{
NoColor,
Color
}
fn detect_color_support() ->
{
#[cfg(any(feature = "fancy-no-syscall", not(feature = "std")))]
{
StyleDetection::NoColor
}
#[cfg(all(not(feature = "fancy-no-syscall"), feature = "std"))]
{
match std::env::var("NO_COLOR") {
if !std::io::stdout().is_terminal() || !std::io::stderr().is_terminal() => {
// TODO: address todo by making this StyleDetection::NoColor?
//TODO: should use ANSI styling instead of 24-bit truecolor here
StyleDetection::Color
}
Ok(string) =>
{
if string == "0"
{
// NO_COLOR = "0" means enable color
StyleDetection::Color
}
else
{
// any other value means disable color.
StyleDetection::NoColor
}
}
// This could be NotPresent (color since disablement env var not set) or NotUnicode (hopefully never happens, but we'llt reat it the same)
Err(_) => StyleDetection::Color
}
}
}| @@ -1,6 +1,8 @@ | |||
| #![no_std] | |||
| #![deny(missing_docs, missing_debug_implementations, nonstandard_style)] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to be merged, I think at least some of these should probably be enabled?
![deny(clippy::alloc_instead_of_core,clippy::std_instead_of_core,clippy::std_instead_of_alloc)]| #[cfg(feature = "std")] | ||
| pub use std::error::Error as StdError; | ||
|
|
||
| #[cfg(not(feature = "std"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written, this isn't additive. Can't there be an enum MietteErr with two enum variants, one of which is only enabled when std is? or would that be breaking? not saying it should change, just seeking to understand
| /// as a [`SourceOffset`]. | ||
| /// | ||
| /// In no_std environments, this is not supported and will return an error. | ||
| #[cfg(not(feature = "std"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me, why not just compile it out entirely? why add a useless implementation?
|
|
||
| let error = MyError { | ||
| source: io::Error::new(io::ErrorKind::Other, "oh no!!!!"), | ||
| source: io::Error::other("oh no!!!!"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth pulling all the io::Error changes out into another PR to keep the diff down, but idk.
| exclude = ["images/", "tests/", "miette-derive/"] | ||
|
|
||
| [dependencies] | ||
| thiserror = "2.0.11" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
| unicode-width = "0.2.0" | ||
| unicode-width = { version = "0.2.0", default-features = false } | ||
| cfg-if = "1.0.0" | ||
| spin = { version = "0.9", default-features = false, features = ["mutex", "spin_mutex", "lazy"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem like it should be a dependency for std environments. I see the simplicity benefit, but adding a dependency for consumers for whom OnceLock is an option, doesn't seem like it makes sense.
Also, do you need all those features?
| thiserror = "2.0.11" | ||
| miette-derive = { path = "miette-derive", version = "=7.6.0", optional = true } | ||
| unicode-width = "0.2.0" | ||
| unicode-width = { version = "0.2.0", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a size optimization? If so, maybe default features should have an option that turns this back on? As written any consumers with cjk character requirements will be broken unless they otherwise include unicode-width with the relevant featur eon
|
To be clear, my comments are just my opinion - I'm neither the author nor am I a maintainer of this library, I'm just a person who has submitted a few contributions previously and was curious. Hope the feedback is helpful. |
- Fixed compilation errors in no-std mode (TestError Debug derive, ToString imports) - Addressed consistency issues: Option qualification, StdError naming, Borrow impl - Fixed feature flag logic for supports_color function with proper priority ordering - Added conditional compilation for std-dependent features in panic.rs, handler.rs, theme.rs - Made syntect-highlighter feature explicitly require std - Consolidated duplicate Borrow implementations using core::borrow::Borrow
- Add #[allow(unused_assignments)] to test structs/enums with unused fields - Fix Miri test failures caused by derive macro feature interactions - Miri tests now pass: 52 passed, 0 failed, 6 ignored - Issue occurs when fancy feature changes how derive macros process fields - Suppression is appropriate as fields are only unused under specific feature combinations
| true | ||
| } | ||
| } | ||
| #[cfg(all(not(feature = "fancy-no-syscall"), not(feature = "std")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are not opposites. Above is !A && B. this is !A && !B
This should probably be A || !B?
| #[cfg(feature = "std")] | ||
| use std::boxed::Box; | ||
| #[cfg(feature = "std")] | ||
| use std::{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe make the whole module conditional? I don't think a fake no-op set_panic_hook should be provided when std is not set, it's correct to not have it there and will not break any program that depends on miette with std support feature
- Remove rustc_version build dependency that was no longer needed - Remove nightly detection since nightly cfg flag wasn't used anywhere - Simplify build.rs to only set track_caller cfg flag
Adds support for no-std.
This is a slight modernization of the miette fork by @bitwalker mentioned in #443, found at bitwalker/miette@main...no-std
All the credit for the contribution goes to @bitwalker, I'm just doing janitorial work around the edges.