diff options
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 85 | ||||
| -rw-r--r-- | clippy_lints/src/utils/mod.rs | 11 | ||||
| -rw-r--r-- | tests/ui/new_ret_no_self.rs | 134 | ||||
| -rw-r--r-- | tests/ui/new_ret_no_self.stderr | 30 |
4 files changed, 230 insertions, 30 deletions
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 1ef54d285f6..9996df69470 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -15,11 +15,11 @@ use rustc_ast::ast; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::intravisit::{self, Visitor}; +use rustc_hir::{TraitItem, TraitItemKind}; use rustc_lint::{LateContext, LateLintPass, Lint, LintContext}; use rustc_middle::hir::map::Map; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::subst::GenericArgKind; -use rustc_middle::ty::{self, Ty, TyS}; +use rustc_middle::ty::{self, TraitRef, Ty, TyS}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; use rustc_span::symbol::{sym, SymbolStr}; @@ -27,12 +27,12 @@ use rustc_span::symbol::{sym, SymbolStr}; use crate::consts::{constant, Constant}; use crate::utils::usage::mutated_variables; use crate::utils::{ - get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, is_copy, - is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment, - match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls, method_chain_args, paths, - remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, - span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty, - walk_ptrs_ty_depth, SpanlessEq, + contains_ty, get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, + is_copy, is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats, + last_path_segment, match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls, + method_chain_args, paths, remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability, + snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, + span_lint_and_then, sugg, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq, }; declare_clippy_lint! { @@ -724,6 +724,7 @@ declare_clippy_lint! { /// **Known problems:** None. /// /// **Example:** + /// In an impl block: /// ```rust /// # struct Foo; /// # struct NotAFoo; @@ -736,25 +737,40 @@ declare_clippy_lint! { /// /// ```rust /// # struct Foo; - /// # struct FooError; + /// struct Bar(Foo); /// impl Foo { - /// // Good. Return type contains `Self` - /// fn new() -> Result<Foo, FooError> { - /// # Ok(Foo) + /// // Bad. The type name must contain `Self` + /// fn new() -> Bar { + /// # Bar(Foo) /// } /// } /// ``` /// /// ```rust /// # struct Foo; - /// struct Bar(Foo); + /// # struct FooError; /// impl Foo { - /// // Bad. The type name must contain `Self`. - /// fn new() -> Bar { - /// # Bar(Foo) + /// // Good. Return type contains `Self` + /// fn new() -> Result<Foo, FooError> { + /// # Ok(Foo) /// } /// } /// ``` + /// + /// Or in a trait definition: + /// ```rust + /// pub trait Trait { + /// // Bad. The type name must contain `Self` + /// fn new(); + /// } + /// ``` + /// + /// ```rust + /// pub trait Trait { + /// // Good. Return type contains `Self` + /// fn new() -> Self; + /// } + /// ``` pub NEW_RET_NO_SELF, style, "not returning type containing `Self` in a `new` method" @@ -1631,19 +1647,16 @@ impl<'tcx> LateLintPass<'tcx> for Methods { } } + // if this impl block implements a trait, lint in trait definition instead + if let hir::ItemKind::Impl { of_trait: Some(_), .. } = item.kind { + return; + } + if let hir::ImplItemKind::Fn(_, _) = impl_item.kind { let ret_ty = return_ty(cx, impl_item.hir_id); - let contains_self_ty = |ty: Ty<'tcx>| { - ty.walk().any(|inner| match inner.unpack() { - GenericArgKind::Type(inner_ty) => TyS::same_type(self_ty, inner_ty), - - GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, - }) - }; - // walk the return type and check for Self (this does not check associated types) - if contains_self_ty(ret_ty) { + if contains_ty(ret_ty, self_ty) { return; } @@ -1653,7 +1666,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods { for &(predicate, _span) in cx.tcx.predicates_of(def_id).predicates { if let ty::PredicateAtom::Projection(projection_predicate) = predicate.skip_binders() { // walk the associated type and check for Self - if contains_self_ty(projection_predicate.ty) { + if contains_ty(projection_predicate.ty, self_ty) { return; } } @@ -1670,6 +1683,26 @@ impl<'tcx> LateLintPass<'tcx> for Methods { } } } + + fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) { + if_chain! { + if !in_external_macro(cx.tcx.sess, item.span); + if item.ident.name == sym!(new); + if let TraitItemKind::Fn(_, _) = item.kind; + let ret_ty = return_ty(cx, item.hir_id); + let self_ty = TraitRef::identity(cx.tcx, item.hir_id.owner.to_def_id()).self_ty(); + if !contains_ty(ret_ty, self_ty); + + then { + span_lint( + cx, + NEW_RET_NO_SELF, + item.span, + "methods called `new` usually return `Self`", + ); + } + } + } } /// Checks for the `OR_FUN_CALL` lint. diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index e6be5c4588f..82005257115 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -42,7 +42,8 @@ use rustc_hir::{ use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::map::Map; -use rustc_middle::ty::{self, layout::IntegerExt, subst::GenericArg, Ty, TyCtxt, TypeFoldable}; +use rustc_middle::ty::subst::{GenericArg, GenericArgKind}; +use rustc_middle::ty::{self, layout::IntegerExt, Ty, TyCtxt, TypeFoldable}; use rustc_mir::const_eval; use rustc_span::hygiene::{ExpnKind, MacroKind}; 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> cx.tcx.erase_late_bound_regions(&ret_ty) } +/// Walks into `ty` and returns `true` if any inner type is the same as `other_ty` +pub fn contains_ty(ty: Ty<'_>, other_ty: Ty<'_>) -> bool { + ty.walk().any(|inner| match inner.unpack() { + GenericArgKind::Type(inner_ty) => ty::TyS::same_type(other_ty, inner_ty), + GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, + }) +} + /// Returns `true` if the given type is an `unsafe` function. pub fn type_is_unsafe_function<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { match ty.kind { diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 2c2d1e27589..e82873629a5 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -137,9 +137,9 @@ impl MutPointerReturnerOk { } } -struct MutPointerReturnerOk2; +struct ConstPointerReturnerOk2; -impl MutPointerReturnerOk2 { +impl ConstPointerReturnerOk2 { // should not trigger lint pub fn new() -> *const Self { unimplemented!(); @@ -210,3 +210,133 @@ impl<'a> WithLifetime<'a> { unimplemented!(); } } + +mod issue5435 { + struct V; + + pub trait TraitRetSelf { + // should not trigger lint + fn new() -> Self; + } + + pub trait TraitRet { + // should trigger lint as we are in trait definition + fn new() -> String; + } + pub struct StructRet; + impl TraitRet for StructRet { + // should not trigger lint as we are in the impl block + fn new() -> String { + unimplemented!(); + } + } + + pub trait TraitRet2 { + // should trigger lint + fn new(_: String) -> String; + } + + trait TupleReturnerOk { + // should not trigger lint + fn new() -> (Self, u32) + where + Self: Sized, + { + unimplemented!(); + } + } + + trait TupleReturnerOk2 { + // should not trigger lint (it doesn't matter which element in the tuple is Self) + fn new() -> (u32, Self) + where + Self: Sized, + { + unimplemented!(); + } + } + + trait TupleReturnerOk3 { + // should not trigger lint (tuple can contain multiple Self) + fn new() -> (Self, Self) + where + Self: Sized, + { + unimplemented!(); + } + } + + trait TupleReturnerBad { + // should trigger lint + fn new() -> (u32, u32) { + unimplemented!(); + } + } + + trait MutPointerReturnerOk { + // should not trigger lint + fn new() -> *mut Self + where + Self: Sized, + { + unimplemented!(); + } + } + + trait ConstPointerReturnerOk2 { + // should not trigger lint + fn new() -> *const Self + where + Self: Sized, + { + unimplemented!(); + } + } + + trait MutPointerReturnerBad { + // should trigger lint + fn new() -> *mut V { + unimplemented!(); + } + } + + trait GenericReturnerOk { + // should not trigger lint + fn new() -> Option<Self> + where + Self: Sized, + { + unimplemented!(); + } + } + + trait NestedReturnerOk { + // should not trigger lint + fn new() -> (Option<Self>, u32) + where + Self: Sized, + { + unimplemented!(); + } + } + + trait NestedReturnerOk2 { + // should not trigger lint + fn new() -> ((Self, u32), u32) + where + Self: Sized, + { + unimplemented!(); + } + } + + trait NestedReturnerOk3 { + // should not trigger lint + fn new() -> Option<(Self, u32)> + where + Self: Sized, + { + unimplemented!(); + } + } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index dd5a24bcbe7..8217bc6187f 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -48,5 +48,33 @@ LL | | unimplemented!(); LL | | } | |_____^ -error: aborting due to 6 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:224:9 + | +LL | fn new() -> String; + | ^^^^^^^^^^^^^^^^^^^ + +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:236:9 + | +LL | fn new(_: String) -> String; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:271:9 + | +LL | / fn new() -> (u32, u32) { +LL | | unimplemented!(); +LL | | } + | |_________^ + +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:298:9 + | +LL | / fn new() -> *mut V { +LL | | unimplemented!(); +LL | | } + | |_________^ + +error: aborting due to 10 previous errors |