-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 new useless_concat
lint
#13829
base: master
Are you sure you want to change the base?
Add new useless_concat
lint
#13829
Conversation
a627a09
to
4918abc
Compare
4918abc
to
b92d91b
Compare
Finally was able to only trigger the lint when appropriate. |
r? clippy |
// Get the direct parent of the expression. | ||
&& let Some(macro_call) = macro_backtrace(expr.span).next() | ||
// Check if the `concat` macro from the `core` library. | ||
&& match_def_path(cx, macro_call.def_id, &["core", "macros", "builtin", "concat"]) |
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.
Nit: can you move this path to clippy_utils::paths
? Better to have the hardcoded paths in one place so we can every once in a while go through them and make them diagnostic items
/// ``` | ||
#[clippy::version = "1.85.0"] | ||
pub USELESS_CONCAT, | ||
suspicious, |
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.
suspicious, | |
complexity, |
Since it just simplifies an expression and concat!
isn't wrong to use, complexity feels more correct
if literal.is_some() { | ||
return; | ||
} | ||
literal = Some(&original_code[current_pos..current_pos + token.len as usize]); |
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.
concat!(1)
currently has 1
as the suggestion but it needs to be "1"
with quotes to preserve the right type
{ | ||
let mut literal = None; | ||
let mut current_pos = 0; | ||
let mut cursor = Cursor::new(&original_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.
Nit: can this use tokenize_with_text
? It also gives access to the string, so that feels like it could simplify some parts like keeping track of positions
Fixes #13793.
Interestingly enough, to actually check that the macro call has at least two arguments, we need to use the rust lexer after getting the original source code snippet.
changelog: Add new
useless_concat
lint