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

Incorrect warning: Unnecessary unsafe block #49112

Open
Boscop opened this issue Mar 17, 2018 · 8 comments
Open

Incorrect warning: Unnecessary unsafe block #49112

Boscop opened this issue Mar 17, 2018 · 8 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Boscop
Copy link

Boscop commented Mar 17, 2018

image

This warning is incorrect. It says for both unsafe blocks that they are unnecessary, and when either of the unsafe keywords is removed, it gives an error (of course).
It wrongly says that the first block is nested under the second unsafe block.

Before (nightly at end of January) this warning didn't occur.

@Boscop
Copy link
Author

Boscop commented Mar 17, 2018

Ah, nevermind, it occurs only when the macro is called in the context of another unsafe block, like:

arr![arr![None; 128]; 16]

Still, it should detect this, and not display the warning.

@sapphire-arches sapphire-arches added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-diagnostics Area: Messages for errors, warnings, and lints labels Mar 19, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 12, 2018

This is not an enhancement, this is a regression. As mentioned by the OP, this warning didn't used to occur.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 12, 2018

Are there any simple workarounds for this warning? #[allow(unused_unsafe)] doesn't work on expressions.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 12, 2018

So the workarounds is to turn this code:

unsafe { ... } // expression

into this:

{
    #[allow(unused_unsafe)]
    let tmp = unsafe { ... };
    tmp
}

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 20, 2020
@hallya
Copy link

hallya commented Dec 12, 2020

same issue, not sure how to fix it in this case:

.map(|_| {
  if unsafe { js_sys::Math::random() < 0.5 } {
    Cell::Alive
  } else {
    Cell::Dead
  }
})
.collect();

@steffahn
Copy link
Member

steffahn commented Feb 6, 2022

Is there any actual issue being reported here? If yes, what’s the code to reproduce it and what’s the actual issue?

@kpreid
Copy link
Contributor

kpreid commented Apr 26, 2022

what’s the code to reproduce it and what’s the actual issue?

I thought I'd help out by retyping (and modernizing) the code and error from the original screenshot:

macro_rules! arr {
    ($item:expr; $n:expr) => {
        {
            let mut array_uninit: [_; $n] =
                [::core::mem::MaybeUninit::uninit(); $n];
            for x in &mut array_uninit {
                unsafe { ::core::ptr::write(x.as_mut_ptr(), $item) };
            }
            array_uninit.map(|mu| unsafe { mu.assume_init() })
        }
    }
}

fn main() {
    dbg!(
        arr![arr![5; 3]; 2]
    );
}

This produces the following warnings (on stable 1.60.0):

Warnings
warning: unnecessary `unsafe` block
  [--> src/main.rs:7:17
](https://play.rust-lang.org/#)   |
7  |                 unsafe { ::core::ptr::write(x.as_mut_ptr(), $item) };
   |                 ^^^^^^
   |                 |
   |                 unnecessary `unsafe` block
   |                 because it's nested under this `unsafe` block
...
16 |         arr![arr![5; 3]; 2]
   |              ---------- in this macro invocation
   |
   = note: `#[warn(unused_unsafe)]` on by default
   = note: this warning originates in the macro `arr` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: unnecessary `unsafe` block
  [--> src/main.rs:9:35
](https://play.rust-lang.org/#)   |
7  |                 unsafe { ::core::ptr::write(x.as_mut_ptr(), $item) };
   |                 ------ because it's nested under this `unsafe` block
8  |             }
9  |             array_uninit.map(|mu| unsafe { mu.assume_init() })
   |                                   ^^^^^^ unnecessary `unsafe` block
...
16 |         arr![arr![5; 3]; 2]
   |              ---------- in this macro invocation
   |
   = note: this warning originates in the macro `arr` (in Nightly builds, run with -Z macro-backtrace for more info)

That is, the stated problem still reproduces. However, thinking about what's going on here, this code is hazardous and should be improved: the macro is evaluating $item in an unsafe block, even though it probably shouldn't, and the warning appears because the inner macro call is in an unsafe block provided by the outer macro call. The warning doesn't explain this, but it is reflective of an actual problem. The warning (and the hazard) can be removed by using a temporary for evaluating $item, so that it is not inside an unsafe block:

                let item = $item;
                unsafe { ::core::ptr::write(x.as_mut_ptr(), item) };

So, not-a-bug? Well, even with this fix, the arr! macro will still cause a warning if it is used within any unsafe block, which seems suboptimal, since the caller may have reason to do so (unsafe { arr![some_unsafe_fn(); 3] }). I'm not sure suppressing this warning whenever it appears would be best, though. A better semantics might be some kind of “unsafe hygiene”, where the macro body is not interpreted as unsafe simply due to the caller's enclosing use of unsafe, but that would raise further issues for code generation that isn't as simple as pasting expressions into other expressions, and it isn't exactly an A-diagnostics issue.

@steffahn
Copy link
Member

So, not-a-bug? Well, even with this fix, the arr! macro will still cause a warning if it is used within any unsafe block, which seems suboptimal, since the caller may have reason to do so (unsafe { arr![some_unsafe_fn(); 3] }).

I think that’s essentially “working as intended”, for now.

Something like

macro_rules! arr {
    ($item:expr; $n:expr) => {
        {
            let mut array_uninit: [_; $n] =
                [::core::mem::MaybeUninit::uninit(); $n];
            for x in &mut array_uninit {
                let item = $item;
                #[allow(unused_unsafe)]
                unsafe { ::core::ptr::write(x.as_mut_ptr(), item) };
            }
            #[allow(unused_unsafe)]
            array_uninit.map(|mu| unsafe { mu.assume_init() })
        }
    }
}

should work fine. (Ignoring the soundness problem of possible break or continue in the $item expression. And the fact that previous items are leaked on panic.)

And a larger unsafe hygiene featire for macros is likely going to be more than just “diagnostics enhancements” (the current labels of this issue).

zmlgirl pushed a commit to taosdata/taos-connector-rust that referenced this issue Feb 17, 2023
1.Use camel case instead of underscores for type names
2.for functions and variables never used comment out
3.for field never be read or enum never be constructed add underscore
4.there is another warning which is 'unnecessary `unsafe` block' seems like a Incorrect warning: rust-lang/rust#49112
zmlgirl pushed a commit to taosdata/taos-connector-rust that referenced this issue Feb 17, 2023
1.Use camel case instead of underscores for type names 2.for functions and variables never used comment out 3.for field never be read or enum never be constructed add underscore 4.there is another warning which is 'unnecessary `unsafe` block' seems like a Incorrect warning: rust-lang/rust#49112
zitsen pushed a commit to taosdata/taos-connector-rust that referenced this issue Mar 8, 2023
1.Use camel case instead of underscores for type names
2.for functions and variables never used comment out
3.for field never be read or enum never be constructed add underscore
4.there is another warning which is 'unnecessary `unsafe` block' seems like a Incorrect warning: rust-lang/rust#49112
zitsen pushed a commit to taosdata/taos-connector-rust that referenced this issue Mar 8, 2023
1.Use camel case instead of underscores for type names 2.for functions and variables never used comment out 3.for field never be read or enum never be constructed add underscore 4.there is another warning which is 'unnecessary `unsafe` block' seems like a Incorrect warning: rust-lang/rust#49112
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

7 participants