-
Notifications
You must be signed in to change notification settings - Fork 174
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
Should we have a fallible Writeable? #4741
Comments
This will be a pain for components like list formatter that take Writeables as inputs. I like the current model because you can pass around Writeables and don't have to deal with errors from other domains, figure out what a good replacement value would be, etc. Once you create the Writeable it will work. |
Letting clients choose how to handle their errors seems beneficial. The
|
I guess we could also do this only in the datetime crate without making a trait, and we can make it into a trait later. |
I don't think we should ever panic implicitly through |
This comes up again in the named placeholder pattern. The map of placeholder keys to values may or may not have all of the required placeholders. Checking it in the constructor takes time and we still need unwraps or GIGO in the interpolate function. |
Writing down some more thoughts:
|
See #4772 for an implementation of this. |
I'd like to reconsider this statement:
We could very well do this the other way around. It might even be more natural, as once constructed a |
Current design: pub trait FallibleWriteable {
type Error;
fn fallible_write_to<
W: fmt::Write + ?Sized,
L: Writeable,
E,
F: FnMut(Self::Error) -> Result<L, E>,
>(
&self,
sink: &mut W,
handler: F,
) -> Result<Result<(), E>, fmt::Error>;
}
pub trait TryWriteable: FallibleWriteable {
#[inline]
fn checked(&self) -> CheckedWriteable<&Self> {
CheckedWriteable(self)
}
#[inline]
fn lossy(&self) -> LossyWriteable<&Self> {
LossyWriteable(self)
}
}
impl<T> TryWriteable for T where T: FallibleWriteable {} Simpler design: pub trait TryWriteable {
type Error;
/// Implementers MUST finish writing the writeable in the error case
/// with appropriate replacements, and then return the error. The
/// error can be dropped by the caller in lossy mode.
fn try_write_to<
W: fmt::Write + ?Sized,
>(
&self,
sink: &mut W,
) -> Result<Result<(), Self::Error>, fmt::Error>;
// Other functions:
fn try_write_to_parts(...) -> Result<Result<(), Self::Error>, fmt::Error>
fn try_write_to_string(...) -> Result<String, Self::Error>;
/// Invariant: returns length of lossy string
fn try_writeable_length(...) -> LengthHint;
/// Invariant: compares to lossy string
fn try_write_cmp_bytes(...) -> cmp::Ordering;
}
// No need for `.lossy()` etc functions. At call sites:
let _ = dtf.format(...).try_write_to(...)?; // lossy
dtf.format(...).try_write_to(...)?.unwrap(); // panicky
let _ = dtf.format(...).try_write_to(...)?.map_err(|e| {debug_assert!(false, "{e:?}"); e}); // gigo
Discussion:
Approved above design. LGTM: @robertbastian @Manishearth @sffc |
The GIGO case can be written as follows since Rust 1.76 (a few characters shorter): let _ = dtf.format(...).try_write_to(...)?
.inspect_err(|e| debug_assert!(false, "{e:?}")); https://doc.rust-lang.org/std/result/enum.Result.html#method.inspect_err |
An issue I encountered while implementing this regarding
Note on point 4: the default impl of Another note: it might be useful to detect early when a Writeable is in a failure state. It seems consistent with the design of Writeable for us to bail early if EDIT: Detecting the error case might be expensive; we don't implement LengthHint in FormattedDateTime in part because of this. So I think we still want a way to make the function return an undefined hint without regard to the error state. |
In favour of 2. I think it's an advantage that they would be the same names as on |
Wait, why does |
Because it's what I proposed in #4402 and it didn't get any pushback. I think it's more a bug that |
We haven't released |
Why would it be |
Because all the other functions are called |
We have voted on this: https://docs.google.com/document/d/1zm_pQsrO7kTBjHbtmoQiriFNedUyd1_AjJ2Um3u_kD4/edit?pli=1 |
aha, we did already vote... ok |
Fixes #4741 --------- Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
Our
.format
functions are generally low-cost, pushing all business logic to the stage when we have asink
available. Unfortunately, this means that these code paths cannot have any errors. Currently we convert theDateTimeError
to acore::fmt::Error
, but this is wrong because it makesToString
panic.We could implement a trait such as
(or an enum between core::fmt::Error and Self::Error)
The text was updated successfully, but these errors were encountered: