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

Respect MSRV (MAYBE_BOUND_IN_WHERE) #12388

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Expand Up @@ -634,6 +634,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
* [`match_like_matches_macro`](https://rust-lang.github.io/rust-clippy/master/index.html#match_like_matches_macro)
* [`mem_replace_with_default`](https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default)
* [`missing_const_for_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn)
* [`multiple_bound_locations`](https://rust-lang.github.io/rust-clippy/master/index.html#multiple_bound_locations)
* [`needless_borrow`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow)
* [`option_as_ref_deref`](https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref)
* [`option_map_unwrap_or`](https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or)
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/declared_lints.rs
Expand Up @@ -502,7 +502,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::module_style::MOD_MODULE_FILES_INFO,
crate::module_style::SELF_NAMED_MODULE_FILES_INFO,
crate::multi_assignments::MULTI_ASSIGNMENTS_INFO,
crate::multiple_bound_locations::MULTIPLE_BOUND_LOCATIONS_INFO,
crate::multiple_unsafe_ops_per_block::MULTIPLE_UNSAFE_OPS_PER_BLOCK_INFO,
crate::mut_key::MUTABLE_KEY_TYPE_INFO,
crate::mut_mut::MUT_MUT_INFO,
Expand Down Expand Up @@ -669,6 +668,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::to_digit_is_some::TO_DIGIT_IS_SOME_INFO,
crate::to_string_trait_impl::TO_STRING_TRAIT_IMPL_INFO,
crate::trailing_empty_array::TRAILING_EMPTY_ARRAY_INFO,
crate::trait_bounds::MULTIPLE_BOUND_LOCATIONS_INFO,
crate::trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS_INFO,
crate::trait_bounds::TYPE_REPETITION_IN_BOUNDS_INFO,
crate::transmute::CROSSPOINTER_TRANSMUTE_INFO,
Expand Down
2 changes: 0 additions & 2 deletions clippy_lints/src/lib.rs
Expand Up @@ -231,7 +231,6 @@ mod missing_trait_methods;
mod mixed_read_write_in_expression;
mod module_style;
mod multi_assignments;
mod multiple_bound_locations;
mod multiple_unsafe_ops_per_block;
mod mut_key;
mod mut_mut;
Expand Down Expand Up @@ -1117,7 +1116,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
});
store.register_late_pass(move |_| Box::new(incompatible_msrv::IncompatibleMsrv::new(msrv())));
store.register_late_pass(|_| Box::new(to_string_trait_impl::ToStringTraitImpl));
store.register_early_pass(|| Box::new(multiple_bound_locations::MultipleBoundLocations));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
84 changes: 0 additions & 84 deletions clippy_lints/src/multiple_bound_locations.rs

This file was deleted.

86 changes: 84 additions & 2 deletions clippy_lints/src/trait_bounds.rs
@@ -1,5 +1,5 @@
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_sugg};
use clippy_utils::source::{snippet, snippet_opt, snippet_with_applicability};
use clippy_utils::{is_from_proc_macro, SpanlessEq, SpanlessHash};
use core::hash::{Hash, Hasher};
Expand Down Expand Up @@ -86,6 +86,33 @@ declare_clippy_lint! {
"check if the same trait bounds are specified more than once during a generic declaration"
}

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

#[derive(Clone)]
pub struct TraitBounds {
max_trait_bounds: u64,
Expand All @@ -99,10 +126,11 @@ impl TraitBounds {
}
}

impl_lint_pass!(TraitBounds => [TYPE_REPETITION_IN_BOUNDS, TRAIT_DUPLICATION_IN_BOUNDS]);
impl_lint_pass!(TraitBounds => [TYPE_REPETITION_IN_BOUNDS, TRAIT_DUPLICATION_IN_BOUNDS, MULTIPLE_BOUND_LOCATIONS]);

impl<'tcx> LateLintPass<'tcx> for TraitBounds {
fn check_generics(&mut self, cx: &LateContext<'tcx>, gen: &'tcx Generics<'_>) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we now not only lint on functions, but also impls and a bunch of other items, I wonder how this interacts with derive macros such as Debug. Would be good to have a test, to make sure it doesn't lint here:

#[derive(Debug)]
struct S<T> where T: Sized { t: T }

self.check_multiple_bound_location(cx, gen);
self.check_type_repetition(cx, gen);
check_trait_bound_duplication(cx, gen);
}
Expand Down Expand Up @@ -237,6 +265,60 @@ impl TraitBounds {
}
}

fn check_multiple_bound_location<'tcx>(&self, cx: &LateContext<'tcx>, gen: &'tcx Generics<'_>) {
let mut generic_params_with_bounds = FxHashMap::default();

for bound in gen.predicates {
if let WherePredicate::BoundPredicate(p) = bound
&& p.origin == PredicateOrigin::GenericParam
&& let bounds = p
.bounds
.iter()
.filter(|b| !self.cannot_combine_maybe_bound(cx, b))
.collect::<Vec<_>>()
&& !bounds.is_empty()
sanxiyn marked this conversation as resolved.
Show resolved Hide resolved
&& let Some((_, ident)) = p.bounded_ty.as_generic_param()
{
generic_params_with_bounds.insert(ident.name.as_str().to_owned(), ident.span);
sanxiyn marked this conversation as resolved.
Show resolved Hide resolved
} else if let WherePredicate::RegionPredicate(p) = bound
&& !p.in_where_clause
&& !p.bounds.is_empty()
{
let ident = p.lifetime.ident;
generic_params_with_bounds.insert(ident.name.as_str().to_owned(), ident.span);
}
}
for bound in gen.predicates {
if let WherePredicate::BoundPredicate(p) = bound
&& p.origin == PredicateOrigin::WhereClause
&& !p.bounds.is_empty()
&& let Some((_, ident)) = p.bounded_ty.as_generic_param()
&& let Some(bound_span) = generic_params_with_bounds.get(ident.name.as_str())
{
let where_span = ident.span;
span_lint(
cx,
MULTIPLE_BOUND_LOCATIONS,
vec![*bound_span, where_span],
"bound is defined in more than one place",
);
} else if let WherePredicate::RegionPredicate(p) = bound
&& p.in_where_clause
&& !p.bounds.is_empty()
&& let ident = p.lifetime.ident
&& let Some(bound_span) = generic_params_with_bounds.get(ident.name.as_str())
{
let where_span = ident.span;
span_lint(
cx,
MULTIPLE_BOUND_LOCATIONS,
vec![*bound_span, where_span],
"bound is defined in more than one place",
);
sanxiyn marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

fn check_type_repetition<'tcx>(&self, cx: &LateContext<'tcx>, gen: &'tcx Generics<'_>) {
struct SpanlessTy<'cx, 'tcx> {
ty: &'tcx Ty<'tcx>,
Expand Down
22 changes: 22 additions & 0 deletions tests/ui/multiple_bound_locations.rs
Expand Up @@ -57,4 +57,26 @@ impl<F> C<F> {
}
}

#[clippy::msrv = "1.14.0"]
mod issue12370_fail {
trait Trait {}

fn f<T: ?Sized>()
where
T: Trait,
{
}
}

#[clippy::msrv = "1.15.0"]
mod issue12370_pass {
trait Trait {}

fn f<T: ?Sized>()
where
T: Trait,
{
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Can we have a test case for something like

fn f<'a, a: Sized>() where 'a: 'static {}

where the type parameter is named after a lifetime? I think this currently happens to work (ie not get linted) because the name of lifetimes include the quote, which differentiates them, but it would be good to have this as a guarantee in the testsuite.

fn main() {}
11 changes: 10 additions & 1 deletion tests/ui/multiple_bound_locations.stderr
Expand Up @@ -55,5 +55,14 @@ LL | fn ty_pred<F: Sized>()
LL | for<'a> F: Send + 'a,
| ^

error: aborting due to 6 previous errors
error: bound is defined in more than one place
--> tests/ui/multiple_bound_locations.rs:75:10
|
LL | fn f<T: ?Sized>()
| ^
LL | where
LL | T: Trait,
| ^

error: aborting due to 7 previous errors