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

Macros wrapping expressions in unsafe blocks when using unsafe_op_in_unsafe_fn #7323

Open
ojeda opened this issue Jun 4, 2021 · 6 comments
Labels
A-lint Area: New lints

Comments

@ojeda
Copy link
Contributor

ojeda commented Jun 4, 2021

What it does

For projects using unsafe_op_in_unsafe_fn (and specially those requiring // SAFETY comments), it would be nice to detect cases where a macro is wrapping an expression passed as a parameter in a "hidden" unsafe block, and thus bypassing the intention of unsafe_op_in_unsafe_fn.

The logic could be something like:

  • If inside an unsafe fn...
  • And there is a macro call outside an unsafe block...
  • And unsafe_op_in_unsafe_fn is enabled in this context...
  • And if the node/AST/tree of a parameter appears as-is...
  • Then check that node/AST/tree is not inside an introduced unsafe block.

Callers of the macro would need to wrap the entire macro call in an unsafe block to avoid hitting the lint (plus perhaps playing with #[allow(unused_unsafe)] inside the macro to avoid the unneeded nested unsafe warning from rustc).

Categories (optional)

  • Kind: clippy::pedantic?

Drawbacks

None.

Example

Assume #![deny(unsafe_op_in_unsafe_fn)], and that container_of! wraps the first parameter in an unsafe block. Then:

let reg = container_of!((*file).private_data, Self, mdev);

Could be written as:

let reg = unsafe { container_of!((*file).private_data, Self, mdev) };
@llogiq
Copy link
Contributor

llogiq commented Jun 5, 2021

I have a case in compact_arena where a macro ensures the safe call of an unsafe function.

I would consider that a false positive. I would not see a problem with allowing the lint in this situation, but the lint docs should consider this possibility.

@camsteffen
Copy link
Contributor

camsteffen commented Jul 18, 2021

I am reviewing #7469 and want to raise a question before merging. @flip1995

I think these two things are orthogonal:

  1. I want to be warned when an unsafe function lacks an unsafe block. (unsafe_op_in_unsafe_fn)
  2. I want to be warned when a macro hides an unsafe block. (new lint)

The fact that you are in an unsafe function does not make the hidden unsafe block any more egregious IMO. So I don't think this lint should be dependent on #[warn(unsafe_op_in_unsafe_fn)] unsafe fn. Am I making sense?

#[warn(unsafe_op_in_unsafe_fn)]
unsafe fn f() {
    unsafe_macro!(); // this is bad
}

fn g() {
    unsafe_macro!(); // so isn't this just as bad?
}

@flip1995
Copy link
Member

flip1995 commented Jul 19, 2021

I think it should depend on the unsafe_op_in_unsafe_fn lint if the macro is in an unsafe function. Without enabling the unsafe_op_in_unsafe_fn your saying that it is ok to mark a function as unsafe and then everything that happens in this function may or may not be unsafe, which includes macro calls that may hide unsafe ops.

For your example in fn g() I think you're right. This is also problematic and the lint should probably trigger here regardless whether the unsafe_op_in_unsafe_fn lint is enabled or not.

@camsteffen
Copy link
Contributor

camsteffen commented Jul 19, 2021

I see what you mean.

#[allow(unsafe_op_in_unsafe_fn)]
unsafe fn h() {
    unsafe_macro!(); // this is ok
}

@ojeda
Copy link
Contributor Author

ojeda commented Aug 4, 2024

I guess this ended up being macro_metavars_in_unsafe, right?

@ojeda
Copy link
Contributor Author

ojeda commented Aug 5, 2024

Cc'ing @y21 here as well, in case he wasn't aware of this one, since he implemented macro_metavars_in_unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants