String.recalc chops last byte when recalc'd _size == _alloc

[Perelandric]

This is similar to the issue with .trucate(), #1427.

actor Main
	new create(env: Env) =>
    var s = String.from_iso_array(recover
      [ '1', '1', '1', '1', '1', '1', '1', '1' ]
    end)
    env.out.print(s.clone())
    env.out.print("size: " + s.size().string())
    env.out.print("space: " + s.space().string())
    env.out.print("-------- recalc --------")
    s.recalc()
    env.out.print(s.clone())
    env.out.print("size: " + s.size().string())
    env.out.print("space: " + s.space().string())

Output:

11111111
size: 8
space: 8
-------- recalc --------
1111111
size: 7
space: 7

The fix is simple enough, though in order to maintain the documented behavior, an extra byte will need to be reserve()'d, causing an allocation to the next power of two.

[jemc]

I'm not even sure I believe that recalc should be a part of the String API anymore now that not all strings are null terminated, but that would be RFC territory.

For now, I would suggest changing the documented behaviour to say "If a null terminator byte is not found within the allocated length, the entire allocated length will be used as the size". Like many of the bugs you've found recently, this should have been changed when we removed the null terminator requirement, but it was overlooked (the blame should mainly fall on me for this).

[Perelandric]

@jemc

Trouble is that even if a null terminator is found, we really have no idea if it was what the user intended, since pre-allocated space is not zeroed. With pre-allocation, which seems to be almost always the case, the method would seem to be non-deterministic. Even if we do let s = recover "111.clone()" end, we can't truly know what a recalc() will give us.

I was about to open a PR, but my master ended up getting an unexpected merge, so I'm fixing that first. I'll hold off on the PR for now until a final decision is made.

[jemc]

We discussed on the sync call that the documented/implemented behaviour of recalc in the did-not-find-a-null-terminator case should be changed to: "If a null terminator byte is not found within the allocated length, the size will not be changed".

This behaviour would not reveal any non-deterministic bytes from the pre-allocation, and would not cause any problems if you happen to call the method on a String that never actually had its buffer changed by an FFI call - the same bytes from the buffer that were exposed before would now still be exposed.

[Perelandric]

Sounds good @jemc. I'll finish this up in a few hours.

[Perelandric]

@jemc I just submitted a PR. Still seems non-deterministic since we won't know if a preallocated byte will be null, but I guess there's probably no way around that.

[jemc]

Yeah, it's true - but as long as this method exists to do what it does, we have that problem... ¯\_(ツ)_/¯

It's only necessary because some C functions won't return the number of bytes written to the buffer.