Skip to content
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

Add TryWriteable for writeables that can fail #4772

Closed
wants to merge 14 commits into from

Conversation

sffc
Copy link
Member

@sffc sffc commented Apr 3, 2024

Fixes #4741

We need something of this shape for at least two things I'm currently collaborating on: datetime formatting and freeform multi-placeholder patterns.

In this PR, I add a new trait TryWriteable, which is implemented on Option<T>, with the following behavior:

let mut sink = String::new();

Some("abcdefg")
    .checked()
    .try_write_to(&mut sink)
    .unwrap()
    .unwrap();
assert_eq!(sink, "abcdefg");
assert!(matches!(
    None::<&str>.checked().try_write_to(&mut sink),
    Ok(Err(MissingWriteableError))
));

assert_writeable_eq!(Some("abcdefg").lossy(), "abcdefg");
assert_eq!(
    Some("abcdefg").lossy().writeable_length_hint(),
    LengthHint::exact(7)
);

assert_writeable_eq!(None::<&str>.lossy(), "�");
assert_eq!(
    None::<&str>.lossy().writeable_length_hint(),
    LengthHint::exact(3)
);

assert_writeable_eq!(Some("abcdefg").panicky(), "abcdefg");
assert_eq!(
    Some("abcdefg").panicky().writeable_length_hint(),
    LengthHint::exact(7)
);

assert_writeable_eq!(Some("abcdefg").gigo(), "abcdefg");
assert_eq!(
    Some("abcdefg").gigo().writeable_length_hint(),
    LengthHint::exact(7)
);

fn unwrap_infallible(self) -> Self::T;
}

impl<T> UnwrapInfallible for Result<T, Infallible> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's just use a free function for this

Comment on lines +156 to +165
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>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: This signature is nice in the sense that it enforces consistency while handling errors via the type system, but it also may cause more monomorphization. We'd probably need to add it to write_pattern. However, the creep should be limited to which version of TryWriteable the user calls.

pub(crate) fn write_pattern<'data, T, W, DS, TS>(
    pattern_items: impl Iterator<Item = PatternItem>,
    pattern_metadata: PatternMetadata,
    date_symbols: Option<&DS>,
    time_symbols: Option<&TS>,
    loc_datetime: &impl LocalizedDateTimeInput<T>,
    fixed_decimal_format: &FixedDecimalFormatter,
    w: &mut W,
) -> Result<(), Error>
where
    T: DateTimeInput,
    W: fmt::Write + ?Sized,
    DS: DateSymbols<'data>,
    TS: TimeSymbols,
{

@sffc
Copy link
Member Author

sffc commented Apr 4, 2024

Working Group Discussion:

  • @zbraniecki / @Manishearth - Double unwrap is weird, can we do something else?
  • @sffc - I tried an enum but double result was cleaner
  • @zbraniecki - Do we want to change all our APIs to this?
  • @sffc - We probably don't want to start adding failure cases where they don't already exist, but we could use format and try_format. Maybe even in DateTimeFormatter we should have try_format.
  • @zbraniecki - This looks good to me, still unsure about the double result

enum NeverWriteable {}

impl Writeable for NeverWriteable {
#[inline(always)] // to help the compiler find unreachable code paths
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: does this actually work with the annotation, and not without?

}
}

impl<T> TryWriteable for T where T: FallibleWriteable {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these separate traits if TryWriteable is blanket-implemented?

match self.fallible_write_to(&mut wc, handler) {
Ok(Ok(())) => (),
Ok(Err(e)) => return Err(e),
Err(core::fmt::Error) => unreachable!("writing to string is infallible"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: wrong message, you're not writing to string

>(
&self,
sink: &mut W,
handler: F,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing error handlers as closures is unusual. Can this not be done at the call site?

/// Lower-level trait for writeables that can fail while writing.
///
/// The methods on this trait should be implemented on custom writeables, but they generally
/// should not called directly. For methods more useful in client code, see [`TryWriteable`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// should not called directly. For methods more useful in client code, see [`TryWriteable`].
/// should not be called directly. For methods more useful in client code, see [`TryWriteable`].

/// writeable::assert_writeable_eq!(try_writeable.lossy(), "�");
/// ```
#[inline]
fn lossy(&self) -> LossyWriteable<&Self> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LossyWriteable needs Self::Error: Writeable to work, but the bound is not here. This sounds like it will lead to very annoying errors.

I do find it a bit odd that lossy allows custom errors to be written, it's not really lossy then because no information is lost. If it unconditionally wrote U+FFFD, it'd be lossy.

/// );
/// assert_writeable_eq!(
/// HelloWriteable { name: None::<&str> }.lossy(),
/// "Hello, ___!"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lossy is documented to return U+FFFD in the error case, but here it returns ___ because the writeable can somehow overwrite that? I think this is very confusing.

///
/// let try_writeable = None::<&str>;
/// assert!(matches!(
/// try_writeable.checked().try_write_to_string(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: why is try_write_to_string not on TryWriteable itself?

/// }
/// ```
#[inline]
fn gigo(&self) -> GigoWriteable<&Self> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, so we haven't been using the term "gigo" on our API before. I think it's not one of the universally known acronyms, so I'd like to find a better name for it.


impl_display_with_writeable!([T] GigoWriteable<T> where T: FallibleWriteable, T::Error: Writeable, T::Error: fmt::Debug);

impl<T> FallibleWriteable for Option<T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer implementing FallibleWriteable on Result<T, E>, so that the caller can control the error type. Using Option means the caller has to keep track of errors outside of this fallibility, because the handler will always receive the ZST.

@sffc
Copy link
Member Author

sffc commented Apr 9, 2024

Putting in draft because we might merge #4787 instead

@sffc sffc marked this pull request as draft April 9, 2024 22:04
@sffc sffc closed this Apr 10, 2024
@sffc sffc deleted the fallible-writeable branch April 10, 2024 18:21
@sffc
Copy link
Member Author

sffc commented Apr 10, 2024

Replaced by #4787

Code backed up at https://github.com/sffc/omnicu/tree/archive/fallible-writeable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we have a fallible Writeable?
4 participants