Skip to content

Commit 5cb7e40

Browse files
llogiqllogiq
authored
Reenable linting on UFCS deref calls (#14808)
Was disabled in #10855 due to a broken suggestion. This will only lint if a type is not proveded (e.g. `T::deref`). changelog: none
2 parents 5239b7f + 27acbf2 commit 5cb7e40

File tree

4 files changed

+73
-60
lines changed

4 files changed

+73
-60
lines changed

‎clippy_lints/src/dereference.rs

Lines changed: 28 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt, TypeckResults};
2222
use rustc_session::impl_lint_pass;
2323
use rustc_span::symbol::sym;
2424
use rustc_span::{Span, Symbol};
25+
use std::borrow::Cow;
2526

2627
declare_clippy_lint! {
2728
/// ### What it does
@@ -252,13 +253,14 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
252253
}
253254

254255
let typeck = cx.typeck_results();
255-
let Some((kind, sub_expr)) = try_parse_ref_op(cx.tcx, typeck, expr) else {
256+
let Some((kind, sub_expr, skip_expr)) = try_parse_ref_op(cx.tcx, typeck, expr) else {
256257
// The whole chain of reference operations has been seen
257258
if let Some((state, data)) = self.state.take() {
258259
report(cx, expr, state, data, typeck);
259260
}
260261
return;
261262
};
263+
self.skip_expr = skip_expr;
262264

263265
match (self.state.take(), kind) {
264266
(None, kind) => {
@@ -671,42 +673,38 @@ fn try_parse_ref_op<'tcx>(
671673
tcx: TyCtxt<'tcx>,
672674
typeck: &'tcx TypeckResults<'_>,
673675
expr: &'tcx Expr<'_>,
674-
) -> Option<(RefOp, &'tcx Expr<'tcx>)> {
675-
let (is_ufcs, def_id, arg) = match expr.kind {
676-
ExprKind::MethodCall(_, arg, [], _) => (false, typeck.type_dependent_def_id(expr.hir_id)?, arg),
676+
) -> Option<(RefOp, &'tcx Expr<'tcx>, Option<HirId>)> {
677+
let (call_path_id, def_id, arg) = match expr.kind {
678+
ExprKind::MethodCall(_, arg, [], _) => (None, typeck.type_dependent_def_id(expr.hir_id)?, arg),
677679
ExprKind::Call(
678-
Expr {
679-
kind: ExprKind::Path(path),
680+
&Expr {
681+
kind: ExprKind::Path(QPath::Resolved(None, path)),
680682
hir_id,
681683
..
682684
},
683685
[arg],
684-
) => (true, typeck.qpath_res(path, *hir_id).opt_def_id()?, arg),
686+
) => (Some(hir_id), path.res.opt_def_id()?, arg),
685687
ExprKind::Unary(UnOp::Deref, sub_expr) if !typeck.expr_ty(sub_expr).is_raw_ptr() => {
686-
return Some((RefOp::Deref, sub_expr));
688+
return Some((RefOp::Deref, sub_expr, None));
689+
},
690+
ExprKind::AddrOf(BorrowKind::Ref, mutability, sub_expr) => {
691+
return Some((RefOp::AddrOf(mutability), sub_expr, None));
687692
},
688-
ExprKind::AddrOf(BorrowKind::Ref, mutability, sub_expr) => return Some((RefOp::AddrOf(mutability), sub_expr)),
689693
_ => return None,
690694
};
691-
if tcx.is_diagnostic_item(sym::deref_method, def_id) {
692-
Some((
693-
RefOp::Method {
694-
mutbl: Mutability::Not,
695-
is_ufcs,
696-
},
697-
arg,
698-
))
699-
} else if tcx.trait_of_item(def_id)? == tcx.lang_items().deref_mut_trait()? {
700-
Some((
701-
RefOp::Method {
702-
mutbl: Mutability::Mut,
703-
is_ufcs,
704-
},
705-
arg,
706-
))
707-
} else {
708-
None
709-
}
695+
let mutbl = match tcx.get_diagnostic_name(def_id) {
696+
Some(sym::deref_method) => Mutability::Not,
697+
Some(sym::deref_mut_method) => Mutability::Mut,
698+
_ => return None,
699+
};
700+
Some((
701+
RefOp::Method {
702+
mutbl,
703+
is_ufcs: call_path_id.is_some(),
704+
},
705+
arg,
706+
call_path_id,
707+
))
710708
}
711709

712710
// Checks if the adjustments contains a deref of `ManuallyDrop<_>`
@@ -944,7 +942,7 @@ fn report<'tcx>(
944942
mutbl,
945943
} => {
946944
let mut app = Applicability::MachineApplicable;
947-
let (expr_str, _expr_is_macro_call) =
945+
let (expr_str, expr_is_macro_call) =
948946
snippet_with_context(cx, expr.span, data.first_expr.span.ctxt(), "..", &mut app);
949947
let ty = typeck.expr_ty(expr);
950948
let (_, ref_count) = peel_middle_ty_refs(ty);
@@ -968,20 +966,11 @@ fn report<'tcx>(
968966
"&"
969967
};
970968

971-
// expr_str (the suggestion) is never shown if is_final_ufcs is true, since it's
972-
// `expr.kind == ExprKind::Call`. Therefore, this is, afaik, always unnecessary.
973-
/*
974-
expr_str = if !expr_is_macro_call && is_final_ufcs && expr.precedence() < ExprPrecedence::Prefix {
969+
let expr_str = if !expr_is_macro_call && is_ufcs && expr.precedence() < ExprPrecedence::Prefix {
975970
Cow::Owned(format!("({expr_str})"))
976971
} else {
977972
expr_str
978973
};
979-
*/
980-
981-
// Fix #10850, do not lint if it's `Foo::deref` instead of `foo.deref()`.
982-
if is_ufcs {
983-
return;
984-
}
985974

986975
span_lint_and_sugg(
987976
cx,

‎tests/ui/explicit_deref_methods.fixed

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@
88
clippy::needless_borrow,
99
clippy::no_effect,
1010
clippy::uninlined_format_args,
11-
clippy::unnecessary_literal_unwrap
11+
clippy::unnecessary_literal_unwrap,
12+
clippy::deref_addrof
1213
)]
1314

1415
use std::ops::{Deref, DerefMut};
@@ -87,9 +88,6 @@ fn main() {
8788
let b = &*opt_a.unwrap();
8889
//~^ explicit_deref_methods
8990

90-
// make sure `Aaa::deref` instead of `aaa.deref()` is not linted, as well as fully qualified
91-
// syntax
92-
9391
Aaa::deref(&Aaa);
9492
Aaa::deref_mut(&mut Aaa);
9593
<Aaa as Deref>::deref(&Aaa);
@@ -139,4 +137,9 @@ fn main() {
139137
let no_lint = NoLint(42);
140138
let b = no_lint.deref();
141139
let b = no_lint.deref_mut();
140+
141+
let _ = &*&"foo"; //~ explicit_deref_methods
142+
let mut x = String::new();
143+
let _ = &&mut **&mut x; //~ explicit_deref_methods
144+
let _ = &&mut ***(&mut &mut x); //~ explicit_deref_methods
142145
}

‎tests/ui/explicit_deref_methods.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@
88
clippy::needless_borrow,
99
clippy::no_effect,
1010
clippy::uninlined_format_args,
11-
clippy::unnecessary_literal_unwrap
11+
clippy::unnecessary_literal_unwrap,
12+
clippy::deref_addrof
1213
)]
1314

1415
use std::ops::{Deref, DerefMut};
@@ -87,9 +88,6 @@ fn main() {
8788
let b = opt_a.unwrap().deref();
8889
//~^ explicit_deref_methods
8990

90-
// make sure `Aaa::deref` instead of `aaa.deref()` is not linted, as well as fully qualified
91-
// syntax
92-
9391
Aaa::deref(&Aaa);
9492
Aaa::deref_mut(&mut Aaa);
9593
<Aaa as Deref>::deref(&Aaa);
@@ -139,4 +137,9 @@ fn main() {
139137
let no_lint = NoLint(42);
140138
let b = no_lint.deref();
141139
let b = no_lint.deref_mut();
140+
141+
let _ = &Deref::deref(&"foo"); //~ explicit_deref_methods
142+
let mut x = String::new();
143+
let _ = &DerefMut::deref_mut(&mut x); //~ explicit_deref_methods
144+
let _ = &DerefMut::deref_mut((&mut &mut x).deref_mut()); //~ explicit_deref_methods
142145
}

‎tests/ui/explicit_deref_methods.stderr

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: explicit `deref` method call
2-
--> tests/ui/explicit_deref_methods.rs:54:19
2+
--> tests/ui/explicit_deref_methods.rs:55:19
33
|
44
LL | let b: &str = a.deref();
55
| ^^^^^^^^^ help: try: `&*a`
@@ -8,70 +8,88 @@ LL | let b: &str = a.deref();
88
= help: to override `-D warnings` add `#[allow(clippy::explicit_deref_methods)]`
99

1010
error: explicit `deref_mut` method call
11-
--> tests/ui/explicit_deref_methods.rs:57:23
11+
--> tests/ui/explicit_deref_methods.rs:58:23
1212
|
1313
LL | let b: &mut str = a.deref_mut();
1414
| ^^^^^^^^^^^^^ help: try: `&mut **a`
1515

1616
error: explicit `deref` method call
17-
--> tests/ui/explicit_deref_methods.rs:61:39
17+
--> tests/ui/explicit_deref_methods.rs:62:39
1818
|
1919
LL | let b: String = format!("{}, {}", a.deref(), a.deref());
2020
| ^^^^^^^^^ help: try: `&*a`
2121

2222
error: explicit `deref` method call
23-
--> tests/ui/explicit_deref_methods.rs:61:50
23+
--> tests/ui/explicit_deref_methods.rs:62:50
2424
|
2525
LL | let b: String = format!("{}, {}", a.deref(), a.deref());
2626
| ^^^^^^^^^ help: try: `&*a`
2727

2828
error: explicit `deref` method call
29-
--> tests/ui/explicit_deref_methods.rs:65:20
29+
--> tests/ui/explicit_deref_methods.rs:66:20
3030
|
3131
LL | println!("{}", a.deref());
3232
| ^^^^^^^^^ help: try: `&*a`
3333

3434
error: explicit `deref` method call
35-
--> tests/ui/explicit_deref_methods.rs:69:11
35+
--> tests/ui/explicit_deref_methods.rs:70:11
3636
|
3737
LL | match a.deref() {
3838
| ^^^^^^^^^ help: try: `&*a`
3939

4040
error: explicit `deref` method call
41-
--> tests/ui/explicit_deref_methods.rs:74:28
41+
--> tests/ui/explicit_deref_methods.rs:75:28
4242
|
4343
LL | let b: String = concat(a.deref());
4444
| ^^^^^^^^^ help: try: `&*a`
4545

4646
error: explicit `deref` method call
47-
--> tests/ui/explicit_deref_methods.rs:77:13
47+
--> tests/ui/explicit_deref_methods.rs:78:13
4848
|
4949
LL | let b = just_return(a).deref();
5050
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `just_return(a)`
5151

5252
error: explicit `deref` method call
53-
--> tests/ui/explicit_deref_methods.rs:80:28
53+
--> tests/ui/explicit_deref_methods.rs:81:28
5454
|
5555
LL | let b: String = concat(just_return(a).deref());
5656
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `just_return(a)`
5757

5858
error: explicit `deref` method call
59-
--> tests/ui/explicit_deref_methods.rs:83:19
59+
--> tests/ui/explicit_deref_methods.rs:84:19
6060
|
6161
LL | let b: &str = a.deref().deref();
6262
| ^^^^^^^^^^^^^^^^^ help: try: `&**a`
6363

6464
error: explicit `deref` method call
65-
--> tests/ui/explicit_deref_methods.rs:87:13
65+
--> tests/ui/explicit_deref_methods.rs:88:13
6666
|
6767
LL | let b = opt_a.unwrap().deref();
6868
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `&*opt_a.unwrap()`
6969

7070
error: explicit `deref` method call
71-
--> tests/ui/explicit_deref_methods.rs:125:31
71+
--> tests/ui/explicit_deref_methods.rs:123:31
7272
|
7373
LL | let b: &str = expr_deref!(a.deref());
7474
| ^^^^^^^^^ help: try: `&*a`
7575

76-
error: aborting due to 12 previous errors
76+
error: explicit `deref` method call
77+
--> tests/ui/explicit_deref_methods.rs:141:14
78+
|
79+
LL | let _ = &Deref::deref(&"foo");
80+
| ^^^^^^^^^^^^^^^^^^^^ help: try: `*&"foo"`
81+
82+
error: explicit `deref_mut` method call
83+
--> tests/ui/explicit_deref_methods.rs:143:14
84+
|
85+
LL | let _ = &DerefMut::deref_mut(&mut x);
86+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&mut **&mut x`
87+
88+
error: explicit `deref_mut` method call
89+
--> tests/ui/explicit_deref_methods.rs:144:14
90+
|
91+
LL | let _ = &DerefMut::deref_mut((&mut &mut x).deref_mut());
92+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&mut ***(&mut &mut x)`
93+
94+
error: aborting due to 15 previous errors
7795

0 commit comments

Comments
 (0)