diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_all.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_suspicious.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/return_self_not_must_use.rs | 105 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 7 | ||||
| -rw-r--r-- | tests/ui/return_self_not_must_use.rs | 42 | ||||
| -rw-r--r-- | tests/ui/return_self_not_must_use.stderr | 26 |
9 files changed, 186 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 157ea0c963a..2f64cd0e914 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3118,6 +3118,7 @@ Released 2018-09-13 [`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn [`result_unit_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err +[`return_self_not_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#return_self_not_must_use [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition [`same_item_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 04912120e27..f836d6443cd 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -245,6 +245,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(reference::REF_IN_DEREF), LintId::of(regex::INVALID_REGEX), LintId::of(repeat_once::REPEAT_ONCE), + LintId::of(return_self_not_must_use::RETURN_SELF_NOT_MUST_USE), LintId::of(returns::LET_AND_RETURN), LintId::of(returns::NEEDLESS_RETURN), LintId::of(self_assignment::SELF_ASSIGNMENT), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index bb159e50373..68889f4f50a 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -422,6 +422,7 @@ store.register_lints(&[ regex::INVALID_REGEX, regex::TRIVIAL_REGEX, repeat_once::REPEAT_ONCE, + return_self_not_must_use::RETURN_SELF_NOT_MUST_USE, returns::LET_AND_RETURN, returns::NEEDLESS_RETURN, same_name_method::SAME_NAME_METHOD, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 414bfc42fdf..70dc9254879 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -17,6 +17,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(mut_key::MUTABLE_KEY_TYPE), LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY), LintId::of(octal_escapes::OCTAL_ESCAPES), + LintId::of(return_self_not_must_use::RETURN_SELF_NOT_MUST_USE), LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), ]) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bd9710ec407..c6b14ecac43 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -341,6 +341,7 @@ mod ref_option_ref; mod reference; mod regex; mod repeat_once; +mod return_self_not_must_use; mod returns; mod same_name_method; mod self_assignment; @@ -853,6 +854,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray)); store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes)); store.register_late_pass(|| Box::new(needless_late_init::NeedlessLateInit)); + store.register_late_pass(|| Box::new(return_self_not_must_use::ReturnSelfNotMustUse)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/return_self_not_must_use.rs b/clippy_lints/src/return_self_not_must_use.rs new file mode 100644 index 00000000000..1118da6c8cb --- /dev/null +++ b/clippy_lints/src/return_self_not_must_use.rs @@ -0,0 +1,105 @@ +use clippy_utils::{diagnostics::span_lint, must_use_attr, nth_arg, return_ty}; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::intravisit::FnKind; +use rustc_hir::{Body, FnDecl, HirId, TraitItem, TraitItemKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// This lint warns when a method returning `Self` doesn't have the `#[must_use]` attribute. + /// + /// ### Why is this bad? + /// It prevents to "forget" to use the newly created value. + /// + /// ### Limitations + /// This lint is only applied on methods taking a `self` argument. It would be mostly noise + /// if it was added on constructors for example. + /// + /// ### Example + /// ```rust + /// pub struct Bar; + /// + /// impl Bar { + /// // Bad + /// pub fn bar(&self) -> Self { + /// Self + /// } + /// + /// // Good + /// #[must_use] + /// pub fn foo(&self) -> Self { + /// Self + /// } + /// } + /// ``` + #[clippy::version = "1.59.0"] + pub RETURN_SELF_NOT_MUST_USE, + suspicious, + "missing `#[must_use]` annotation on a method returning `Self`" +} + +declare_lint_pass!(ReturnSelfNotMustUse => [RETURN_SELF_NOT_MUST_USE]); + +fn check_method(cx: &LateContext<'tcx>, decl: &'tcx FnDecl<'tcx>, fn_def: LocalDefId, span: Span, hir_id: HirId) { + if_chain! { + // If it comes from an external macro, better ignore it. + if !in_external_macro(cx.sess(), span); + if decl.implicit_self.has_implicit_self(); + // We only show this warning for public exported methods. + if cx.access_levels.is_exported(fn_def); + if cx.tcx.visibility(fn_def.to_def_id()).is_public(); + // No need to warn if the attribute is already present. + if must_use_attr(cx.tcx.hir().attrs(hir_id)).is_none(); + let ret_ty = return_ty(cx, hir_id); + let self_arg = nth_arg(cx, hir_id, 0); + // If `Self` has the same type as the returned type, then we want to warn. + // + // For this check, we don't want to remove the reference on the returned type because if + // there is one, we shouldn't emit a warning! + if self_arg.peel_refs() == ret_ty; + + then { + span_lint( + cx, + RETURN_SELF_NOT_MUST_USE, + span, + "missing `#[must_use]` attribute on a method returning `Self`", + ); + } + } +} + +impl<'tcx> LateLintPass<'tcx> for ReturnSelfNotMustUse { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + kind: FnKind<'tcx>, + decl: &'tcx FnDecl<'tcx>, + _: &'tcx Body<'tcx>, + span: Span, + hir_id: HirId, + ) { + if_chain! { + // We are only interested in methods, not in functions or associated functions. + if matches!(kind, FnKind::Method(_, _, _)); + if let Some(fn_def) = cx.tcx.hir().opt_local_def_id(hir_id); + if let Some(impl_def) = cx.tcx.impl_of_method(fn_def.to_def_id()); + // We don't want this method to be te implementation of a trait because the + // `#[must_use]` should be put on the trait definition directly. + if cx.tcx.trait_id_of_impl(impl_def).is_none(); + + then { + check_method(cx, decl, fn_def, span, hir_id); + } + } + } + + fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'tcx>) { + if let TraitItemKind::Fn(ref sig, _) = item.kind { + check_method(cx, sig.decl, item.def_id, item.span, item.hir_id()); + } + } +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index f011380c127..779c812ca0b 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1329,6 +1329,13 @@ pub fn return_ty<'tcx>(cx: &LateContext<'tcx>, fn_item: hir::HirId) -> Ty<'tcx> cx.tcx.erase_late_bound_regions(ret_ty) } +/// Convenience function to get the nth argument type of a function. +pub fn nth_arg<'tcx>(cx: &LateContext<'tcx>, fn_item: hir::HirId, nth: usize) -> Ty<'tcx> { + let fn_def_id = cx.tcx.hir().local_def_id(fn_item); + let arg = cx.tcx.fn_sig(fn_def_id).input(nth); + cx.tcx.erase_late_bound_regions(arg) +} + /// Checks if an expression is constructing a tuple-like enum variant or struct pub fn is_ctor_or_promotable_const_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { if let ExprKind::Call(fun, _) = expr.kind { diff --git a/tests/ui/return_self_not_must_use.rs b/tests/ui/return_self_not_must_use.rs new file mode 100644 index 00000000000..bdf3f3d7995 --- /dev/null +++ b/tests/ui/return_self_not_must_use.rs @@ -0,0 +1,42 @@ +#![crate_type = "lib"] + +#[derive(Clone)] +pub struct Bar; + +pub trait Whatever { + fn what(&self) -> Self; + // There should be no warning here! + fn what2(&self) -> &Self; +} + +impl Bar { + // There should be no warning here! + pub fn not_new() -> Self { + Self + } + pub fn foo(&self) -> Self { + Self + } + pub fn bar(self) -> Self { + self + } + // There should be no warning here! + fn foo2(&self) -> Self { + Self + } + // There should be no warning here! + pub fn foo3(&self) -> &Self { + self + } +} + +impl Whatever for Bar { + // There should be no warning here! + fn what(&self) -> Self { + self.foo2() + } + // There should be no warning here! + fn what2(&self) -> &Self { + self + } +} diff --git a/tests/ui/return_self_not_must_use.stderr b/tests/ui/return_self_not_must_use.stderr new file mode 100644 index 00000000000..3793a5559ba --- /dev/null +++ b/tests/ui/return_self_not_must_use.stderr @@ -0,0 +1,26 @@ +error: missing `#[must_use]` attribute on a method returning `Self` + --> $DIR/return_self_not_must_use.rs:7:5 + | +LL | fn what(&self) -> Self; + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::return-self-not-must-use` implied by `-D warnings` + +error: missing `#[must_use]` attribute on a method returning `Self` + --> $DIR/return_self_not_must_use.rs:17:5 + | +LL | / pub fn foo(&self) -> Self { +LL | | Self +LL | | } + | |_____^ + +error: missing `#[must_use]` attribute on a method returning `Self` + --> $DIR/return_self_not_must_use.rs:20:5 + | +LL | / pub fn bar(self) -> Self { +LL | | self +LL | | } + | |_____^ + +error: aborting due to 3 previous errors + |