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.
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?
| } | ||
|
|
||
| impl Diag for Report { | ||
| #[cfg_attr(track_caller, track_caller)] |
There was a problem hiding this comment.
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.
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.
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.
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.
There was a problem hiding this comment.
OnceLock isn't available in no_std, hence spin::Once
| fn source(&self) -> Option<&(dyn StdError + 'static)> { | ||
| self.0.source() | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a breaking change, no?
There was a problem hiding this comment.
| boxed::Box, | ||
| string::{String, ToString}, | ||
| }; | ||
| #[cfg(feature = "fancy")] |
There was a problem hiding this comment.
shouldn't many of these still be alloc:: qualified types instead, in case someone wants to make fancy no-std too in future?
There was a problem hiding this comment.
Fair point for future-proofing. Right now the fancy features all require std (terminal detection, env vars, etc.) so these std imports are fine. If someone wanted no_std_fancy later they'd need to rework the syscall detection anyway. Left it as-is to keep the diff smaller but happy to change if you feel strongly.
| 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.
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
}
}
}There was a problem hiding this comment.
Thought about this. The two defaults have different return types (GraphicalTheme vs MietteHighlighter) so a shared helper would need callbacks or an enum. The duplicated logic is small (just the NO_COLOR check) and both places already have cfg guards. A helper could work but might add more complexity than it saves. Open to ideas if you see a clean way to do it.
| @@ -1,6 +1,8 @@ | |||
| #![no_std] | |||
| #![deny(missing_docs, missing_debug_implementations, nonstandard_style)] | |||
There was a problem hiding this comment.
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)]|
|
||
| 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.
Might be worth pulling all the io::Error changes out into another PR to keep the diff down, but idk.
| 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.
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?
There was a problem hiding this comment.
spin is optional = true and only activated when not(feature = "std") (see the cfg guards in src/eyreish/mod.rs). Std users don't pull in spin at all. The minimal features (once only) are intentional to keep the dependency small for no_std users who need it.
|
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
| #[cfg(feature = "std")] | ||
| use std::boxed::Box; | ||
| #[cfg(feature = "std")] | ||
| use std::{ |
There was a problem hiding this comment.
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
- Use core::error::Error directly (MSRV 1.82 supports it) - Remove build.rs (track_caller is always available with MSRV 1.82) - Simplify feature detection in handler.rs syscall module - Add clippy lints to catch std/core/alloc misuse - Remove custom StdError trait, re-export core::error::Error - Add __alloc module for derive macro compatibility - Clean up verbose comments - Remove extern crate alloc from tests (not needed with std) - Fix derive macros to use miette::__alloc paths
- spin is now optional, only needed for no_std (provides Once) - std builds use OnceLock, avoiding unnecessary dependency - Restore unicode-width default features (keeps CJK support)
| 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.
Thought about this. The two defaults have different return types (GraphicalTheme vs MietteHighlighter) so a shared helper would need callbacks or an enum. The duplicated logic is small (just the NO_COLOR check) and both places already have cfg guards. A helper could work but might add more complexity than it saves. Open to ideas if you see a clean way to do it.
| boxed::Box, | ||
| string::{String, ToString}, | ||
| }; | ||
| #[cfg(feature = "fancy")] |
There was a problem hiding this comment.
Fair point for future-proofing. Right now the fancy features all require std (terminal detection, env vars, etc.) so these std imports are fine. If someone wanted no_std_fancy later they'd need to rework the syscall detection anyway. Left it as-is to keep the diff smaller but happy to change if you feel strongly.
| 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.
OnceLock isn't available in no_std, hence spin::Once
| #[cfg(feature = "syntect-highlighter")] | ||
| { | ||
| use std::io::IsTerminal; | ||
| match std::env::var("NO_COLOR") { |
There was a problem hiding this comment.
The two defaults have different return types (GraphicalTheme vs MietteHighlighter) so a shared helper would need callbacks or an enum. The duplicated logic is small (just the NO_COLOR check) and both places already have cfg guards. A helper could work but might add more complexity than it saves. Open to ideas if you see a clean way to do it.
| 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.
spin is optional = true and only activated when not(feature = "std") (see the cfg guards in src/eyreish/mod.rs). Std users don't pull in spin at all. The minimal features (once only) are intentional to keep the dependency small for no_std users who need it.
- Make spin a required dependency for no_std hook support (OnceLock unavailable) - Simplify cfg guards: not(std) instead of not(std) && spin - Change clippy alloc/std lints from warn to deny - Remove no_std stub for from_current_location (compile-time vs runtime error) - Fix is_graphical cfg logic for fancy-base without fancy-no-backtrace - Add cargo-hack feature powerset CI job to verify all 56 combinations
3 participants
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.