diff --git a/CHANGELOG.md b/CHANGELOG.md index 776b04295f94..43d83d978b8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1454,6 +1454,7 @@ Released 2018-09-13 [`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver [`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof [`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq +[`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord [`diverging_sub_expression`]: https://rust-lang.github.io/rust-clippy/master/index.html#diverging_sub_expression [`doc_markdown`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown [`double_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#double_comparisons @@ -1723,6 +1724,7 @@ Released 2018-09-13 [`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments [`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines [`toplevel_ref_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg +[`trait_duplication_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#trait_duplication_in_bounds [`transmute_bytes_to_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_bytes_to_str [`transmute_float_to_int`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_float_to_int [`transmute_int_to_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_int_to_bool diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 27a7fa886223..dcd4e1dd438f 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -1,6 +1,5 @@ //! checks for attributes -use crate::reexport::Name; use crate::utils::{ first_line_of_span, is_present_in_source, match_def_path, paths, snippet_opt, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, without_block_comments, @@ -517,7 +516,7 @@ fn is_relevant_expr(cx: &LateContext<'_>, typeck_results: &ty::TypeckResults<'_> } } -fn check_attrs(cx: &LateContext<'_>, span: Span, name: Name, attrs: &[Attribute]) { +fn check_attrs(cx: &LateContext<'_>, span: Span, name: Symbol, attrs: &[Attribute]) { if span.from_expansion() { return; } diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index 59c62f1ae944..08d8100a8854 100644 --- a/clippy_lints/src/derive.rs +++ b/clippy_lints/src/derive.rs @@ -1,6 +1,7 @@ use crate::utils::paths; use crate::utils::{ - is_automatically_derived, is_copy, match_path, span_lint_and_help, span_lint_and_note, span_lint_and_then, + get_trait_def_id, is_automatically_derived, is_copy, match_path, span_lint_and_help, span_lint_and_note, + span_lint_and_then, }; use if_chain::if_chain; use rustc_hir::def_id::DefId; @@ -43,6 +44,57 @@ declare_clippy_lint! { "deriving `Hash` but implementing `PartialEq` explicitly" } +declare_clippy_lint! { + /// **What it does:** Checks for deriving `Ord` but implementing `PartialOrd` + /// explicitly or vice versa. + /// + /// **Why is this bad?** The implementation of these traits must agree (for + /// example for use with `sort`) so it’s probably a bad idea to use a + /// default-generated `Ord` implementation with an explicitly defined + /// `PartialOrd`. In particular, the following must hold for any type + /// implementing `Ord`: + /// + /// ```text + /// k1.cmp(&k2) == k1.partial_cmp(&k2).unwrap() + /// ``` + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust,ignore + /// #[derive(Ord, PartialEq, Eq)] + /// struct Foo; + /// + /// impl PartialOrd for Foo { + /// ... + /// } + /// ``` + /// Use instead: + /// ```rust,ignore + /// #[derive(PartialEq, Eq)] + /// struct Foo; + /// + /// impl PartialOrd for Foo { + /// fn partial_cmp(&self, other: &Foo) -> Option { + /// Some(self.cmp(other)) + /// } + /// } + /// + /// impl Ord for Foo { + /// ... + /// } + /// ``` + /// or, if you don't need a custom ordering: + /// ```rust,ignore + /// #[derive(Ord, PartialOrd, PartialEq, Eq)] + /// struct Foo; + /// ``` + pub DERIVE_ORD_XOR_PARTIAL_ORD, + correctness, + "deriving `Ord` but implementing `PartialOrd` explicitly" +} + declare_clippy_lint! { /// **What it does:** Checks for explicit `Clone` implementations for `Copy` /// types. @@ -103,7 +155,12 @@ declare_clippy_lint! { "deriving `serde::Deserialize` on a type that has methods using `unsafe`" } -declare_lint_pass!(Derive => [EXPL_IMPL_CLONE_ON_COPY, DERIVE_HASH_XOR_EQ, UNSAFE_DERIVE_DESERIALIZE]); +declare_lint_pass!(Derive => [ + EXPL_IMPL_CLONE_ON_COPY, + DERIVE_HASH_XOR_EQ, + DERIVE_ORD_XOR_PARTIAL_ORD, + UNSAFE_DERIVE_DESERIALIZE +]); impl<'tcx> LateLintPass<'tcx> for Derive { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { @@ -116,6 +173,7 @@ impl<'tcx> LateLintPass<'tcx> for Derive { let is_automatically_derived = is_automatically_derived(&*item.attrs); check_hash_peq(cx, item.span, trait_ref, ty, is_automatically_derived); + check_ord_partial_ord(cx, item.span, trait_ref, ty, is_automatically_derived); if is_automatically_derived { check_unsafe_derive_deserialize(cx, item, trait_ref, ty); @@ -180,6 +238,60 @@ fn check_hash_peq<'tcx>( } } +/// Implementation of the `DERIVE_ORD_XOR_PARTIAL_ORD` lint. +fn check_ord_partial_ord<'tcx>( + cx: &LateContext<'tcx>, + span: Span, + trait_ref: &TraitRef<'_>, + ty: Ty<'tcx>, + ord_is_automatically_derived: bool, +) { + if_chain! { + if let Some(ord_trait_def_id) = get_trait_def_id(cx, &paths::ORD); + if let Some(partial_ord_trait_def_id) = cx.tcx.lang_items().partial_ord_trait(); + if let Some(def_id) = &trait_ref.trait_def_id(); + if *def_id == ord_trait_def_id; + then { + // Look for the PartialOrd implementations for `ty` + cx.tcx.for_each_relevant_impl(partial_ord_trait_def_id, ty, |impl_id| { + let partial_ord_is_automatically_derived = is_automatically_derived(&cx.tcx.get_attrs(impl_id)); + + if partial_ord_is_automatically_derived == ord_is_automatically_derived { + return; + } + + let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation"); + + // Only care about `impl PartialOrd for Foo` + // For `impl PartialOrd for A, input_types is [A, B] + if trait_ref.substs.type_at(1) == ty { + let mess = if partial_ord_is_automatically_derived { + "you are implementing `Ord` explicitly but have derived `PartialOrd`" + } else { + "you are deriving `Ord` but have implemented `PartialOrd` explicitly" + }; + + span_lint_and_then( + cx, + DERIVE_ORD_XOR_PARTIAL_ORD, + span, + mess, + |diag| { + if let Some(local_def_id) = impl_id.as_local() { + let hir_id = cx.tcx.hir().as_local_hir_id(local_def_id); + diag.span_note( + cx.tcx.hir().span(hir_id), + "`PartialOrd` implemented here" + ); + } + } + ); + } + }); + } + } +} + /// Implementation of the `EXPL_IMPL_CLONE_ON_COPY` lint. fn check_copy_clone<'tcx>(cx: &LateContext<'tcx>, item: &Item<'_>, trait_ref: &TraitRef<'_>, ty: Ty<'tcx>) { if match_path(&trait_ref.path, &paths::CLONE_TRAIT) { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f371942dbeec..26aff6af8cdb 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -322,10 +322,6 @@ mod zero_div_zero; pub use crate::utils::conf::Conf; -mod reexport { - pub use rustc_span::Symbol as Name; -} - /// Register all pre expansion lints /// /// Pre-expansion lints run before any macro expansion has happened. @@ -513,6 +509,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &default_trait_access::DEFAULT_TRAIT_ACCESS, &dereference::EXPLICIT_DEREF_METHODS, &derive::DERIVE_HASH_XOR_EQ, + &derive::DERIVE_ORD_XOR_PARTIAL_ORD, &derive::EXPL_IMPL_CLONE_ON_COPY, &derive::UNSAFE_DERIVE_DESERIALIZE, &doc::DOC_MARKDOWN, @@ -786,6 +783,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &tabs_in_doc_comments::TABS_IN_DOC_COMMENTS, &temporary_assignment::TEMPORARY_ASSIGNMENT, &to_digit_is_some::TO_DIGIT_IS_SOME, + &trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS, &trait_bounds::TYPE_REPETITION_IN_BOUNDS, &transmute::CROSSPOINTER_TRANSMUTE, &transmute::TRANSMUTE_BYTES_TO_STR, @@ -1174,6 +1172,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&ranges::RANGE_PLUS_ONE), LintId::of(&shadow::SHADOW_UNRELATED), LintId::of(&strings::STRING_ADD_ASSIGN), + LintId::of(&trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS), LintId::of(&trait_bounds::TYPE_REPETITION_IN_BOUNDS), LintId::of(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF), LintId::of(&types::CAST_LOSSLESS), @@ -1230,6 +1229,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&copies::IFS_SAME_COND), LintId::of(&copies::IF_SAME_THEN_ELSE), LintId::of(&derive::DERIVE_HASH_XOR_EQ), + LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD), LintId::of(&doc::MISSING_SAFETY_DOC), LintId::of(&doc::NEEDLESS_DOCTEST_MAIN), LintId::of(&double_comparison::DOUBLE_COMPARISONS), @@ -1648,6 +1648,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&copies::IFS_SAME_COND), LintId::of(&copies::IF_SAME_THEN_ELSE), LintId::of(&derive::DERIVE_HASH_XOR_EQ), + LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD), LintId::of(&drop_bounds::DROP_BOUNDS), LintId::of(&drop_forget_ref::DROP_COPY), LintId::of(&drop_forget_ref::DROP_REF), diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 168f9f953e4d..4df6827d77f9 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -13,9 +13,8 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; -use rustc_span::symbol::kw; +use rustc_span::symbol::{kw, Symbol}; -use crate::reexport::Name; use crate::utils::{in_macro, last_path_segment, span_lint, trait_ref_of_method}; declare_clippy_lint! { @@ -113,7 +112,7 @@ impl<'tcx> LateLintPass<'tcx> for Lifetimes { enum RefLt { Unnamed, Static, - Named(Name), + Named(Symbol), } fn check_fn_inner<'tcx>( @@ -456,7 +455,7 @@ fn has_where_lifetimes<'tcx>(cx: &LateContext<'tcx>, where_clause: &'tcx WhereCl } struct LifetimeChecker { - map: FxHashMap, + map: FxHashMap, } impl<'tcx> Visitor<'tcx> for LifetimeChecker { diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 7e3876ff49b4..6359c20040c7 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1,14 +1,13 @@ use crate::consts::constant; -use crate::reexport::Name; use crate::utils::paths; +use crate::utils::sugg::Sugg; use crate::utils::usage::{is_unused, mutated_variables}; use crate::utils::{ get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, - is_integer_const, is_no_std_crate, is_refutable, last_path_segment, match_trait_method, match_type, match_var, - multispan_sugg, snippet, snippet_opt, snippet_with_applicability, span_lint, span_lint_and_help, - span_lint_and_sugg, span_lint_and_then, SpanlessEq, + is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method, + match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, snippet_with_applicability, span_lint, + span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq, }; -use crate::utils::{is_type_diagnostic_item, qpath_res, sugg}; use if_chain::if_chain; use rustc_ast::ast; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; @@ -17,7 +16,7 @@ use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit::{walk_block, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor}; use rustc_hir::{ def_id, BinOpKind, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, GenericArg, HirId, InlineAsmOperand, - LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind, + Local, LoopSource, MatchSource, Mutability, Node, Pat, PatKind, QPath, Stmt, StmtKind, }; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -27,7 +26,7 @@ use rustc_middle::middle::region; use rustc_middle::ty::{self, Ty, TyS}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; -use rustc_span::symbol::Symbol; +use rustc_span::symbol::{sym, Ident, Symbol}; use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use std::iter::{once, Iterator}; use std::mem; @@ -1184,7 +1183,7 @@ fn check_for_loop_range<'tcx>( } } -fn is_len_call(expr: &Expr<'_>, var: Name) -> bool { +fn is_len_call(expr: &Expr<'_>, var: Symbol) -> bool { if_chain! { if let ExprKind::MethodCall(ref method, _, ref len_args, _) = expr.kind; if len_args.len() == 1; @@ -1640,15 +1639,15 @@ struct VarVisitor<'a, 'tcx> { /// var name to look for as index var: HirId, /// indexed variables that are used mutably - indexed_mut: FxHashSet, + indexed_mut: FxHashSet, /// indirectly indexed variables (`v[(i + 4) % N]`), the extend is `None` for global - indexed_indirectly: FxHashMap>, + indexed_indirectly: FxHashMap>, /// subset of `indexed` of vars that are indexed directly: `v[i]` /// this will not contain cases like `v[calc_index(i)]` or `v[(i + 4) % N]` - indexed_directly: FxHashMap, Ty<'tcx>)>, + indexed_directly: FxHashMap, Ty<'tcx>)>, /// Any names that are used outside an index operation. /// Used to detect things like `&mut vec` used together with `vec[i]` - referenced: FxHashSet, + referenced: FxHashSet, /// has the loop variable been used in expressions other than the index of /// an index op? nonindex: bool, @@ -2004,7 +2003,7 @@ struct InitializeVisitor<'a, 'tcx> { end_expr: &'tcx Expr<'tcx>, // the for loop. Stop scanning here. var_id: HirId, state: VarState, - name: Option, + name: Option, depth: u32, // depth of conditional expressions past_loop: bool, } @@ -2167,7 +2166,7 @@ use self::Nesting::{LookFurther, RuledOut, Unknown}; struct LoopNestVisitor { hir_id: HirId, - iterator: Name, + iterator: Symbol, nesting: Nesting, } @@ -2218,7 +2217,7 @@ impl<'tcx> Visitor<'tcx> for LoopNestVisitor { } } -fn path_name(e: &Expr<'_>) -> Option { +fn path_name(e: &Expr<'_>) -> Option { if let ExprKind::Path(QPath::Resolved(_, ref path)) = e.kind { let segments = &path.segments; if segments.len() == 1 { @@ -2358,6 +2357,10 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { + check_needless_collect_direct_usage(expr, cx); + check_needless_collect_indirect_usage(expr, cx); +} +fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { if_chain! { if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind; if let ExprKind::MethodCall(ref chain_method, _, _, _) = args[0].kind; @@ -2425,6 +2428,157 @@ fn check_needless_collect<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { } } +fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) { + if let ExprKind::Block(ref block, _) = expr.kind { + for ref stmt in block.stmts { + if_chain! { + if let StmtKind::Local( + Local { pat: Pat { kind: PatKind::Binding(_, _, ident, .. ), .. }, + init: Some(ref init_expr), .. } + ) = stmt.kind; + if let ExprKind::MethodCall(ref method_name, _, &[ref iter_source], ..) = init_expr.kind; + if method_name.ident.name == sym!(collect) && match_trait_method(cx, &init_expr, &paths::ITERATOR); + if let Some(ref generic_args) = method_name.args; + if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0); + if let ty = cx.typeck_results().node_type(ty.hir_id); + if is_type_diagnostic_item(cx, ty, sym::vec_type) || + is_type_diagnostic_item(cx, ty, sym!(vecdeque_type)) || + match_type(cx, ty, &paths::LINKED_LIST); + if let Some(iter_calls) = detect_iter_and_into_iters(block, *ident); + if iter_calls.len() == 1; + then { + // Suggest replacing iter_call with iter_replacement, and removing stmt + let iter_call = &iter_calls[0]; + span_lint_and_then( + cx, + NEEDLESS_COLLECT, + stmt.span.until(iter_call.span), + NEEDLESS_COLLECT_MSG, + |diag| { + let iter_replacement = format!("{}{}", Sugg::hir(cx, iter_source, ".."), iter_call.get_iter_method(cx)); + diag.multipart_suggestion( + iter_call.get_suggestion_text(), + vec![ + (stmt.span, String::new()), + (iter_call.span, iter_replacement) + ], + Applicability::MachineApplicable,// MaybeIncorrect, + ).emit(); + }, + ); + } + } + } + } +} + +struct IterFunction { + func: IterFunctionKind, + span: Span, +} +impl IterFunction { + fn get_iter_method(&self, cx: &LateContext<'_>) -> String { + match &self.func { + IterFunctionKind::IntoIter => String::new(), + IterFunctionKind::Len => String::from(".count()"), + IterFunctionKind::IsEmpty => String::from(".next().is_none()"), + IterFunctionKind::Contains(span) => format!(".any(|x| x == {})", snippet(cx, *span, "..")), + } + } + fn get_suggestion_text(&self) -> &'static str { + match &self.func { + IterFunctionKind::IntoIter => { + "Use the original Iterator instead of collecting it and then producing a new one" + }, + IterFunctionKind::Len => { + "Take the original Iterator's count instead of collecting it and finding the length" + }, + IterFunctionKind::IsEmpty => { + "Check if the original Iterator has anything instead of collecting it and seeing if it's empty" + }, + IterFunctionKind::Contains(_) => { + "Check if the original Iterator contains an element instead of collecting then checking" + }, + } + } +} +enum IterFunctionKind { + IntoIter, + Len, + IsEmpty, + Contains(Span), +} + +struct IterFunctionVisitor { + uses: Vec, + seen_other: bool, + target: Ident, +} +impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + // Check function calls on our collection + if_chain! { + if let ExprKind::MethodCall(method_name, _, ref args, _) = &expr.kind; + if let Some(Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. }) = args.get(0); + if let &[name] = &path.segments; + if name.ident == self.target; + then { + let len = sym!(len); + let is_empty = sym!(is_empty); + let contains = sym!(contains); + match method_name.ident.name { + sym::into_iter => self.uses.push( + IterFunction { func: IterFunctionKind::IntoIter, span: expr.span } + ), + name if name == len => self.uses.push( + IterFunction { func: IterFunctionKind::Len, span: expr.span } + ), + name if name == is_empty => self.uses.push( + IterFunction { func: IterFunctionKind::IsEmpty, span: expr.span } + ), + name if name == contains => self.uses.push( + IterFunction { func: IterFunctionKind::Contains(args[1].span), span: expr.span } + ), + _ => self.seen_other = true, + } + return + } + } + // Check if the collection is used for anything else + if_chain! { + if let Expr { kind: ExprKind::Path(QPath::Resolved(_, ref path)), .. } = expr; + if let &[name] = &path.segments; + if name.ident == self.target; + then { + self.seen_other = true; + } else { + walk_expr(self, expr); + } + } + } + + type Map = Map<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +} + +/// Detect the occurences of calls to `iter` or `into_iter` for the +/// given identifier +fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option> { + let mut visitor = IterFunctionVisitor { + uses: Vec::new(), + target: identifier, + seen_other: false, + }; + visitor.visit_block(block); + if visitor.seen_other { + None + } else { + Some(visitor.uses) + } +} + fn shorten_span(expr: &Expr<'_>, target_fn_name: Symbol) -> Span { let mut current_expr = expr; while let ExprKind::MethodCall(ref path, ref span, ref args, _) = current_expr.kind { diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9edcdd979ff4..72828bde27aa 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1496,16 +1496,31 @@ impl<'tcx> LateLintPass<'tcx> for Methods { then { if cx.access_levels.is_exported(impl_item.hir_id) { - // check missing trait implementations - for &(method_name, n_args, fn_header, self_kind, out_type, trait_name) in &TRAIT_METHODS { - if name == method_name && - sig.decl.inputs.len() == n_args && - out_type.matches(cx, &sig.decl.output) && - self_kind.matches(cx, self_ty, first_arg_ty) && - fn_header_equals(*fn_header, sig.header) { - span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!( - "defining a method called `{}` on this type; consider implementing \ - the `{}` trait or choosing a less ambiguous name", name, trait_name)); + // check missing trait implementations + for method_config in &TRAIT_METHODS { + if name == method_config.method_name && + sig.decl.inputs.len() == method_config.param_count && + method_config.output_type.matches(cx, &sig.decl.output) && + method_config.self_kind.matches(cx, self_ty, first_arg_ty) && + fn_header_equals(*method_config.fn_header, sig.header) && + method_config.lifetime_param_cond(&impl_item) + { + span_lint_and_help( + cx, + SHOULD_IMPLEMENT_TRAIT, + impl_item.span, + &format!( + "method `{}` can be confused for the standard trait method `{}::{}`", + method_config.method_name, + method_config.trait_name, + method_config.method_name + ), + None, + &format!( + "consider implementing the trait `{}` or choosing a less ambiguous method name", + method_config.trait_name + ) + ); } } } @@ -2569,17 +2584,34 @@ fn lint_ok_expect(cx: &LateContext<'_>, expr: &hir::Expr<'_>, ok_args: &[hir::Ex fn lint_map_flatten<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, map_args: &'tcx [hir::Expr<'_>]) { // lint if caller of `.map().flatten()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `map(..).flatten()` on an `Iterator`. \ - This is more succinctly expressed by calling `.flat_map(..)`"; - let self_snippet = snippet(cx, map_args[0].span, ".."); + let map_closure_ty = cx.typeck_results().expr_ty(&map_args[1]); + let is_map_to_option = match map_closure_ty.kind { + ty::Closure(_, _) | ty::FnDef(_, _) | ty::FnPtr(_) => { + let map_closure_sig = match map_closure_ty.kind { + ty::Closure(_, substs) => substs.as_closure().sig(), + _ => map_closure_ty.fn_sig(cx.tcx), + }; + let map_closure_return_ty = cx.tcx.erase_late_bound_regions(&map_closure_sig.output()); + is_type_diagnostic_item(cx, map_closure_return_ty, sym!(option_type)) + }, + _ => false, + }; + + let method_to_use = if is_map_to_option { + // `(...).map(...)` has type `impl Iterator> + "filter_map" + } else { + // `(...).map(...)` has type `impl Iterator> + "flat_map" + }; let func_snippet = snippet(cx, map_args[1].span, ".."); - let hint = format!("{0}.flat_map({1})", self_snippet, func_snippet); + let hint = format!(".{0}({1})", method_to_use, func_snippet); span_lint_and_sugg( cx, MAP_FLATTEN, - expr.span, - msg, - "try using `flat_map` instead", + expr.span.with_lo(map_args[0].span.hi()), + "called `map(..).flatten()` on an `Iterator`", + &format!("try using `{}` instead", method_to_use), hint, Applicability::MachineApplicable, ); @@ -2587,16 +2619,13 @@ fn lint_map_flatten<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, map // lint if caller of `.map().flatten()` is an Option if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&map_args[0]), sym!(option_type)) { - let msg = "called `map(..).flatten()` on an `Option`. \ - This is more succinctly expressed by calling `.and_then(..)`"; - let self_snippet = snippet(cx, map_args[0].span, ".."); let func_snippet = snippet(cx, map_args[1].span, ".."); - let hint = format!("{0}.and_then({1})", self_snippet, func_snippet); + let hint = format!(".and_then({})", func_snippet); span_lint_and_sugg( cx, MAP_FLATTEN, - expr.span, - msg, + expr.span.with_lo(map_args[0].span.hi()), + "called `map(..).flatten()` on an `Option`", "try using `and_then` instead", hint, Applicability::MachineApplicable, @@ -3390,38 +3419,85 @@ const FN_HEADER: hir::FnHeader = hir::FnHeader { abi: rustc_target::spec::abi::Abi::Rust, }; +struct ShouldImplTraitCase { + trait_name: &'static str, + method_name: &'static str, + param_count: usize, + fn_header: &'static hir::FnHeader, + // implicit self kind expected (none, self, &self, ...) + self_kind: SelfKind, + // checks against the output type + output_type: OutType, + // certain methods with explicit lifetimes can't implement the equivalent trait method + lint_explicit_lifetime: bool, +} +impl ShouldImplTraitCase { + const fn new( + trait_name: &'static str, + method_name: &'static str, + param_count: usize, + fn_header: &'static hir::FnHeader, + self_kind: SelfKind, + output_type: OutType, + lint_explicit_lifetime: bool, + ) -> ShouldImplTraitCase { + ShouldImplTraitCase { + trait_name, + method_name, + param_count, + fn_header, + self_kind, + output_type, + lint_explicit_lifetime, + } + } + + fn lifetime_param_cond(&self, impl_item: &hir::ImplItem<'_>) -> bool { + self.lint_explicit_lifetime + || !impl_item.generics.params.iter().any(|p| { + matches!( + p.kind, + hir::GenericParamKind::Lifetime { + kind: hir::LifetimeParamKind::Explicit + } + ) + }) + } +} + #[rustfmt::skip] -const TRAIT_METHODS: [(&str, usize, &hir::FnHeader, SelfKind, OutType, &str); 30] = [ - ("add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Add"), - ("as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"), - ("as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"), - ("bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitAnd"), - ("bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitOr"), - ("bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitXor"), - ("borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"), - ("borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"), - ("clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::clone::Clone"), - ("cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::cmp::Ord"), - ("default", 0, &FN_HEADER, SelfKind::No, OutType::Any, "std::default::Default"), - ("deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Deref"), - ("deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"), - ("div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Div"), - ("drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"), - ("eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"), - ("from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::iter::FromIterator"), - ("from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::str::FromStr"), - ("hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, "std::hash::Hash"), - ("index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Index"), - ("index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"), - ("into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"), - ("mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Mul"), - ("neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Neg"), - ("next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"), - ("not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Not"), - ("rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Rem"), - ("shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shl"), - ("shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shr"), - ("sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Sub"), +const TRAIT_METHODS: [ShouldImplTraitCase; 30] = [ + ShouldImplTraitCase::new("std::ops::Add", "add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::convert::AsMut", "as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true), + ShouldImplTraitCase::new("std::convert::AsRef", "as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true), + ShouldImplTraitCase::new("std::ops::BitAnd", "bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::BitOr", "bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::BitXor", "bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::borrow::Borrow", "borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true), + ShouldImplTraitCase::new("std::borrow::BorrowMut", "borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true), + ShouldImplTraitCase::new("std::clone::Clone", "clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, true), + ShouldImplTraitCase::new("std::cmp::Ord", "cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, true), + // FIXME: default doesn't work + ShouldImplTraitCase::new("std::default::Default", "default", 0, &FN_HEADER, SelfKind::No, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Deref", "deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true), + ShouldImplTraitCase::new("std::ops::DerefMut", "deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true), + ShouldImplTraitCase::new("std::ops::Div", "div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Drop", "drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, true), + ShouldImplTraitCase::new("std::cmp::PartialEq", "eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, true), + ShouldImplTraitCase::new("std::iter::FromIterator", "from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, true), + ShouldImplTraitCase::new("std::str::FromStr", "from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, true), + ShouldImplTraitCase::new("std::hash::Hash", "hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, true), + ShouldImplTraitCase::new("std::ops::Index", "index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, true), + ShouldImplTraitCase::new("std::ops::IndexMut", "index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true), + ShouldImplTraitCase::new("std::iter::IntoIterator", "into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Mul", "mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Neg", "neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::iter::Iterator", "next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, false), + ShouldImplTraitCase::new("std::ops::Not", "not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Rem", "rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Shl", "shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Shr", "shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), + ShouldImplTraitCase::new("std::ops::Sub", "sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true), ]; #[rustfmt::skip] diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index 6f03e92bde36..2610157763a8 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -1,4 +1,3 @@ -use crate::reexport::Name; use crate::utils::{contains_name, higher, iter_input_pats, snippet, span_lint_and_then}; use rustc_hir::intravisit::FnKind; use rustc_hir::{ @@ -10,6 +9,7 @@ use rustc_middle::lint::in_external_macro; use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; +use rustc_span::symbol::Symbol; declare_clippy_lint! { /// **What it does:** Checks for bindings that shadow other bindings already in @@ -123,7 +123,7 @@ fn check_fn<'tcx>(cx: &LateContext<'tcx>, decl: &'tcx FnDecl<'_>, body: &'tcx Bo check_expr(cx, &body.value, &mut bindings); } -fn check_block<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'_>, bindings: &mut Vec<(Name, Span)>) { +fn check_block<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'_>, bindings: &mut Vec<(Symbol, Span)>) { let len = bindings.len(); for stmt in block.stmts { match stmt.kind { @@ -138,7 +138,7 @@ fn check_block<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'_>, bindings: & bindings.truncate(len); } -fn check_local<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>, bindings: &mut Vec<(Name, Span)>) { +fn check_local<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>, bindings: &mut Vec<(Symbol, Span)>) { if in_external_macro(cx.sess(), local.span) { return; } @@ -173,7 +173,7 @@ fn check_pat<'tcx>( pat: &'tcx Pat<'_>, init: Option<&'tcx Expr<'_>>, span: Span, - bindings: &mut Vec<(Name, Span)>, + bindings: &mut Vec<(Symbol, Span)>, ) { // TODO: match more stuff / destructuring match pat.kind { @@ -254,7 +254,7 @@ fn check_pat<'tcx>( fn lint_shadow<'tcx>( cx: &LateContext<'tcx>, - name: Name, + name: Symbol, span: Span, pattern_span: Span, init: Option<&'tcx Expr<'_>>, @@ -315,7 +315,7 @@ fn lint_shadow<'tcx>( } } -fn check_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, bindings: &mut Vec<(Name, Span)>) { +fn check_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, bindings: &mut Vec<(Symbol, Span)>) { if in_external_macro(cx.sess(), expr.span) { return; } @@ -351,7 +351,7 @@ fn check_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, bindings: &mut } } -fn check_ty<'tcx>(cx: &LateContext<'tcx>, ty: &'tcx Ty<'_>, bindings: &mut Vec<(Name, Span)>) { +fn check_ty<'tcx>(cx: &LateContext<'tcx>, ty: &'tcx Ty<'_>, bindings: &mut Vec<(Symbol, Span)>) { match ty.kind { TyKind::Slice(ref sty) => check_ty(cx, sty, bindings), TyKind::Array(ref fty, ref anon_const) => { @@ -371,7 +371,7 @@ fn check_ty<'tcx>(cx: &LateContext<'tcx>, ty: &'tcx Ty<'_>, bindings: &mut Vec<( } } -fn is_self_shadow(name: Name, expr: &Expr<'_>) -> bool { +fn is_self_shadow(name: Symbol, expr: &Expr<'_>) -> bool { match expr.kind { ExprKind::Box(ref inner) | ExprKind::AddrOf(_, _, ref inner) => is_self_shadow(name, inner), ExprKind::Block(ref block, _) => { @@ -383,6 +383,6 @@ fn is_self_shadow(name: Name, expr: &Expr<'_>) -> bool { } } -fn path_eq_name(name: Name, path: &Path<'_>) -> bool { +fn path_eq_name(name: Symbol, path: &Path<'_>) -> bool { !path.is_global() && path.segments.len() == 1 && path.segments[0].ident.as_str() == name.as_str() } diff --git a/clippy_lints/src/trait_bounds.rs b/clippy_lints/src/trait_bounds.rs index 0ef70311fb1c..06631f89f27d 100644 --- a/clippy_lints/src/trait_bounds.rs +++ b/clippy_lints/src/trait_bounds.rs @@ -2,9 +2,10 @@ use crate::utils::{in_macro, snippet, snippet_with_applicability, span_lint_and_ use if_chain::if_chain; use rustc_data_structures::fx::FxHashMap; use rustc_errors::Applicability; -use rustc_hir::{GenericBound, Generics, WherePredicate}; +use rustc_hir::{def::Res, GenericBound, Generics, ParamName, Path, QPath, TyKind, WherePredicate}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::Span; declare_clippy_lint! { /// **What it does:** This lint warns about unnecessary type repetitions in trait bounds @@ -29,6 +30,35 @@ declare_clippy_lint! { "Types are repeated unnecessary in trait bounds use `+` instead of using `T: _, T: _`" } +declare_clippy_lint! { + /// **What it does:** Checks for cases where generics are being used and multiple + /// syntax specifications for trait bounds are used simultaneously. + /// + /// **Why is this bad?** Duplicate bounds makes the code + /// less readable than specifing them only once. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// fn func(arg: T) where T: Clone + Default {} + /// ``` + /// + /// Could be written as: + /// + /// ```rust + /// fn func(arg: T) {} + /// ``` + /// or + /// /// + /// ```rust + /// fn func(arg: T) where T: Clone + Default {} + /// ``` + pub TRAIT_DUPLICATION_IN_BOUNDS, + pedantic, + "Check if the same trait bounds are specified twice during a function declaration" +} + #[derive(Copy, Clone)] pub struct TraitBounds { max_trait_bounds: u64, @@ -41,10 +71,25 @@ impl TraitBounds { } } -impl_lint_pass!(TraitBounds => [TYPE_REPETITION_IN_BOUNDS]); +impl_lint_pass!(TraitBounds => [TYPE_REPETITION_IN_BOUNDS, TRAIT_DUPLICATION_IN_BOUNDS]); impl<'tcx> LateLintPass<'tcx> for TraitBounds { fn check_generics(&mut self, cx: &LateContext<'tcx>, gen: &'tcx Generics<'_>) { + self.check_type_repetition(cx, gen); + check_trait_bound_duplication(cx, gen); + } +} + +fn get_trait_res_span_from_bound(bound: &GenericBound<'_>) -> Option<(Res, Span)> { + if let GenericBound::Trait(t, _) = bound { + Some((t.trait_ref.path.res, t.span)) + } else { + None + } +} + +impl TraitBounds { + fn check_type_repetition(self, cx: &LateContext<'_>, gen: &'_ Generics<'_>) { if in_macro(gen.span) { return; } @@ -101,3 +146,48 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds { } } } + +fn check_trait_bound_duplication(cx: &LateContext<'_>, gen: &'_ Generics<'_>) { + if in_macro(gen.span) || gen.params.is_empty() || gen.where_clause.predicates.is_empty() { + return; + } + + let mut map = FxHashMap::default(); + for param in gen.params { + if let ParamName::Plain(ref ident) = param.name { + let res = param + .bounds + .iter() + .filter_map(get_trait_res_span_from_bound) + .collect::>(); + map.insert(*ident, res); + } + } + + for predicate in gen.where_clause.predicates { + if_chain! { + if let WherePredicate::BoundPredicate(ref bound_predicate) = predicate; + if !in_macro(bound_predicate.span); + if let TyKind::Path(ref path) = bound_predicate.bounded_ty.kind; + if let QPath::Resolved(_, Path { ref segments, .. }) = path; + if let Some(segment) = segments.first(); + if let Some(trait_resolutions_direct) = map.get(&segment.ident); + then { + for (res_where, _) in bound_predicate.bounds.iter().filter_map(get_trait_res_span_from_bound) { + if let Some((_, span_direct)) = trait_resolutions_direct + .iter() + .find(|(res_direct, _)| *res_direct == res_where) { + span_lint_and_help( + cx, + TRAIT_DUPLICATION_IN_BOUNDS, + *span_direct, + "this trait bound is already specified in the where clause", + None, + "consider removing this trait bound", + ); + } + } + } + } + } +} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 655b1133cf74..b06ccf10db90 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -52,7 +52,6 @@ use rustc_trait_selection::traits::query::normalize::AtExt; use smallvec::SmallVec; use crate::consts::{constant, Constant}; -use crate::reexport::Name; /// Returns `true` if the two spans come from differing expansions (i.e., one is /// from a macro and one isn't). @@ -150,7 +149,7 @@ pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str]) } /// Checks if an expression references a variable of the given name. -pub fn match_var(expr: &Expr<'_>, var: Name) -> bool { +pub fn match_var(expr: &Expr<'_>, var: Symbol) -> bool { if let ExprKind::Path(QPath::Resolved(None, ref path)) = expr.kind { if let [p] = path.segments { return p.ident.name == var; @@ -420,7 +419,7 @@ pub fn is_entrypoint_fn(cx: &LateContext<'_>, def_id: DefId) -> bool { } /// Gets the name of the item the expression is in, if available. -pub fn get_item_name(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { +pub fn get_item_name(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { let parent_id = cx.tcx.hir().get_parent_item(expr.hir_id); match cx.tcx.hir().find(parent_id) { Some( @@ -433,7 +432,7 @@ pub fn get_item_name(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { } /// Gets the name of a `Pat`, if any. -pub fn get_pat_name(pat: &Pat<'_>) -> Option { +pub fn get_pat_name(pat: &Pat<'_>) -> Option { match pat.kind { PatKind::Binding(.., ref spname, _) => Some(spname.name), PatKind::Path(ref qpath) => single_segment_path(qpath).map(|ps| ps.ident.name), @@ -443,14 +442,14 @@ pub fn get_pat_name(pat: &Pat<'_>) -> Option { } struct ContainsName { - name: Name, + name: Symbol, result: bool, } impl<'tcx> Visitor<'tcx> for ContainsName { type Map = Map<'tcx>; - fn visit_name(&mut self, _: Span, name: Name) { + fn visit_name(&mut self, _: Span, name: Symbol) { if self.name == name { self.result = true; } @@ -461,7 +460,7 @@ impl<'tcx> Visitor<'tcx> for ContainsName { } /// Checks if an `Expr` contains a certain name. -pub fn contains_name(name: Name, expr: &Expr<'_>) -> bool { +pub fn contains_name(name: Symbol, expr: &Expr<'_>) -> bool { let mut cn = ContainsName { name, result: false }; cn.visit_expr(expr); cn.result @@ -1027,7 +1026,7 @@ pub fn is_allowed(cx: &LateContext<'_>, lint: &'static Lint, id: HirId) -> bool cx.tcx.lint_level_at_node(lint, id).0 == Level::Allow } -pub fn get_arg_name(pat: &Pat<'_>) -> Option { +pub fn get_arg_name(pat: &Pat<'_>) -> Option { match pat.kind { PatKind::Binding(.., ident, None) => Some(ident.name), PatKind::Ref(ref subpat, _) => get_arg_name(subpat), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 1879aae77fb6..a08d7da6dcb8 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -360,6 +360,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "derive", }, + Lint { + name: "derive_ord_xor_partial_ord", + group: "correctness", + desc: "deriving `Ord` but implementing `PartialOrd` explicitly", + deprecation: None, + module: "derive", + }, Lint { name: "diverging_sub_expression", group: "complexity", @@ -2166,6 +2173,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "misc", }, + Lint { + name: "trait_duplication_in_bounds", + group: "pedantic", + desc: "Check if the same trait bounds are specified twice during a function declaration", + deprecation: None, + module: "trait_bounds", + }, Lint { name: "transmute_bytes_to_str", group: "complexity", diff --git a/tests/ui/derive_ord_xor_partial_ord.rs b/tests/ui/derive_ord_xor_partial_ord.rs new file mode 100644 index 000000000000..b82dc518a3ba --- /dev/null +++ b/tests/ui/derive_ord_xor_partial_ord.rs @@ -0,0 +1,68 @@ +#![warn(clippy::derive_ord_xor_partial_ord)] + +use std::cmp::Ordering; + +#[derive(PartialOrd, Ord, PartialEq, Eq)] +struct DeriveBoth; + +impl PartialEq for DeriveBoth { + fn eq(&self, _: &u64) -> bool { + true + } +} + +impl PartialOrd for DeriveBoth { + fn partial_cmp(&self, _: &u64) -> Option { + Some(Ordering::Equal) + } +} + +#[derive(Ord, PartialEq, Eq)] +struct DeriveOrd; + +impl PartialOrd for DeriveOrd { + fn partial_cmp(&self, other: &Self) -> Option { + Some(other.cmp(self)) + } +} + +#[derive(Ord, PartialEq, Eq)] +struct DeriveOrdWithExplicitTypeVariable; + +impl PartialOrd for DeriveOrdWithExplicitTypeVariable { + fn partial_cmp(&self, other: &Self) -> Option { + Some(other.cmp(self)) + } +} + +#[derive(PartialOrd, PartialEq, Eq)] +struct DerivePartialOrd; + +impl std::cmp::Ord for DerivePartialOrd { + fn cmp(&self, other: &Self) -> Ordering { + Ordering::Less + } +} + +#[derive(PartialOrd, PartialEq, Eq)] +struct ImplUserOrd; + +trait Ord {} + +// We don't want to lint on user-defined traits called `Ord` +impl Ord for ImplUserOrd {} + +mod use_ord { + use std::cmp::{Ord, Ordering}; + + #[derive(PartialOrd, PartialEq, Eq)] + struct DerivePartialOrdInUseOrd; + + impl Ord for DerivePartialOrdInUseOrd { + fn cmp(&self, other: &Self) -> Ordering { + Ordering::Less + } + } +} + +fn main() {} diff --git a/tests/ui/derive_ord_xor_partial_ord.stderr b/tests/ui/derive_ord_xor_partial_ord.stderr new file mode 100644 index 000000000000..66bc4d42ce8c --- /dev/null +++ b/tests/ui/derive_ord_xor_partial_ord.stderr @@ -0,0 +1,71 @@ +error: you are deriving `Ord` but have implemented `PartialOrd` explicitly + --> $DIR/derive_ord_xor_partial_ord.rs:20:10 + | +LL | #[derive(Ord, PartialEq, Eq)] + | ^^^ + | + = note: `-D clippy::derive-ord-xor-partial-ord` implied by `-D warnings` +note: `PartialOrd` implemented here + --> $DIR/derive_ord_xor_partial_ord.rs:23:1 + | +LL | / impl PartialOrd for DeriveOrd { +LL | | fn partial_cmp(&self, other: &Self) -> Option { +LL | | Some(other.cmp(self)) +LL | | } +LL | | } + | |_^ + = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: you are deriving `Ord` but have implemented `PartialOrd` explicitly + --> $DIR/derive_ord_xor_partial_ord.rs:29:10 + | +LL | #[derive(Ord, PartialEq, Eq)] + | ^^^ + | +note: `PartialOrd` implemented here + --> $DIR/derive_ord_xor_partial_ord.rs:32:1 + | +LL | / impl PartialOrd for DeriveOrdWithExplicitTypeVariable { +LL | | fn partial_cmp(&self, other: &Self) -> Option { +LL | | Some(other.cmp(self)) +LL | | } +LL | | } + | |_^ + = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: you are implementing `Ord` explicitly but have derived `PartialOrd` + --> $DIR/derive_ord_xor_partial_ord.rs:41:1 + | +LL | / impl std::cmp::Ord for DerivePartialOrd { +LL | | fn cmp(&self, other: &Self) -> Ordering { +LL | | Ordering::Less +LL | | } +LL | | } + | |_^ + | +note: `PartialOrd` implemented here + --> $DIR/derive_ord_xor_partial_ord.rs:38:10 + | +LL | #[derive(PartialOrd, PartialEq, Eq)] + | ^^^^^^^^^^ + = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: you are implementing `Ord` explicitly but have derived `PartialOrd` + --> $DIR/derive_ord_xor_partial_ord.rs:61:5 + | +LL | / impl Ord for DerivePartialOrdInUseOrd { +LL | | fn cmp(&self, other: &Self) -> Ordering { +LL | | Ordering::Less +LL | | } +LL | | } + | |_____^ + | +note: `PartialOrd` implemented here + --> $DIR/derive_ord_xor_partial_ord.rs:58:14 + | +LL | #[derive(PartialOrd, PartialEq, Eq)] + | ^^^^^^^^^^ + = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 4 previous errors + diff --git a/tests/ui/map_flatten.fixed b/tests/ui/map_flatten.fixed index 4171d80f48a3..a5fdf7df613d 100644 --- a/tests/ui/map_flatten.fixed +++ b/tests/ui/map_flatten.fixed @@ -5,6 +5,20 @@ #![allow(clippy::map_identity)] fn main() { + // mapping to Option on Iterator + fn option_id(x: i8) -> Option { + Some(x) + } + let option_id_ref: fn(i8) -> Option = option_id; + let option_id_closure = |x| Some(x); + let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id).collect(); + let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id_ref).collect(); + let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id_closure).collect(); + let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(|x| x.checked_add(1)).collect(); + + // mapping to Iterator on Iterator let _: Vec<_> = vec![5_i8; 6].into_iter().flat_map(|x| 0..x).collect(); + + // mapping to Option on Option let _: Option<_> = (Some(Some(1))).and_then(|x| x); } diff --git a/tests/ui/map_flatten.rs b/tests/ui/map_flatten.rs index 16a0fd090ad0..abbc4e16e567 100644 --- a/tests/ui/map_flatten.rs +++ b/tests/ui/map_flatten.rs @@ -5,6 +5,20 @@ #![allow(clippy::map_identity)] fn main() { + // mapping to Option on Iterator + fn option_id(x: i8) -> Option { + Some(x) + } + let option_id_ref: fn(i8) -> Option = option_id; + let option_id_closure = |x| Some(x); + let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id).flatten().collect(); + let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_ref).flatten().collect(); + let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_closure).flatten().collect(); + let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect(); + + // mapping to Iterator on Iterator let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); + + // mapping to Option on Option let _: Option<_> = (Some(Some(1))).map(|x| x).flatten(); } diff --git a/tests/ui/map_flatten.stderr b/tests/ui/map_flatten.stderr index 00bc41c15e9b..b6479cd69eac 100644 --- a/tests/ui/map_flatten.stderr +++ b/tests/ui/map_flatten.stderr @@ -1,16 +1,40 @@ -error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` - --> $DIR/map_flatten.rs:8:21 +error: called `map(..).flatten()` on an `Iterator` + --> $DIR/map_flatten.rs:14:46 | -LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `vec![5_i8; 6].into_iter().flat_map(|x| 0..x)` +LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id).flatten().collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id)` | = note: `-D clippy::map-flatten` implied by `-D warnings` -error: called `map(..).flatten()` on an `Option`. This is more succinctly expressed by calling `.and_then(..)` - --> $DIR/map_flatten.rs:9:24 +error: called `map(..).flatten()` on an `Iterator` + --> $DIR/map_flatten.rs:15:46 + | +LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_ref).flatten().collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id_ref)` + +error: called `map(..).flatten()` on an `Iterator` + --> $DIR/map_flatten.rs:16:46 + | +LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_closure).flatten().collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id_closure)` + +error: called `map(..).flatten()` on an `Iterator` + --> $DIR/map_flatten.rs:17:46 + | +LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(|x| x.checked_add(1))` + +error: called `map(..).flatten()` on an `Iterator` + --> $DIR/map_flatten.rs:20:46 + | +LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `.flat_map(|x| 0..x)` + +error: called `map(..).flatten()` on an `Option` + --> $DIR/map_flatten.rs:23:39 | LL | let _: Option<_> = (Some(Some(1))).map(|x| x).flatten(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `(Some(Some(1))).and_then(|x| x)` + | ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `.and_then(|x| x)` -error: aborting due to 2 previous errors +error: aborting due to 6 previous errors diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 7880cf36415f..80dd2f744b3a 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -10,6 +10,7 @@ clippy::non_ascii_literal, clippy::new_without_default, clippy::needless_pass_by_value, + clippy::needless_lifetimes, clippy::print_stdout, clippy::must_use_candidate, clippy::use_self, @@ -33,71 +34,6 @@ use std::sync::{self, Arc}; use option_helpers::IteratorFalsePositives; -pub struct T; - -impl T { - pub fn add(self, other: T) -> T { - self - } - - // no error, not public interface - pub(crate) fn drop(&mut self) {} - - // no error, private function - fn neg(self) -> Self { - self - } - - // no error, private function - fn eq(&self, other: T) -> bool { - true - } - - // No error; self is a ref. - fn sub(&self, other: T) -> &T { - self - } - - // No error; different number of arguments. - fn div(self) -> T { - self - } - - // No error; wrong return type. - fn rem(self, other: T) {} - - // Fine - fn into_u32(self) -> u32 { - 0 - } - - fn into_u16(&self) -> u16 { - 0 - } - - fn to_something(self) -> u32 { - 0 - } - - fn new(self) -> Self { - unimplemented!(); - } -} - -pub struct T1; - -impl T1 { - // Shouldn't trigger lint as it is unsafe. - pub unsafe fn add(self, rhs: T1) -> T1 { - self - } - - // Should not trigger lint since this is an async function. - pub async fn next(&mut self) -> Option { - None - } -} - struct Lt<'a> { foo: &'a u32, } @@ -171,6 +107,8 @@ impl BadNew { } } +struct T; + impl Mul for T { type Output = T; // No error, obviously. diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 01cf487ac148..2a0a43e83a65 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -1,15 +1,5 @@ -error: defining a method called `add` on this type; consider implementing the `std::ops::Add` trait or choosing a less ambiguous name - --> $DIR/methods.rs:39:5 - | -LL | / pub fn add(self, other: T) -> T { -LL | | self -LL | | } - | |_____^ - | - = note: `-D clippy::should-implement-trait` implied by `-D warnings` - error: methods called `new` usually return `Self` - --> $DIR/methods.rs:169:5 + --> $DIR/methods.rs:105:5 | LL | / fn new() -> i32 { LL | | 0 @@ -19,7 +9,7 @@ LL | | } = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead. - --> $DIR/methods.rs:188:13 + --> $DIR/methods.rs:126:13 | LL | let _ = v.iter().filter(|&x| *x < 0).next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -28,7 +18,7 @@ LL | let _ = v.iter().filter(|&x| *x < 0).next(); = note: replace `filter(|&x| *x < 0).next()` with `find(|&x| *x < 0)` error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead. - --> $DIR/methods.rs:191:13 + --> $DIR/methods.rs:129:13 | LL | let _ = v.iter().filter(|&x| { | _____________^ @@ -38,7 +28,7 @@ LL | | ).next(); | |___________________________^ error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:208:22 + --> $DIR/methods.rs:146:22 | LL | let _ = v.iter().find(|&x| *x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)` @@ -46,25 +36,25 @@ LL | let _ = v.iter().find(|&x| *x < 0).is_some(); = note: `-D clippy::search-is-some` implied by `-D warnings` error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:209:20 + --> $DIR/methods.rs:147:20 | LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| **y == x)` error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:210:20 + --> $DIR/methods.rs:148:20 | LL | let _ = (0..1).find(|x| *x == 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)` error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:211:22 + --> $DIR/methods.rs:149:22 | LL | let _ = v.iter().find(|x| **x == 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x == 0)` error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:214:13 + --> $DIR/methods.rs:152:13 | LL | let _ = v.iter().find(|&x| { | _____________^ @@ -74,13 +64,13 @@ LL | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:220:22 + --> $DIR/methods.rs:158:22 | LL | let _ = v.iter().position(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:223:13 + --> $DIR/methods.rs:161:13 | LL | let _ = v.iter().position(|&x| { | _____________^ @@ -90,13 +80,13 @@ LL | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:229:22 + --> $DIR/methods.rs:167:22 | LL | let _ = v.iter().rposition(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:232:13 + --> $DIR/methods.rs:170:13 | LL | let _ = v.iter().rposition(|&x| { | _____________^ @@ -105,5 +95,5 @@ LL | | } LL | | ).is_some(); | |______________________________^ -error: aborting due to 13 previous errors +error: aborting due to 12 previous errors diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs new file mode 100644 index 000000000000..4cf03e820352 --- /dev/null +++ b/tests/ui/needless_collect_indirect.rs @@ -0,0 +1,19 @@ +use std::collections::{HashMap, VecDeque}; + +fn main() { + let sample = [1; 5]; + let indirect_iter = sample.iter().collect::>(); + indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); + let indirect_len = sample.iter().collect::>(); + indirect_len.len(); + let indirect_empty = sample.iter().collect::>(); + indirect_empty.is_empty(); + let indirect_contains = sample.iter().collect::>(); + indirect_contains.contains(&&5); + let indirect_negative = sample.iter().collect::>(); + indirect_negative.len(); + indirect_negative + .into_iter() + .map(|x| (*x, *x + 1)) + .collect::>(); +} diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr new file mode 100644 index 000000000000..0c1e61d74966 --- /dev/null +++ b/tests/ui/needless_collect_indirect.stderr @@ -0,0 +1,55 @@ +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:5:5 + | +LL | / let indirect_iter = sample.iter().collect::>(); +LL | | indirect_iter.into_iter().map(|x| (x, x + 1)).collect::>(); + | |____^ + | + = note: `-D clippy::needless-collect` implied by `-D warnings` +help: Use the original Iterator instead of collecting it and then producing a new one + | +LL | +LL | sample.iter().map(|x| (x, x + 1)).collect::>(); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:7:5 + | +LL | / let indirect_len = sample.iter().collect::>(); +LL | | indirect_len.len(); + | |____^ + | +help: Take the original Iterator's count instead of collecting it and finding the length + | +LL | +LL | sample.iter().count(); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:9:5 + | +LL | / let indirect_empty = sample.iter().collect::>(); +LL | | indirect_empty.is_empty(); + | |____^ + | +help: Check if the original Iterator has anything instead of collecting it and seeing if it's empty + | +LL | +LL | sample.iter().next().is_none(); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:11:5 + | +LL | / let indirect_contains = sample.iter().collect::>(); +LL | | indirect_contains.contains(&&5); + | |____^ + | +help: Check if the original Iterator contains an element instead of collecting then checking + | +LL | +LL | sample.iter().any(|x| x == &&5); + | + +error: aborting due to 4 previous errors + diff --git a/tests/ui/should_impl_trait/corner_cases.rs b/tests/ui/should_impl_trait/corner_cases.rs new file mode 100644 index 000000000000..6c5ffe6aba8b --- /dev/null +++ b/tests/ui/should_impl_trait/corner_cases.rs @@ -0,0 +1,83 @@ +// edition:2018 + +#![warn(clippy::all, clippy::pedantic)] +#![allow( + clippy::missing_errors_doc, + clippy::needless_pass_by_value, + clippy::must_use_candidate, + clippy::unused_self, + clippy::needless_lifetimes, + clippy::missing_safety_doc, + clippy::wrong_self_convention +)] + +use std::ops::Mul; +use std::rc::{self, Rc}; +use std::sync::{self, Arc}; + +fn main() {} + +pub struct T1; +impl T1 { + // corner cases: should not lint + + // no error, not public interface + pub(crate) fn drop(&mut self) {} + + // no error, private function + fn neg(self) -> Self { + self + } + + // no error, private function + fn eq(&self, other: Self) -> bool { + true + } + + // No error; self is a ref. + fn sub(&self, other: Self) -> &Self { + self + } + + // No error; different number of arguments. + fn div(self) -> Self { + self + } + + // No error; wrong return type. + fn rem(self, other: Self) {} + + // Fine + fn into_u32(self) -> u32 { + 0 + } + + fn into_u16(&self) -> u16 { + 0 + } + + fn to_something(self) -> u32 { + 0 + } + + fn new(self) -> Self { + unimplemented!(); + } + + pub fn next<'b>(&'b mut self) -> Option<&'b mut T1> { + unimplemented!(); + } +} + +pub struct T2; +impl T2 { + // Shouldn't trigger lint as it is unsafe. + pub unsafe fn add(self, rhs: Self) -> Self { + self + } + + // Should not trigger lint since this is an async function. + pub async fn next(&mut self) -> Option { + None + } +} diff --git a/tests/ui/should_impl_trait/method_list_1.rs b/tests/ui/should_impl_trait/method_list_1.rs new file mode 100644 index 000000000000..f8d248fc98d8 --- /dev/null +++ b/tests/ui/should_impl_trait/method_list_1.rs @@ -0,0 +1,87 @@ +// edition:2018 + +#![warn(clippy::all, clippy::pedantic)] +#![allow( + clippy::missing_errors_doc, + clippy::needless_pass_by_value, + clippy::must_use_candidate, + clippy::unused_self, + clippy::needless_lifetimes, + clippy::missing_safety_doc, + clippy::wrong_self_convention +)] + +use std::ops::Mul; +use std::rc::{self, Rc}; +use std::sync::{self, Arc}; + +fn main() {} +pub struct T; + +impl T { + // ***************************************** + // trait method list part 1, should lint all + // ***************************************** + pub fn add(self, other: T) -> T { + unimplemented!() + } + + pub fn as_mut(&mut self) -> &mut T { + unimplemented!() + } + + pub fn as_ref(&self) -> &T { + unimplemented!() + } + + pub fn bitand(self, rhs: T) -> T { + unimplemented!() + } + + pub fn bitor(self, rhs: Self) -> Self { + unimplemented!() + } + + pub fn bitxor(self, rhs: Self) -> Self { + unimplemented!() + } + + pub fn borrow(&self) -> &str { + unimplemented!() + } + + pub fn borrow_mut(&mut self) -> &mut str { + unimplemented!() + } + + pub fn clone(&self) -> Self { + unimplemented!() + } + + pub fn cmp(&self, other: &Self) -> Self { + unimplemented!() + } + + pub fn default() -> Self { + unimplemented!() + } + + pub fn deref(&self) -> &Self { + unimplemented!() + } + + pub fn deref_mut(&mut self) -> &mut Self { + unimplemented!() + } + + pub fn div(self, rhs: Self) -> Self { + unimplemented!() + } + + pub fn drop(&mut self) { + unimplemented!() + } + // ********** + // part 1 end + // ********** +} diff --git a/tests/ui/should_impl_trait/method_list_1.stderr b/tests/ui/should_impl_trait/method_list_1.stderr new file mode 100644 index 000000000000..2b7d4628c3fa --- /dev/null +++ b/tests/ui/should_impl_trait/method_list_1.stderr @@ -0,0 +1,143 @@ +error: method `add` can be confused for the standard trait method `std::ops::Add::add` + --> $DIR/method_list_1.rs:25:5 + | +LL | / pub fn add(self, other: T) -> T { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = note: `-D clippy::should-implement-trait` implied by `-D warnings` + = help: consider implementing the trait `std::ops::Add` or choosing a less ambiguous method name + +error: method `as_mut` can be confused for the standard trait method `std::convert::AsMut::as_mut` + --> $DIR/method_list_1.rs:29:5 + | +LL | / pub fn as_mut(&mut self) -> &mut T { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::convert::AsMut` or choosing a less ambiguous method name + +error: method `as_ref` can be confused for the standard trait method `std::convert::AsRef::as_ref` + --> $DIR/method_list_1.rs:33:5 + | +LL | / pub fn as_ref(&self) -> &T { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::convert::AsRef` or choosing a less ambiguous method name + +error: method `bitand` can be confused for the standard trait method `std::ops::BitAnd::bitand` + --> $DIR/method_list_1.rs:37:5 + | +LL | / pub fn bitand(self, rhs: T) -> T { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::BitAnd` or choosing a less ambiguous method name + +error: method `bitor` can be confused for the standard trait method `std::ops::BitOr::bitor` + --> $DIR/method_list_1.rs:41:5 + | +LL | / pub fn bitor(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::BitOr` or choosing a less ambiguous method name + +error: method `bitxor` can be confused for the standard trait method `std::ops::BitXor::bitxor` + --> $DIR/method_list_1.rs:45:5 + | +LL | / pub fn bitxor(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::BitXor` or choosing a less ambiguous method name + +error: method `borrow` can be confused for the standard trait method `std::borrow::Borrow::borrow` + --> $DIR/method_list_1.rs:49:5 + | +LL | / pub fn borrow(&self) -> &str { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::borrow::Borrow` or choosing a less ambiguous method name + +error: method `borrow_mut` can be confused for the standard trait method `std::borrow::BorrowMut::borrow_mut` + --> $DIR/method_list_1.rs:53:5 + | +LL | / pub fn borrow_mut(&mut self) -> &mut str { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::borrow::BorrowMut` or choosing a less ambiguous method name + +error: method `clone` can be confused for the standard trait method `std::clone::Clone::clone` + --> $DIR/method_list_1.rs:57:5 + | +LL | / pub fn clone(&self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::clone::Clone` or choosing a less ambiguous method name + +error: method `cmp` can be confused for the standard trait method `std::cmp::Ord::cmp` + --> $DIR/method_list_1.rs:61:5 + | +LL | / pub fn cmp(&self, other: &Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::cmp::Ord` or choosing a less ambiguous method name + +error: method `deref` can be confused for the standard trait method `std::ops::Deref::deref` + --> $DIR/method_list_1.rs:69:5 + | +LL | / pub fn deref(&self) -> &Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Deref` or choosing a less ambiguous method name + +error: method `deref_mut` can be confused for the standard trait method `std::ops::DerefMut::deref_mut` + --> $DIR/method_list_1.rs:73:5 + | +LL | / pub fn deref_mut(&mut self) -> &mut Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::DerefMut` or choosing a less ambiguous method name + +error: method `div` can be confused for the standard trait method `std::ops::Div::div` + --> $DIR/method_list_1.rs:77:5 + | +LL | / pub fn div(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Div` or choosing a less ambiguous method name + +error: method `drop` can be confused for the standard trait method `std::ops::Drop::drop` + --> $DIR/method_list_1.rs:81:5 + | +LL | / pub fn drop(&mut self) { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Drop` or choosing a less ambiguous method name + +error: aborting due to 14 previous errors + diff --git a/tests/ui/should_impl_trait/method_list_2.rs b/tests/ui/should_impl_trait/method_list_2.rs new file mode 100644 index 000000000000..ed5e0d384bf5 --- /dev/null +++ b/tests/ui/should_impl_trait/method_list_2.rs @@ -0,0 +1,88 @@ +// edition:2018 + +#![warn(clippy::all, clippy::pedantic)] +#![allow( + clippy::missing_errors_doc, + clippy::needless_pass_by_value, + clippy::must_use_candidate, + clippy::unused_self, + clippy::needless_lifetimes, + clippy::missing_safety_doc, + clippy::wrong_self_convention +)] + +use std::ops::Mul; +use std::rc::{self, Rc}; +use std::sync::{self, Arc}; + +fn main() {} +pub struct T; + +impl T { + // ***************************************** + // trait method list part 2, should lint all + // ***************************************** + + pub fn eq(&self, other: &Self) -> bool { + unimplemented!() + } + + pub fn from_iter(iter: T) -> Self { + unimplemented!() + } + + pub fn from_str(s: &str) -> Result { + unimplemented!() + } + + pub fn hash(&self, state: &mut T) { + unimplemented!() + } + + pub fn index(&self, index: usize) -> &Self { + unimplemented!() + } + + pub fn index_mut(&mut self, index: usize) -> &mut Self { + unimplemented!() + } + + pub fn into_iter(self) -> Self { + unimplemented!() + } + + pub fn mul(self, rhs: Self) -> Self { + unimplemented!() + } + + pub fn neg(self) -> Self { + unimplemented!() + } + + pub fn next(&mut self) -> Option { + unimplemented!() + } + + pub fn not(self) -> Self { + unimplemented!() + } + + pub fn rem(self, rhs: Self) -> Self { + unimplemented!() + } + + pub fn shl(self, rhs: Self) -> Self { + unimplemented!() + } + + pub fn shr(self, rhs: Self) -> Self { + unimplemented!() + } + + pub fn sub(self, rhs: Self) -> Self { + unimplemented!() + } + // ********** + // part 2 end + // ********** +} diff --git a/tests/ui/should_impl_trait/method_list_2.stderr b/tests/ui/should_impl_trait/method_list_2.stderr new file mode 100644 index 000000000000..b6fd43569569 --- /dev/null +++ b/tests/ui/should_impl_trait/method_list_2.stderr @@ -0,0 +1,153 @@ +error: method `eq` can be confused for the standard trait method `std::cmp::PartialEq::eq` + --> $DIR/method_list_2.rs:26:5 + | +LL | / pub fn eq(&self, other: &Self) -> bool { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = note: `-D clippy::should-implement-trait` implied by `-D warnings` + = help: consider implementing the trait `std::cmp::PartialEq` or choosing a less ambiguous method name + +error: method `from_iter` can be confused for the standard trait method `std::iter::FromIterator::from_iter` + --> $DIR/method_list_2.rs:30:5 + | +LL | / pub fn from_iter(iter: T) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::iter::FromIterator` or choosing a less ambiguous method name + +error: method `from_str` can be confused for the standard trait method `std::str::FromStr::from_str` + --> $DIR/method_list_2.rs:34:5 + | +LL | / pub fn from_str(s: &str) -> Result { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::str::FromStr` or choosing a less ambiguous method name + +error: method `hash` can be confused for the standard trait method `std::hash::Hash::hash` + --> $DIR/method_list_2.rs:38:5 + | +LL | / pub fn hash(&self, state: &mut T) { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::hash::Hash` or choosing a less ambiguous method name + +error: method `index` can be confused for the standard trait method `std::ops::Index::index` + --> $DIR/method_list_2.rs:42:5 + | +LL | / pub fn index(&self, index: usize) -> &Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Index` or choosing a less ambiguous method name + +error: method `index_mut` can be confused for the standard trait method `std::ops::IndexMut::index_mut` + --> $DIR/method_list_2.rs:46:5 + | +LL | / pub fn index_mut(&mut self, index: usize) -> &mut Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::IndexMut` or choosing a less ambiguous method name + +error: method `into_iter` can be confused for the standard trait method `std::iter::IntoIterator::into_iter` + --> $DIR/method_list_2.rs:50:5 + | +LL | / pub fn into_iter(self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::iter::IntoIterator` or choosing a less ambiguous method name + +error: method `mul` can be confused for the standard trait method `std::ops::Mul::mul` + --> $DIR/method_list_2.rs:54:5 + | +LL | / pub fn mul(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Mul` or choosing a less ambiguous method name + +error: method `neg` can be confused for the standard trait method `std::ops::Neg::neg` + --> $DIR/method_list_2.rs:58:5 + | +LL | / pub fn neg(self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Neg` or choosing a less ambiguous method name + +error: method `next` can be confused for the standard trait method `std::iter::Iterator::next` + --> $DIR/method_list_2.rs:62:5 + | +LL | / pub fn next(&mut self) -> Option { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::iter::Iterator` or choosing a less ambiguous method name + +error: method `not` can be confused for the standard trait method `std::ops::Not::not` + --> $DIR/method_list_2.rs:66:5 + | +LL | / pub fn not(self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Not` or choosing a less ambiguous method name + +error: method `rem` can be confused for the standard trait method `std::ops::Rem::rem` + --> $DIR/method_list_2.rs:70:5 + | +LL | / pub fn rem(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Rem` or choosing a less ambiguous method name + +error: method `shl` can be confused for the standard trait method `std::ops::Shl::shl` + --> $DIR/method_list_2.rs:74:5 + | +LL | / pub fn shl(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Shl` or choosing a less ambiguous method name + +error: method `shr` can be confused for the standard trait method `std::ops::Shr::shr` + --> $DIR/method_list_2.rs:78:5 + | +LL | / pub fn shr(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Shr` or choosing a less ambiguous method name + +error: method `sub` can be confused for the standard trait method `std::ops::Sub::sub` + --> $DIR/method_list_2.rs:82:5 + | +LL | / pub fn sub(self, rhs: Self) -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::ops::Sub` or choosing a less ambiguous method name + +error: aborting due to 15 previous errors + diff --git a/tests/ui/trait_duplication_in_bounds.rs b/tests/ui/trait_duplication_in_bounds.rs new file mode 100644 index 000000000000..cb2b0054e352 --- /dev/null +++ b/tests/ui/trait_duplication_in_bounds.rs @@ -0,0 +1,31 @@ +#![deny(clippy::trait_duplication_in_bounds)] + +use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign}; + +fn bad_foo(arg0: T, arg1: Z) +where + T: Clone, + T: Default, +{ + unimplemented!(); +} + +fn good_bar(arg: T) { + unimplemented!(); +} + +fn good_foo(arg: T) +where + T: Clone + Default, +{ + unimplemented!(); +} + +fn good_foobar(arg: T) +where + T: Clone, +{ + unimplemented!(); +} + +fn main() {} diff --git a/tests/ui/trait_duplication_in_bounds.stderr b/tests/ui/trait_duplication_in_bounds.stderr new file mode 100644 index 000000000000..027e1c752041 --- /dev/null +++ b/tests/ui/trait_duplication_in_bounds.stderr @@ -0,0 +1,23 @@ +error: this trait bound is already specified in the where clause + --> $DIR/trait_duplication_in_bounds.rs:5:15 + | +LL | fn bad_foo(arg0: T, arg1: Z) + | ^^^^^ + | +note: the lint level is defined here + --> $DIR/trait_duplication_in_bounds.rs:1:9 + | +LL | #![deny(clippy::trait_duplication_in_bounds)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: consider removing this trait bound + +error: this trait bound is already specified in the where clause + --> $DIR/trait_duplication_in_bounds.rs:5:23 + | +LL | fn bad_foo(arg0: T, arg1: Z) + | ^^^^^^^ + | + = help: consider removing this trait bound + +error: aborting due to 2 previous errors +