fmt::Write for NgxString does not grow the string, and leaves a partial write on overflow

[u5surf]

What happens

NgxString's fmt::Write impl is wired to append_within_capacity, so write!
never allocates:

impl<A> fmt::Write for NgxString<A>
where
    A: Allocator + Clone,
{
    fn write_str(&mut self, s: &str) -> fmt::Result {
        self.append_within_capacity(s).map_err(|_| fmt::Error)
    }
}

Two consequences, both of which I hit while writing a module:

use core::fmt::Write;

use ngx::allocator::Global;
use ngx::core::NgxString;

#[test]
fn shows_write_behaviour() {
    // 1. A fresh string has zero capacity, so the first write fails outright.
    let mut s: NgxString<Global> = NgxString::new_in(Global);
    let r = write!(s, "hello");
    println!("fresh      -> {:?} len={} {:?}", r, s.len(), core::str::from_utf8(s.as_bytes()));

    // 2. With some capacity, an oversized write fills it and *then* errors,
    //    leaving a truncated value behind.
    let long = "0123456789abcdefghijklmnopqrstuvwxyz";

    let mut s: NgxString<Global> = NgxString::new_in(Global);
    s.try_reserve(8).unwrap();
    let r = write!(s, "{}", long);
    println!("reserved 8 -> {:?} len={} {:?}", r, s.len(), core::str::from_utf8(s.as_bytes()));

    // 3. Each format fragment is a separate write_str, so a partial result can
    //    mix fragments that fit with one that did not.
    let mut s: NgxString<Global> = NgxString::new_in(Global);
    s.try_reserve(8).unwrap();
    let r = write!(s, "n={}", long);
    println!("multi frag -> {:?} len={} {:?}", r, s.len(), core::str::from_utf8(s.as_bytes()));
}

fresh      -> Err(Error) len=0 Ok("")
reserved 8 -> Err(Error) len=8 Ok("01234567")
multi frag -> Err(Error) len=8 Ok("n=012345")

Why I am raising it rather than sending a patch

I first read this as a bug and was going to propose wiring write_str to
try_append, which does grow. Then I found examples/shared_dict.rs, which
walks the whole dictionary to compute an exact byte count, including the digit
count of a number via checked_ilog10, and calls try_reserve before its first
write!. That is clearly written in full knowledge of the behaviour, and
"reserve once, then fill without further allocation" is a reasonable thing to
want on a pool or slab allocator.

So I assume this is intended. Three things still seem worth changing:

1. It is not documented anywhere. Neither the fmt::Write impl nor
   NgxString mentions it. The only signal in the tree is the arithmetic in
   that example. std::string::String grows on write!, so the default
   expectation points the other way, and getting it wrong is quiet: my status
   endpoint returned an empty response with nothing in the error log, because
   the first write! into a fresh string failed and the handler turned that
   into NGX_ERROR.

2. The partial write on overflow looks like a hazard rather than a feature.
   If the design is "reserve exactly, then fill", then an overflow is a
   miscalculation, and leaving a truncated prefix behind is the least useful
   outcome. Failing without writing anything would let callers that ignore the
   Result produce nothing instead of a corrupt value. As it stands,
   let _ = write!(json, ...) silently emits malformed output.

3. Computing the length up front is expensive for structured output. The
   example's arithmetic is manageable for key = value; pairs. For something
   like a JSON document with numbers of unknown width it stops being practical,
   and the fallback is to over-reserve by a guess. A growing writer alongside
   the current one, or a documented reserve_fmt-style helper, would cover
   that case without changing the existing behaviour.

I'd like to work on this issue. Let me know which of the three you would
accept and I'll open a PR.

Environment

• ngx-rust at cda9d83 (ngx 0.5.0)
• Reproduced with cargo test -p ngx --features vendored

[bavshin-f5]

The main thing we have to avoid here is unchecked allocation; std::string::String indeed grows on write!(), however it would panic when the reallocation fails, and that is not acceptable.

try_append with error propagation might be my preferred fix, as it is closer to the expected behavior while adhering to the above. I don't think we need a separate fixed-size formatter right now though.
Note that fmt::Write explicitly mentions that an incomplete write must raise an error, and strict allocation handling makes this outcome possible. Which means you always have to handle the write!() result, unless you can prove that the operation is infallible.

[u5surf]

Agreed on all three. Thanks for the correction on String: the panic on
reallocation failure is the part I glossed over. No separate fixed-size
formatter from me either, since append_within_capacity stays public for
anyone who wants that.

One note on the partial write from my second point. try_append reserves
before it appends, so a failed write_str leaves the string untouched. That
removes the truncation case I raised, where a half-filled value came back
alongside the error.

A write! spanning several fragments can still stop midway if a later
allocation fails, but that is inherent to the trait, and is exactly why the
result has to be checked, as you say.

I'll open a PR for the try_append switch.