Add safety comments

[Manishearth]

I recently performed safety review on 0.19. Besides finding the already-fixed #1491, I also found that these cases of unsafety were a bit hard to disentangle, and documented them further.

[sftse]

Does the referenced UCG discussion 412 apply to this case? Exercising the offending code under miri with and without the flags mentioned in the issue does not make a difference.

MIRIFLAGS='-Zmiri-env-set=TOKENIZERS_PARALLELISM=false -Zmiri-recursive-validation' cargo +nightly miri test bpe::trainer::tests::test_train

[Manishearth]

@sftse IIRC miri doesn't currently do anything special around Vecs in this way, so it's not useful to rely on it for whether the rules apply. I did mention "this and related issues" because I know this type of thing has been the subject of discussion. For now I'm comfortable giving a sense of "this is potentially but not probably problematic"

[sftse]

[...] does not make a difference.

This may have been poor wording on my part, and may have communicated that miri thinks the code is fine. What I meant is under both the liberal and more restrictive validity rules for references, miri does not think the code is fine. If this was not a misunderstanding, this reply would be very puzzling to me and would hope you can expand on that.

[sftse]

It's worth pointing out that this appears to not even be something solely detectable by miri. Prior to commit 4322056 rustc would refuse to compile this crate, as mentioned in #1485. That commit, although not introducing any relevant changes to the unsafe block, managed to successfully subvert rustc's detection of this UB pattern.

[Manishearth]

Oh! The & to &mut transmute is the problem, I see. We can use &raw or addr_of there instead. The recursive validity issue made me not notice the other issue there.

Given that I might just write it with manual unsafe (which would not be open to recusrive validity issues or transmute issues) then.

[Manishearth]

I attempted a fix.

[sftse]

Thanks!

Before seeing your PR I had made #1664, trying to hit the same angle but a bit more verbose, I should have mentioned that. #1651 and #1664 have a bit of overlap, the maintainers may want to deduplicate how they see fit. No longer.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[ArthurZucker]

Thanks a lot both! 🤗 quite honored to see core rust people here 👀
LGTM!

[ArthurZucker]

FMI, why is &T UB?

[Manishearth]

It's UB to mutate data that currently has an active &T. This persists through raw pointers in some ways (and not in others).

[ArthurZucker]

cargo fmt should be enough for the CI!

[ArthurZucker]

Error:    --> src/models/bpe/trainer.rs:535:17
    |
535 |             let ref pos: HashSet<usize> = top.pos;
    |             ----^^^^^^^--------------------------- help: try: `let pos: &HashSet<usize> = &top.pos;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg
    = note: `-D clippy::toplevel-ref-arg` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::toplevel_ref_arg)]`

seems to cause an issue with compilation, not sure if it's actionable on my side

[Manishearth]

Fixed

[ArthurZucker]

Thanks!