Skip to content

Commit 3d0b0e6

Browse files
borsbors
committed
Auto merge of #5774 - ThibsG:FixNewRetNoSelf, r=ebroto
Fix FP in `new_ret_no_self`: trigger in trait def instead of impl block Lint in trait def instead of impl block. Fixes: #5435 changelog: none
2 parents 07c5e9e + 73b1ee1 commit 3d0b0e6

File tree

4 files changed

+230
-30
lines changed

4 files changed

+230
-30
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 59 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,24 +15,24 @@ use rustc_ast::ast;
1515
use rustc_errors::Applicability;
1616
use rustc_hir as hir;
1717
use rustc_hir::intravisit::{self, Visitor};
18+
use rustc_hir::{TraitItem, TraitItemKind};
1819
use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
1920
use rustc_middle::hir::map::Map;
2021
use rustc_middle::lint::in_external_macro;
21-
use rustc_middle::ty::subst::GenericArgKind;
22-
use rustc_middle::ty::{self, Ty, TyS};
22+
use rustc_middle::ty::{self, TraitRef, Ty, TyS};
2323
use rustc_session::{declare_lint_pass, declare_tool_lint};
2424
use rustc_span::source_map::Span;
2525
use rustc_span::symbol::{sym, SymbolStr};
2626

2727
use crate::consts::{constant, Constant};
2828
use crate::utils::usage::mutated_variables;
2929
use crate::utils::{
30-
get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, is_copy,
31-
is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment,
32-
match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls, method_chain_args, paths,
33-
remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite,
34-
span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty,
35-
walk_ptrs_ty_depth, SpanlessEq,
30+
contains_ty, get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro,
31+
is_copy, is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats,
32+
last_path_segment, match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls,
33+
method_chain_args, paths, remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability,
34+
snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
35+
span_lint_and_then, sugg, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
3636
};
3737

3838
declare_clippy_lint! {
@@ -724,6 +724,7 @@ declare_clippy_lint! {
724724
/// **Known problems:** None.
725725
///
726726
/// **Example:**
727+
/// In an impl block:
727728
/// ```rust
728729
/// # struct Foo;
729730
/// # struct NotAFoo;
@@ -736,25 +737,40 @@ declare_clippy_lint! {
736737
///
737738
/// ```rust
738739
/// # struct Foo;
739-
/// # struct FooError;
740+
/// struct Bar(Foo);
740741
/// impl Foo {
741-
/// // Good. Return type contains `Self`
742-
/// fn new() -> Result<Foo, FooError> {
743-
/// # Ok(Foo)
742+
/// // Bad. The type name must contain `Self`
743+
/// fn new() -> Bar {
744+
/// # Bar(Foo)
744745
/// }
745746
/// }
746747
/// ```
747748
///
748749
/// ```rust
749750
/// # struct Foo;
750-
/// struct Bar(Foo);
751+
/// # struct FooError;
751752
/// impl Foo {
752-
/// // Bad. The type name must contain `Self`.
753-
/// fn new() -> Bar {
754-
/// # Bar(Foo)
753+
/// // Good. Return type contains `Self`
754+
/// fn new() -> Result<Foo, FooError> {
755+
/// # Ok(Foo)
755756
/// }
756757
/// }
757758
/// ```
759+
///
760+
/// Or in a trait definition:
761+
/// ```rust
762+
/// pub trait Trait {
763+
/// // Bad. The type name must contain `Self`
764+
/// fn new();
765+
/// }
766+
/// ```
767+
///
768+
/// ```rust
769+
/// pub trait Trait {
770+
/// // Good. Return type contains `Self`
771+
/// fn new() -> Self;
772+
/// }
773+
/// ```
758774
pub NEW_RET_NO_SELF,
759775
style,
760776
"not returning type containing `Self` in a `new` method"
@@ -1631,19 +1647,16 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
16311647
}
16321648
}
16331649

1650+
// if this impl block implements a trait, lint in trait definition instead
1651+
if let hir::ItemKind::Impl { of_trait: Some(_), .. } = item.kind {
1652+
return;
1653+
}
1654+
16341655
if let hir::ImplItemKind::Fn(_, _) = impl_item.kind {
16351656
let ret_ty = return_ty(cx, impl_item.hir_id);
16361657

1637-
let contains_self_ty = |ty: Ty<'tcx>| {
1638-
ty.walk().any(|inner| match inner.unpack() {
1639-
GenericArgKind::Type(inner_ty) => TyS::same_type(self_ty, inner_ty),
1640-
1641-
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
1642-
})
1643-
};
1644-
16451658
// walk the return type and check for Self (this does not check associated types)
1646-
if contains_self_ty(ret_ty) {
1659+
if contains_ty(ret_ty, self_ty) {
16471660
return;
16481661
}
16491662

@@ -1653,7 +1666,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
16531666
for &(predicate, _span) in cx.tcx.predicates_of(def_id).predicates {
16541667
if let ty::PredicateAtom::Projection(projection_predicate) = predicate.skip_binders() {
16551668
// walk the associated type and check for Self
1656-
if contains_self_ty(projection_predicate.ty) {
1669+
if contains_ty(projection_predicate.ty, self_ty) {
16571670
return;
16581671
}
16591672
}
@@ -1670,6 +1683,26 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
16701683
}
16711684
}
16721685
}
1686+
1687+
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) {
1688+
if_chain! {
1689+
if !in_external_macro(cx.tcx.sess, item.span);
1690+
if item.ident.name == sym!(new);
1691+
if let TraitItemKind::Fn(_, _) = item.kind;
1692+
let ret_ty = return_ty(cx, item.hir_id);
1693+
let self_ty = TraitRef::identity(cx.tcx, item.hir_id.owner.to_def_id()).self_ty();
1694+
if !contains_ty(ret_ty, self_ty);
1695+
1696+
then {
1697+
span_lint(
1698+
cx,
1699+
NEW_RET_NO_SELF,
1700+
item.span,
1701+
"methods called `new` usually return `Self`",
1702+
);
1703+
}
1704+
}
1705+
}
16731706
}
16741707

16751708
/// Checks for the `OR_FUN_CALL` lint.

‎clippy_lints/src/utils/mod.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ use rustc_hir::{
4242
use rustc_infer::infer::TyCtxtInferExt;
4343
use rustc_lint::{LateContext, Level, Lint, LintContext};
4444
use rustc_middle::hir::map::Map;
45-
use rustc_middle::ty::{self, layout::IntegerExt, subst::GenericArg, Ty, TyCtxt, TypeFoldable};
45+
use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
46+
use rustc_middle::ty::{self, layout::IntegerExt, Ty, TyCtxt, TypeFoldable};
4647
use rustc_mir::const_eval;
4748
use rustc_span::hygiene::{ExpnKind, MacroKind};
4849
use rustc_span::source_map::original_sp;
@@ -866,6 +867,14 @@ pub fn return_ty<'tcx>(cx: &LateContext<'tcx>, fn_item: hir::HirId) -> Ty<'tcx>
866867
cx.tcx.erase_late_bound_regions(&ret_ty)
867868
}
868869

870+
/// Walks into `ty` and returns `true` if any inner type is the same as `other_ty`
871+
pub fn contains_ty(ty: Ty<'_>, other_ty: Ty<'_>) -> bool {
872+
ty.walk().any(|inner| match inner.unpack() {
873+
GenericArgKind::Type(inner_ty) => ty::TyS::same_type(other_ty, inner_ty),
874+
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
875+
})
876+
}
877+
869878
/// Returns `true` if the given type is an `unsafe` function.
870879
pub fn type_is_unsafe_function<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
871880
match ty.kind {

‎tests/ui/new_ret_no_self.rs

Lines changed: 132 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,9 +137,9 @@ impl MutPointerReturnerOk {
137137
}
138138
}
139139

140-
struct MutPointerReturnerOk2;
140+
struct ConstPointerReturnerOk2;
141141

142-
impl MutPointerReturnerOk2 {
142+
impl ConstPointerReturnerOk2 {
143143
// should not trigger lint
144144
pub fn new() -> *const Self {
145145
unimplemented!();
@@ -210,3 +210,133 @@ impl<'a> WithLifetime<'a> {
210210
unimplemented!();
211211
}
212212
}
213+
214+
mod issue5435 {
215+
struct V;
216+
217+
pub trait TraitRetSelf {
218+
// should not trigger lint
219+
fn new() -> Self;
220+
}
221+
222+
pub trait TraitRet {
223+
// should trigger lint as we are in trait definition
224+
fn new() -> String;
225+
}
226+
pub struct StructRet;
227+
impl TraitRet for StructRet {
228+
// should not trigger lint as we are in the impl block
229+
fn new() -> String {
230+
unimplemented!();
231+
}
232+
}
233+
234+
pub trait TraitRet2 {
235+
// should trigger lint
236+
fn new(_: String) -> String;
237+
}
238+
239+
trait TupleReturnerOk {
240+
// should not trigger lint
241+
fn new() -> (Self, u32)
242+
where
243+
Self: Sized,
244+
{
245+
unimplemented!();
246+
}
247+
}
248+
249+
trait TupleReturnerOk2 {
250+
// should not trigger lint (it doesn't matter which element in the tuple is Self)
251+
fn new() -> (u32, Self)
252+
where
253+
Self: Sized,
254+
{
255+
unimplemented!();
256+
}
257+
}
258+
259+
trait TupleReturnerOk3 {
260+
// should not trigger lint (tuple can contain multiple Self)
261+
fn new() -> (Self, Self)
262+
where
263+
Self: Sized,
264+
{
265+
unimplemented!();
266+
}
267+
}
268+
269+
trait TupleReturnerBad {
270+
// should trigger lint
271+
fn new() -> (u32, u32) {
272+
unimplemented!();
273+
}
274+
}
275+
276+
trait MutPointerReturnerOk {
277+
// should not trigger lint
278+
fn new() -> *mut Self
279+
where
280+
Self: Sized,
281+
{
282+
unimplemented!();
283+
}
284+
}
285+
286+
trait ConstPointerReturnerOk2 {
287+
// should not trigger lint
288+
fn new() -> *const Self
289+
where
290+
Self: Sized,
291+
{
292+
unimplemented!();
293+
}
294+
}
295+
296+
trait MutPointerReturnerBad {
297+
// should trigger lint
298+
fn new() -> *mut V {
299+
unimplemented!();
300+
}
301+
}
302+
303+
trait GenericReturnerOk {
304+
// should not trigger lint
305+
fn new() -> Option<Self>
306+
where
307+
Self: Sized,
308+
{
309+
unimplemented!();
310+
}
311+
}
312+
313+
trait NestedReturnerOk {
314+
// should not trigger lint
315+
fn new() -> (Option<Self>, u32)
316+
where
317+
Self: Sized,
318+
{
319+
unimplemented!();
320+
}
321+
}
322+
323+
trait NestedReturnerOk2 {
324+
// should not trigger lint
325+
fn new() -> ((Self, u32), u32)
326+
where
327+
Self: Sized,
328+
{
329+
unimplemented!();
330+
}
331+
}
332+
333+
trait NestedReturnerOk3 {
334+
// should not trigger lint
335+
fn new() -> Option<(Self, u32)>
336+
where
337+
Self: Sized,
338+
{
339+
unimplemented!();
340+
}
341+
}
342+
}

‎tests/ui/new_ret_no_self.stderr

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,5 +48,33 @@ LL | | unimplemented!();
4848
LL | | }
4949
| |_____^
5050

51-
error: aborting due to 6 previous errors
51+
error: methods called `new` usually return `Self`
52+
--> $DIR/new_ret_no_self.rs:224:9
53+
|
54+
LL | fn new() -> String;
55+
| ^^^^^^^^^^^^^^^^^^^
56+
57+
error: methods called `new` usually return `Self`
58+
--> $DIR/new_ret_no_self.rs:236:9
59+
|
60+
LL | fn new(_: String) -> String;
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
62+
63+
error: methods called `new` usually return `Self`
64+
--> $DIR/new_ret_no_self.rs:271:9
65+
|
66+
LL | / fn new() -> (u32, u32) {
67+
LL | | unimplemented!();
68+
LL | | }
69+
| |_________^
70+
71+
error: methods called `new` usually return `Self`
72+
--> $DIR/new_ret_no_self.rs:298:9
73+
|
74+
LL | / fn new() -> *mut V {
75+
LL | | unimplemented!();
76+
LL | | }
77+
| |_________^
78+
79+
error: aborting due to 10 previous errors
5280

0 commit comments

Comments
 (0)