Skip to content

Respect MSRV (MAYBE_BOUND_IN_WHERE) #12388

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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.

81 changes: 79 additions & 2 deletions clippy_lints/src/trait_bounds.rs
Original file line number Diff line number Diff line change
@@ -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,55 @@ 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
&& p.bounds
.iter()
.any(|b| !self.cannot_combine_maybe_bound(cx, b))
&& let Some((_, ident)) = p.bounded_ty.as_generic_param()
{
generic_params_with_bounds.insert(ident.name, ident.span);
} 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, ident.span);
}
}

fn emit_lint(cx: &LateContext<'_>, 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",
);
}

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)
{
emit_lint(cx, *bound_span, ident.span);
} 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)
{
emit_lint(cx, *bound_span, ident.span);
}
}
}

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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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