Add new lint macros_hiding_unsafe_code

[flip1995]

This lint only triggers if the unsafe_op_in_unsafe_fn lint is enabled.

This lint is placed in the restriction group, since it suggests code,
that requires to allow the unused_unsafe lint locally. Instead of
allowing this lint, the unsafe block in the macro could be removed.

Closes #7323

changelog: Add new lint [macros_hiding_unsafe_code]

[flip1995]

r? @camsteffen (highfive down again? 🤔)

[camsteffen]

I think this needs to be adjusted for nested functions.

unsafe fn f() {
    fn g() {}
    unsafe_macro!();
}

[flip1995]

Ah right. Your example shouldn't break this code, but

unsafe fn f() {
    unsafe fn g() {}
    unsafe_macro!();
}

will.

[popzxc]

While I totally like the idea of this lint, I think its design should be thought through a bit more carefully.
This lint should not suggest anything a user normally won't do.

Additionally, if some library has such a macro already, dependent crates will have no choice but to either get rid of dependency or add a bunch of allow(...), since they can't directly fix the issue.

Logically, it's the macro fault that it hides unsafe code, so the lint should be placed on the macro. Not sure whether it can be implemented though.

[popzxc]

I don't think that being against the intention of lint is a strong motivation since it's the nature of the lint to catch what it's supposed to catch. Like, chicken and egg problem.

I'd emphasize that the macro containing an unsafe can hide the fact that it uses unsafe, and thus makes it easier to use in the context where required invariants are not held.

[flip1995]

Good point, I'll word this differently.

[popzxc]

IMHO, suggesting introducing an unused unsafe block is not a good thing to do; and suggesting allow(unused_unsafe) is worse.

Imagine the following chain of actions:

1. Someone introduces a macro with an unsafe code.
2. In the use places it gets wrapped with #[allow(unused_unsafe)] unsafe { ... }.
3. Unsafe code is removed from macro.

There is a good chance that this unused_unsafe will remain there unnoticed.

[flip1995]

Yes, I also think this isn't nice. But I don't really see how else to handle this, without rendering the lint completely useless.

[flip1995]

When implementing the lint and noticing, that this is necessary, I also thought about whether we should allow lints that contradict rustc lints at all. We have lints contradicting other Clippy lints in the restriction group. But we don't have lints that contradict rustc lints (yet).

[bors]

☔ The latest upstream changes (presumably #7403) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

I think its design should be thought through a bit more carefully.

I'm also not really satisfied with how the lint currently behaves.

if some library has such a macro already, dependent crates will have no choice but to either get rid of dependency or add a bunch of allow(...), since they can't directly fix the issue.

I don't think this is a problem. I placed this lint in restriction, so that you'll have to enable it explicitly if you want it. If a dependent crate uses this lint and uses an external macro, that hides unsafe operations, the dependent crate probably wants to wrap this macro in an unsafe block, rather than adding allows.

We could just not lint on external macros, but as I just explained, this lint is supposed to restrict what you can and cannot do with macros that hide unsafe code, no matter the origin.

Logically, it's the macro fault that it hides unsafe code, so the lint should be placed on the macro.

With that, I think you would run into the problem with dependent crates. If you would lint a macro definition and tell the user to remove an unsafe block from the macro, all dependent crates using that macro would now have to add an unsafe block around the macro.

We could just not lint exported macros, but I'm not even sure if we're able to lint macro definitions at all, since Clippy is run after macro expansion.

[flip1995]

I'll address all comments and further lint design decisions once we have consensus on how the lint should behave.

[popzxc]

With that, I think you would run into the problem with dependent crates. If you would lint a macro definition and tell the user to remove an unsafe block from the macro, all dependent crates using that macro would now have to add an unsafe block around the macro.

Actually, I think that's the way it should work. If the crate author thinks that unsafe is justified and always valid, they'll just put an allow statement on it and leave it as-is. No problem here.
Otherwise, they'll prepare a semver major release explaining that they discovered a potentially dangerous place, so since now users have to wrap the macro usage in unsafe. It's also not a problem, and even a good thing, since now the API of the dependency crate is more transparent.

[camsteffen]

I see this lint as an extra precaution for macro users. Library authors with macros should not worry about triggering the lint, and they should not allow it. Users of this lint should expect false positives. A macro should have unsafe {} if and only if it can guarantee safety. If the guarantee is good, the lint is a false positive. If the guarantee is bad, the macro is wrong. But we can only lint expanded code. I think we should still lint for external macros.

For the lint suggestion, I'm not sure it makes sense to add an unsafe block to appease this lint when it will be deemed unused by rustc. I think it would be better to just allow the lint? This also can be a lint that you run manually to see what you see and just ignore the false positives. I see this lint as being in conflict with rustc's unsafe rules, in its false positive cases, so there is no "fix" to be made.

[camsteffen]

Another reason to not suggest adding unsafe {} is that it could widen the scope of unsafe {}.

[flip1995]

Thanks for all the input!

What do you think about suggesting to add an unsafe block only for

1. external macros
2. optionally: if the unused_unsafe lint is allowed

But for local macros the correct fix would be to either remove the unsafe block from the macro and put it around every macro call (or the macro argument that makes the unsafe block necessary), OR allow this lint on the macro calls as a marker, that the macro is unsafe. But without producing an auto-applicable suggestion.

[popzxc]

Sounds reasonable to me.

[camsteffen]

For local macros, yes I agree.

For external macros, I guess it is more reasonable to add unsafe {}, but I think allow-this-lint is still the preferred way to suppress because of the conflict with unused_unsafe. Maybe output something like this:

help: suppress this lint by writing `#[allow(macros_hiding_unsafe_code)] { macro_name!(..) }`
help: or write `unsafe { macro_name!(..) }`, but this will trigger the `unused_unsafe` lint

[camsteffen]

I think macros_hiding_unsafe would be a better name. Technically it is looking for an unsafe block, where _unsafe_code kinda makes it sound like macros can just call unsafe functions anywhere.

[flip1995]

For external macros, I guess it is more reasonable to add unsafe {}, but I think allow-this-lint is still the preferred way to suppress because of the conflict with unused_unsafe. Maybe output something like this:

The unused_unsafe lint doesn't trigger for external macros. I've added a test for this here. Do you still think suggesting to just allow this lint in this case makes more sense, than suggesting unsafe code. I'm good with suggesting both alternatives.

I think macros_hiding_unsafe would be a better name. Technically it is looking for an unsafe block, where _unsafe_code kinda makes it sound like macros can just call unsafe functions anywhere.

Ah yes, thanks for the better name! Now I know what irritated me with the name that I chose 😄

[camsteffen]

The unused_unsafe lint doesn't trigger for external macros.

Huh. Isn't that a bug?

[flip1995]

This has been open way to long with no update by me. This is pretty low on my priority list, so I'm closing it for now, maybe I'll get back to it some time in the future.