Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shrink unsafe blocks #183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Shrink unsafe blocks #183

wants to merge 1 commit into from

Conversation

peamaeq
Copy link

@peamaeq peamaeq commented Jun 23, 2022

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

@workingjubilee
Copy link

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.

@dtolnay
Copy link
Contributor

dtolnay commented Apr 22, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants