bitvec icon indicating copy to clipboard operation
bitvec copied to clipboard

Shrink unsafe blocks

Open peamaeq opened this issue 3 years ago • 2 comments

In these function you use the unsafe keyword for some safe expressions. However, I found that only 3 functions are real unsafe operations (see the list below).

We need to mark unsafe operations more precisely using unsafe keyword. Keeping unsafe blocks small can bring many benefits. For example, when mistakes happen, we can locate any errors related to memory safety within an unsafe block. This is the balance between Safe and Unsafe Rust. The separation is designed to make using Safe Rust as ergonomic as possible, but requires extra effort and care when writing Unsafe Rust. Real unsafe operation list:

  1. the split_at_unchecked_mut_noalias()\split_at_unchecked()\get_unchecked() function(these are unsafe functions)

@myrrlyn Hope this PR can help you. Best regards. References https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html

peamaeq avatar Jun 23 '22 12:06 peamaeq

Hello. I am not the author of this crate, nor a maintainer of it, and so I have standing neither to approve or deny this pull request. However, I was reviewing the Unsafe Rust in this crate (it does in fact need some tender love and care, as it turns out) when this pull request caught my eye due to the common subject. This patch seems interesting, but I am slightly concerned about the grounds on which it is made.

One part is that can actually be stylistically normative, in Unsafe Rust code, to have the unsafe {} span multiple statements. This is especially true when the invariants required before returning to Safe Rust control flow beyond the function have some trespass against them early in the block and then are restored before the end of the block. An example of this can be witnessed in the documentation for std::ptr::write.

But there are many other reasons to use a single unsafe {} as well. I was curious, are you aware of the implications of the {} in the unsafe {}, and the let bindings in these examples? As unsafe {} includes the {}, it creates a scope like any other block, as unsafe is a keyword but does not have a complete meaning on its own. Instead it attaches to and modifies the meaning of another fragment of Rust code. Thus, a programmer cannot simply ignore the {}. For example, if inside the unsafe {} an identifier that was visible was shadowed...

let x = create_value();

unsafe { 
    let x = create_value_unchecked();
    unsafely_use(x); // uses "x the second"
}

use(x); // uses "x the first"

And then that unsafe {} was narrowed, like so...

let x = create_value();

let x = unsafe { create_value_unchecked() };
unsafe { unsafely_use(x); } // uses "x the second"

use(x); // uses "x the second"

This has changed what the code does, just by altering where the unsafe blocks are. And Rust programmers cannot expect the borrow checker to always catch this, as it is entirely possible these are Copy and taken by value. Obviously, this caveat is not a problem here, but there are many possible configurations of Unsafe Rust, and I noticed you made a similar PR many times throughout the Rust ecosystem.

I only reviewed a few of those PRs, and I found some of them were entirely correct and warranted. So I certainly do not want to discourage you from improving the overall code quality on crates.io! However, I was wondering if you were aware of this detail as a possible concern when making this kind of change? I am somewhat concerned that maintainers may be inclined to accept your PR without considering this risk when reviewing the code.

workingjubilee avatar Jun 26 '22 07:06 workingjubilee

FYI I think this was a misguided bot. They opened 50+ similar PRs on the same day, largely broken in a variety of ways, and have not engaged with review feedback on a single one of them.

dtolnay avatar Apr 22 '23 04:04 dtolnay