Skip to content

Commit 4010788

Browse files
flip1995flip1995
authored
Merge pull request #2917 from mikerite/issue2894
Fix #2894
2 parents 0aeb82c + d7ddb2a commit 4010788

File tree

5 files changed

+355
-119
lines changed

5 files changed

+355
-119
lines changed

‎clippy_lints/src/use_self.rs

Lines changed: 126 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
2-
use rustc::{declare_lint, lint_array};
1+
use crate::utils::{in_macro, span_lint_and_sugg};
32
use if_chain::if_chain;
3+
use rustc::hir::intravisit::{walk_path, walk_ty, NestedVisitorMap, Visitor};
44
use rustc::hir::*;
5-
use rustc::hir::intravisit::{walk_path, NestedVisitorMap, Visitor};
6-
use crate::utils::{in_macro, span_lint_and_then};
5+
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
6+
use rustc::ty;
7+
use rustc::{declare_lint, lint_array};
78
use syntax::ast::NodeId;
89
use syntax_pos::symbol::keywords::SelfType;
910

@@ -51,6 +52,105 @@ impl LintPass for UseSelf {
5152

5253
const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element";
5354

55+
fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path) {
56+
span_lint_and_sugg(
57+
cx,
58+
USE_SELF,
59+
path.span,
60+
"unnecessary structure name repetition",
61+
"use the applicable keyword",
62+
"Self".to_owned(),
63+
);
64+
}
65+
66+
struct TraitImplTyVisitor<'a, 'tcx: 'a> {
67+
item_path: &'a Path,
68+
cx: &'a LateContext<'a, 'tcx>,
69+
trait_type_walker: ty::walk::TypeWalker<'tcx>,
70+
impl_type_walker: ty::walk::TypeWalker<'tcx>,
71+
}
72+
73+
impl<'a, 'tcx> Visitor<'tcx> for TraitImplTyVisitor<'a, 'tcx> {
74+
fn visit_ty(&mut self, t: &'tcx Ty) {
75+
let trait_ty = self.trait_type_walker.next();
76+
let impl_ty = self.impl_type_walker.next();
77+
78+
if let TyKind::Path(QPath::Resolved(_, path)) = &t.node {
79+
if self.item_path.def == path.def {
80+
let is_self_ty = if let def::Def::SelfTy(..) = path.def {
81+
true
82+
} else {
83+
false
84+
};
85+
86+
if !is_self_ty && impl_ty != trait_ty {
87+
// The implementation and trait types don't match which means that
88+
// the concrete type was specified by the implementation but
89+
// it didn't use `Self`
90+
span_use_self_lint(self.cx, path);
91+
}
92+
}
93+
}
94+
walk_ty(self, t)
95+
}
96+
97+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
98+
NestedVisitorMap::None
99+
}
100+
}
101+
102+
fn check_trait_method_impl_decl<'a, 'tcx: 'a>(
103+
cx: &'a LateContext<'a, 'tcx>,
104+
item_path: &'a Path,
105+
impl_item: &ImplItem,
106+
impl_decl: &'tcx FnDecl,
107+
impl_trait_ref: &ty::TraitRef<'_>,
108+
) {
109+
let trait_method = cx
110+
.tcx
111+
.associated_items(impl_trait_ref.def_id)
112+
.find(|assoc_item| {
113+
assoc_item.kind == ty::AssociatedKind::Method
114+
&& cx
115+
.tcx
116+
.hygienic_eq(impl_item.ident, assoc_item.ident, impl_trait_ref.def_id)
117+
})
118+
.expect("impl method matches a trait method");
119+
120+
let trait_method_sig = cx.tcx.fn_sig(trait_method.def_id);
121+
let trait_method_sig = cx.tcx.erase_late_bound_regions(&trait_method_sig);
122+
123+
let impl_method_def_id = cx.tcx.hir.local_def_id(impl_item.id);
124+
let impl_method_sig = cx.tcx.fn_sig(impl_method_def_id);
125+
let impl_method_sig = cx.tcx.erase_late_bound_regions(&impl_method_sig);
126+
127+
let output_ty = if let FunctionRetTy::Return(ty) = &impl_decl.output {
128+
Some(&**ty)
129+
} else {
130+
None
131+
};
132+
133+
// `impl_decl_ty` (of type `hir::Ty`) represents the type declared in the signature.
134+
// `impl_ty` (of type `ty:TyS`) is the concrete type that the compiler has determined for
135+
// that declaration. We use `impl_decl_ty` to see if the type was declared as `Self`
136+
// and use `impl_ty` to check its concrete type.
137+
for (impl_decl_ty, (impl_ty, trait_ty)) in impl_decl.inputs.iter().chain(output_ty).zip(
138+
impl_method_sig
139+
.inputs_and_output
140+
.iter()
141+
.zip(trait_method_sig.inputs_and_output),
142+
) {
143+
let mut visitor = TraitImplTyVisitor {
144+
cx,
145+
item_path,
146+
trait_type_walker: trait_ty.walk(),
147+
impl_type_walker: impl_ty.walk(),
148+
};
149+
150+
visitor.visit_ty(&impl_decl_ty);
151+
}
152+
}
153+
54154
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf {
55155
fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) {
56156
if in_macro(item.span) {
@@ -69,13 +169,32 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf {
69169
} else {
70170
true
71171
};
172+
72173
if should_check {
73174
let visitor = &mut UseSelfVisitor {
74175
item_path,
75176
cx,
76177
};
77-
for impl_item_ref in refs {
78-
visitor.visit_impl_item(cx.tcx.hir.impl_item(impl_item_ref.id));
178+
let impl_def_id = cx.tcx.hir.local_def_id(item.id);
179+
let impl_trait_ref = cx.tcx.impl_trait_ref(impl_def_id);
180+
181+
if let Some(impl_trait_ref) = impl_trait_ref {
182+
for impl_item_ref in refs {
183+
let impl_item = cx.tcx.hir.impl_item(impl_item_ref.id);
184+
if let ImplItemKind::Method(MethodSig{ decl: impl_decl, .. }, impl_body_id)
185+
= &impl_item.node {
186+
check_trait_method_impl_decl(cx, item_path, impl_item, impl_decl, &impl_trait_ref);
187+
let body = cx.tcx.hir.body(*impl_body_id);
188+
visitor.visit_body(body);
189+
} else {
190+
visitor.visit_impl_item(impl_item);
191+
}
192+
}
193+
} else {
194+
for impl_item_ref in refs {
195+
let impl_item = cx.tcx.hir.impl_item(impl_item_ref.id);
196+
visitor.visit_impl_item(impl_item);
197+
}
79198
}
80199
}
81200
}
@@ -91,9 +210,7 @@ struct UseSelfVisitor<'a, 'tcx: 'a> {
91210
impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> {
92211
fn visit_path(&mut self, path: &'tcx Path, _id: NodeId) {
93212
if self.item_path.def == path.def && path.segments.last().expect(SEGMENTS_MSG).ident.name != SelfType.name() {
94-
span_lint_and_then(self.cx, USE_SELF, path.span, "unnecessary structure name repetition", |db| {
95-
db.span_suggestion(path.span, "use the applicable keyword", "Self".to_owned());
96-
});
213+
span_use_self_lint(self.cx, path);
97214
}
98215

99216
walk_path(self, path);

‎tests/ui/methods.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
#![warn(clippy, clippy_pedantic, option_unwrap_used)]
55
#![allow(blacklisted_name, unused, print_stdout, non_ascii_literal, new_without_default,
66
new_without_default_derive, missing_docs_in_private_items, needless_pass_by_value,
7-
default_trait_access)]
7+
default_trait_access, use_self)]
88

99
use std::collections::BTreeMap;
1010
use std::collections::HashMap;

‎tests/ui/methods.stderr

Lines changed: 1 addition & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,3 @@
1-
error: unnecessary structure name repetition
2-
--> $DIR/methods.rs:21:29
3-
|
4-
21 | pub fn add(self, other: T) -> T { self }
5-
| ^ help: use the applicable keyword: `Self`
6-
|
7-
= note: `-D use-self` implied by `-D warnings`
8-
9-
error: unnecessary structure name repetition
10-
--> $DIR/methods.rs:21:35
11-
|
12-
21 | pub fn add(self, other: T) -> T { self }
13-
| ^ help: use the applicable keyword: `Self`
14-
15-
error: unnecessary structure name repetition
16-
--> $DIR/methods.rs:25:25
17-
|
18-
25 | fn eq(&self, other: T) -> bool { true } // no error, private function
19-
| ^ help: use the applicable keyword: `Self`
20-
21-
error: unnecessary structure name repetition
22-
--> $DIR/methods.rs:27:26
23-
|
24-
27 | fn sub(&self, other: T) -> &T { self } // no error, self is a ref
25-
| ^ help: use the applicable keyword: `Self`
26-
27-
error: unnecessary structure name repetition
28-
--> $DIR/methods.rs:27:33
29-
|
30-
27 | fn sub(&self, other: T) -> &T { self } // no error, self is a ref
31-
| ^ help: use the applicable keyword: `Self`
32-
33-
error: unnecessary structure name repetition
34-
--> $DIR/methods.rs:28:21
35-
|
36-
28 | fn div(self) -> T { self } // no error, different #arguments
37-
| ^ help: use the applicable keyword: `Self`
38-
39-
error: unnecessary structure name repetition
40-
--> $DIR/methods.rs:29:25
41-
|
42-
29 | fn rem(self, other: T) { } // no error, wrong return type
43-
| ^ help: use the applicable keyword: `Self`
44-
451
error: defining a method called `add` on this type; consider implementing the `std::ops::Add` trait or choosing a less ambiguous name
462
--> $DIR/methods.rs:21:5
473
|
@@ -78,30 +34,6 @@ error: methods called `new` usually return `Self`
7834
|
7935
= note: `-D new-ret-no-self` implied by `-D warnings`
8036

81-
error: unnecessary structure name repetition
82-
--> $DIR/methods.rs:80:24
83-
|
84-
80 | fn new() -> Option<V<T>> { None }
85-
| ^^^^ help: use the applicable keyword: `Self`
86-
87-
error: unnecessary structure name repetition
88-
--> $DIR/methods.rs:84:19
89-
|
90-
84 | type Output = T;
91-
| ^ help: use the applicable keyword: `Self`
92-
93-
error: unnecessary structure name repetition
94-
--> $DIR/methods.rs:85:25
95-
|
96-
85 | fn mul(self, other: T) -> T { self } // no error, obviously
97-
| ^ help: use the applicable keyword: `Self`
98-
99-
error: unnecessary structure name repetition
100-
--> $DIR/methods.rs:85:31
101-
|
102-
85 | fn mul(self, other: T) -> T { self } // no error, obviously
103-
| ^ help: use the applicable keyword: `Self`
104-
10537
error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
10638
--> $DIR/methods.rs:104:13
10739
|
@@ -251,24 +183,6 @@ error: called `map(f).unwrap_or_else(g)` on a Result value. This can be done mor
251183
174 | | );
252184
| |_________________^
253185

254-
error: unnecessary structure name repetition
255-
--> $DIR/methods.rs:200:24
256-
|
257-
200 | fn filter(self) -> IteratorFalsePositives {
258-
| ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
259-
260-
error: unnecessary structure name repetition
261-
--> $DIR/methods.rs:204:22
262-
|
263-
204 | fn next(self) -> IteratorFalsePositives {
264-
| ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
265-
266-
error: unnecessary structure name repetition
267-
--> $DIR/methods.rs:224:32
268-
|
269-
224 | fn skip(self, _: usize) -> IteratorFalsePositives {
270-
| ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self`
271-
272186
error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
273187
--> $DIR/methods.rs:234:13
274188
|
@@ -343,12 +257,6 @@ error: called `is_some()` after searching an `Iterator` with rposition. This is
343257
276 | | ).is_some();
344258
| |______________________________^
345259

346-
error: unnecessary structure name repetition
347-
--> $DIR/methods.rs:290:21
348-
|
349-
290 | fn new() -> Foo { Foo }
350-
| ^^^ help: use the applicable keyword: `Self`
351-
352260
error: use of `unwrap_or` followed by a function call
353261
--> $DIR/methods.rs:308:22
354262
|
@@ -527,5 +435,5 @@ error: used unwrap() on an Option value. If you don't want to handle the None ca
527435
|
528436
= note: `-D option-unwrap-used` implied by `-D warnings`
529437

530-
error: aborting due to 70 previous errors
438+
error: aborting due to 55 previous errors
531439

0 commit comments

Comments
 (0)