Skip to content

Commit 28759b2

Browse files
borsbors
committed
Auto merge of #6865 - Jarcho:explicit_deref_methods, r=llogiq
`explicit_deref_methods` improvements Breaking up #6837 changelog: `explicit_deref_methods` will lint chained `deref` calls and ufcs style calls
2 parents 65d046c + 2713ad4 commit 28759b2

File tree

8 files changed

+402
-167
lines changed

8 files changed

+402
-167
lines changed

‎clippy_lints/src/dereference.rs

+254-65
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
1-
use crate::utils::{get_parent_expr, implements_trait, snippet, span_lint_and_sugg};
2-
use if_chain::if_chain;
3-
use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX, PREC_PREFIX};
1+
use crate::utils::{get_parent_node, in_macro, is_allowed, peel_mid_ty_refs, snippet_with_context, span_lint_and_sugg};
2+
use rustc_ast::util::parser::PREC_PREFIX;
43
use rustc_errors::Applicability;
5-
use rustc_hir::{Expr, ExprKind};
4+
use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, MatchSource, Mutability, Node, UnOp};
65
use rustc_lint::{LateContext, LateLintPass};
7-
use rustc_session::{declare_lint_pass, declare_tool_lint};
8-
use rustc_span::source_map::Span;
6+
use rustc_middle::ty::{self, Ty, TyCtxt, TyS, TypeckResults};
7+
use rustc_session::{declare_tool_lint, impl_lint_pass};
8+
use rustc_span::{symbol::sym, Span};
99

1010
declare_clippy_lint! {
1111
/// **What it does:** Checks for explicit `deref()` or `deref_mut()` method calls.
@@ -34,76 +34,265 @@ declare_clippy_lint! {
3434
"Explicit use of deref or deref_mut method while not in a method chain."
3535
}
3636

37-
declare_lint_pass!(Dereferencing => [
38-
EXPLICIT_DEREF_METHODS
37+
impl_lint_pass!(Dereferencing => [
38+
EXPLICIT_DEREF_METHODS,
3939
]);
4040

41+
#[derive(Default)]
42+
pub struct Dereferencing {
43+
state: Option<(State, StateData)>,
44+
45+
// While parsing a `deref` method call in ufcs form, the path to the function is itself an
46+
// expression. This is to store the id of that expression so it can be skipped when
47+
// `check_expr` is called for it.
48+
skip_expr: Option<HirId>,
49+
}
50+
51+
struct StateData {
52+
/// Span of the top level expression
53+
span: Span,
54+
/// The required mutability
55+
target_mut: Mutability,
56+
}
57+
58+
enum State {
59+
// Any number of deref method calls.
60+
DerefMethod {
61+
// The number of calls in a sequence which changed the referenced type
62+
ty_changed_count: usize,
63+
is_final_ufcs: bool,
64+
},
65+
}
66+
67+
// A reference operation considered by this lint pass
68+
enum RefOp {
69+
Method(Mutability),
70+
Deref,
71+
AddrOf,
72+
}
73+
4174
impl<'tcx> LateLintPass<'tcx> for Dereferencing {
4275
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
43-
if_chain! {
44-
if !expr.span.from_expansion();
45-
if let ExprKind::MethodCall(ref method_name, _, ref args, _) = &expr.kind;
46-
if args.len() == 1;
47-
48-
then {
49-
if let Some(parent_expr) = get_parent_expr(cx, expr) {
50-
// Check if we have the whole call chain here
51-
if let ExprKind::MethodCall(..) = parent_expr.kind {
52-
return;
53-
}
54-
// Check for Expr that we don't want to be linted
55-
let precedence = parent_expr.precedence();
56-
match precedence {
57-
// Lint a Call is ok though
58-
ExprPrecedence::Call | ExprPrecedence::AddrOf => (),
59-
_ => {
60-
if precedence.order() >= PREC_PREFIX && precedence.order() <= PREC_POSTFIX {
61-
return;
62-
}
63-
}
76+
// Skip path expressions from deref calls. e.g. `Deref::deref(e)`
77+
if Some(expr.hir_id) == self.skip_expr.take() {
78+
return;
79+
}
80+
81+
// Stop processing sub expressions when a macro call is seen
82+
if in_macro(expr.span) {
83+
if let Some((state, data)) = self.state.take() {
84+
report(cx, expr, state, data);
85+
}
86+
return;
87+
}
88+
89+
let typeck = cx.typeck_results();
90+
let (kind, sub_expr) = if let Some(x) = try_parse_ref_op(cx.tcx, typeck, expr) {
91+
x
92+
} else {
93+
// The whole chain of reference operations has been seen
94+
if let Some((state, data)) = self.state.take() {
95+
report(cx, expr, state, data);
96+
}
97+
return;
98+
};
99+
100+
match (self.state.take(), kind) {
101+
(None, kind) => {
102+
let parent = get_parent_node(cx.tcx, expr.hir_id);
103+
let expr_ty = typeck.expr_ty(expr);
104+
105+
match kind {
106+
RefOp::Method(target_mut)
107+
if !is_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id)
108+
&& is_linted_explicit_deref_position(parent, expr.hir_id, expr.span) =>
109+
{
110+
self.state = Some((
111+
State::DerefMethod {
112+
ty_changed_count: if deref_method_same_type(expr_ty, typeck.expr_ty(sub_expr)) {
113+
0
114+
} else {
115+
1
116+
},
117+
is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
118+
},
119+
StateData {
120+
span: expr.span,
121+
target_mut,
122+
},
123+
));
64124
}
125+
_ => (),
65126
}
66-
let name = method_name.ident.as_str();
67-
lint_deref(cx, &*name, &args[0], args[0].span, expr.span);
68-
}
127+
},
128+
(Some((State::DerefMethod { ty_changed_count, .. }, data)), RefOp::Method(_)) => {
129+
self.state = Some((
130+
State::DerefMethod {
131+
ty_changed_count: if deref_method_same_type(typeck.expr_ty(expr), typeck.expr_ty(sub_expr)) {
132+
ty_changed_count
133+
} else {
134+
ty_changed_count + 1
135+
},
136+
is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
137+
},
138+
data,
139+
));
140+
},
141+
142+
(Some((state, data)), _) => report(cx, expr, state, data),
69143
}
70144
}
71145
}
72146

73-
fn lint_deref(cx: &LateContext<'_>, method_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) {
74-
match method_name {
75-
"deref" => {
76-
let impls_deref_trait = cx.tcx.lang_items().deref_trait().map_or(false, |id| {
77-
implements_trait(cx, cx.typeck_results().expr_ty(&call_expr), id, &[])
78-
});
79-
if impls_deref_trait {
80-
span_lint_and_sugg(
81-
cx,
82-
EXPLICIT_DEREF_METHODS,
83-
expr_span,
84-
"explicit deref method call",
85-
"try this",
86-
format!("&*{}", &snippet(cx, var_span, "..")),
87-
Applicability::MachineApplicable,
88-
);
89-
}
147+
fn try_parse_ref_op(
148+
tcx: TyCtxt<'tcx>,
149+
typeck: &'tcx TypeckResults<'_>,
150+
expr: &'tcx Expr<'_>,
151+
) -> Option<(RefOp, &'tcx Expr<'tcx>)> {
152+
let (def_id, arg) = match expr.kind {
153+
ExprKind::MethodCall(_, _, [arg], _) => (typeck.type_dependent_def_id(expr.hir_id)?, arg),
154+
ExprKind::Call(
155+
Expr {
156+
kind: ExprKind::Path(path),
157+
hir_id,
158+
..
159+
},
160+
[arg],
161+
) => (typeck.qpath_res(path, *hir_id).opt_def_id()?, arg),
162+
ExprKind::Unary(UnOp::Deref, sub_expr) if !typeck.expr_ty(sub_expr).is_unsafe_ptr() => {
163+
return Some((RefOp::Deref, sub_expr));
90164
},
91-
"deref_mut" => {
92-
let impls_deref_mut_trait = cx.tcx.lang_items().deref_mut_trait().map_or(false, |id| {
93-
implements_trait(cx, cx.typeck_results().expr_ty(&call_expr), id, &[])
94-
});
95-
if impls_deref_mut_trait {
96-
span_lint_and_sugg(
97-
cx,
98-
EXPLICIT_DEREF_METHODS,
99-
expr_span,
100-
"explicit deref_mut method call",
101-
"try this",
102-
format!("&mut *{}", &snippet(cx, var_span, "..")),
103-
Applicability::MachineApplicable,
104-
);
105-
}
165+
ExprKind::AddrOf(BorrowKind::Ref, _, sub_expr) => return Some((RefOp::AddrOf, sub_expr)),
166+
_ => return None,
167+
};
168+
if tcx.is_diagnostic_item(sym::deref_method, def_id) {
169+
Some((RefOp::Method(Mutability::Not), arg))
170+
} else if tcx.trait_of_item(def_id)? == tcx.lang_items().deref_mut_trait()? {
171+
Some((RefOp::Method(Mutability::Mut), arg))
172+
} else {
173+
None
174+
}
175+
}
176+
177+
// Checks whether the type for a deref call actually changed the type, not just the mutability of
178+
// the reference.
179+
fn deref_method_same_type(result_ty: Ty<'tcx>, arg_ty: Ty<'tcx>) -> bool {
180+
match (result_ty.kind(), arg_ty.kind()) {
181+
(ty::Ref(_, result_ty, _), ty::Ref(_, arg_ty, _)) => TyS::same_type(result_ty, arg_ty),
182+
183+
// The result type for a deref method is always a reference
184+
// Not matching the previous pattern means the argument type is not a reference
185+
// This means that the type did change
186+
_ => false,
187+
}
188+
}
189+
190+
// Checks whether the parent node is a suitable context for switching from a deref method to the
191+
// deref operator.
192+
fn is_linted_explicit_deref_position(parent: Option<Node<'_>>, child_id: HirId, child_span: Span) -> bool {
193+
let parent = match parent {
194+
Some(Node::Expr(e)) if e.span.ctxt() == child_span.ctxt() => e,
195+
_ => return true,
196+
};
197+
match parent.kind {
198+
// Leave deref calls in the middle of a method chain.
199+
// e.g. x.deref().foo()
200+
ExprKind::MethodCall(_, _, [self_arg, ..], _) if self_arg.hir_id == child_id => false,
201+
202+
// Leave deref calls resulting in a called function
203+
// e.g. (x.deref())()
204+
ExprKind::Call(func_expr, _) if func_expr.hir_id == child_id => false,
205+
206+
// Makes an ugly suggestion
207+
// e.g. *x.deref() => *&*x
208+
ExprKind::Unary(UnOp::Deref, _)
209+
// Postfix expressions would require parens
210+
| ExprKind::Match(_, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar)
211+
| ExprKind::Field(..)
212+
| ExprKind::Index(..)
213+
| ExprKind::Err => false,
214+
215+
ExprKind::Box(..)
216+
| ExprKind::ConstBlock(..)
217+
| ExprKind::Array(_)
218+
| ExprKind::Call(..)
219+
| ExprKind::MethodCall(..)
220+
| ExprKind::Tup(..)
221+
| ExprKind::Binary(..)
222+
| ExprKind::Unary(..)
223+
| ExprKind::Lit(..)
224+
| ExprKind::Cast(..)
225+
| ExprKind::Type(..)
226+
| ExprKind::DropTemps(..)
227+
| ExprKind::If(..)
228+
| ExprKind::Loop(..)
229+
| ExprKind::Match(..)
230+
| ExprKind::Closure(..)
231+
| ExprKind::Block(..)
232+
| ExprKind::Assign(..)
233+
| ExprKind::AssignOp(..)
234+
| ExprKind::Path(..)
235+
| ExprKind::AddrOf(..)
236+
| ExprKind::Break(..)
237+
| ExprKind::Continue(..)
238+
| ExprKind::Ret(..)
239+
| ExprKind::InlineAsm(..)
240+
| ExprKind::LlvmInlineAsm(..)
241+
| ExprKind::Struct(..)
242+
| ExprKind::Repeat(..)
243+
| ExprKind::Yield(..) => true,
244+
}
245+
}
246+
247+
#[allow(clippy::needless_pass_by_value)]
248+
fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: StateData) {
249+
match state {
250+
State::DerefMethod {
251+
ty_changed_count,
252+
is_final_ufcs,
253+
} => {
254+
let mut app = Applicability::MachineApplicable;
255+
let (expr_str, expr_is_macro_call) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app);
256+
let ty = cx.typeck_results().expr_ty(expr);
257+
let (_, ref_count) = peel_mid_ty_refs(ty);
258+
let deref_str = if ty_changed_count >= ref_count && ref_count != 0 {
259+
// a deref call changing &T -> &U requires two deref operators the first time
260+
// this occurs. One to remove the reference, a second to call the deref impl.
261+
"*".repeat(ty_changed_count + 1)
262+
} else {
263+
"*".repeat(ty_changed_count)
264+
};
265+
let addr_of_str = if ty_changed_count < ref_count {
266+
// Check if a reborrow from &mut T -> &T is required.
267+
if data.target_mut == Mutability::Not && matches!(ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
268+
"&*"
269+
} else {
270+
""
271+
}
272+
} else if data.target_mut == Mutability::Mut {
273+
"&mut "
274+
} else {
275+
"&"
276+
};
277+
278+
let expr_str = if !expr_is_macro_call && is_final_ufcs && expr.precedence().order() < PREC_PREFIX {
279+
format!("({})", expr_str)
280+
} else {
281+
expr_str.into_owned()
282+
};
283+
284+
span_lint_and_sugg(
285+
cx,
286+
EXPLICIT_DEREF_METHODS,
287+
data.span,
288+
match data.target_mut {
289+
Mutability::Not => "explicit `deref` method call",
290+
Mutability::Mut => "explicit `deref_mut` method call",
291+
},
292+
"try this",
293+
format!("{}{}{}", addr_of_str, deref_str, expr_str),
294+
app,
295+
);
106296
},
107-
_ => (),
108297
}
109298
}

‎clippy_lints/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1241,7 +1241,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12411241
store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
12421242
store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
12431243
store.register_late_pass(|| box unnamed_address::UnnamedAddress);
1244-
store.register_late_pass(|| box dereference::Dereferencing);
1244+
store.register_late_pass(|| box dereference::Dereferencing::default());
12451245
store.register_late_pass(|| box option_if_let_else::OptionIfLetElse);
12461246
store.register_late_pass(|| box future_not_send::FutureNotSend);
12471247
store.register_late_pass(|| box if_let_mutex::IfLetMutex);

‎clippy_lints/src/manual_map.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ impl LateLintPass<'_> for ManualMap {
129129
// Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
130130
// it's being passed by value.
131131
let scrutinee = peel_hir_expr_refs(scrutinee).0;
132-
let scrutinee_str = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app);
132+
let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app);
133133
let scrutinee_str =
134134
if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX {
135135
format!("({})", scrutinee_str)
@@ -160,16 +160,16 @@ impl LateLintPass<'_> for ManualMap {
160160
"|{}{}| {}",
161161
annotation,
162162
some_binding,
163-
snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app)
163+
snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app).0
164164
)
165165
},
166166
}
167167
} else if !is_wild_none && explicit_ref.is_none() {
168168
// TODO: handle explicit reference annotations.
169169
format!(
170170
"|{}| {}",
171-
snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app),
172-
snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app)
171+
snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0,
172+
snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app).0
173173
)
174174
} else {
175175
// Refutable bindings and mixed reference annotations can't be handled by `map`.

0 commit comments

Comments
 (0)