Skip to content

Commit 63d2d06

Browse files
borsbors
committed
Auto merge of #4369 - mikerite:fix-4293, r=flip1995
 Fix `wrong_self_convention` issue Resolves #4293 changelog: Fix `wrong_self_convention` issue
2 parents 4f8bdf3 + 77278cc commit 63d2d06

File tree

2 files changed

+118
-160
lines changed

2 files changed

+118
-160
lines changed

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 99 additions & 160 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,10 @@ use crate::utils::sugg;
2323
use crate::utils::usage::mutated_variables;
2424
use crate::utils::{
2525
get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy,
26-
is_ctor_function, is_expn_of, is_self, is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path,
27-
match_qpath, match_trait_method, match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty,
28-
same_tys, single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
29-
span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
26+
is_ctor_function, is_expn_of, iter_input_pats, last_path_segment, match_def_path, match_qpath, match_trait_method,
27+
match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path,
28+
snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_sugg,
29+
span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
3030
};
3131

3232
declare_clippy_lint! {
@@ -1015,69 +1015,76 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
10151015
}
10161016
}
10171017

1018-
fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, implitem: &'tcx hir::ImplItem) {
1019-
if in_external_macro(cx.sess(), implitem.span) {
1018+
fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, impl_item: &'tcx hir::ImplItem) {
1019+
if in_external_macro(cx.sess(), impl_item.span) {
10201020
return;
10211021
}
1022-
let name = implitem.ident.name.as_str();
1023-
let parent = cx.tcx.hir().get_parent_item(implitem.hir_id);
1022+
let name = impl_item.ident.name.as_str();
1023+
let parent = cx.tcx.hir().get_parent_item(impl_item.hir_id);
10241024
let item = cx.tcx.hir().expect_item(parent);
10251025
let def_id = cx.tcx.hir().local_def_id(item.hir_id);
10261026
let ty = cx.tcx.type_of(def_id);
10271027
if_chain! {
1028-
if let hir::ImplItemKind::Method(ref sig, id) = implitem.node;
1029-
if let Some(first_arg_ty) = sig.decl.inputs.get(0);
1028+
if let hir::ImplItemKind::Method(ref sig, id) = impl_item.node;
10301029
if let Some(first_arg) = iter_input_pats(&sig.decl, cx.tcx.hir().body(id)).next();
1031-
if let hir::ItemKind::Impl(_, _, _, _, None, ref self_ty, _) = item.node;
1030+
if let hir::ItemKind::Impl(_, _, _, _, None, _, _) = item.node;
10321031
then {
1033-
if cx.access_levels.is_exported(implitem.hir_id) {
1034-
// check missing trait implementations
1035-
for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
1036-
if name == method_name &&
1037-
sig.decl.inputs.len() == n_args &&
1038-
out_type.matches(cx, &sig.decl.output) &&
1039-
self_kind.matches(cx, first_arg_ty, first_arg, self_ty, false, &implitem.generics) {
1040-
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, implitem.span, &format!(
1041-
"defining a method called `{}` on this type; consider implementing \
1042-
the `{}` trait or choosing a less ambiguous name", name, trait_name));
1043-
}
1044-
}
1045-
}
1032+
let method_def_id = cx.tcx.hir().local_def_id(impl_item.hir_id);
1033+
let method_sig = cx.tcx.fn_sig(method_def_id);
1034+
let method_sig = cx.tcx.erase_late_bound_regions(&method_sig);
1035+
1036+
let first_arg_ty = &method_sig.inputs().iter().next();
10461037

10471038
// check conventions w.r.t. conversion method names and predicates
1048-
let is_copy = is_copy(cx, ty);
1049-
for &(ref conv, self_kinds) in &CONVENTIONS {
1050-
if conv.check(&name) {
1051-
if !self_kinds
1052-
.iter()
1053-
.any(|k| k.matches(cx, first_arg_ty, first_arg, self_ty, is_copy, &implitem.generics)) {
1054-
let lint = if item.vis.node.is_pub() {
1055-
WRONG_PUB_SELF_CONVENTION
1056-
} else {
1057-
WRONG_SELF_CONVENTION
1058-
};
1059-
span_lint(cx,
1060-
lint,
1061-
first_arg.pat.span,
1062-
&format!("methods called `{}` usually take {}; consider choosing a less \
1063-
ambiguous name",
1064-
conv,
1065-
&self_kinds.iter()
1066-
.map(|k| k.description())
1067-
.collect::<Vec<_>>()
1068-
.join(" or ")));
1039+
if let Some(first_arg_ty) = first_arg_ty {
1040+
1041+
if cx.access_levels.is_exported(impl_item.hir_id) {
1042+
// check missing trait implementations
1043+
for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
1044+
if name == method_name &&
1045+
sig.decl.inputs.len() == n_args &&
1046+
out_type.matches(cx, &sig.decl.output) &&
1047+
self_kind.matches(cx, ty, first_arg_ty) {
1048+
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
1049+
"defining a method called `{}` on this type; consider implementing \
1050+
the `{}` trait or choosing a less ambiguous name", name, trait_name));
1051+
}
10691052
}
1053+
}
10701054

1071-
// Only check the first convention to match (CONVENTIONS should be listed from most to least
1072-
// specific)
1073-
break;
1055+
for &(ref conv, self_kinds) in &CONVENTIONS {
1056+
if conv.check(&name) {
1057+
if !self_kinds
1058+
.iter()
1059+
.any(|k| k.matches(cx, ty, first_arg_ty)) {
1060+
let lint = if item.vis.node.is_pub() {
1061+
WRONG_PUB_SELF_CONVENTION
1062+
} else {
1063+
WRONG_SELF_CONVENTION
1064+
};
1065+
span_lint(cx,
1066+
lint,
1067+
first_arg.pat.span,
1068+
&format!("methods called `{}` usually take {}; consider choosing a less \
1069+
ambiguous name",
1070+
conv,
1071+
&self_kinds.iter()
1072+
.map(|k| k.description())
1073+
.collect::<Vec<_>>()
1074+
.join(" or ")));
1075+
}
1076+
1077+
// Only check the first convention to match (CONVENTIONS should be listed from most to least
1078+
// specific)
1079+
break;
1080+
}
10741081
}
10751082
}
10761083
}
10771084
}
10781085

1079-
if let hir::ImplItemKind::Method(_, _) = implitem.node {
1080-
let ret_ty = return_ty(cx, implitem.hir_id);
1086+
if let hir::ImplItemKind::Method(_, _) = impl_item.node {
1087+
let ret_ty = return_ty(cx, impl_item.hir_id);
10811088

10821089
// walk the return type and check for Self (this does not check associated types)
10831090
for inner_type in ret_ty.walk() {
@@ -1111,7 +1118,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
11111118
span_lint(
11121119
cx,
11131120
NEW_RET_NO_SELF,
1114-
implitem.span,
1121+
impl_item.span,
11151122
"methods called `new` usually return `Self`",
11161123
);
11171124
}
@@ -2614,55 +2621,49 @@ enum SelfKind {
26142621
}
26152622

26162623
impl SelfKind {
2617-
fn matches(
2618-
self,
2619-
cx: &LateContext<'_, '_>,
2620-
ty: &hir::Ty,
2621-
arg: &hir::Arg,
2622-
self_ty: &hir::Ty,
2623-
allow_value_for_ref: bool,
2624-
generics: &hir::Generics,
2625-
) -> bool {
2626-
// Self types in the HIR are desugared to explicit self types. So it will
2627-
// always be `self:
2628-
// SomeType`,
2629-
// where SomeType can be `Self` or an explicit impl self type (e.g., `Foo` if
2630-
// the impl is on `Foo`)
2631-
// Thus, we only need to test equality against the impl self type or if it is
2632-
// an explicit
2633-
// `Self`. Furthermore, the only possible types for `self: ` are `&Self`,
2634-
// `Self`, `&mut Self`,
2635-
// and `Box<Self>`, including the equivalent types with `Foo`.
2636-
2637-
let is_actually_self = |ty| is_self_ty(ty) || SpanlessEq::new(cx).eq_ty(ty, self_ty);
2638-
if is_self(arg) {
2639-
match self {
2640-
Self::Value => is_actually_self(ty),
2641-
Self::Ref | Self::RefMut => {
2642-
if allow_value_for_ref && is_actually_self(ty) {
2643-
return true;
2644-
}
2645-
match ty.node {
2646-
hir::TyKind::Rptr(_, ref mt_ty) => {
2647-
let mutability_match = if self == Self::Ref {
2648-
mt_ty.mutbl == hir::MutImmutable
2649-
} else {
2650-
mt_ty.mutbl == hir::MutMutable
2651-
};
2652-
is_actually_self(&mt_ty.ty) && mutability_match
2653-
},
2654-
_ => false,
2655-
}
2656-
},
2657-
_ => false,
2624+
fn matches<'a>(self, cx: &LateContext<'_, 'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
2625+
fn matches_value(parent_ty: Ty<'_>, ty: Ty<'_>) -> bool {
2626+
if ty == parent_ty {
2627+
true
2628+
} else if ty.is_box() {
2629+
ty.boxed_ty() == parent_ty
2630+
} else if ty.is_rc() || ty.is_arc() {
2631+
if let ty::Adt(_, substs) = ty.sty {
2632+
substs.types().next().map_or(false, |t| t == parent_ty)
2633+
} else {
2634+
false
2635+
}
2636+
} else {
2637+
false
26582638
}
2659-
} else {
2660-
match self {
2661-
Self::Value => false,
2662-
Self::Ref => is_as_ref_or_mut_trait(ty, self_ty, generics, &paths::ASREF_TRAIT),
2663-
Self::RefMut => is_as_ref_or_mut_trait(ty, self_ty, generics, &paths::ASMUT_TRAIT),
2664-
Self::No => true,
2639+
}
2640+
2641+
fn matches_ref<'a>(
2642+
cx: &LateContext<'_, 'a>,
2643+
mutability: hir::Mutability,
2644+
parent_ty: Ty<'a>,
2645+
ty: Ty<'a>,
2646+
) -> bool {
2647+
if let ty::Ref(_, t, m) = ty.sty {
2648+
return m == mutability && t == parent_ty;
26652649
}
2650+
2651+
let trait_path = match mutability {
2652+
hir::Mutability::MutImmutable => &paths::ASREF_TRAIT,
2653+
hir::Mutability::MutMutable => &paths::ASMUT_TRAIT,
2654+
};
2655+
2656+
let trait_def_id = get_trait_def_id(cx, trait_path).expect("trait def id not found");
2657+
implements_trait(cx, ty, trait_def_id, &[parent_ty.into()])
2658+
}
2659+
2660+
match self {
2661+
Self::Value => matches_value(parent_ty, ty),
2662+
Self::Ref => {
2663+
matches_ref(cx, hir::Mutability::MutImmutable, parent_ty, ty) || ty == parent_ty && is_copy(cx, ty)
2664+
},
2665+
Self::RefMut => matches_ref(cx, hir::Mutability::MutMutable, parent_ty, ty),
2666+
Self::No => ty != parent_ty,
26662667
}
26672668
}
26682669

@@ -2676,68 +2677,6 @@ impl SelfKind {
26762677
}
26772678
}
26782679

2679-
fn is_as_ref_or_mut_trait(ty: &hir::Ty, self_ty: &hir::Ty, generics: &hir::Generics, name: &[&str]) -> bool {
2680-
single_segment_ty(ty).map_or(false, |seg| {
2681-
generics.params.iter().any(|param| match param.kind {
2682-
hir::GenericParamKind::Type { .. } => {
2683-
param.name.ident().name == seg.ident.name
2684-
&& param.bounds.iter().any(|bound| {
2685-
if let hir::GenericBound::Trait(ref ptr, ..) = *bound {
2686-
let path = &ptr.trait_ref.path;
2687-
match_path(path, name)
2688-
&& path.segments.last().map_or(false, |s| {
2689-
if let Some(ref params) = s.args {
2690-
if params.parenthesized {
2691-
false
2692-
} else {
2693-
// FIXME(flip1995): messy, improve if there is a better option
2694-
// in the compiler
2695-
let types: Vec<_> = params
2696-
.args
2697-
.iter()
2698-
.filter_map(|arg| match arg {
2699-
hir::GenericArg::Type(ty) => Some(ty),
2700-
_ => None,
2701-
})
2702-
.collect();
2703-
types.len() == 1 && (is_self_ty(&types[0]) || is_ty(&*types[0], self_ty))
2704-
}
2705-
} else {
2706-
false
2707-
}
2708-
})
2709-
} else {
2710-
false
2711-
}
2712-
})
2713-
},
2714-
_ => false,
2715-
})
2716-
})
2717-
}
2718-
2719-
fn is_ty(ty: &hir::Ty, self_ty: &hir::Ty) -> bool {
2720-
match (&ty.node, &self_ty.node) {
2721-
(
2722-
&hir::TyKind::Path(hir::QPath::Resolved(_, ref ty_path)),
2723-
&hir::TyKind::Path(hir::QPath::Resolved(_, ref self_ty_path)),
2724-
) => ty_path
2725-
.segments
2726-
.iter()
2727-
.map(|seg| seg.ident.name)
2728-
.eq(self_ty_path.segments.iter().map(|seg| seg.ident.name)),
2729-
_ => false,
2730-
}
2731-
}
2732-
2733-
fn single_segment_ty(ty: &hir::Ty) -> Option<&hir::PathSegment> {
2734-
if let hir::TyKind::Path(ref path) = ty.node {
2735-
single_segment_path(path)
2736-
} else {
2737-
None
2738-
}
2739-
}
2740-
27412680
impl Convention {
27422681
fn check(&self, other: &str) -> bool {
27432682
match *self {

‎tests/ui/wrong_self_convention.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,22 @@ impl Bar {
5656
fn from_(self) {}
5757
fn to_mut(&mut self) {}
5858
}
59+
60+
// Allow Box<Self>, Rc<Self>, Arc<Self> for methods that take conventionally take Self by value
61+
#[allow(clippy::boxed_local)]
62+
mod issue4293 {
63+
use std::rc::Rc;
64+
use std::sync::Arc;
65+
66+
struct T;
67+
68+
impl T {
69+
fn into_s1(self: Box<Self>) {}
70+
fn into_s2(self: Rc<Self>) {}
71+
fn into_s3(self: Arc<Self>) {}
72+
73+
fn into_t1(self: Box<T>) {}
74+
fn into_t2(self: Rc<T>) {}
75+
fn into_t3(self: Arc<T>) {}
76+
}
77+
}

0 commit comments

Comments
 (0)