-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add new multiple_bound_locations
lint
#12259
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
Changes from all commits
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,84 @@ | ||
use rustc_ast::visit::FnKind; | ||
use rustc_ast::{NodeId, WherePredicate}; | ||
use rustc_data_structures::fx::FxHashMap; | ||
use rustc_lint::{EarlyContext, EarlyLintPass}; | ||
use rustc_session::declare_lint_pass; | ||
use rustc_span::Span; | ||
|
||
use clippy_utils::diagnostics::span_lint; | ||
use clippy_utils::source::snippet_opt; | ||
|
||
declare_clippy_lint! { | ||
/// ### What it does | ||
/// Check if a generic is defined both in the bound predicate and in the `where` clause. | ||
/// | ||
/// ### Why is this bad? | ||
/// It can be confusing for developers when seeing bounds for a generic in multiple places. | ||
/// | ||
/// ### Example | ||
/// ```no_run | ||
/// fn ty<F: std::fmt::Debug>(a: F) | ||
/// where | ||
/// F: Sized, | ||
/// {} | ||
/// ``` | ||
/// Use instead: | ||
/// ```no_run | ||
/// fn ty<F>(a: F) | ||
/// where | ||
/// F: Sized + std::fmt::Debug, | ||
/// {} | ||
/// ``` | ||
#[clippy::version = "1.77.0"] | ||
pub MULTIPLE_BOUND_LOCATIONS, | ||
suspicious, | ||
"defining generic bounds in multiple locations" | ||
} | ||
|
||
declare_lint_pass!(MultipleBoundLocations => [MULTIPLE_BOUND_LOCATIONS]); | ||
|
||
impl EarlyLintPass for MultipleBoundLocations { | ||
fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, _: Span, _: NodeId) { | ||
if let FnKind::Fn(_, _, _, _, generics, _) = kind | ||
&& !generics.params.is_empty() | ||
&& !generics.where_clause.predicates.is_empty() | ||
{ | ||
let mut generic_params_with_bounds = FxHashMap::default(); | ||
|
||
for param in &generics.params { | ||
if !param.bounds.is_empty() { | ||
generic_params_with_bounds.insert(param.ident.name.as_str(), param.ident.span); | ||
} | ||
} | ||
for clause in &generics.where_clause.predicates { | ||
match clause { | ||
WherePredicate::BoundPredicate(pred) => { | ||
if (!pred.bound_generic_params.is_empty() || !pred.bounds.is_empty()) | ||
&& let Some(name) = snippet_opt(cx, pred.bounded_ty.span) | ||
&& let Some(bound_span) = generic_params_with_bounds.get(name.as_str()) | ||
{ | ||
emit_lint(cx, *bound_span, pred.bounded_ty.span); | ||
} | ||
}, | ||
WherePredicate::RegionPredicate(pred) => { | ||
if !pred.bounds.is_empty() | ||
&& let Some(bound_span) = generic_params_with_bounds.get(&pred.lifetime.ident.name.as_str()) | ||
{ | ||
emit_lint(cx, *bound_span, pred.lifetime.ident.span); | ||
} | ||
}, | ||
WherePredicate::EqPredicate(_) => {}, | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
fn emit_lint(cx: &EarlyContext<'_>, bound_span: Span, where_span: Span) { | ||
span_lint( | ||
cx, | ||
MULTIPLE_BOUND_LOCATIONS, | ||
vec![bound_span, where_span], | ||
"bound is defined in more than one place", | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
#![warn(clippy::multiple_bound_locations)] | ||
|
||
fn ty<F: std::fmt::Debug>(a: F) | ||
//~^ ERROR: bound is defined in more than one place | ||
where | ||
F: Sized, | ||
{ | ||
} | ||
|
||
fn lifetime<'a, 'b: 'a, 'c>(a: &'b str, b: &'a str, c: &'c str) | ||
//~^ ERROR: bound is defined in more than one place | ||
where | ||
'b: 'c, | ||
{ | ||
} | ||
|
||
fn ty_pred<F: Sized>() | ||
//~^ ERROR: bound is defined in more than one place | ||
where | ||
for<'a> F: Send + 'a, | ||
{ | ||
} | ||
|
||
struct B; | ||
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. What if B had generics, say one type and one lifetime, just to mix it up a bit? 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. Not sure to see what you want to test here. ^^' 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. Say what if you had 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. It won't emit the lint but I can add a check for it if you want? 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. Added it. |
||
|
||
impl B { | ||
fn ty<F: std::fmt::Debug>(a: F) | ||
//~^ ERROR: bound is defined in more than one place | ||
where | ||
F: Sized, | ||
{ | ||
} | ||
|
||
fn lifetime<'a, 'b: 'a, 'c>(a: &'b str, b: &'a str, c: &'c str) | ||
//~^ ERROR: bound is defined in more than one place | ||
where | ||
'b: 'c, | ||
{ | ||
} | ||
|
||
fn ty_pred<F: Sized>() | ||
//~^ ERROR: bound is defined in more than one place | ||
where | ||
for<'a> F: Send + 'a, | ||
{ | ||
} | ||
} | ||
|
||
struct C<F>(F); | ||
|
||
impl<F> C<F> { | ||
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. What if we had another bound on F here? Also: Should that even lint? Perhaps it's useful to have a secondary bound for just one method of the impl? 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. I have no opinion about this, hence why I didn't add checks on impl blocks. So up to you. :) |
||
fn foo(_f: F) -> Self | ||
where | ||
F: std::fmt::Display, | ||
{ | ||
todo!() | ||
} | ||
} | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
error: bound is defined in more than one place | ||
--> tests/ui/multiple_bound_locations.rs:3:7 | ||
| | ||
LL | fn ty<F: std::fmt::Debug>(a: F) | ||
| ^ | ||
... | ||
LL | F: Sized, | ||
| ^ | ||
| | ||
= note: `-D clippy::multiple-bound-locations` implied by `-D warnings` | ||
= help: to override `-D warnings` add `#[allow(clippy::multiple_bound_locations)]` | ||
|
||
error: bound is defined in more than one place | ||
--> tests/ui/multiple_bound_locations.rs:10:17 | ||
| | ||
LL | fn lifetime<'a, 'b: 'a, 'c>(a: &'b str, b: &'a str, c: &'c str) | ||
| ^^ | ||
... | ||
LL | 'b: 'c, | ||
| ^^ | ||
|
||
error: bound is defined in more than one place | ||
--> tests/ui/multiple_bound_locations.rs:17:12 | ||
| | ||
LL | fn ty_pred<F: Sized>() | ||
| ^ | ||
... | ||
LL | for<'a> F: Send + 'a, | ||
| ^ | ||
|
||
error: bound is defined in more than one place | ||
--> tests/ui/multiple_bound_locations.rs:27:11 | ||
| | ||
LL | fn ty<F: std::fmt::Debug>(a: F) | ||
| ^ | ||
... | ||
LL | F: Sized, | ||
| ^ | ||
|
||
error: bound is defined in more than one place | ||
--> tests/ui/multiple_bound_locations.rs:34:21 | ||
| | ||
LL | fn lifetime<'a, 'b: 'a, 'c>(a: &'b str, b: &'a str, c: &'c str) | ||
| ^^ | ||
... | ||
LL | 'b: 'c, | ||
| ^^ | ||
|
||
error: bound is defined in more than one place | ||
--> tests/ui/multiple_bound_locations.rs:41:16 | ||
| | ||
LL | fn ty_pred<F: Sized>() | ||
| ^ | ||
... | ||
LL | for<'a> F: Send + 'a, | ||
| ^ | ||
|
||
error: aborting due to 6 previous errors | ||
|
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.
I'd also like to see tests for the various possible combinations of generic impls with generic methods.