dnrops
/
rust
mirror of https://github.com/rust-lang/rust.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #12105 - GuillaumeGomez:useless-asf-extension, r=llogiq 

Extend `useless_asref` lint on `map(clone)`

If you have code like:

```rust
Some(String::new()).as_ref().map(Clone::clone)
```

the `as_ref` call is unneeded.

Interestingly enough, this lint and `map_clone` are starting to share a same "space" where both lints warn about different things for the same code. Not sure what's the policy about such cases though...

r? `@llogiq`

changelog: Extend `useless_asref` lint on `map(clone)`
This commit is contained in:
bors 2024-01-10 01:13:42 +00:00
parent 3b8323d790 f7ef9a6f1f
commit e899684254
8 changed files with 267 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
clippy_lints/src/methods/map_clone.rs
View File

@ -5,6 +5,7 @@ use clippy_utils::ty::{is_copy, is_type_diagnostic_item};
use clippy_utils::{is_diag_trait_item, match_def_path, paths, peel_blocks};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_lint::LateContext;
use rustc_middle::mir::Mutability;
use rustc_middle::ty;
@ -14,11 +15,35 @@ use rustc_span::{sym, Span};
use super::MAP_CLONE;
// If this `map` is called on an `Option` or a `Result` and the previous call is `as_ref`, we don't
// run this lint because it would overlap with `useless_asref` which provides a better suggestion
// in this case.
fn should_run_lint(cx: &LateContext<'_>, e: &hir::Expr<'_>, method_id: DefId) -> bool {
if is_diag_trait_item(cx, method_id, sym::Iterator) {
return true;
}
// We check if it's an `Option` or a `Result`.
if let Some(id) = cx.tcx.impl_of_method(method_id) {
let identity = cx.tcx.type_of(id).instantiate_identity();
if !is_type_diagnostic_item(cx, identity, sym::Option) && !is_type_diagnostic_item(cx, identity, sym::Result) {
return false;
}
} else {
return false;
}
// We check if the previous method call is `as_ref`.
if let hir::ExprKind::MethodCall(path1, receiver, _, _) = &e.kind
&& let hir::ExprKind::MethodCall(path2, _, _, _) = &receiver.kind
{
return path2.ident.name != sym::as_ref || path1.ident.name != sym::map;
}
true
}
pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>, msrv: &Msrv) {
if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
&& (cx.tcx.impl_of_method(method_id).map_or(false, |id| {
is_type_diagnostic_item(cx, cx.tcx.type_of(id).instantiate_identity(), sym::Option)
}) || is_diag_trait_item(cx, method_id, sym::Iterator))
&& should_run_lint(cx, e, method_id)
{
match arg.kind {
hir::ExprKind::Closure(&hir::Closure { body, .. }) => {

123
clippy_lints/src/methods/useless_asref.rs
View File

@ -1,19 +1,52 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::walk_ptrs_ty_depth;
use clippy_utils::{get_parent_expr, is_trait_method};
use clippy_utils::{get_parent_expr, is_diag_trait_item, match_def_path, paths, peel_blocks};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;
use rustc_span::sym;
use rustc_middle::ty::adjustment::Adjust;
use rustc_middle::ty::{Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor};
use rustc_span::{sym, Span};
use core::ops::ControlFlow;
use super::USELESS_ASREF;
/// Returns the first type inside the `Option`/`Result` type passed as argument.
fn get_enum_ty(enum_ty: Ty<'_>) -> Option<Ty<'_>> {
struct ContainsTyVisitor {
level: usize,
}
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for ContainsTyVisitor {
type BreakTy = Ty<'tcx>;
fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
self.level += 1;
if self.level == 1 {
t.super_visit_with(self)
} else {
ControlFlow::Break(t)
}
}
}
match enum_ty.visit_with(&mut ContainsTyVisitor { level: 0 }) {
ControlFlow::Break(ty) => Some(ty),
ControlFlow::Continue(()) => None,
}
}
/// Checks for the `USELESS_ASREF` lint.
pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, recvr: &hir::Expr<'_>) {
// when we get here, we've already checked that the call name is "as_ref" or "as_mut"
// check if the call is to the actual `AsRef` or `AsMut` trait
if is_trait_method(cx, expr, sym::AsRef) || is_trait_method(cx, expr, sym::AsMut) {
let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else {
return;
};
if is_diag_trait_item(cx, def_id, sym::AsRef) || is_diag_trait_item(cx, def_id, sym::AsMut) {
// check if the type after `as_ref` or `as_mut` is the same as before
let rcv_ty = cx.typeck_results().expr_ty(recvr);
let res_ty = cx.typeck_results().expr_ty(expr);
@ -39,5 +72,89 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str,
applicability,
);
}
} else if match_def_path(cx, def_id, &["core", "option", "Option", call_name])
|| match_def_path(cx, def_id, &["core", "result", "Result", call_name])
{
let rcv_ty = cx.typeck_results().expr_ty(recvr).peel_refs();
let res_ty = cx.typeck_results().expr_ty(expr).peel_refs();
if let Some(rcv_ty) = get_enum_ty(rcv_ty)
&& let Some(res_ty) = get_enum_ty(res_ty)
// If the only thing the `as_mut`/`as_ref` call is doing is adding references and not
// changing the type, then we can move forward.
&& rcv_ty.peel_refs() == res_ty.peel_refs()
&& let Some(parent) = get_parent_expr(cx, expr)
&& let hir::ExprKind::MethodCall(segment, _, args, _) = parent.kind
&& segment.ident.span != expr.span
// We check that the called method name is `map`.
&& segment.ident.name == sym::map
// And that it only has one argument.
&& let [arg] = args
&& is_calling_clone(cx, arg)
{
lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name);
}
}
}
fn check_qpath(cx: &LateContext<'_>, qpath: hir::QPath<'_>, hir_id: hir::HirId) -> bool {
// We check it's calling the `clone` method of the `Clone` trait.
if let Some(path_def_id) = cx.qpath_res(&qpath, hir_id).opt_def_id() {
match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD)
} else {
false
}
}
fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool {
match arg.kind {
hir::ExprKind::Closure(&hir::Closure { body, .. }) => {
// If it's a closure, we need to check what is called.
let closure_body = cx.tcx.hir().body(body);
let closure_expr = peel_blocks(closure_body.value);
match closure_expr.kind {
hir::ExprKind::MethodCall(method, obj, [], _) => {
if method.ident.name == sym::clone
&& let Some(fn_id) = cx.typeck_results().type_dependent_def_id(closure_expr.hir_id)
&& let Some(trait_id) = cx.tcx.trait_of_item(fn_id)
// We check it's the `Clone` trait.
&& cx.tcx.lang_items().clone_trait().map_or(false, |id| id == trait_id)
// no autoderefs
&& !cx.typeck_results().expr_adjustments(obj).iter()
.any(|a| matches!(a.kind, Adjust::Deref(Some(..))))
{
true
} else {
false
}
},
hir::ExprKind::Call(call, [_]) => {
if let hir::ExprKind::Path(qpath) = call.kind {
check_qpath(cx, qpath, call.hir_id)
} else {
false
}
},
_ => false,
}
},
hir::ExprKind::Path(qpath) => check_qpath(cx, qpath, arg.hir_id),
_ => false,
}
}
fn lint_as_ref_clone(cx: &LateContext<'_>, span: Span, recvr: &hir::Expr<'_>, call_name: &str) {
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
USELESS_ASREF,
span,
&format!("this call to `{call_name}.map(...)` does nothing"),
"try",
format!(
"{}.clone()",
snippet_with_applicability(cx, recvr.span, "..", &mut applicability)
),
applicability,
);
}

21
tests/ui/map_clone.fixed
View File

@ -5,6 +5,7 @@
clippy::many_single_char_names,
clippy::redundant_clone,
clippy::redundant_closure,
clippy::useless_asref,
clippy::useless_vec
)]
@ -63,6 +64,24 @@ fn main() {
}
let x = Some(String::new());
let y = x.as_ref().cloned();
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
let y = x.cloned();
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.cloned();
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.cloned();
//~^ ERROR: you are explicitly cloning with `.map()`
// Testing with `Result` now.
let x: Result<String, ()> = Ok(String::new());
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
let y = x.cloned();
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.cloned();
// We ensure that no warning is emitted here because `useless_asref` is taking over.
let x = Some(String::new());
let y = x.as_ref().map(|x| String::clone(x));
let x: Result<String, ()> = Ok(String::new());
let y = x.as_ref().map(|x| String::clone(x));
}

21
tests/ui/map_clone.rs
View File

@ -5,6 +5,7 @@
clippy::many_single_char_names,
clippy::redundant_clone,
clippy::redundant_closure,
clippy::useless_asref,
clippy::useless_vec
)]
@ -63,6 +64,24 @@ fn main() {
}
let x = Some(String::new());
let y = x.as_ref().map(|x| String::clone(x));
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
let y = x.map(|x| String::clone(x));
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.map(Clone::clone);
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.map(String::clone);
//~^ ERROR: you are explicitly cloning with `.map()`
// Testing with `Result` now.
let x: Result<String, ()> = Ok(String::new());
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
let y = x.map(|x| String::clone(x));
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.map(|x| String::clone(x));
// We ensure that no warning is emitted here because `useless_asref` is taking over.
let x = Some(String::new());
let y = x.as_ref().map(|x| String::clone(x));
let x: Result<String, ()> = Ok(String::new());
let y = x.as_ref().map(|x| String::clone(x));
}

44
tests/ui/map_clone.stderr
View File

@ -1,5 +1,5 @@
error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:12:22
--> $DIR/map_clone.rs:13:22
|
LL | let _: Vec<i8> = vec![5_i8; 6].iter().map(|x| *x).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![5_i8; 6].iter().copied()`
@ -8,40 +8,64 @@ LL | let _: Vec<i8> = vec![5_i8; 6].iter().map(|x| *x).collect();
= help: to override `-D warnings` add `#[allow(clippy::map_clone)]`
error: you are using an explicit closure for cloning elements
--> $DIR/map_clone.rs:13:26
--> $DIR/map_clone.rs:14:26
|
LL | let _: Vec<String> = vec![String::new()].iter().map(|x| x.clone()).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()`
error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:14:23
--> $DIR/map_clone.rs:15:23
|
LL | let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![42, 43].iter().copied()`
error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:16:26
--> $DIR/map_clone.rs:17:26
|
LL | let _: Option<u64> = Some(&16).map(|b| *b);
| ^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&16).copied()`
error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:17:25
--> $DIR/map_clone.rs:18:25
|
LL | let _: Option<u8> = Some(&1).map(|x| x.clone());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&1).copied()`
error: you are needlessly cloning iterator elements
--> $DIR/map_clone.rs:28:29
--> $DIR/map_clone.rs:29:29
|
LL | let _ = std::env::args().map(|v| v.clone());
| ^^^^^^^^^^^^^^^^^^^ help: remove the `map` call
error: you are explicitly cloning with `.map()`
--> $DIR/map_clone.rs:66:13
--> $DIR/map_clone.rs:68:13
|
LL | let y = x.as_ref().map(|x| String::clone(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.as_ref().cloned()`
LL | let y = x.map(|x| String::clone(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
error: aborting due to 7 previous errors
error: you are explicitly cloning with `.map()`
--> $DIR/map_clone.rs:70:13
|
LL | let y = x.map(Clone::clone);
| ^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
error: you are explicitly cloning with `.map()`
--> $DIR/map_clone.rs:72:13
|
LL | let y = x.map(String::clone);
| ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
error: you are explicitly cloning with `.map()`
--> $DIR/map_clone.rs:78:13
|
LL | let y = x.map(|x| String::clone(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
error: you are explicitly cloning with `.map()`
--> $DIR/map_clone.rs:80:13
|
LL | let y = x.map(|x| String::clone(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
error: aborting due to 11 previous errors

14
tests/ui/useless_asref.fixed
View File

@ -2,7 +2,9 @@
#![allow(
clippy::explicit_auto_deref,
clippy::uninlined_format_args,
clippy::needless_pass_by_ref_mut
clippy::map_clone,
clippy::needless_pass_by_ref_mut,
clippy::redundant_closure
)]
use std::fmt::Debug;
@ -134,10 +136,12 @@ fn generic_ok<U: AsMut<T> + AsRef<T> + ?Sized, T: Debug + ?Sized>(mru: &mut U) {
fn foo() {
let x = Some(String::new());
let y = x.as_ref().cloned();
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.as_ref().cloned();
//~^ ERROR: you are explicitly cloning with `.map()`
let z = x.clone();
//~^ ERROR: this call to `as_ref.map(...)` does nothing
let z = x.clone();
//~^ ERROR: this call to `as_ref.map(...)` does nothing
let z = x.clone();
//~^ ERROR: this call to `as_ref.map(...)` does nothing
}
fn main() {

14
tests/ui/useless_asref.rs
View File

@ -2,7 +2,9 @@
#![allow(
clippy::explicit_auto_deref,
clippy::uninlined_format_args,
clippy::needless_pass_by_ref_mut
clippy::map_clone,
clippy::needless_pass_by_ref_mut,
clippy::redundant_closure
)]
use std::fmt::Debug;
@ -134,10 +136,12 @@ fn generic_ok<U: AsMut<T> + AsRef<T> + ?Sized, T: Debug + ?Sized>(mru: &mut U) {
fn foo() {
let x = Some(String::new());
let y = x.as_ref().map(Clone::clone);
//~^ ERROR: you are explicitly cloning with `.map()`
let y = x.as_ref().map(String::clone);
//~^ ERROR: you are explicitly cloning with `.map()`
let z = x.as_ref().map(String::clone);
//~^ ERROR: this call to `as_ref.map(...)` does nothing
let z = x.as_ref().map(|z| z.clone());
//~^ ERROR: this call to `as_ref.map(...)` does nothing
let z = x.as_ref().map(|z| String::clone(z));
//~^ ERROR: this call to `as_ref.map(...)` does nothing
}
fn main() {

51
tests/ui/useless_asref.stderr
View File

@ -1,5 +1,5 @@
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:46:18
--> $DIR/useless_asref.rs:48:18
|
LL | foo_rstr(rstr.as_ref());
| ^^^^^^^^^^^^^ help: try: `rstr`
@ -11,79 +11,82 @@ LL | #![deny(clippy::useless_asref)]
| ^^^^^^^^^^^^^^^^^^^^^
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:48:20
--> $DIR/useless_asref.rs:50:20
|
LL | foo_rslice(rslice.as_ref());
| ^^^^^^^^^^^^^^^ help: try: `rslice`
error: this call to `as_mut` does nothing
--> $DIR/useless_asref.rs:52:21
--> $DIR/useless_asref.rs:54:21
|
LL | foo_mrslice(mrslice.as_mut());
| ^^^^^^^^^^^^^^^^ help: try: `mrslice`
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:54:20
--> $DIR/useless_asref.rs:56:20
|
LL | foo_rslice(mrslice.as_ref());
| ^^^^^^^^^^^^^^^^ help: try: `mrslice`
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:61:20
--> $DIR/useless_asref.rs:63:20
|
LL | foo_rslice(rrrrrslice.as_ref());
| ^^^^^^^^^^^^^^^^^^^ help: try: `rrrrrslice`
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:63:18
--> $DIR/useless_asref.rs:65:18
|
LL | foo_rstr(rrrrrstr.as_ref());
| ^^^^^^^^^^^^^^^^^ help: try: `rrrrrstr`
error: this call to `as_mut` does nothing
--> $DIR/useless_asref.rs:68:21
--> $DIR/useless_asref.rs:70:21
|
LL | foo_mrslice(mrrrrrslice.as_mut());
| ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice`
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:70:20
--> $DIR/useless_asref.rs:72:20
|
LL | foo_rslice(mrrrrrslice.as_ref());
| ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice`
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:74:16
--> $DIR/useless_asref.rs:76:16
|
LL | foo_rrrrmr((&&&&MoreRef).as_ref());
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `(&&&&MoreRef)`
error: this call to `as_mut` does nothing
--> $DIR/useless_asref.rs:124:13
--> $DIR/useless_asref.rs:126:13
|
LL | foo_mrt(mrt.as_mut());
| ^^^^^^^^^^^^ help: try: `mrt`
error: this call to `as_ref` does nothing
--> $DIR/useless_asref.rs:126:12
--> $DIR/useless_asref.rs:128:12
|
LL | foo_rt(mrt.as_ref());
| ^^^^^^^^^^^^ help: try: `mrt`
error: you are explicitly cloning with `.map()`
--> $DIR/useless_asref.rs:137:13
|
LL | let y = x.as_ref().map(Clone::clone);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.as_ref().cloned()`
|
= note: `-D clippy::map-clone` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::map_clone)]`
error: you are explicitly cloning with `.map()`
error: this call to `as_ref.map(...)` does nothing
--> $DIR/useless_asref.rs:139:13
|
LL | let y = x.as_ref().map(String::clone);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.as_ref().cloned()`
LL | let z = x.as_ref().map(String::clone);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
error: aborting due to 13 previous errors
error: this call to `as_ref.map(...)` does nothing
--> $DIR/useless_asref.rs:141:13
|
LL | let z = x.as_ref().map(|z| z.clone());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
error: this call to `as_ref.map(...)` does nothing
--> $DIR/useless_asref.rs:143:13
|
LL | let z = x.as_ref().map(|z| String::clone(z));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()`
error: aborting due to 14 previous errors