Skip to content

Commit 1227571

Browse files
y21y21
committed
support inherent impls and trait impls
1 parent 09506f4 commit 1227571

File tree

4 files changed

+124
-87
lines changed

4 files changed

+124
-87
lines changed

‎clippy_lints/src/implied_bounds_in_impls.rs

Lines changed: 95 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::source::snippet;
3+
use rustc_errors::{Applicability, SuggestionStyle};
34
use rustc_hir::def_id::LocalDefId;
45
use rustc_hir::intravisit::FnKind;
5-
use rustc_hir::{Body, FnDecl, FnRetTy, GenericArg, GenericBound, ItemKind, TraitBoundModifier, TyKind};
6+
use rustc_hir::{
7+
Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, TraitBoundModifier, TraitItem,
8+
TraitItemKind, TyKind,
9+
};
610
use rustc_hir_analysis::hir_ty_to_ty;
711
use rustc_lint::{LateContext, LateLintPass};
8-
use rustc_errors::{Applicability, SuggestionStyle};
912
use rustc_middle::ty::{self, ClauseKind, TyCtxt};
1013
use rustc_session::{declare_lint_pass, declare_tool_lint};
1114
use rustc_span::Span;
@@ -94,6 +97,86 @@ fn is_same_generics(
9497
})
9598
}
9699

100+
fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) {
101+
if let FnRetTy::Return(ty) = decl.output
102+
&&let TyKind::OpaqueDef(item_id, ..) = ty.kind
103+
&& let item = cx.tcx.hir().item(item_id)
104+
&& let ItemKind::OpaqueTy(opaque_ty) = item.kind
105+
// Very often there is only a single bound, e.g. `impl Deref<..>`, in which case
106+
// we can avoid doing a bunch of stuff unnecessarily.
107+
&& opaque_ty.bounds.len() > 1
108+
{
109+
// Get all the (implied) trait predicates in the bounds.
110+
// For `impl Deref + DerefMut` this will contain [`Deref`].
111+
// The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`.
112+
// N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits.
113+
// Example:
114+
// `impl Deref<Target = i32> + DerefMut<Target = u32>` is not allowed.
115+
// `DerefMut::Target` needs to match `Deref::Target`.
116+
let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| {
117+
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
118+
&& let [.., path] = poly_trait.trait_ref.path.segments
119+
&& poly_trait.bound_generic_params.is_empty()
120+
&& let Some(trait_def_id) = path.res.opt_def_id()
121+
&& let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates
122+
&& !predicates.is_empty() // If the trait has no supertrait, there is nothing to add.
123+
{
124+
Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates))
125+
} else {
126+
None
127+
}
128+
}).collect();
129+
130+
// Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec.
131+
// This involves some extra logic when generic arguments are present, since
132+
// simply comparing trait `DefId`s won't be enough. We also need to compare the generics.
133+
for (index, bound) in opaque_ty.bounds.iter().enumerate() {
134+
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
135+
&& let [.., path] = poly_trait.trait_ref.path.segments
136+
&& let implied_args = path.args.map_or([].as_slice(), |a| a.args)
137+
&& let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id()
138+
&& let Some(implied_by_span) = implied_bounds.iter().find_map(|&(span, implied_by_args, preds)| {
139+
preds.iter().find_map(|(clause, _)| {
140+
if let ClauseKind::Trait(tr) = clause.kind().skip_binder()
141+
&& tr.def_id() == def_id
142+
&& is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args)
143+
{
144+
Some(span)
145+
} else {
146+
None
147+
}
148+
})
149+
})
150+
{
151+
let implied_by = snippet(cx, implied_by_span, "..");
152+
span_lint_and_then(
153+
cx, IMPLIED_BOUNDS_IN_IMPLS,
154+
poly_trait.span,
155+
&format!("this bound is already specified as the supertrait of `{implied_by}`"),
156+
|diag| {
157+
// If we suggest removing a bound, we may also need extend the span
158+
// to include the `+` token, so we don't end up with something like `impl + B`
159+
160+
let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) {
161+
poly_trait.span.to(next_bound.span().shrink_to_lo())
162+
} else {
163+
poly_trait.span
164+
};
165+
166+
diag.span_suggestion_with_style(
167+
implied_span_extended,
168+
"try removing this bound",
169+
"",
170+
Applicability::MachineApplicable,
171+
SuggestionStyle::ShowAlways
172+
);
173+
}
174+
);
175+
}
176+
}
177+
}
178+
}
179+
97180
impl LateLintPass<'_> for ImpliedBoundsInImpls {
98181
fn check_fn(
99182
&mut self,
@@ -104,82 +187,16 @@ impl LateLintPass<'_> for ImpliedBoundsInImpls {
104187
_: Span,
105188
_: LocalDefId,
106189
) {
107-
if let FnRetTy::Return(ty) = decl.output
108-
&&let TyKind::OpaqueDef(item_id, ..) = ty.kind
109-
&& let item = cx.tcx.hir().item(item_id)
110-
&& let ItemKind::OpaqueTy(opaque_ty) = item.kind
111-
// Very often there is only a single bound, e.g. `impl Deref<..>`, in which case
112-
// we can avoid doing a bunch of stuff unnecessarily.
113-
&& opaque_ty.bounds.len() > 1
114-
{
115-
// Get all the (implied) trait predicates in the bounds.
116-
// For `impl Deref + DerefMut` this will contain [`Deref`].
117-
// The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`.
118-
// N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits.
119-
// Example:
120-
// `impl Deref<Target = i32> + DerefMut<Target = u32>` is not allowed.
121-
// `DerefMut::Target` needs to match `Deref::Target`.
122-
let implied_bounds: Vec<_> = opaque_ty.bounds.iter().filter_map(|bound| {
123-
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
124-
&& let [.., path] = poly_trait.trait_ref.path.segments
125-
&& poly_trait.bound_generic_params.is_empty()
126-
&& let Some(trait_def_id) = path.res.opt_def_id()
127-
&& let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates
128-
&& !predicates.is_empty() // If the trait has no supertrait, there is nothing to add.
129-
{
130-
Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates))
131-
} else {
132-
None
133-
}
134-
}).collect();
135-
136-
// Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec.
137-
// This involves some extra logic when generic arguments are present, since
138-
// simply comparing trait `DefId`s won't be enough. We also need to compare the generics.
139-
for (index, bound) in opaque_ty.bounds.iter().enumerate() {
140-
if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound
141-
&& let [.., path] = poly_trait.trait_ref.path.segments
142-
&& let implied_args = path.args.map_or([].as_slice(), |a| a.args)
143-
&& let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id()
144-
&& let Some(implied_by_span) = implied_bounds.iter().find_map(|&(span, implied_by_args, preds)| {
145-
preds.iter().find_map(|(clause, _)| {
146-
if let ClauseKind::Trait(tr) = clause.kind().skip_binder()
147-
&& tr.def_id() == def_id
148-
&& is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args)
149-
{
150-
Some(span)
151-
} else {
152-
None
153-
}
154-
})
155-
})
156-
{
157-
let implied_by = snippet(cx, implied_by_span, "..");
158-
span_lint_and_then(
159-
cx, IMPLIED_BOUNDS_IN_IMPLS,
160-
poly_trait.span,
161-
&format!("this bound is already specified as the supertrait of `{}`", implied_by),
162-
|diag| {
163-
// If we suggest removing a bound, we may also need extend the span
164-
// to include the `+` token, so we don't end up with something like `impl + B`
165-
166-
let implied_span_extended = if let Some(next_bound) = opaque_ty.bounds.get(index + 1) {
167-
poly_trait.span.to(next_bound.span().shrink_to_lo())
168-
} else {
169-
poly_trait.span
170-
};
171-
172-
diag.span_suggestion_with_style(
173-
implied_span_extended,
174-
"try removing this bound",
175-
"",
176-
Applicability::MachineApplicable,
177-
SuggestionStyle::ShowAlways
178-
);
179-
}
180-
);
181-
}
182-
}
190+
check(cx, decl);
191+
}
192+
fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) {
193+
if let TraitItemKind::Fn(sig, ..) = &item.kind {
194+
check(cx, sig.decl);
195+
}
196+
}
197+
fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &ImplItem<'_>) {
198+
if let ImplItemKind::Fn(sig, ..) = &item.kind {
199+
check(cx, sig.decl);
183200
}
184201
}
185202
}

‎tests/ui/implied_bounds_in_impls.fixed

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,7 @@ fn generics_same() -> impl GenericSubtrait<(), i32, ()> {}
4949

5050
trait SomeTrait {
5151
// Check that it works in trait declarations.
52-
fn f() -> impl DerefMut<Target = u8> {
53-
Box::new(0)
54-
}
52+
fn f() -> impl DerefMut<Target = u8>;
5553
}
5654
struct SomeStruct;
5755
impl SomeStruct {

‎tests/ui/implied_bounds_in_impls.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,20 +49,18 @@ fn generics_same() -> impl GenericTrait<i32> + GenericSubtrait<(), i32, ()> {}
4949

5050
trait SomeTrait {
5151
// Check that it works in trait declarations.
52-
fn f() -> impl Deref + DerefMut<Target = u8> {
53-
Box::new(0)
54-
}
52+
fn f() -> impl Deref + DerefMut<Target = u8>;
5553
}
5654
struct SomeStruct;
5755
impl SomeStruct {
5856
// Check that it works in inherent impl blocks.
59-
fn f() -> impl DerefMut<Target = u8> {
57+
fn f() -> impl Deref + DerefMut<Target = u8> {
6058
Box::new(123)
6159
}
6260
}
6361
impl SomeTrait for SomeStruct {
6462
// Check that it works in trait impls.
65-
fn f() -> impl DerefMut<Target = u8> {
63+
fn f() -> impl Deref + DerefMut<Target = u8> {
6664
Box::new(42)
6765
}
6866
}

‎tests/ui/implied_bounds_in_impls.stderr

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,30 @@ LL + fn generics_same() -> impl GenericSubtrait<(), i32, ()> {}
8686
error: this bound is already specified as the supertrait of `DerefMut<Target = u8>`
8787
--> $DIR/implied_bounds_in_impls.rs:52:20
8888
|
89+
LL | fn f() -> impl Deref + DerefMut<Target = u8>;
90+
| ^^^^^
91+
|
92+
help: try removing this bound
93+
|
94+
LL - fn f() -> impl Deref + DerefMut<Target = u8>;
95+
LL + fn f() -> impl DerefMut<Target = u8>;
96+
|
97+
98+
error: this bound is already specified as the supertrait of `DerefMut<Target = u8>`
99+
--> $DIR/implied_bounds_in_impls.rs:57:20
100+
|
101+
LL | fn f() -> impl Deref + DerefMut<Target = u8> {
102+
| ^^^^^
103+
|
104+
help: try removing this bound
105+
|
106+
LL - fn f() -> impl Deref + DerefMut<Target = u8> {
107+
LL + fn f() -> impl DerefMut<Target = u8> {
108+
|
109+
110+
error: this bound is already specified as the supertrait of `DerefMut<Target = u8>`
111+
--> $DIR/implied_bounds_in_impls.rs:63:20
112+
|
89113
LL | fn f() -> impl Deref + DerefMut<Target = u8> {
90114
| ^^^^^
91115
|
@@ -95,5 +119,5 @@ LL - fn f() -> impl Deref + DerefMut<Target = u8> {
95119
LL + fn f() -> impl DerefMut<Target = u8> {
96120
|
97121

98-
error: aborting due to 8 previous errors
122+
error: aborting due to 10 previous errors
99123

0 commit comments

Comments
 (0)