Skip to content

Commit 640051b

Browse files
author
Tim Nielens
committed
rework use_self impl based on ty::Ty comparison rust-lang#3410 
1 parent be47b7a commit 640051b

File tree

6 files changed

+182
-267
lines changed

6 files changed

+182
-267
lines changed

‎clippy_lints/src/use_self.rs

+104-142
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,19 @@
11
use if_chain::if_chain;
22
use rustc_errors::Applicability;
33
use rustc_hir as hir;
4-
use rustc_hir::def::{DefKind, Res};
5-
use rustc_hir::intravisit::{walk_item, walk_path, walk_ty, NestedVisitorMap, Visitor};
4+
use rustc_hir::intravisit::{walk_expr, walk_impl_item, walk_ty, NestedVisitorMap, Visitor};
65
use rustc_hir::{
7-
def, FnDecl, FnRetTy, FnSig, GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Path, PathSegment, QPath,
8-
TyKind,
6+
def, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, ImplItem, ImplItemKind, Item, ItemKind, Path,
7+
PathSegment, QPath, TyKind,
98
};
109
use rustc_lint::{LateContext, LateLintPass, LintContext};
1110
use rustc_middle::hir::map::Map;
1211
use rustc_middle::lint::in_external_macro;
1312
use rustc_middle::ty;
14-
use rustc_middle::ty::{DefIdTree, Ty};
13+
use rustc_middle::ty::Ty;
1514
use rustc_session::{declare_lint_pass, declare_tool_lint};
16-
use rustc_span::symbol::kw;
1715
use rustc_typeck::hir_ty_to_ty;
16+
use rustc_span::{Span};
1817

1918
use crate::utils::{differing_macro_contexts, span_lint_and_sugg};
2019

@@ -81,22 +80,96 @@ fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path<'_>, last_segment: O
8180
);
8281
}
8382

84-
// FIXME: always use this (more correct) visitor, not just in method signatures.
85-
struct SemanticUseSelfVisitor<'a, 'tcx> {
83+
fn simple_span(cx: &LateContext<'_, '_>, span: Span) {
84+
span_lint_and_sugg(
85+
cx,
86+
USE_SELF,
87+
span,
88+
"unnecessary structure name repetition",
89+
"use the applicable keyword",
90+
"Self".to_owned(),
91+
Applicability::MachineApplicable,
92+
);
93+
}
94+
95+
struct ImplVisitor<'a, 'tcx> {
8696
cx: &'a LateContext<'a, 'tcx>,
97+
item: &'tcx Item<'tcx>,
8798
self_ty: Ty<'tcx>,
8899
}
89100

90-
impl<'a, 'tcx> Visitor<'tcx> for SemanticUseSelfVisitor<'a, 'tcx> {
101+
impl<'a, 'tcx> ImplVisitor<'a, 'tcx> {
102+
fn check_trait_method_impl_decl(
103+
&mut self,
104+
impl_item: &ImplItem<'tcx>,
105+
impl_decl: &'tcx FnDecl<'tcx>,
106+
impl_trait_ref: ty::TraitRef<'tcx>,
107+
) {
108+
let tcx = self.cx.tcx;
109+
let trait_method = tcx
110+
.associated_items(impl_trait_ref.def_id)
111+
.find_by_name_and_kind(tcx, impl_item.ident, ty::AssocKind::Fn, impl_trait_ref.def_id)
112+
.expect("impl method matches a trait method");
113+
114+
let trait_method_sig = tcx.fn_sig(trait_method.def_id);
115+
let trait_method_sig = tcx.erase_late_bound_regions(&trait_method_sig);
116+
117+
let output_hir_ty = if let FnRetTy::Return(ty) = &impl_decl.output {
118+
Some(&**ty)
119+
} else {
120+
None
121+
};
122+
123+
// `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature.
124+
// `trait_ty` (of type `ty::Ty`) is the semantic type for the signature in the trait.
125+
// We use `impl_hir_ty` to see if the type was written as `Self`,
126+
// `hir_ty_to_ty(...)` to check semantic types of paths, and
127+
// `trait_ty` to determine which parts of the signature in the trait, mention
128+
// the type being implemented verbatim (as opposed to `Self`).
129+
for (impl_hir_ty, trait_ty) in impl_decl
130+
.inputs
131+
.iter()
132+
.chain(output_hir_ty)
133+
.zip(trait_method_sig.inputs_and_output)
134+
{
135+
// Check if the input/output type in the trait method specifies the implemented
136+
// type verbatim, and only suggest `Self` if that isn't the case.
137+
// This avoids suggestions to e.g. replace `Vec<u8>` with `Vec<Self>`,
138+
// in an `impl Trait for u8`, when the trait always uses `Vec<u8>`.
139+
// See also https://github.com/rust-lang/rust-clippy/issues/2894.
140+
let self_ty = impl_trait_ref.self_ty();
141+
if !trait_ty.walk().any(|inner| inner == self_ty.into()) {
142+
self.visit_ty(&impl_hir_ty);
143+
}
144+
}
145+
}
146+
}
147+
148+
impl<'a, 'tcx> Visitor<'tcx> for ImplVisitor<'a, 'tcx> {
91149
type Map = Map<'tcx>;
92150

151+
fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
152+
if let ExprKind::Struct(QPath::Resolved(_, _path), ..) = ex.kind {
153+
// todo: this doesn't work, `expr_ty_opt` returns None
154+
155+
// if self.cx.tcx.body.expr_ty_opt(ex) == self.self_ty {
156+
// span_use_self_lint(self.cx, path, None);
157+
// }
158+
}
159+
walk_expr(self, ex)
160+
}
161+
162+
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
163+
NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
164+
}
165+
93166
fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'_>) {
94167
if let TyKind::Path(QPath::Resolved(_, path)) = &hir_ty.kind {
95168
match path.res {
96169
def::Res::SelfTy(..) => {},
97170
_ => {
98171
if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty {
99-
span_use_self_lint(self.cx, path, None);
172+
simple_span(self.cx, hir_ty.span);
100173
}
101174
},
102175
}
@@ -105,54 +178,20 @@ impl<'a, 'tcx> Visitor<'tcx> for SemanticUseSelfVisitor<'a, 'tcx> {
105178
walk_ty(self, hir_ty)
106179
}
107180

108-
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
109-
NestedVisitorMap::None
110-
}
111-
}
112-
113-
fn check_trait_method_impl_decl<'a, 'tcx>(
114-
cx: &'a LateContext<'a, 'tcx>,
115-
impl_item: &ImplItem<'_>,
116-
impl_decl: &'tcx FnDecl<'_>,
117-
impl_trait_ref: ty::TraitRef<'tcx>,
118-
) {
119-
let trait_method = cx
120-
.tcx
121-
.associated_items(impl_trait_ref.def_id)
122-
.find_by_name_and_kind(cx.tcx, impl_item.ident, ty::AssocKind::Fn, impl_trait_ref.def_id)
123-
.expect("impl method matches a trait method");
124-
125-
let trait_method_sig = cx.tcx.fn_sig(trait_method.def_id);
126-
let trait_method_sig = cx.tcx.erase_late_bound_regions(&trait_method_sig);
127-
128-
let output_hir_ty = if let FnRetTy::Return(ty) = &impl_decl.output {
129-
Some(&**ty)
130-
} else {
131-
None
132-
};
133-
134-
// `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature.
135-
// `trait_ty` (of type `ty::Ty`) is the semantic type for the signature in the trait.
136-
// We use `impl_hir_ty` to see if the type was written as `Self`,
137-
// `hir_ty_to_ty(...)` to check semantic types of paths, and
138-
// `trait_ty` to determine which parts of the signature in the trait, mention
139-
// the type being implemented verbatim (as opposed to `Self`).
140-
for (impl_hir_ty, trait_ty) in impl_decl
141-
.inputs
142-
.iter()
143-
.chain(output_hir_ty)
144-
.zip(trait_method_sig.inputs_and_output)
145-
{
146-
// Check if the input/output type in the trait method specifies the implemented
147-
// type verbatim, and only suggest `Self` if that isn't the case.
148-
// This avoids suggestions to e.g. replace `Vec<u8>` with `Vec<Self>`,
149-
// in an `impl Trait for u8`, when the trait always uses `Vec<u8>`.
150-
// See also https://github.com/rust-lang/rust-clippy/issues/2894.
151-
let self_ty = impl_trait_ref.self_ty();
152-
if !trait_ty.walk().any(|inner| inner == self_ty.into()) {
153-
let mut visitor = SemanticUseSelfVisitor { cx, self_ty };
154-
155-
visitor.visit_ty(&impl_hir_ty);
181+
fn visit_impl_item(&mut self, ii: &'tcx ImplItem<'tcx>) {
182+
let tcx = self.cx.tcx;
183+
let impl_def_id = tcx.hir().local_def_id(self.item.hir_id);
184+
let impl_trait_ref = tcx.impl_trait_ref(impl_def_id);
185+
if_chain! {
186+
if let Some(impl_trait_ref) = impl_trait_ref;
187+
if let ImplItemKind::Fn(FnSig { decl: impl_decl, .. }, impl_body_id) = &ii.kind;
188+
then {
189+
self.check_trait_method_impl_decl(ii, impl_decl, impl_trait_ref);
190+
let body = tcx.hir().body(*impl_body_id);
191+
self.visit_body(body);
192+
} else {
193+
walk_impl_item(self, ii)
194+
}
156195
}
157196
}
158197
}
@@ -177,96 +216,19 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf {
177216
};
178217

179218
if should_check {
180-
let visitor = &mut UseSelfVisitor {
181-
item_path,
219+
let self_ty= hir_ty_to_ty(cx.tcx, item_type);
220+
let visitor = &mut ImplVisitor {
182221
cx,
222+
item,
223+
self_ty,
183224
};
184-
let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id);
185-
let impl_trait_ref = cx.tcx.impl_trait_ref(impl_def_id);
186-
187-
if let Some(impl_trait_ref) = impl_trait_ref {
188-
for impl_item_ref in refs {
189-
let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
190-
if let ImplItemKind::Fn(FnSig{ decl: impl_decl, .. }, impl_body_id)
191-
= &impl_item.kind {
192-
check_trait_method_impl_decl(cx, impl_item, impl_decl, impl_trait_ref);
193-
194-
let body = cx.tcx.hir().body(*impl_body_id);
195-
visitor.visit_body(body);
196-
} else {
197-
visitor.visit_impl_item(impl_item);
198-
}
199-
}
200-
} else {
201-
for impl_item_ref in refs {
202-
let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
203-
visitor.visit_impl_item(impl_item);
204-
}
205-
}
206-
}
207-
}
208-
}
209-
}
210-
}
211-
212-
struct UseSelfVisitor<'a, 'tcx> {
213-
item_path: &'a Path<'a>,
214-
cx: &'a LateContext<'a, 'tcx>,
215-
}
216-
217-
impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> {
218-
type Map = Map<'tcx>;
219-
220-
fn visit_path(&mut self, path: &'tcx Path<'_>, _id: HirId) {
221-
if !path.segments.iter().any(|p| p.ident.span.is_dummy()) {
222-
if path.segments.len() >= 2 {
223-
let last_but_one = &path.segments[path.segments.len() - 2];
224-
if last_but_one.ident.name != kw::SelfUpper {
225-
let enum_def_id = match path.res {
226-
Res::Def(DefKind::Variant, variant_def_id) => self.cx.tcx.parent(variant_def_id),
227-
Res::Def(DefKind::Ctor(def::CtorOf::Variant, _), ctor_def_id) => {
228-
let variant_def_id = self.cx.tcx.parent(ctor_def_id);
229-
variant_def_id.and_then(|def_id| self.cx.tcx.parent(def_id))
230-
},
231-
_ => None,
232-
};
233-
234-
if self.item_path.res.opt_def_id() == enum_def_id {
235-
span_use_self_lint(self.cx, path, Some(last_but_one));
236-
}
237-
}
238-
}
239225

240-
if path.segments.last().expect(SEGMENTS_MSG).ident.name != kw::SelfUpper {
241-
if self.item_path.res == path.res {
242-
span_use_self_lint(self.cx, path, None);
243-
} else if let Res::Def(DefKind::Ctor(def::CtorOf::Struct, _), ctor_def_id) = path.res {
244-
if self.item_path.res.opt_def_id() == self.cx.tcx.parent(ctor_def_id) {
245-
span_use_self_lint(self.cx, path, None);
226+
for impl_item_ref in refs {
227+
let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
228+
visitor.visit_impl_item(impl_item);
246229
}
247230
}
248231
}
249232
}
250-
251-
walk_path(self, path);
252-
}
253-
254-
fn visit_item(&mut self, item: &'tcx Item<'_>) {
255-
match item.kind {
256-
ItemKind::Use(..)
257-
| ItemKind::Static(..)
258-
| ItemKind::Enum(..)
259-
| ItemKind::Struct(..)
260-
| ItemKind::Union(..)
261-
| ItemKind::Impl { .. }
262-
| ItemKind::Fn(..) => {
263-
// Don't check statements that shadow `Self` or where `Self` can't be used
264-
},
265-
_ => walk_item(self, item),
266-
}
267-
}
268-
269-
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
270-
NestedVisitorMap::All(self.cx.tcx.hir())
271233
}
272234
}

0 commit comments

Comments
 (0)