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

Nightly: unsafe code is allowed outside unsafe blocks when the code is unreachable #45087

Closed
Thiez opened this issue Oct 7, 2017 · 8 comments
Assignees
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Thiez
Copy link
Contributor

Thiez commented Oct 7, 2017

I'm not sure if this is a feature or a bug, but the compiler behaves differently for the following program on stable, beta, and nightly:

fn main() {
    return;
    let _ = ();
    *(&true as *const bool);
}

On stable:

error[E0133]: dereference of raw pointer requires unsafe function or block
 --> src/main.rs:4:5
  |
4 |     *(&true as *const bool);
  |     ^^^^^^^^^^^^^^^^^^^^^^^ dereference of raw pointer

error: aborting due to previous error

On beta:

warning: unreachable statement
 --> src/main.rs:3:5
  |
3 |     let _ = ();
  |     ^^^^^^^^^^^
  |
  = note: #[warn(unreachable_code)] on by default

error[E0133]: dereference of raw pointer requires unsafe function or block
 --> src/main.rs:4:5
  |
4 |     *(&true as *const bool);
  |     ^^^^^^^^^^^^^^^^^^^^^^^ dereference of raw pointer

error: aborting due to previous error

On nightly:

warning: unreachable statement
 --> src/main.rs:3:5
  |
3 |     let _ = ();
  |     ^^^^^^^^^^^
  |
  = note: #[warn(unreachable_code)] on by default

Note that wrapping the final statement in an unsafe block will allow the program to compile on all three versions, but on nightly the compiler will complain:

warning: unnecessary `unsafe` block
 --> src/main.rs:4:5
  |
4 |     unsafe { *(&true as *const bool); }
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unnecessary `unsafe` block
  |
  = note: #[warn(unused_unsafe)] on by default

I suppose that the compiler is technically correct, because dead code tells no tales cannot cause memory unsafety, so no unsafe block is needed... but the current situation does seem like an accident, as the compiler does perform other checks on dead code (typechecking, borrow-checking, ...).

@bstrie bstrie added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 7, 2017
@hanna-kruppe
Copy link
Contributor

Probably due to #44700, cc @arielb1

@TimNN TimNN added the C-bug Category: This is a bug. label Oct 10, 2017
@nikomatsakis
Copy link
Contributor

cc @rust-lang/lang as well

This is clearly a result of doing unsafe checking on MIR. My preference would be to continue issuing the error, but we have sometimes decided to let errors in unreachable code slide -- but that was primarily around the borrow check, i.e., checks that are very flow-sensitive. Effect check doesn't seem to fall into that category. Probably if we just perform the unsafe check before we remove dead code, it'd be fine? But @arielb1 can correct.

@nikomatsakis
Copy link
Contributor

triage: P-high

We have to reach a decision of some kind.

@rust-highfive rust-highfive added the P-high High priority label Oct 19, 2017
@joshtriplett
Copy link
Member

I think it's entirely wrong to fail to issue the warning, here. Even if code is unreachable, we shouldn't allow incorrect usage/non-usage of unsafe.

Among other things, that might also break code in if cfg!(...) blocks.

@Thiez
Copy link
Contributor Author

Thiez commented Oct 19, 2017

I think the problem is not so much that the compiler dosen't mind unreachable unsafe code outside of unsafe blocks (although that is a bit silly), but that it gives "unnecessary unsafe block" warnings for correct unsafe{ ... } blocks that happen to be unreachable.

@arielb1
Copy link
Contributor

arielb1 commented Nov 2, 2017

Probably if we just perform the unsafe check before we remove dead code, it'd be fine?

Yeah it shouldn't be a problem to fix this

@arielb1 arielb1 self-assigned this Nov 2, 2017
@kennytm
Copy link
Member

kennytm commented Nov 2, 2017

Affects 1.22 beta too.

@kennytm kennytm added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Nov 2, 2017
@nikomatsakis
Copy link
Contributor

Fix is in queue, nice work @arielb1.

@bors bors closed this as completed in cd279a5 Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants