new lint: `macro_metavars_in_unsafe` #12107

y21 · 2024-01-07T03:38:41Z

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.

Note: this has gone through some major rewrites, so any comment before #12107 (comment) is outdated and was based on an older version that has since been completely rewritten.

changelog: new lint: [macro_metavars_in_unsafe]

rustbot · 2024-01-07T03:38:47Z

r? @giraffate

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

samueltardieu · 2024-01-07T10:57:01Z

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 · 2024-01-07T14:23:51Z

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 · 2024-01-07T15:10:23Z

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 · 2024-01-07T18:54:20Z

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 · 2024-01-13T03:01:33Z

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 · 2024-01-26T13:54:40Z

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

xFrednet · 2024-04-01T09:52:29Z

@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

GuillaumeGomez

Code is good and well documented. Nice work! Looks ready to go for me.

y21 · 2024-04-01T10:56:38Z

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 · 2024-04-06T13:41:21Z

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 :)

xFrednet

Very nice implementation! The structure and docs make the code very understandable. I have several nit-picky comments, but hopefully nothing that will require a lot of work. :D

clippy_lints/src/expr_metavars_in_unsafe.rs

xFrednet · 2024-04-08T18:53:22Z

clippy_lints/src/expr_metavars_in_unsafe.rs

+        if from_expn {
+            self.expn_depth += 1;
+        }


It feels weird that this only checks if the span is from an expansion. If we nested statements from the same macro it would still increase the self.expn_depth counter. This is not a problem directly, but the name of the field might be misleading. Can you add a test for this?

Can you elaborate why this is weird or what you would rename this field to to make it less misleading? Or what exact test case you'd want?

To clarify, It's okay for this to falsely return true and it doesn't matter for correctness if we increment the counter multiple times for the same macro or for unrelated macros, as long as we arrive back at 0 when we leave all expansion spans in that function and that the counter is >0 if there are any enclosing macro spans.
This check is intentionally kept "cheap" because we're likely going to execute that a lot.

To clarify, It's okay for this to falsely return true and it doesn't matter for correctness if we increment the counter multiple times for the same macro or for unrelated macros, as long as we arrive back at 0 when we leave all expansion spans

That's a good point. I can't think of an example that would violate the implementation. Then let's leave it at this :)

clippy_lints/src/expr_metavars_in_unsafe.rs

tests/ui/expr_metavars_in_unsafe.rs

y21 · 2024-04-08T20:11:21Z

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 · 2024-04-25T22:24:19Z

clippy_config/src/conf.rs

+    /// Lint: MACRO_METAVARS_IN_UNSAFE.
+    ///
+    /// Whether to also emit warnings for unsafe blocks with metavariable expansions in **private** macros.
+    (warn_unsafe_macro_metavars_in_private_macros: bool = false),


This is quite lengthy, if someone has a better and more concise name for this, do tell

I like the name, it's lengthy but descriptive 👍

xFrednet

Tiny error message NIT, but the rest looks good to me.

Edit 2: You can r=me after the changed message, if you take one of my suggestions. :D

xFrednet · 2024-05-04T14:18:26Z

clippy_config/src/conf.rs

+    /// Lint: MACRO_METAVARS_IN_UNSAFE.
+    ///
+    /// Whether to also emit warnings for unsafe blocks with metavariable expansions in **private** macros.
+    (warn_unsafe_macro_metavars_in_private_macros: bool = false),


I like the name, it's lengthy but descriptive 👍

tests/ui-toml/macro_metavars_in_unsafe/default/test.stderr

y21 · 2024-05-06T17:35:54Z

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

xFrednet · 2024-05-12T13:43:01Z

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 · 2024-05-12T14:34:34Z

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

y21 · 2024-05-12T15:17:31Z

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 · 2024-05-12T15:17:35Z

📌 Commit 9747c80 has been approved by xFrednet

It is now in the queue for this repository.

new lint: `macro_metavars_in_unsafe` 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. Note: this has gone through some major rewrites, so any comment before #12107 (comment) is outdated and was based on an older version that has since been completely rewritten. changelog: new lint: [`macro_metavars_in_unsafe`]

bors · 2024-05-12T15:18:42Z

⌛ Testing commit 9747c80 with merge 50e1065...

bors · 2024-05-12T15:27:57Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 50e1065 to master...

bors · 2024-05-12T15:28:56Z

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

xFrednet · 2024-05-12T16:03:51Z

The merge conflict had perfect timing xD

@bors retry

new lint: `macro_metavars_in_unsafe` 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. Note: this has gone through some major rewrites, so any comment before #12107 (comment) is outdated and was based on an older version that has since been completely rewritten. changelog: new lint: [`macro_metavars_in_unsafe`]

bors · 2024-05-12T16:04:58Z

⌛ Testing commit 9747c80 with merge 323eca6...

bors · 2024-05-12T16:12:46Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 323eca6 to master...

bors · 2024-05-12T16:13:44Z

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

Resolved

xFrednet · 2024-05-12T16:31:35Z

@bors r+

bors · 2024-05-12T16:31:37Z

💡 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 · 2024-05-12T16:31:37Z

📌 Commit 9747c80 has been approved by xFrednet

It is now in the queue for this repository.

bors · 2024-05-12T16:32:43Z

⌛ Testing commit 9747c80 with merge a4a1a73...

bors · 2024-05-12T16:41:24Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing a4a1a73 to master...

rustbot assigned giraffate Jan 7, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 7, 2024

y21 force-pushed the expr_metavars_in_unsafe branch from 067e21f to 490b25a Compare January 7, 2024 03:47

y21 marked this pull request as draft January 7, 2024 18:52

y21 force-pushed the expr_metavars_in_unsafe branch from 651307c to 7d36300 Compare January 13, 2024 02:43

rustbot assigned xFrednet and unassigned giraffate Apr 1, 2024

GuillaumeGomez approved these changes Apr 1, 2024

View reviewed changes

xFrednet reviewed Apr 8, 2024

View reviewed changes

y21 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 8, 2024

y21 force-pushed the expr_metavars_in_unsafe branch 2 times, most recently from 55ffbed to ad424da Compare April 25, 2024 22:06

y21 marked this pull request as ready for review April 25, 2024 22:16

y21 commented Apr 25, 2024

View reviewed changes

y21 changed the title ~~new lint: expr_metavars_in_unsafe~~ new lint: macro_metavars_in_unsafe Apr 25, 2024

xFrednet approved these changes May 4, 2024

View reviewed changes

y21 force-pushed the expr_metavars_in_unsafe branch from ad424da to 064aa5a Compare May 6, 2024 17:25

xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) labels May 12, 2024

y21 force-pushed the expr_metavars_in_unsafe branch from 064aa5a to aa54572 Compare May 12, 2024 15:02

new lint: macro_metavars_in_unsafe

9747c80

y21 force-pushed the expr_metavars_in_unsafe branch from aa54572 to 9747c80 Compare May 12, 2024 15:03

xFrednet approved these changes May 12, 2024

View reviewed changes

bors merged commit a4a1a73 into rust-lang:master May 12, 2024
8 checks passed

This was referenced May 12, 2024

new lint: zombie_processes #11476

Merged

Add lint which checks that duration conversion aren't losing precision #12539

Open

New lint: [duplicate_map_keys] #12575

Closed

y21 mentioned this pull request May 13, 2024

improve [match_same_arms] messages, enable rustfix test #12794

Merged

new lint: macro_metavars_in_unsafe #12107

new lint: macro_metavars_in_unsafe #12107

Conversation

y21 commented Jan 7, 2024 • edited Loading

rustbot commented Jan 7, 2024

samueltardieu commented Jan 7, 2024

Reports

Stats:

y21 commented Jan 7, 2024

poy3

bitvec

static_assertions

y21 commented Jan 7, 2024

y21 commented Jan 7, 2024 • edited Loading

y21 commented Jan 13, 2024

bors commented Jan 26, 2024

xFrednet commented Apr 1, 2024

GuillaumeGomez left a comment

Choose a reason for hiding this comment

y21 commented Apr 1, 2024 • edited Loading

xFrednet commented Apr 6, 2024

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Apr 8, 2024

Choose a reason for hiding this comment

y21 Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

xFrednet May 4, 2024

Choose a reason for hiding this comment

y21 commented Apr 8, 2024

y21 Apr 25, 2024

Choose a reason for hiding this comment

xFrednet May 4, 2024

Choose a reason for hiding this comment

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

xFrednet May 4, 2024

Choose a reason for hiding this comment

y21 commented May 6, 2024

xFrednet commented May 12, 2024

bors commented May 12, 2024

y21 commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

xFrednet commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

xFrednet commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

new lint: `macro_metavars_in_unsafe` #12107

new lint: `macro_metavars_in_unsafe` #12107

y21 commented Jan 7, 2024 •

edited

Loading

y21 commented Jan 7, 2024 •

edited

Loading

y21 commented Apr 1, 2024 •

edited

Loading

y21 Apr 25, 2024 •

edited

Loading

xFrednet left a comment •

edited

Loading