-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix min_ident_chars
: ignore on trait impl.
#15275
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?
Conversation
Lintcheck changes for b5b8d33
This comment will be updated if you push new changes |
For a local trait, it should warn about the short parameter name in the trait method definition. I don't see a test for this, and I suspect it won't warn. To suppress warnings about trait implementations, we should ensure it warns at the trait definition so that users are warned at least once about the usage of short identifiers. |
Yes, I didn't look at that case, and it seems it never fired the lint even before my PR: #![allow(dead_code)]
#![warn(clippy::min_ident_chars)]
trait Trait {
fn func(x: u32) -> u32;
}
struct Struct;
impl Trait for Struct {
fn func(x: u32) -> u32 {
x
}
}
fn main() {} This code doesn't produce any warnings. For functions with one char, it works though, and it warns on trait definition only, so I guess I should do the same thing for one-char-long params |
My previous message wasn't correct, as Strange thing that the CIs stopped passing on an error in the source file, that I didn't change on this commit |
fe9ea10
to
4641bce
Compare
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.
Could you add a "Limitations" section to the lint definition, after the "Example"? Something like
/// ### Limitations
/// Trait implementations which use the same function or parameter name as the trait declaration will
/// not be warned about, even if the name is below the configured limit.
Also, it would be great to open an issue with label C-enhancement
suggesting to add a new configuration option for this lint so that it triggers when using short parameter names even in the case of trait implementation: while we cannot do anything about the function names themselves, we can suggest renaming the function parameters in the implementation.
r? samueltardieu |
Reminder, once the PR becomes ready for a review, use |
cf. #15365 |
Trait implementations which use the same function or parameter name as the trait declaration will not be warned about, even if the name is below the configured limit.
from trait definition
@rustbot ready |
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 is starting to look good. Could you test the suggested change (and added tests), and tell me what you think about it? Also, you may squash the commits into one, that will be necessary before merging.
for (_, parent_node) in cx.tcx.hir_parent_iter(hir_id) { | ||
if let Node::Item(item) = parent_node | ||
&& let ItemKind::Impl(Impl { of_trait: Some(_), .. }) = &item.kind | ||
{ | ||
return; | ||
} | ||
} |
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.
Won't that test prevent linting of inner functions? For example, if a function in a trait implementation defines a closure using a single letter parameter name, will it still get linted?
Can't you just look at the direct parent?
for (_, parent_node) in cx.tcx.hir_parent_iter(hir_id) { | |
if let Node::Item(item) = parent_node | |
&& let ItemKind::Impl(Impl { of_trait: Some(_), .. }) = &item.kind | |
{ | |
return; | |
} | |
} | |
if matches!(cx.tcx.parent_hir_node(hir_id), Node::TraitRef(_)) { | |
return; | |
} |
Can you also check that inner function and their parameters are checked, even in such a function?
impl D for Issue13396 {
fn f(g: i32) {
fn h() {}
//~^ min_ident_chars
fn inner(a: i32) {}
//~^ min_ident_chars
}
fn long(long: i32) {}
}
fixes #13396
changelog: [
min_ident_chars
]: ignore lint when implementing a trait, to respect [renamed_function_params
]