new lint: `macro_metavars_in_unsafe`

[y21]

This implements a lint that I've been meaning to write for a while: a macro with an expr metavariable that is then expanded in an unsafe context. It's bad because it lets the user write unsafe code without an unsafe block.

changelog: new lint: [macro_metavars_in_unsafe]

[rustbot]

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

[samueltardieu]

I have run your lint against the top 500 crates. Here are the results, I haven't analyzed it, but it might help you check whether it does what you intend it to.

Reports

target/lintcheck/sources/bitvec-1.0.1/src/macros.rs:219:27 clippy::expr_metavars_in_unsafe "expanding an expression metavariable in an unsafe block" target/lintcheck/sources/bitvec-1.0.1/src/macros.rs:244:34 clippy::expr_metavars_in_unsafe "expanding an expression metavariable in an unsafe block" target/lintcheck/sources/bitvec-1.0.1/src/macros.rs:249:34 clippy::expr_metavars_in_unsafe "expanding an expression metavariable in an unsafe block" target/lintcheck/sources/pyo3-0.20.2/src/types/mod.rs:185:49 clippy::expr_metavars_in_unsafe "expanding an expression metavariable in an unsafe block" target/lintcheck/sources/static_assertions-1.1.0/src/assert_eq_size.rs:81:38 clippy::expr_metavars_in_unsafe "expanding an expression metavariable in an unsafe block" target/lintcheck/sources/static_assertions-1.1.0/src/assert_eq_size.rs:82:62 clippy::expr_metavars_in_unsafe "expanding an expression metavariable in an unsafe block"

Stats:

lint	count
clippy::expr_metavars_in_unsafe	6

[y21]

Thanks! My thoughts on the results:

poy3

The pyo3 case looks like a real case to me. pyo3::pyobject_native_static_type_object!((*std::ptr::null_mut::<u8>()))(()); invokes undefined behavior with no unsafe block in sight.

bitvec

Looks like a false positive. This is basically:

macro_rules! x {
  ($v:expr) => {
    $v; unsafe { $v; };
  }
}

It's technically expanding $v in an unsafe block, but it's still not possible to write unsafe without an unsafe block at callsite because it's expanded twice. The first use of $v is outside of an unsafe block. I think this is fixable.

static_assertions

The two static_assertions cases I already saw and mentioned in my description. I suppose those are technically FPs because they're in a closure that is never called, thus never reached, so it doesn't matter what kinds of unsafety is done, but I don't think this is something we can detect. I want to say that if this is an uncommon case, it could be something that can be #[allow]ed, and they already do have multiple #[allow]s above it.

[y21]

pyo3::pyobject_native_static_type_object!((*std::ptr::null_mut::<u8>()))(());

Oh sorry this specifically is not UB (anymore?). pyo3's macro is using addr_of_mut!, which creates a place. I'm pretty sure that the documentation here was updated, it used to say that even dereferencing a null pointer in addr_of_mut! is undefined behavior, but not anymore. Anyways, there are still other ways to create UB with this macro

But I also completely missed that the macro is annotated with #[doc(hidden)], so this is probably also a FP then, since they're not considered to be part of the public API

[y21]

Fixed those 2 FPs. The more logic I add to this, though, I feel like this isn't a good idea, since this is effectively implementing an unsafety checker at the parser level... I think I'm going to experiment with different ways to do this. Maybe it's possible to use the rustc_expand crate. Another way I can think of is, instead of looking at the macro definition, look at the expansions of local macros. The upside is that we have an actual AST/HIR to work with.

[y21]

Finally got around to rewriting this by having it look at local macro expansions, rather than trying to parse the macro definition tokenstream. It did make the code significantly simpler, but has a few downsides:

• ideally we would tell the user the metavariable that was expanded in an unsafe block, but there is (afaik) no way to get the span of the $ident in the macro definition, given its expansion. I tried many hacks (such as getting the parent of the $ident expansion, match on the node kind and retokenize everything in between to find something that looks like $ident but there are cases where it doesn't work)
  • this currently works around that limitation by mentioning the unsafe block instead
• as a consequence, we have no way of checking that the metavariable is actually an :expr metavariable
  • making this work probably requires parsing some of the macro tokenstream? unless there's something in the rustc crates
• this won't catch macros that are never invoked locally, which can be seen by lintcheck (I'm getting 0 hits now). In practice however I think the fact that crates usually have tests for macros in the same crate makes this less bad.
• since a macro can expand the same metavariable practically anywhere (even across multiple functions), we need to keep track of spans so that we can undo warnings
  • we really only need spans from macros, so this cuts the number of spans that need to be stored quite a bit

... so for now I put this in the nursery category. I'm not sure which one of these can be worked around or improved. Will think about it a bit more.

[bors]

:umbrella: The latest upstream changes (presumably #12160) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

@GuillaumeGomez would you mind taking a look at this PR?

It has some comments, which you probably first need to catch up on.

r? xFrednet

[y21]

Yeah the PR history is a bit of a mess, it went through a few rewrites :D Though half the comments are no longer really relevant.

It started by inspecting/"parsing" the TokenStream of a macro definition to find unsafe blocks and $exprs (since it has no AST) which very quickly got out of hand when trying to handle edge cases, so instead of doing that we simply look for expanded expressions in the HIR.

I think the main "blocker" more or less is that this currently does not check that the expanded metavariable is an expression, contrary to the lint name:

macro_rules! m {
  ($v:literal) => { unsafe { $v } }
}
m!(1)

This is linted even though $v:literal.

But... maybe it's fine and we could just rename the lint to not mention "expr_metavars"? What about something like macro_metavars_in_unsafe?

[xFrednet]

But... maybe it's fine and we could just rename the lint to not mention "expr_metavars"? What about something like macro_metavars_in_unsafe?

This sounds good to me. Then it'll also catch statements and other tokens :)

[y21]

Thanks for reviewing, I'll address the comments eventually, but I will be busy for a couple more weeks, so it might take a while.

[y21]

I created an FCP thread on zulip for this. Will squash when that's complete.

[xFrednet]

Right, I forgot about the FCP. Thank you for remembering :heart:

[xFrednet]

Since no comments have come up during the FCP it should be safe to merge this now.

@y21 can you resolve the last comment regarding the version? Then you can r=me =^.^=

[bors]

:umbrella: The latest upstream changes (presumably #12620) made this pull request unmergeable. Please resolve the merge conflicts.

[y21]

Changed the version atribute to 1.80 and rebased to fix conflicts + squashed. (had to force push twice since the first commit still had the old lint name in its commit message which was slightly misleading)

@bors r=xFrednet

[bors]

:pushpin: Commit 9747c80644fc9f705029c254ca1d14668a72f904 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 9747c80644fc9f705029c254ca1d14668a72f904 with merge 50e1065aed5e90300433af654acdb3334962bdc9...

[bors]

:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: xFrednet Pushing 50e1065aed5e90300433af654acdb3334962bdc9 to master...

[bors]

:eyes: Test was successful, but fast-forwarding failed: 422 1 review requesting changes and 1 approving review by reviewers with write access.

[xFrednet]

The merge conflict had perfect timing xD

@bors retry

[bors]

:hourglass: Testing commit 9747c80644fc9f705029c254ca1d14668a72f904 with merge 323eca63579bef4a09ad9c990256a7fab588e061...

[bors]

:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: xFrednet Pushing 323eca63579bef4a09ad9c990256a7fab588e061 to master...

[bors]

:eyes: Test was successful, but fast-forwarding failed: 422 1 review requesting changes and 1 approving review by reviewers with write access.

[xFrednet]

@bors r+

[bors]

:bulb: This pull request was already approved, no need to approve it again.

• This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

[bors]

:pushpin: Commit 9747c80644fc9f705029c254ca1d14668a72f904 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 9747c80644fc9f705029c254ca1d14668a72f904 with merge a4a1a7365d60280b83d83374fda97742fb959239...

[bors]

:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: xFrednet Pushing a4a1a7365d60280b83d83374fda97742fb959239 to master...