Add new and severity based constructors to BevyError#23684
Add new and severity based constructors to BevyError#23684loreball wants to merge 7 commits intobevyengine:mainfrom
new and severity based constructors to BevyError#23684Conversation
4224b5a to
1edb7f9
Compare
| impl BevyError { | ||
| /// Creates a new [`BevyError`] with the given [severity](Severity). | ||
| pub fn new<E: Error + Sync + Send + 'static>(severity: Severity, error: E) -> Self { | ||
| Self::from(error).with_severity(severity) |
There was a problem hiding this comment.
Slightly simpler than my solution 🤔
|
|
||
| /// Creates a new [`BevyError`] from a printable error message. | ||
| /// | ||
| /// Prefer using other constructors if possible. |
There was a problem hiding this comment.
This guidance isn't great. When would you do this? Why should you prefer other constructors?
| Self::new(severity, error) | ||
| } | ||
|
|
||
| /// Checks if we're holding the internal error. |
| /// Prefer using other constructors if possible. | ||
| pub fn msg<M: Display + Debug + Send + Sync + 'static>(severity: Severity, message: M) -> Self { | ||
| // TODO: We've already crossed the simple to complex function threshold by implementing a whole new type. | ||
| // Consider downcasting to Box<dyn Error> to prevent double boxing. |
There was a problem hiding this comment.
I think we should do this :)
There was a problem hiding this comment.
I tried it and it doesn't work cleanly. The problem is that we want to downcast the type into itself but dyn Any only allows downcasting to references. The only two ways to do this are either box the value and downcast that or to check if the type is the same using Any::is and then transmute. The first option allocates; the second one uses unsafe code. This is supposed to be a simple optimization.
I've removed the comment for now. The eventual switch to using vtables should obviate the issue.
| /// A helper struct for constructing errors in [`BevyError::msg`]. | ||
| /// | ||
| /// We special case error downcasting to get to it's content when necessary. | ||
| /// Never expose to the user. |
|
|
||
| /// Creates a new [`BevyError`] from a string. | ||
| /// | ||
| /// Allows formatting the string like [`format!`](std::format!) and also take an optional severity argument. |
There was a problem hiding this comment.
This needs to be explicit that the severity argument comes first.
| } | ||
|
|
||
| #[test] | ||
| /// Testing the macros |
|
|
||
| /// Returns early with an error. | ||
| /// | ||
| /// Equivalent to <code>return Err([bevy_error!(\...)](bevy_error!))</code> |
There was a problem hiding this comment.
This needs to mention the default error severity here too.
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
I really like this, but we should incorporate this into the error handling release note and get it in for 0.19. |
bevy_error! and bail!` macros, along with other small helpers
bevy_error! and bail!` macros, along with other small helpers bevy_error! and bail! macros, along with other small helpers
|
|
||
| impl BevyError { | ||
| /// Creates a new [`BevyError`] with the given [severity](Severity). | ||
| pub fn new<E: Error + Sync + Send + 'static>(severity: Severity, error: E) -> Self { |
There was a problem hiding this comment.
You can use the same trick as the From impl to let this accept &'static str and String in addition to other errors:
| pub fn new<E: Error + Sync + Send + 'static>(severity: Severity, error: E) -> Self { | |
| pub fn new<E>(severity: Severity, error: E) -> Self | |
| where | |
| Box<dyn Error + Sync + Send>: From<E>, | |
| { |
That would also supersede from_boxed, since it would use the identity From impl (and not double-box!).
Accepting both &str and impl Error requires unstable language features because they could theoretically overlap, but the standard library cheats and uses them in the implementation of Error, which we can take advantage of using that bound.
There was a problem hiding this comment.
Yep, this is what I did in my impl :)
There was a problem hiding this comment.
Also makes from_boxed unnecessary since From is reflexive.
| /// Creates a new [`BevyError`] from a printable error message. | ||
| /// | ||
| /// Prefer using other constructors if possible. | ||
| pub fn msg<M: Display + Debug + Send + Sync + 'static>(severity: Severity, message: M) -> Self { |
There was a problem hiding this comment.
Is msg mainly intended to support the macro? format! returns a String, so if you restrict this to only take String then you can get rid of DisplayError and just use Self::from(error).
And if you generalize new, then you can get rid of this method entirely in favor of the macro calling new.
There was a problem hiding this comment.
At first it only supported strings for the macro. Then I looked at eyre and anyhow saw the method and stole it. I don't think we need it for now. I'll switch to using new.
5f7a242 to
4f79e79
Compare
| /// } | ||
| /// ``` | ||
| #[macro_export] | ||
| macro_rules! bevy_error { |
There was a problem hiding this comment.
IMO the core benefit over BevyError::new is the formatting. The docs should make that clearer.
| }; | ||
| // Doing the severity things means rustfmt can't format it but sadly it's the only way to make it work | ||
| (severity: $severity:expr, $($args:tt)+) => { | ||
| $crate::error::BevyError::msg($severity, std::format!($($args)*)) |
There was a problem hiding this comment.
| $crate::error::BevyError::msg($severity, std::format!($($args)*)) | |
| $crate::error::BevyError::msg($severity, ::alloc::format!($($args)*)) |
| (severity: $severity:expr) => { | ||
| compile_error!("missing error message") | ||
| }; | ||
| // Doing the severity things means rustfmt can't format it but sadly it's the only way to make it work |
There was a problem hiding this comment.
Since the format string is a literal and the severity isn't, we can check for that:
macro_rules! bevy_error {
($fmt:literal $(, $arg:tt)*) => {
$crate::error::BevyError::new(::alloc::format!($fmt $(, $arg)*), $crate::error::Severity::Panic)
};
($severity:expr, $($arg:tt)*) => {
$crate::error::BevyError::new(::alloc::format!($($arg)*), $severity)
};
($severity:expr) => {
compile_error!("missing error message")
};
}There was a problem hiding this comment.
Note that requiring the format string to be a literal means it can't be a variable etc.
There was a problem hiding this comment.
Note that requiring the format string to be a literal means it can't be a variable etc.
That's already the case with format!.
|
Maybe instead of a separate return bevy_error!(severity: Severity::Debug, "Not exactly one player: {:?}", error);
return BevyError::debug(format!("Not exactly one player: {:?}", error));This would be a bit shorter and work better with IDEs (suggestions on |
I think we should do this, regardless of our decision on the |
|
Worth noting that use miette::{miette, LabeledSpan, Severity};
let source = "(2 + 2".to_string();
let report = miette!(
// Those fields are optional
severity = Severity::Error,
code = "expected::rparen",
help = "always close your parens",
labels = vec![LabeledSpan::at_offset(6, "here")],
url = "https://example.com",
// Rest of the arguments are passed to `format!`
// to form diagnostic message
"expected closing ')'"
)
.with_source_code(source); |
f36730d to
c88869b
Compare
dsgallups
left a comment
There was a problem hiding this comment.
Really like the idea! I think that this macro is straightforward and approachable. I struggle to see how this could convolute things. Big fan.
| ($fmt:literal) => { | ||
| $crate::error::BevyError::new($crate::error::Severity::Panic, $fmt) | ||
| }; | ||
| ($fmt:literal, $($arg:tt)*) => { | ||
| $crate::error::BevyError::new($crate::error::Severity::Panic, ::alloc::format!($fmt, $($arg)*)) | ||
| }; |
There was a problem hiding this comment.
suggestion: panicking by default continues to be unexpected behavior. If the severity here directly leads to a panic, then I can see users getting upset. At least, I'd get upset.
Nonetheless, this appears to be the behavior now for bevy, so this suggestion might be OOS, and met in a different issue regarding default error handling.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Let's cut the macros from this PR to reduce controversy. The rest of this is really good, and I don't want to see it blocked.
Feel free to open a new PR for just the macros afterwards, and we can discuss the merits of them there.
Some more ways to construct a BevyError. Used for example in the upcoming macro
We set RUST_BACKTRACE in infiltered_backtrace_test to force backtrace printing. But the state of the variable is cached so if any previous test leads to the creation of a backtrace the variable gets cached as unset and backtraces get disabled. This breaks the test. The solution is to stop changing env variables in tests and instead to do it in the ci test harness. The test itself has been modified to check the env state and to fail if it's invalid
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
c88869b to
9ec8180
Compare
bevy_error! and bail! macros, along with other small helpers new and severity based constructors to BevyError
9ec8180 to
c923d8e
Compare
|
Well, it's done. Can't change the branch name without opening a new pr but everything else has been updated. I'll open a pr for the macros if/when this gets merged so I'm not constantly rebasing the pr on top of this one. |
benfrankel
left a comment
There was a problem hiding this comment.
Leaving some optional documentation nits, otherwise looks good to me.
| impl BevyError { | ||
| /// Constructs a new [`BevyError`] with the given [`Severity`]. | ||
| /// | ||
| /// The stored error will be stored as a `Box<dyn Error + Send + Sync>`. |
There was a problem hiding this comment.
| /// The stored error will be stored as a `Box<dyn Error + Send + Sync>`. | |
| /// The error will be stored as a `Box<dyn Error + Send + Sync>`. |
Nit
| /// | ||
| /// The stored error will be stored as a `Box<dyn Error + Send + Sync>`. | ||
| /// | ||
| /// The easiest way to use this is to simply pass in a quoted bit of text. |
There was a problem hiding this comment.
| /// The easiest way to use this is to simply pass in a quoted bit of text. | |
| /// The easiest way to use this is to pass in a string. |
Nit
| /// Create a new [`BevyError`] with the [`Severity::Ignore`] severity | ||
| /// | ||
| /// This is a shorthand for <code>[BevyError::new(Severity::Ignore, error)](BevyError::new)</code> |
There was a problem hiding this comment.
| /// Create a new [`BevyError`] with the [`Severity::Ignore`] severity | |
| /// | |
| /// This is a shorthand for <code>[BevyError::new(Severity::Ignore, error)](BevyError::new)</code> | |
| /// Creates a new [`BevyError`] with the [`Severity::Ignore`] severity. | |
| /// | |
| /// This is a shorthand for <code>[BevyError::new(Severity::Ignore, error)](BevyError::new)</code>. |
Nit
Same comment for the other shorthand constructors.
| Self::new(Severity::Panic, error) | ||
| } | ||
|
|
||
| /// Checks if we're holding the internal error. |
There was a problem hiding this comment.
| /// Checks if we're holding the internal error. | |
| /// Checks if the internal error is of type `E`. |
Nit
Alternatively, "Checks if the internal error is of the given type."
Objective
Add constructors for making
BevyErrors with a specific severity. Closes #23676.Solution
Add a
newconstructor plus 1 constructor for every possibleSeverity.Testing
My eyes and a simple test case that constructs an error and tests if the downcasting works.
Showcase
A
BevyErrornow has multiple constructor to create one with an expected severity