Add crate_in_macro_def lint
#8576
Conversation
|
r? @llogiq (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
tests/ui/crate_in_macro_def_allow.rs
Outdated
| // For cases where use of `crate` is intentional, applying `allow` to the macro definition | ||
| // should suppress the lint. | ||
| #[allow(clippy::crate_in_macro_def)] |
There was a problem hiding this comment.
This can go in the same test file as above or be removed, as you aren't having to do something specific to get it working
Could be worth noting in the description that the allow attribute has to go on the macro_rules itself, ie
#[allow(clippy::crate_in_macro_def)]
macro_rules! ok { ... crate::foo ... }
macro_rules! lints {
...
// Still errors since the allow attribute is part of the macro token stream
#[allow(clippy::crate_in_macro_def)]
crate::foo
...
}There was a problem hiding this comment.
This can go in the same test file as above or be removed
My concern is that someone uses crate intentionally, they apply allow to the macro definition, and then a future change to Clippy breaks their uses of allow. E.g., it would be annoying to have to use allow at every call site. The test is meant to help prevent this possibility.
| /// ### Example | ||
| /// ```rust | ||
| /// macro_rules! print_message { | ||
| /// () => { | ||
| /// println!("{}", crate::MESSAGE); | ||
| /// }; | ||
| /// } | ||
| /// pub const MESSAGE: &str = "Hello!"; | ||
| /// ``` |
There was a problem hiding this comment.
For internal macros crate is fine, you could check for a macro_export attribute using check_item to avoid some false positives
There was a problem hiding this comment.
I am embarrassed that I didn't think of this. 😬
smoelius
force-pushed
the
crate_in_macro_def
branch
from
1006134
to
cb307bb
Compare
|
Thank you for the review @Alexendoo. I think cb307bb addresses all of your comments. |
| /// ``` | ||
| #[clippy::version = "1.61.0"] | ||
| pub CRATE_IN_MACRO_DEF, | ||
| correctness, |
There was a problem hiding this comment.
I don't think this should be a correctness lint. Correctness implies the lint level is deny. While the lint will be right in the general case, there may be exceptions and I would not want to stop the build for that. My suggestion here would be to make it a suspicious lint (which is warn by default).
Co-authored-by: llogiq <[email protected]>
Co-authored-by: llogiq <[email protected]>
Co-authored-by: llogiq <[email protected]>
|
Thank you! @bors r+ |
|
📌 Commit d6eb82c has been approved by |
|
@llogiq Sorry, sorry. One more change was needed to fix the error message. |
|
@bors r- |
|
@bors r+ |
|
📌 Commit aaf04dc has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This PR adds a lint to check for
crateas opposed to$crateused in a macro definition.I think this can close #4798. That issue focused on the case where the macro author "imports something into said macro."
But I think use of
crateis likely to be a bug whether it appears in ausestatement or not. There could be some use case I am failing to see, though. (cc: @nilscript @flip1995)changelog:
crate_in_macro_def