-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add relative_path_in_macro_definition lint (#14472) #14645 #14648
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
base: master
Are you sure you want to change the base?
Changes from all commits
5c986ca
5e12c33
43b3185
851bf60
f73762a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,93 @@ | ||||||||||||||
use clippy_utils::diagnostics::span_lint; | ||||||||||||||
use rustc_ast::ast::{Item, ItemKind}; | ||||||||||||||
use rustc_ast::token::TokenKind; | ||||||||||||||
use rustc_ast::tokenstream::{TokenStream, TokenTree}; | ||||||||||||||
use rustc_lint::{EarlyContext, EarlyLintPass}; | ||||||||||||||
use rustc_session::declare_lint_pass; | ||||||||||||||
use rustc_span::symbol::sym; | ||||||||||||||
|
||||||||||||||
declare_clippy_lint! { | ||||||||||||||
/// ### What it does | ||||||||||||||
/// Checks that references to the `core` or `kernel` crate in macro definitions use absolute paths (`::core` or `::kernel`). | ||||||||||||||
/// | ||||||||||||||
/// ### Why is this bad? | ||||||||||||||
/// Using relative paths (e.g., `core::...`) in macros can lead to ambiguity if the macro is used in a context | ||||||||||||||
/// where a user defines a module named `core` or `kernel`. Absolute paths ensure the macro always refers to the intended crate. | ||||||||||||||
/// | ||||||||||||||
/// ### Example | ||||||||||||||
/// ```rust | ||||||||||||||
/// // Bad | ||||||||||||||
/// macro_rules! my_macro { | ||||||||||||||
/// () => { | ||||||||||||||
/// core::mem::drop(0); | ||||||||||||||
/// }; | ||||||||||||||
/// } | ||||||||||||||
/// | ||||||||||||||
/// // Good | ||||||||||||||
/// macro_rules! my_macro { | ||||||||||||||
/// () => { | ||||||||||||||
/// ::core::mem::drop(0); | ||||||||||||||
/// }; | ||||||||||||||
/// } | ||||||||||||||
/// ``` | ||||||||||||||
#[clippy::version = "1.88.0"] | ||||||||||||||
pub RELATIVE_PATH_IN_MACRO_DEFINITION, | ||||||||||||||
correctness, | ||||||||||||||
"using relative paths in declarative macros can lead to context-dependent behavior" | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
declare_lint_pass!(RelativePathInMacroDefinition => [RELATIVE_PATH_IN_MACRO_DEFINITION]); | ||||||||||||||
|
||||||||||||||
impl EarlyLintPass for RelativePathInMacroDefinition { | ||||||||||||||
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { | ||||||||||||||
if let ItemKind::MacroDef(_, macro_def) = &item.kind { | ||||||||||||||
check_token_stream(cx, ¯o_def.body.tokens); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
fn check_token_stream(cx: &EarlyContext<'_>, tokens: &TokenStream) { | ||||||||||||||
let mut iter = tokens.iter().peekable(); | ||||||||||||||
let mut prev_token: Option<&TokenTree> = None; | ||||||||||||||
|
||||||||||||||
while let Some(tree) = iter.next() { | ||||||||||||||
match tree { | ||||||||||||||
TokenTree::Token(token, _) => { | ||||||||||||||
if let TokenKind::Ident(ident, _) = token.kind | ||||||||||||||
&& (ident == sym::core || ident.as_str() == "kernel") | ||||||||||||||
{ | ||||||||||||||
let is_path_start = iter.peek().is_some_and(|next_tree| { | ||||||||||||||
if let TokenTree::Token(next_token, _) = next_tree { | ||||||||||||||
next_token.kind == TokenKind::PathSep | ||||||||||||||
} else { | ||||||||||||||
false | ||||||||||||||
} | ||||||||||||||
}); | ||||||||||||||
|
||||||||||||||
if is_path_start { | ||||||||||||||
let is_absolute = prev_token.is_some_and(|prev| { | ||||||||||||||
if let TokenTree::Token(prev_token, _) = prev { | ||||||||||||||
prev_token.kind == TokenKind::PathSep | ||||||||||||||
} else { | ||||||||||||||
false | ||||||||||||||
} | ||||||||||||||
Comment on lines
+69
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thx, I'll change it in v2. |
||||||||||||||
}); | ||||||||||||||
|
||||||||||||||
if !is_absolute { | ||||||||||||||
span_lint( | ||||||||||||||
cx, | ||||||||||||||
RELATIVE_PATH_IN_MACRO_DEFINITION, | ||||||||||||||
token.span, | ||||||||||||||
"relative path to `core` or `kernel` used in macro definition", | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we should reuse the first element of the path instead of writing both options. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thx, I'll change it in v2. |
||||||||||||||
); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
}, | ||||||||||||||
TokenTree::Delimited(_open_span, _close_span, _delim, token_stream) => { | ||||||||||||||
check_token_stream(cx, token_stream); | ||||||||||||||
}, | ||||||||||||||
} | ||||||||||||||
prev_token = Some(tree); | ||||||||||||||
} | ||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need
ident.as_str()
. Look atclippy_utils::sym
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your feedback and suggestion to avoid ident.as_str() and refer to clippy_utils::sym. I've encountered a compilation error with the current implementation:
error[E0425]: cannot find value
kernel
in modulesym
--> clippy_lints/src/relative_path_in_macro_definition.rs:57:61
|
57 | && (ident == sym::core || ident == sym::kernel)
| ^^^^^^ not found in
sym
The issue arises because rustc_span::symbol::sym does not define a kernel symbol, unlike core. Upon reviewing clippy_utils::sym, I found it to be a macro that wraps Symbol::intern, used for creating symbols from strings, rather than providing predefined symbols like core.
To address this, I propose using Symbol::intern("kernel") to create the kernel symbol dynamically, allowing direct comparison with ident (of type rustc_span::Ident). The updated code snippet is:
if ident == sym::core || ident == Symbol::intern("kernel") {
// ... rest of the lint logic ...
}
This approach avoids ident.as_str() as you recommended, using direct symbol comparisons for efficiency. I've tested this change locally, and it resolves the compilation error while maintaining the lint's functionality to check for relative paths to core and kernel in macro definitions.
Could you please confirm if using Symbol::intern("kernel") is acceptable for this lint, or if there's a preferred method within Clippy's codebase (e.g., using clippy_utils::sym! macro or another approach)? Thank you for your guidance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can simply add "kernel" into that
generate!
macro call, then you can use clippy_utils::sym::kernel in your code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the definition of "sym::core" is in
https://github.com/rust-lang/rust/blob/master/compiler/rustc_span/src/symbol.rs.
But according to the discussion below, we no longer need to add the extra check. Anyway, thanks for the reply.