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

Add crate_in_macro_def lint #8576

Merged
merged 8 commits into from
Mar 30, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3097,6 +3097,7 @@ Released 2018-09-13
[`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
[`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
[`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator
[`crate_in_macro_def`]: https://rust-lang.github.io/rust-clippy/master/index.html#crate_in_macro_def
[`create_dir`]: https://rust-lang.github.io/rust-clippy/master/index.html#create_dir
[`crosspointer_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#crosspointer_transmute
[`dbg_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro
Expand Down
100 changes: 100 additions & 0 deletions clippy_lints/src/crate_in_macro_def.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use rustc_ast::ast::MacroDef;
use rustc_ast::node_id::NodeId;
use rustc_ast::token::{Token, TokenKind};
use rustc_ast::tokenstream::{TokenStream, TokenTree};
use rustc_errors::Applicability;
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::Span;

declare_clippy_lint! {
/// ### What it does
/// Checks for use of `crate` as opposed to `$crate` in a macro definition.
///
/// ### Why is this bad?
/// `crate` refers to the macro call's crate, whereas `$crate` refers to the macro
/// definition's crate. Rarely is the former intended. See:
/// https://doc.rust-lang.org/reference/macros-by-example.html#hygiene
///
/// ### Example
/// ```rust
/// macro_rules! print_message {
/// () => {
/// println!("{}", crate::MESSAGE);
/// };
/// }
/// pub const MESSAGE: &str = "Hello!";
/// ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For internal macros crate is fine, you could check for a macro_export attribute using check_item to avoid some false positives

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am embarrassed that I didn't think of this. 😬

/// Use instead:
/// ```rust
/// macro_rules! print_message {
/// () => {
/// println!("{}", $crate::MESSAGE);
/// };
/// }
/// pub const MESSAGE: &str = "Hello!";
/// ```
#[clippy::version = "1.61.0"]
pub CRATE_IN_MACRO_DEF,
correctness,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"using `crate` in a macro definition"
}
declare_lint_pass!(CrateInMacroDef => [CRATE_IN_MACRO_DEF]);

impl EarlyLintPass for CrateInMacroDef {
fn check_mac_def(&mut self, cx: &EarlyContext<'_>, macro_def: &MacroDef, _: NodeId) {
let tts = macro_def.body.inner_tokens();
if let Some(span) = contains_unhygienic_crate_reference(&tts) {
span_lint_and_sugg(
cx,
CRATE_IN_MACRO_DEF,
span,
"reference to the macro call's crate, which is rarely intended",
"if reference to the macro definition's crate is intended, use",
String::from("$crate"),
Applicability::MachineApplicable,
);
}
}
}

fn contains_unhygienic_crate_reference(tts: &TokenStream) -> Option<Span> {
let mut prev_is_dollar = false;
let mut cursor = tts.trees();
while let Some(curr) = cursor.next() {
if_chain! {
if !prev_is_dollar;
if let Some(span) = is_crate_keyword(&curr);
if let Some(next) = cursor.look_ahead(0);
if is_token(next, &TokenKind::ModSep);
then {
return Some(span);
}
}
if let TokenTree::Delimited(_, _, tts) = &curr {
let span = contains_unhygienic_crate_reference(tts);
if span.is_some() {
return span;
}
}
prev_is_dollar = is_token(&curr, &TokenKind::Dollar);
}
None
}

fn is_crate_keyword(tt: &TokenTree) -> Option<Span> {
if_chain! {
if let TokenTree::Token(Token { kind: TokenKind::Ident(symbol, _), span }) = tt;
if symbol.as_str() == "crate";
then { Some(*span) } else { None }
}
}

fn is_token(tt: &TokenTree, kind: &TokenKind) -> bool {
if let TokenTree::Token(Token { kind: other, .. }) = tt {
kind == other
} else {
false
}
}
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
LintId::of(comparison_chain::COMPARISON_CHAIN),
LintId::of(copies::IFS_SAME_COND),
LintId::of(copies::IF_SAME_THEN_ELSE),
LintId::of(crate_in_macro_def::CRATE_IN_MACRO_DEF),
LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT),
LintId::of(dereference::NEEDLESS_BORROW),
LintId::of(derivable_impls::DERIVABLE_IMPLS),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_correctness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
LintId::of(casts::CAST_SLICE_DIFFERENT_SIZES),
LintId::of(copies::IFS_SAME_COND),
LintId::of(copies::IF_SAME_THEN_ELSE),
LintId::of(crate_in_macro_def::CRATE_IN_MACRO_DEF),
LintId::of(derive::DERIVE_HASH_XOR_EQ),
LintId::of(derive::DERIVE_ORD_XOR_PARTIAL_ORD),
LintId::of(drop_forget_ref::DROP_COPY),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ store.register_lints(&[
copies::IF_SAME_THEN_ELSE,
copies::SAME_FUNCTIONS_IN_IF_CONDITION,
copy_iterator::COPY_ITERATOR,
crate_in_macro_def::CRATE_IN_MACRO_DEF,
create_dir::CREATE_DIR,
dbg_macro::DBG_MACRO,
default::DEFAULT_TRAIT_ACCESS,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ mod collapsible_match;
mod comparison_chain;
mod copies;
mod copy_iterator;
mod crate_in_macro_def;
mod create_dir;
mod dbg_macro;
mod default;
Expand Down Expand Up @@ -867,6 +868,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
ignore_publish: cargo_ignore_publish,
})
});
store.register_early_pass(|| Box::new(crate_in_macro_def::CrateInMacroDef));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ macro_rules! define_Conf {

#[cfg(feature = "internal")]
pub mod metadata {
use crate::utils::internal_lints::metadata_collector::ClippyConfiguration;
use $crate::utils::internal_lints::metadata_collector::ClippyConfiguration;

macro_rules! wrap_option {
() => (None);
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/crate_in_macro_def.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// run-rustfix
#![warn(clippy::crate_in_macro_def)]

#[macro_use]
mod hygienic {
macro_rules! print_message_hygienic {
() => {
println!("{}", $crate::hygienic::MESSAGE);
};
}

pub const MESSAGE: &str = "Hello!";
}

#[macro_use]
mod unhygienic {
macro_rules! print_message_unhygienic {
() => {
println!("{}", $crate::unhygienic::MESSAGE);
};
}

pub const MESSAGE: &str = "Hello!";
}

fn main() {
print_message_hygienic!();
print_message_unhygienic!();
}
29 changes: 29 additions & 0 deletions tests/ui/crate_in_macro_def.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// run-rustfix
#![warn(clippy::crate_in_macro_def)]

#[macro_use]
mod hygienic {
macro_rules! print_message_hygienic {
() => {
println!("{}", $crate::hygienic::MESSAGE);
};
}

pub const MESSAGE: &str = "Hello!";
}

#[macro_use]
mod unhygienic {
macro_rules! print_message_unhygienic {
() => {
println!("{}", crate::unhygienic::MESSAGE);
};
}

pub const MESSAGE: &str = "Hello!";
}

fn main() {
print_message_hygienic!();
print_message_unhygienic!();
}
14 changes: 14 additions & 0 deletions tests/ui/crate_in_macro_def.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: reference to the macro call's crate, which is rarely intended
--> $DIR/crate_in_macro_def.rs:19:28
|
LL | println!("{}", crate::unhygienic::MESSAGE);
| ^^^^^
|
= note: `-D clippy::crate-in-macro-def` implied by `-D warnings`
help: if reference to the macro definition's crate is intended, use
|
LL | println!("{}", $crate::unhygienic::MESSAGE);
| ~~~~~~

error: aborting due to previous error

19 changes: 19 additions & 0 deletions tests/ui/crate_in_macro_def_allow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![warn(clippy::crate_in_macro_def)]

#[macro_use]
mod intentional {
// For cases where use of `crate` is intentional, applying `allow` to the macro definition
// should suppress the lint.
#[allow(clippy::crate_in_macro_def)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

macro_rules! print_message {
() => {
println!("{}", crate::CALLER_PROVIDED_MESSAGE);
};
}
}

fn main() {
print_message!();
}

pub const CALLER_PROVIDED_MESSAGE: &str = "Hello!";
Empty file.