fix usage of Bytes.unsafe_of_string

[hhugo]

Don't reuse strings after they get (unsafely) converted to bytes for mutation. Instead, explicitly refer to the mutated bytes.

Also remove instances of Bytes.unsafe_of_string as theses usages are incorrect based on the documentation (see https://github.com/ocaml/ocaml/blob/trunk/stdlib/bytes.mli#L408)

This issue was discovered while working on ocsigen/js_of_ocaml#923 which changes the representation of ocaml strings to be different from the representation of bytes.

[gasche]

I agree that the use of unsafe_of_string was incorrect here, but I think that a nicer approach to change the code would be to turn round_futur_last_digit from a bytes -> unit function to a string -> string function, which would remove the need to use bytes for these arguments in the caller code. (On the other hand, the code is pretty bad anyway, so maybe it doesn't matter that much.)

[dra27]

@gasche - the function is bytes -> … -> bool - I don't think the interface will get much neater by getting rid of the mutation of the argument, as you'll still either need to signal the outcome as before or move the logic which processed the boolean result from the call-site into the function. More importantly, a GitHub search revealed two projects which "use" (to some degree) this internal function: https://github.com/8l/focalize and https://github.com/rems-project/lem/. I don't think we should risk any API change, therefore.

[dra27]

This looks good to me, and I checked that the remaining uses of unsafe_ functions are sound (the remaining ones are all Bytes.unsafe_to_string where the constructed bytes value truly is then not subsequently mutated)

[dra27]

@hhugo - there appear to remain some issues with ocamlfind when we release new versions of num which @AltGr and I are still investigating. Once this PR is merged (I'll let @xavierleroy have a glance first), would you be able to continue working with num pinned instead, since I added a num.opam to the repo already? If that works, it would both give us time to fix the opam-repository issue and also potentially allow you to find any more 4.10/safe-string-related bugs 🙂

[gasche]

Well -> string option would be fine -- if the function returns false the input string is unchanged, so None is a fine return value and the caller can reuse the string.

We can remain compatible with other users by providing both variants in the API. It's fairly easy (and slightly less efficient, but we don't really care about that) to implement the nice string API on top of the bytes version, avoiding any code duplication.

[hhugo]

@dra27, I'm not in a rush for this to be merged.

@gasche, I fear that your nicer approach will open a big can of worms. It's not clear that one should spend too much time improving num as the README encourage people to switch to Zarith.

[hhugo]

any news / progress ?

[gasche]

I decided to go ahead and merge this patch.

[hhugo]

Could we have a minor release that include that change ?

[dra27]

The last release of num caused some pain for the Coq team's CI, but we (the opam team) couldn't repro it and they reset everything on their build workers before we had a proper chance to investigate.

So could we tag the release in this repo but please not release it to opam - I'd like to liaise with them and do a test release of it first to see if we can isolate what went wrong.

[hhugo]

@dra27, has the issue with new release of num been resolved ? Can we can have a new release of num now ?

[hhugo]

Any update on the new release ?

[dra27]

I loosely had it in mind to allow 4.11 out of the door first

[hhugo]

4.11 is out. Can we have a release now ?

[hhugo]

I've opened #21 to track the pending new release