icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

TryWriteable should have try_writeable_borrow

Open sffc opened this issue 3 months ago • 8 comments

https://github.com/unicode-org/icu4x/pull/6985#discussion_r2389281839

sffc avatar Sep 29 '25 21:09 sffc

No I don't think so. The borrowing method should be constant time, because otherwise write_to_string would duplicate work. I don't think a scenario where an error can be detected in constant time needs to be a TryWriteable in the first place, so there's no point in making the borrowing fallible.

robertbastian avatar Sep 30 '25 10:09 robertbastian

ok, maybe it should return (Option<&str>, Option<&str>) then?

Or maybe TryWriteable just shouldn't have this function?

sffc avatar Oct 01 '25 21:10 sffc

what would that pair mean?

robertbastian avatar Oct 01 '25 21:10 robertbastian

Just the result of running writeable_borrow on both the Ok and the Err variants. If either needs to be computed, then it is None. For example, often TryWriteable is implement on Result<W, &'static str>, and in that case, try_writeable_borrow might return (None, Some(<the err string>)). Haven't fully thought it through.

sffc avatar Oct 01 '25 21:10 sffc

How would try_write_to_string then know which variant to choose?

This method is required to make TryWriteable borrow in no-alloc mode, so I'd rather not remove it.

robertbastian avatar Oct 01 '25 21:10 robertbastian

What if we had a signature like

trait NewWriteSink<'a>: fmt::Write {
    fn write_finish(s: &'a str) -> fmt::Result;
}

trait TryWriteable {
    // ...
    fn try_write_bikeshed<'a, NewWriteSink>(&'a self, sink: NewWriteSink<'a>) -> Result<Result<(), Self::Error>, fmt::Error>;
}

// No-alloc impl:
struct NoAllocNewWriteSink<'a>(Option<&'a str>);
impl fmt::Write for NoAllocNewWriteSink<'_> {
    // functions that set the inner option to None and return an error
}
impl<'a> NewWriteSink<'a> for NoAllocNewWriteSink<'a> {
    fn write_finish(s: &'a str) -> fmt::Result {
        self.0 = Some(s);
        Ok(())
    }
}

sffc avatar Oct 02 '25 00:10 sffc

  • @robertbastian We could just remove it in 2.1; we don't seem to use it
  • @robertbastian We could use it in impl TryWriteable for PotentialUtf8
  • @sffc That's a linear-time impl; is that ok?
  • @robertbastian Yeah it could be ok
  • @sffc I think we should be able to borrow in either the Ok or the Err case
  • @sffc I think the correct signature is Option<Result<&str, &str>>
  • @robertbastian Maybe, but it still needs the error code in the error case. So it would be Option<Result<&str, (Self::Error, &str)>>. Let's just wait until we really need it

sffc avatar Oct 14 '25 15:10 sffc

This can be removed from the milestone or closed, yes?

Manishearth avatar Oct 14 '25 19:10 Manishearth