Skip to content

Commit 0efafa4

Browse files
Serial-ATASerial-ATA
committed
Better handle method/function calls
1 parent 2666c38 commit 0efafa4

File tree

5 files changed

+105
-47
lines changed

5 files changed

+105
-47
lines changed

‎clippy_lints/src/lib.register_suspicious.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,6 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
3232
LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
3333
LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
3434
LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF),
35-
LintId::of(write::POSITIONAL_NAMED_FORMAT_PARAMETERS),
3635
LintId::of(unused_peekable::UNUSED_PEEKABLE),
36+
LintId::of(write::POSITIONAL_NAMED_FORMAT_PARAMETERS),
3737
])

‎clippy_lints/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -936,7 +936,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
936936
store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed));
937937
store.register_late_pass(|| Box::new(partialeq_to_none::PartialeqToNone));
938938
store.register_late_pass(|| Box::new(manual_empty_string_creations::ManualEmptyStringCreations));
939-
store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable::default()));
939+
store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable));
940940
// add lints here, do not remove this comment, it's used in `new_lint`
941941
}
942942

‎clippy_lints/src/unused_peekable.rs‎

Lines changed: 60 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
use clippy_utils::diagnostics::span_lint_and_help;
22
use clippy_utils::ty::{match_type, peel_mid_ty_refs_is_mutable};
3-
use clippy_utils::{fn_def_id, path_to_local_id, paths, peel_ref_operators};
3+
use clippy_utils::{fn_def_id, is_trait_method, path_to_local_id, paths, peel_ref_operators};
44
use rustc_ast::Mutability;
55
use rustc_hir::intravisit::{walk_expr, Visitor};
66
use rustc_hir::lang_items::LangItem;
77
use rustc_hir::{Block, Expr, ExprKind, HirId, Local, Node, PatKind, PathSegment, StmtKind};
88
use rustc_lint::{LateContext, LateLintPass};
9-
use rustc_middle::ty::Ty;
10-
use rustc_session::{declare_tool_lint, impl_lint_pass};
9+
use rustc_session::{declare_lint_pass, declare_tool_lint};
10+
use rustc_span::sym;
1111

1212
declare_clippy_lint! {
1313
/// ### What it does
@@ -42,12 +42,7 @@ declare_clippy_lint! {
4242
"creating a peekable iterator without using any of its methods"
4343
}
4444

45-
#[derive(Default)]
46-
pub struct UnusedPeekable {
47-
visited: Vec<HirId>,
48-
}
49-
50-
impl_lint_pass!(UnusedPeekable => [UNUSED_PEEKABLE]);
45+
declare_lint_pass!(UnusedPeekable => [UNUSED_PEEKABLE]);
5146

5247
impl<'tcx> LateLintPass<'tcx> for UnusedPeekable {
5348
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &Block<'tcx>) {
@@ -62,15 +57,14 @@ impl<'tcx> LateLintPass<'tcx> for UnusedPeekable {
6257
for (idx, stmt) in block.stmts.iter().enumerate() {
6358
if !stmt.span.from_expansion()
6459
&& let StmtKind::Local(local) = stmt.kind
65-
&& !self.visited.contains(&local.pat.hir_id)
66-
&& let PatKind::Binding(_, _, ident, _) = local.pat.kind
60+
&& let PatKind::Binding(_, binding, ident, _) = local.pat.kind
6761
&& let Some(init) = local.init
6862
&& !init.span.from_expansion()
6963
&& let Some(ty) = cx.typeck_results().expr_ty_opt(init)
7064
&& let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty)
7165
&& match_type(cx, ty, &paths::PEEKABLE)
7266
{
73-
let mut vis = PeekableVisitor::new(cx, local.pat.hir_id);
67+
let mut vis = PeekableVisitor::new(cx, binding);
7468

7569
if idx + 1 == block.stmts.len() && block.expr.is_none() {
7670
return;
@@ -117,6 +111,10 @@ impl<'a, 'tcx> PeekableVisitor<'a, 'tcx> {
117111

118112
impl<'tcx> Visitor<'_> for PeekableVisitor<'_, 'tcx> {
119113
fn visit_expr(&mut self, ex: &'_ Expr<'_>) {
114+
if self.found_peek_call {
115+
return;
116+
}
117+
120118
if path_to_local_id(ex, self.expected_hir_id) {
121119
for (_, node) in self.cx.tcx.hir().parent_iter(ex.hir_id) {
122120
match node {
@@ -138,50 +136,58 @@ impl<'tcx> Visitor<'_> for PeekableVisitor<'_, 'tcx> {
138136
return;
139137
}
140138

141-
for arg in args.iter().map(|arg| peel_ref_operators(self.cx, arg)) {
142-
if let ExprKind::Path(_) = arg.kind
143-
&& let Some(ty) = self
144-
.cx
145-
.typeck_results()
146-
.expr_ty_opt(arg)
147-
.map(Ty::peel_refs)
148-
&& match_type(self.cx, ty, &paths::PEEKABLE)
149-
{
150-
self.found_peek_call = true;
151-
return;
152-
}
139+
if args.iter().any(|arg| {
140+
matches!(arg.kind, ExprKind::Path(_)) && arg_is_mut_peekable(self.cx, arg)
141+
}) {
142+
self.found_peek_call = true;
143+
return;
153144
}
154145
},
155-
// Peekable::peek()
156-
ExprKind::MethodCall(PathSegment { ident: method_name, .. }, [arg, ..], _) => {
157-
let arg = peel_ref_operators(self.cx, arg);
158-
let method_name = method_name.name.as_str();
159-
160-
if (method_name == "peek"
161-
|| method_name == "peek_mut"
162-
|| method_name == "next_if"
163-
|| method_name == "next_if_eq")
164-
&& let ExprKind::Path(_) = arg.kind
165-
&& let Some(ty) = self.cx.typeck_results().expr_ty_opt(arg).map(Ty::peel_refs)
166-
&& match_type(self.cx, ty, &paths::PEEKABLE)
146+
// Catch anything taking a Peekable mutably
147+
ExprKind::MethodCall(
148+
PathSegment {
149+
ident: method_name_ident,
150+
..
151+
},
152+
[self_arg, remaining_args @ ..],
153+
_,
154+
) => {
155+
let method_name = method_name_ident.name.as_str();
156+
157+
// `Peekable` methods
158+
if matches!(method_name, "peek" | "peek_mut" | "next_if" | "next_if_eq")
159+
&& arg_is_mut_peekable(self.cx, self_arg)
160+
{
161+
self.found_peek_call = true;
162+
return;
163+
}
164+
165+
// foo.some_method() excluding Iterator methods
166+
if remaining_args.iter().any(|arg| arg_is_mut_peekable(self.cx, arg))
167+
&& !is_trait_method(self.cx, expr, sym::Iterator)
167168
{
168169
self.found_peek_call = true;
169170
return;
170171
}
172+
173+
// foo.by_ref(), keep checking for `peek`
174+
if method_name == "by_ref" {
175+
continue;
176+
}
177+
178+
return;
179+
},
180+
ExprKind::AddrOf(_, Mutability::Mut, _) | ExprKind::Unary(..) | ExprKind::DropTemps(_) => {
171181
},
172-
// Don't bother if moved into a struct
173-
ExprKind::Struct(..) => {
182+
ExprKind::AddrOf(_, Mutability::Not, _) => return,
183+
_ => {
174184
self.found_peek_call = true;
175185
return;
176186
},
177-
_ => {},
178187
}
179188
},
180189
Node::Local(Local { init: Some(init), .. }) => {
181-
if let Some(ty) = self.cx.typeck_results().expr_ty_opt(init)
182-
&& let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty)
183-
&& match_type(self.cx, ty, &paths::PEEKABLE)
184-
{
190+
if arg_is_mut_peekable(self.cx, init) {
185191
self.found_peek_call = true;
186192
return;
187193
}
@@ -206,3 +212,14 @@ impl<'tcx> Visitor<'_> for PeekableVisitor<'_, 'tcx> {
206212
walk_expr(self, ex);
207213
}
208214
}
215+
216+
fn arg_is_mut_peekable(cx: &LateContext<'_>, arg: &Expr<'_>) -> bool {
217+
if let Some(ty) = cx.typeck_results().expr_ty_opt(arg)
218+
&& let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty)
219+
&& match_type(cx, ty, &paths::PEEKABLE)
220+
{
221+
true
222+
} else {
223+
false
224+
}
225+
}

‎tests/ui/unused_peekable.rs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ fn invalid() {
3232
let mut peekable_using_iterator_method = std::iter::empty::<u32>().peekable();
3333
peekable_using_iterator_method.next();
3434

35+
// Passed by ref to another function
36+
fn takes_ref(_peek: &Peekable<Empty<u32>>) {}
37+
let passed_along_ref = std::iter::empty::<u32>().peekable();
38+
takes_ref(&passed_along_ref);
39+
40+
// `by_ref` without `peek`
41+
let mut by_ref_test = std::iter::empty::<u32>().peekable();
42+
let _by_ref = by_ref_test.by_ref();
43+
3544
let mut peekable_in_for_loop = std::iter::empty::<u32>().peekable();
3645
for x in peekable_in_for_loop {}
3746
}
@@ -43,6 +52,18 @@ fn valid() {
4352
let passed_along = std::iter::empty::<u32>().peekable();
4453
takes_peekable(passed_along);
4554

55+
// Passed to another method
56+
struct PeekableConsumer;
57+
impl PeekableConsumer {
58+
fn consume(&self, _: Peekable<Empty<u32>>) {}
59+
fn consume_mut_ref(&self, _: &mut Peekable<Empty<u32>>) {}
60+
}
61+
62+
let peekable_consumer = PeekableConsumer;
63+
let mut passed_along_to_method = std::iter::empty::<u32>().peekable();
64+
peekable_consumer.consume_mut_ref(&mut passed_along_to_method);
65+
peekable_consumer.consume(passed_along_to_method);
66+
4667
// `peek` called in another block
4768
let mut peekable_in_block = std::iter::empty::<u32>().peekable();
4869
{
@@ -111,6 +132,10 @@ fn valid() {
111132
let struct_test = std::iter::empty::<u32>().peekable();
112133
PeekableWrapper { f: struct_test };
113134

135+
// `by_ref` before `peek`
136+
let mut by_ref_test = std::iter::empty::<u32>().peekable();
137+
let peeked_val = by_ref_test.by_ref().peek();
138+
114139
// `peek` called in another block as the last expression
115140
let mut peekable_last_expr = std::iter::empty::<u32>().peekable();
116141
{

‎tests/ui/unused_peekable.stderr‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,28 @@ LL | let mut peekable_using_iterator_method = std::iter::empty::<u32>().peek
4040
= help: consider removing the call to `peekable`
4141

4242
error: `peek` never called on `Peekable` iterator
43-
--> $DIR/unused_peekable.rs:35:13
43+
--> $DIR/unused_peekable.rs:37:9
44+
|
45+
LL | let passed_along_ref = std::iter::empty::<u32>().peekable();
46+
| ^^^^^^^^^^^^^^^^
47+
|
48+
= help: consider removing the call to `peekable`
49+
50+
error: `peek` never called on `Peekable` iterator
51+
--> $DIR/unused_peekable.rs:42:9
52+
|
53+
LL | let _by_ref = by_ref_test.by_ref();
54+
| ^^^^^^^
55+
|
56+
= help: consider removing the call to `peekable`
57+
58+
error: `peek` never called on `Peekable` iterator
59+
--> $DIR/unused_peekable.rs:44:13
4460
|
4561
LL | let mut peekable_in_for_loop = std::iter::empty::<u32>().peekable();
4662
| ^^^^^^^^^^^^^^^^^^^^
4763
|
4864
= help: consider removing the call to `peekable`
4965

50-
error: aborting due to 6 previous errors
66+
error: aborting due to 8 previous errors
5167

0 commit comments

Comments
 (0)