Yes, it's because Write would require an additional utf-8 check when converting the buffer to a string (or an unsafe section that assumes we only write strings into it), while StrWrite allows us to maintain the invariant that the buffer is valid utf-8 entirely using safe code. If you look at the history you'll see some iteration on this.

Hmm, I don't understand: core::fmt::Write and StrWrite look almost identical.

Write is not implemented for String, but StrWrite is. What we were doing before is using the impl of Write for Vec<u8>.

Write is not implemented for String, but StrWrite is. What we were doing before is using the impl of Write for Vec<u8>.

Hmm, I'm pretty sure fmt::Write is implemented for String (playground):

use std::fmt;

fn write<W: fmt::Write>(writer: &mut W, text: &str) {
    writer.write_str(text).unwrap();
}

fn main() {
    let mut string = String::new();
    write(&mut string, "hello string!");
    println!("{}", string);
}

Ah, I was confusing fmt::Write and io::Write. We also want to be able to write to files (or, more generally, anything which impl's io::Write), which can be done with WriteWrapper, but I'm not sure any similar thing exists for fmt::Write.

We also want to be able to write to files (or, more generally, anything which impl's io::Write), which can be done with WriteWrapper, but I'm not sure any similar thing exists for fmt::Write.

Ah, okay :)

Maybe I can add a blanket impl for fmt::Write if the type implements io::Write...

Opened rust-lang/rust#77733.

It seems like a good idea to reuse a core trait. If we can't have a blanket implementation of fmt::Write for types that implement io::Write, we could just repurpose WriteWrapper for that.

This would be a compatibility break technically, but I think it should require minimal changes to upgrade.

Yes, thinking about this now, switching to fmt::Write and adapting our own WriteWrapper seems quite workable. The blanket in Rust looks like it's not going to happen as it can break coherence. I agree that client changes would be quite minimal, so I'd be ok with the (technical) compatibility breakage.

Cool. @camelid: if you want to do the honors, I'd be happy to merge a PR making this change.

Sure!

Hmm, this code:

impl<W> fmt::Write for WriteWrapper<W>
where
    W: fmt::Write,
{
    #[inline]
    fn write_str(&mut self, s: &str) -> fmt::Result {
        self.0.write_str(s)
    }

    #[inline]
    fn write_fmt(&mut self, args: Arguments) -> fmt::Result {
        self.0.write_fmt(args)
    }
}

impl<W> fmt::Write for WriteWrapper<W>
where
    W: io::Write,
{
    #[inline]
    fn write_str(&mut self, s: &str) -> fmt::Result {
        match self.0.write_all(s.as_bytes()) {
            Ok(()) => Ok(()),
            Err(err) => Err(fmt::Error),
        }
    }

    #[inline]
    fn write_fmt(&mut self, args: Arguments) -> fmt::Result {
        match self.0.write_fmt(args) {
            Ok(()) => Ok(()),
            Err(err) => Err(fmt::Error),
        }
    }
}

Causes this error:

error[E0119]: conflicting implementations of trait `std::fmt::Write` for type `escape::WriteWrapper<_>`:
  --> src/escape.rs:65:1
   |
50 | / impl<W> fmt::Write for WriteWrapper<W>
51 | | where
52 | |     W: fmt::Write,
53 | | {
...  |
62 | |     }
63 | | }
   | |_- first implementation here
64 | 
65 | / impl<W> fmt::Write for WriteWrapper<W>
66 | | where
67 | |     W: io::Write,
68 | | {
...  |
83 | |     }
84 | | }
   | |_^ conflicting implementation for `escape::WriteWrapper<_>`

How do I get around this?

EDIT: Removed duplicate trait bound in code (issue is still there though).

We should only need

impl<W> fmt::Write for WriteWrapper<W>
where
    W: io::Write,
{ .. }

because one wouldn't need WriteWrapper<W> when W already implements fmt::Write.

Hmm, but what about this?

use std::fmt::Write;

// ...

pub fn write_html<'a, I, W>(writer: W, iter: I) -> fmt::Result
where
    I: Iterator<Item = Event<'a>>,
    W: Write,
{
    HtmlWriter::new(iter, WriteWrapper(writer)).run()
}

error[E0277]: the trait bound `W: std::io::Write` is not satisfied
   --> src/html.rs:460:27
    |
57  |     fn new(iter: I, writer: W) -> Self {
    |     ---------------------------------- required by `html::HtmlWriter::<'a, I, W>::new`
...
460 |     HtmlWriter::new(iter, WriteWrapper(writer)).run()
    |                           ^^^^^^^^^^^^^^^^^^^^ the trait `std::io::Write` is not implemented for `W`
    |
    = note: required because of the requirements on the impl of `std::fmt::Write` for `escape::WriteWrapper<W>`
help: consider further restricting this bound
    |
458 |     W: Write + std::io::Write,
    |              ^^^^^^^^^^^^^^^^

Should I accept the suggestion?

Actually I think I figured it out: I just changed the requirement to W: io::Write (what it used to be).

I would be interested in this as well. It would be really nice to be able to reuse the escape_html function for std::fmt::Write such that I could do something like:

struct EscapedHtml(CowStr);

impl Display for EscapedHtml {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        escape_html(f, &self.0)
    }
}

Which would then permit:

// ...
Code(text) => write!(w, "<code>{}</code>", EscapedHtml(&text))
// ...

Note that this pattern might also be useful for the stock html module.