Skip to content

Commit 039b40f

Browse files
samueltardieusamueltardieu
authored
match_as_ref: improve diagnostics (#15928)
- Make the diagnostic message actually desribe the problem - Make the suggestion verbose, because we lint multiline exprs changelog: [`match_as_ref`]: improve diagnostics
2 parents 973e596 + a6e5159 commit 039b40f

File tree

4 files changed

+132
-47
lines changed

4 files changed

+132
-47
lines changed

‎clippy_lints/src/matches/match_as_ref.rs‎

Lines changed: 45 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
1+
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::res::{MaybeDef, MaybeQPath};
3-
use clippy_utils::source::snippet_with_applicability;
3+
use clippy_utils::sugg::Sugg;
4+
use clippy_utils::ty::option_arg_ty;
45
use clippy_utils::{is_none_arm, peel_blocks};
56
use rustc_errors::Applicability;
67
use rustc_hir::{Arm, BindingMode, ByRef, Expr, ExprKind, LangItem, Mutability, PatKind, QPath};
@@ -10,54 +11,58 @@ use rustc_middle::ty;
1011
use super::MATCH_AS_REF;
1112

1213
pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) {
13-
if arms.len() == 2 && arms[0].guard.is_none() && arms[1].guard.is_none() {
14-
let arm_ref_mut = if is_none_arm(cx, &arms[0]) {
15-
is_ref_some_arm(cx, &arms[1])
16-
} else if is_none_arm(cx, &arms[1]) {
17-
is_ref_some_arm(cx, &arms[0])
14+
if let [arm1, arm2] = arms
15+
&& arm1.guard.is_none()
16+
&& arm2.guard.is_none()
17+
&& let Some(arm_ref_mutbl) = if is_none_arm(cx, arm1) {
18+
as_ref_some_arm(cx, arm2)
19+
} else if is_none_arm(cx, arm2) {
20+
as_ref_some_arm(cx, arm1)
1821
} else {
1922
None
23+
}
24+
{
25+
let method = match arm_ref_mutbl {
26+
Mutability::Not => "as_ref",
27+
Mutability::Mut => "as_mut",
2028
};
21-
if let Some(rb) = arm_ref_mut {
22-
let suggestion = match rb {
23-
Mutability::Not => "as_ref",
24-
Mutability::Mut => "as_mut",
25-
};
2629

27-
let output_ty = cx.typeck_results().expr_ty(expr);
28-
let input_ty = cx.typeck_results().expr_ty(ex);
30+
let output_ty = cx.typeck_results().expr_ty(expr);
31+
let input_ty = cx.typeck_results().expr_ty(ex);
2932

30-
let cast = if let ty::Adt(_, args) = input_ty.kind()
31-
&& let input_ty = args.type_at(0)
32-
&& let ty::Adt(_, args) = output_ty.kind()
33-
&& let output_ty = args.type_at(0)
34-
&& let ty::Ref(_, output_ty, _) = *output_ty.kind()
35-
&& input_ty != output_ty
36-
{
37-
".map(|x| x as _)"
38-
} else {
39-
""
40-
};
33+
let cast = if let Some(input_ty) = option_arg_ty(cx, input_ty)
34+
&& let Some(output_ty) = option_arg_ty(cx, output_ty)
35+
&& let ty::Ref(_, output_ty, _) = *output_ty.kind()
36+
&& input_ty != output_ty
37+
{
38+
".map(|x| x as _)"
39+
} else {
40+
""
41+
};
4142

42-
let mut applicability = Applicability::MachineApplicable;
43-
span_lint_and_sugg(
44-
cx,
45-
MATCH_AS_REF,
46-
expr.span,
47-
format!("use `{suggestion}()` instead"),
48-
"try",
49-
format!(
50-
"{}.{suggestion}(){cast}",
51-
snippet_with_applicability(cx, ex.span, "_", &mut applicability),
52-
),
53-
applicability,
54-
);
55-
}
43+
let mut applicability = Applicability::MachineApplicable;
44+
span_lint_and_then(
45+
cx,
46+
MATCH_AS_REF,
47+
expr.span,
48+
format!("manual implementation of `Option::{method}`"),
49+
|diag| {
50+
diag.span_suggestion_verbose(
51+
expr.span,
52+
format!("use `Option::{method}()` directly"),
53+
format!(
54+
"{}.{method}(){cast}",
55+
Sugg::hir_with_applicability(cx, ex, "_", &mut applicability).maybe_paren(),
56+
),
57+
applicability,
58+
);
59+
},
60+
);
5661
}
5762
}
5863

5964
// Checks if arm has the form `Some(ref v) => Some(v)` (checks for `ref` and `ref mut`)
60-
fn is_ref_some_arm(cx: &LateContext<'_>, arm: &Arm<'_>) -> Option<Mutability> {
65+
fn as_ref_some_arm(cx: &LateContext<'_>, arm: &Arm<'_>) -> Option<Mutability> {
6166
if let PatKind::TupleStruct(ref qpath, [first_pat, ..], _) = arm.pat.kind
6267
&& cx
6368
.qpath_res(qpath, arm.pat.hir_id)

‎tests/ui/match_as_ref.fixed‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,3 +71,16 @@ mod issue15691 {
7171
};
7272
}
7373
}
74+
75+
fn recv_requiring_parens() {
76+
struct S;
77+
78+
impl std::ops::Not for S {
79+
type Output = Option<u64>;
80+
fn not(self) -> Self::Output {
81+
None
82+
}
83+
}
84+
85+
let _ = (!S).as_ref();
86+
}

‎tests/ui/match_as_ref.rs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,3 +83,20 @@ mod issue15691 {
8383
};
8484
}
8585
}
86+
87+
fn recv_requiring_parens() {
88+
struct S;
89+
90+
impl std::ops::Not for S {
91+
type Output = Option<u64>;
92+
fn not(self) -> Self::Output {
93+
None
94+
}
95+
}
96+
97+
let _ = match !S {
98+
//~^ match_as_ref
99+
None => None,
100+
Some(ref v) => Some(v),
101+
};
102+
}

‎tests/ui/match_as_ref.stderr‎

Lines changed: 57 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
error: use `as_ref()` instead
1+
error: manual implementation of `Option::as_ref`
22
--> tests/ui/match_as_ref.rs:6:33
33
|
44
LL | let borrowed: Option<&()> = match owned {
@@ -7,12 +7,21 @@ LL | |
77
LL | | None => None,
88
LL | | Some(ref v) => Some(v),
99
LL | | };
10-
| |_____^ help: try: `owned.as_ref()`
10+
| |_____^
1111
|
1212
= note: `-D clippy::match-as-ref` implied by `-D warnings`
1313
= help: to override `-D warnings` add `#[allow(clippy::match_as_ref)]`
14+
help: use `Option::as_ref()` directly
15+
|
16+
LL - let borrowed: Option<&()> = match owned {
17+
LL -
18+
LL - None => None,
19+
LL - Some(ref v) => Some(v),
20+
LL - };
21+
LL + let borrowed: Option<&()> = owned.as_ref();
22+
|
1423

15-
error: use `as_mut()` instead
24+
error: manual implementation of `Option::as_mut`
1625
--> tests/ui/match_as_ref.rs:13:39
1726
|
1827
LL | let borrow_mut: Option<&mut ()> = match mut_owned {
@@ -21,17 +30,58 @@ LL | |
2130
LL | | None => None,
2231
LL | | Some(ref mut v) => Some(v),
2332
LL | | };
24-
| |_____^ help: try: `mut_owned.as_mut()`
33+
| |_____^
34+
|
35+
help: use `Option::as_mut()` directly
36+
|
37+
LL - let borrow_mut: Option<&mut ()> = match mut_owned {
38+
LL -
39+
LL - None => None,
40+
LL - Some(ref mut v) => Some(v),
41+
LL - };
42+
LL + let borrow_mut: Option<&mut ()> = mut_owned.as_mut();
43+
|
2544

26-
error: use `as_ref()` instead
45+
error: manual implementation of `Option::as_ref`
2746
--> tests/ui/match_as_ref.rs:32:13
2847
|
2948
LL | / match self.source {
3049
LL | |
3150
LL | | Some(ref s) => Some(s),
3251
LL | | None => None,
3352
LL | | }
34-
| |_____________^ help: try: `self.source.as_ref().map(|x| x as _)`
53+
| |_____________^
54+
|
55+
help: use `Option::as_ref()` directly
56+
|
57+
LL - match self.source {
58+
LL -
59+
LL - Some(ref s) => Some(s),
60+
LL - None => None,
61+
LL - }
62+
LL + self.source.as_ref().map(|x| x as _)
63+
|
64+
65+
error: manual implementation of `Option::as_ref`
66+
--> tests/ui/match_as_ref.rs:97:13
67+
|
68+
LL | let _ = match !S {
69+
| _____________^
70+
LL | |
71+
LL | | None => None,
72+
LL | | Some(ref v) => Some(v),
73+
LL | | };
74+
| |_____^
75+
|
76+
help: use `Option::as_ref()` directly
77+
|
78+
LL - let _ = match !S {
79+
LL -
80+
LL - None => None,
81+
LL - Some(ref v) => Some(v),
82+
LL - };
83+
LL + let _ = (!S).as_ref();
84+
|
3585

36-
error: aborting due to 3 previous errors
86+
error: aborting due to 4 previous errors
3787

0 commit comments

Comments
 (0)