`ensure!` macro implementation details
Closed this issue · 8 comments
The ensure! macro has a completely different definition in the documentation, and what is actually used in the library.
https://docs.rs/anyhow/latest/src/anyhow/macros.rs.html#120-143
The documentation macro definition is
macro_rules! [ensure](https://docs.rs/anyhow/latest/anyhow/macro.ensure.html) {
($cond:expr $(,)?) => {
if !$cond {
return $crate::__private::Err($crate::Error::msg(
$crate::__private::concat!("Condition failed: `", $crate::__private::stringify!($cond), "`")
));
}
};
($cond:expr, $msg:literal $(,)?) => {
if !$cond {
return $crate::__private::Err($crate::__anyhow!($msg));
}
};
($cond:expr, $err:expr $(,)?) => {
if !$cond {
return $crate::__private::Err($crate::__anyhow!($err));
}
};
($cond:expr, $fmt:expr, $($arg:tt)*) => {
if !$cond {
return $crate::__private::Err($crate::__anyhow!($fmt, $($arg)*));
}
};
}(A pretty good implementation, generally what a rustacean would expect!)
But the macro which is actually used while compiling code is completely different
macro_rules! ensure {
($($tt:tt)*) => {
$crate::__parse_ensure!(
/* state */ 0
/* stack */ ()
/* bail */ ($($tt)*)
/* fuel */ (~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~ ~~~~~~~~~~)
/* parse */ {()}
/* dup */ ($($tt)*)
/* rest */ $($tt)*
)
};
}The problems with this
- The docs are completely misleading on what the macro actually is
The macro shown in the documentation is not what the macro used in code is - The
__parse_ensure!macro is nearly 800 lines of code
Was the parse_ensure macro necessary here? Rather, is it necessary at all? It is completely undocumented as far as I can find, and it looks far more than what is required. - Were the compile time influence not considered during this at all?
Evaluating that macro would take a lot of time. It's massive
Why even have different implementations in the docs and in the implementation? The documentation implementation doesn't even look wrong, and should do the same thing (while being a much simpler, faster to compile macro).
Other crates such as eyre also have a similar, simpler implementation of the ensure macro https://docs.rs/eyre/latest/src/eyre/macros.rs.html#109-125
The __parse_ensure macro is used to allow the following error messages:
let (x, y) = (5, 6);
anyhow::ensure!(x == y);into
Error: Condition failed: `x == y` (5 vs 6)
An example output of the macro
match (&x, &y) {
(lhs, rhs) => {
if !(lhs == rhs) {
#[allow(unused_imports)]
use ::anyhow::__private::{BothDebug, NotBothDebug};
return Err((lhs, rhs).__dispatch_ensure("Condition failed: `x == y`"));
}
}
}It is complicated but necessary for these nicer error messages.
Sure, this is valid. But the documentation is still misleading. (Especially since clicking source leads to the wrong definition) and could the implementation of this not have been simpler?
It's not really misleading, it's documentation on how to use the macro. The actual implementation would generate very useless documentation that would not serve the job of documentation - describing how the macro can be used.
Sure, that can be in the docs itself though, rather than pointing to a completely different macro entirely.
The compile time influence of this is still unconsidered. See: std having different assert macros for special-cased error messages, rather than overloading a single macro.
The current solution is not very accurate either, look at the expansion of this:
use anyhow::ensure;
ensure!(true == true || false == true);use anyhow::ensure;
if !(true == true || false == true) {
return ::anyhow::__private::Err(::anyhow::Error::msg(
"Condition failed: `true == true || false == true`",
));
};It doesn't detect the == operation occurring and add the specialized diagnostic.
The condition is parsed by syn as true == (true || (false == true))
A proc macro for this should be much better, imo. It would both more accurately perform this and should hopefully be faster than the decl macro implementation right now. 🤷🏽♀️
#325 (comment) is accurate about why it's set up this way. The input patterns shown are indeed the 4 patterns that the macro accepts. The real macro implementation just parses the same things token-by-token instead of through macro_rules's built-in $:expr parser.
Sure. But the "source" of the macro according to the docs is still completely misleading.
When I click "source" on the docs, there's no comment or no indication at all (neither in the docs themselves nor in any comment in the source code) to the actual implementation of the macro. It just points to a pseudo-macro.
If I hadn't noticed the tiny cfgs, I would've been completely oblivious to this too.
This is still ignoring
- The compile time affect of this
- The accuracy of this (#325 (comment))
Hacking together a semi-parser of Rust code in a declarative macro seems unnecessary and a misuse of them. A proc-macro seems much better suited here?
If the point of the cfg(docs) macro is to simply be a place holder for the docs, why implement at all?
Especially since the implementation of the only-docs macro is not the expansion of the actual macro