Conversation
|
@mitsuhiko I think having this crate be 100% safe is rather nice: it avoids the need for unsafe review/etc. You're welcome to make a different call here, but Pin unsafe is hard to review and unsafe that is "hard to review" in my experience has a much higher chance of being subtly broken. At the very least, would you consider adding real safety comments that explain why it is safe? At the moment it's just a line of completely unexplained unsafe, dealing with a rather tricky corner of Rust safety.
|
|
I care a lot about dependencies and compile time performance and this showed up when I updated insta in my own project as a regression. Particularly because this is a compile time crate trading a single use of unsafe in an uncommon code path for a dependency to me is a bad tradeoff. I will add a comment about this use. I did not find it particularly offensive given that it's the example from the documentation. At the time this code was written pin-project did not even exist from what I remember and that pattern was generally widespread. |
|
I think pinning is one of the more tricky bits of unsafe Rust; since you need to understand both unsafe Rust and async pinning to really handle it. It's not a particularly offensive use of unsafe Rust, but it requires more specialized knowledge than usual to review. There's basically no harm done in writing copious amounts of unsafe documentation, especially when the crate does not actually use unsafe often. pin-project is a common enough ecosystem crate that I didn't think it would be a big deal, but if it is, it is. |
|
I added a SAFETY comment in 0b2d273 |
|
I think that's not much, but I'm not going to push it, I might try to write a proper safety comment later. (full safety comments, where all the necessary invariants are listed and demonstrated, are rare in the ecosystem anyway) |
Reverts #711 and #737.
It's for a singular unsafe on a rather uncommon path, that is not worth pulling dependencies in.