Skip to content

Commit 98d1d45

Browse files
borsbors
committed
Auto merge of #5756 - ebroto:5754_sort_by, r=flip1995
unnecessary_sort_by: avoid linting if key borrows changelog: Avoid linting if key borrows in [`unnecessary_sort_by`] Fixes #5754 Closes #2313
2 parents 52cc5fc + 18d09c5 commit 98d1d45

File tree

5 files changed

+131
-45
lines changed

5 files changed

+131
-45
lines changed

‎clippy_lints/src/let_and_return.rs

+2-19
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
use if_chain::if_chain;
22
use rustc_errors::Applicability;
3-
use rustc_hir::def_id::DefId;
43
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
54
use rustc_hir::{Block, Expr, ExprKind, PatKind, StmtKind};
65
use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -9,7 +8,7 @@ use rustc_middle::lint::in_external_macro;
98
use rustc_middle::ty::subst::GenericArgKind;
109
use rustc_session::{declare_lint_pass, declare_tool_lint};
1110

12-
use crate::utils::{in_macro, match_qpath, snippet_opt, span_lint_and_then};
11+
use crate::utils::{fn_def_id, in_macro, match_qpath, snippet_opt, span_lint_and_then};
1312

1413
declare_clippy_lint! {
1514
/// **What it does:** Checks for `let`-bindings, which are subsequently
@@ -97,22 +96,6 @@ struct BorrowVisitor<'a, 'tcx> {
9796
borrows: bool,
9897
}
9998

100-
impl BorrowVisitor<'_, '_> {
101-
fn fn_def_id(&self, expr: &Expr<'_>) -> Option<DefId> {
102-
match &expr.kind {
103-
ExprKind::MethodCall(..) => self.cx.tables().type_dependent_def_id(expr.hir_id),
104-
ExprKind::Call(
105-
Expr {
106-
kind: ExprKind::Path(qpath),
107-
..
108-
},
109-
..,
110-
) => self.cx.tables().qpath_res(qpath, expr.hir_id).opt_def_id(),
111-
_ => None,
112-
}
113-
}
114-
}
115-
11699
impl<'tcx> Visitor<'tcx> for BorrowVisitor<'_, 'tcx> {
117100
type Map = Map<'tcx>;
118101

@@ -121,7 +104,7 @@ impl<'tcx> Visitor<'tcx> for BorrowVisitor<'_, 'tcx> {
121104
return;
122105
}
123106

124-
if let Some(def_id) = self.fn_def_id(expr) {
107+
if let Some(def_id) = fn_def_id(self.cx, expr) {
125108
self.borrows = self
126109
.cx
127110
.tcx

‎clippy_lints/src/unnecessary_sort_by.rs

+30-18
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,23 @@ use if_chain::if_chain;
55
use rustc_errors::Applicability;
66
use rustc_hir::{Expr, ExprKind, Mutability, Param, Pat, PatKind, Path, PathSegment, QPath};
77
use rustc_lint::{LateContext, LateLintPass};
8+
use rustc_middle::ty::{self, subst::GenericArgKind};
89
use rustc_session::{declare_lint_pass, declare_tool_lint};
910
use rustc_span::symbol::Ident;
1011

1112
declare_clippy_lint! {
1213
/// **What it does:**
13-
/// Detects when people use `Vec::sort_by` and pass in a function
14+
/// Detects uses of `Vec::sort_by` passing in a closure
1415
/// which compares the two arguments, either directly or indirectly.
1516
///
1617
/// **Why is this bad?**
1718
/// It is more clear to use `Vec::sort_by_key` (or `Vec::sort` if
18-
/// possible) than to use `Vec::sort_by` and and a more complicated
19+
/// possible) than to use `Vec::sort_by` and a more complicated
1920
/// closure.
2021
///
2122
/// **Known problems:**
22-
/// If the suggested `Vec::sort_by_key` uses Reverse and it isn't
23-
/// imported by a use statement in the current frame, then a `use`
24-
/// statement that imports it will need to be added (which this lint
25-
/// can't do).
23+
/// If the suggested `Vec::sort_by_key` uses Reverse and it isn't already
24+
/// imported by a use statement, then it will need to be added manually.
2625
///
2726
/// **Example:**
2827
///
@@ -201,28 +200,41 @@ fn detect_lint(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<LintTrigger>
201200
};
202201
let vec_name = Sugg::hir(cx, &args[0], "..").to_string();
203202
let unstable = name == "sort_unstable_by";
203+
204204
if_chain! {
205205
if let ExprKind::Path(QPath::Resolved(_, Path {
206206
segments: [PathSegment { ident: left_name, .. }], ..
207207
})) = &left_expr.kind;
208208
if left_name == left_ident;
209209
then {
210-
Some(LintTrigger::Sort(SortDetection { vec_name, unstable }))
211-
}
212-
else {
213-
Some(LintTrigger::SortByKey(SortByKeyDetection {
214-
vec_name,
215-
unstable,
216-
closure_arg,
217-
closure_body,
218-
reverse
219-
}))
210+
return Some(LintTrigger::Sort(SortDetection { vec_name, unstable }))
211+
} else {
212+
if !key_returns_borrow(cx, left_expr) {
213+
return Some(LintTrigger::SortByKey(SortByKeyDetection {
214+
vec_name,
215+
unstable,
216+
closure_arg,
217+
closure_body,
218+
reverse
219+
}))
220+
}
220221
}
221222
}
222-
} else {
223-
None
224223
}
225224
}
225+
226+
None
227+
}
228+
229+
fn key_returns_borrow(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
230+
if let Some(def_id) = utils::fn_def_id(cx, expr) {
231+
let output = cx.tcx.fn_sig(def_id).output();
232+
let ty = output.skip_binder();
233+
return matches!(ty.kind, ty::Ref(..))
234+
|| ty.walk().any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)));
235+
}
236+
237+
false
226238
}
227239

228240
impl LateLintPass<'_, '_> for UnnecessarySortBy {

‎clippy_lints/src/utils/mod.rs

+15
Original file line numberDiff line numberDiff line change
@@ -1385,6 +1385,21 @@ pub fn fn_has_unsatisfiable_preds(cx: &LateContext<'_, '_>, did: DefId) -> bool
13851385
)
13861386
}
13871387

1388+
/// Returns the `DefId` of the callee if the given expression is a function or method call.
1389+
pub fn fn_def_id(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<DefId> {
1390+
match &expr.kind {
1391+
ExprKind::MethodCall(..) => cx.tables().type_dependent_def_id(expr.hir_id),
1392+
ExprKind::Call(
1393+
Expr {
1394+
kind: ExprKind::Path(qpath),
1395+
..
1396+
},
1397+
..,
1398+
) => cx.tables().qpath_res(qpath, expr.hir_id).opt_def_id(),
1399+
_ => None,
1400+
}
1401+
}
1402+
13881403
pub fn run_lints(cx: &LateContext<'_, '_>, lints: &[&'static Lint], id: HirId) -> bool {
13891404
lints.iter().any(|lint| {
13901405
matches!(

‎tests/ui/unnecessary_sort_by.fixed

+42-4
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@
22

33
use std::cmp::Reverse;
44

5-
fn id(x: isize) -> isize {
6-
x
7-
}
5+
fn unnecessary_sort_by() {
6+
fn id(x: isize) -> isize {
7+
x
8+
}
89

9-
fn main() {
1010
let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
1111
// Forward examples
1212
vec.sort();
@@ -24,3 +24,41 @@ fn main() {
2424
vec.sort_by(|_, b| b.cmp(c));
2525
vec.sort_unstable_by(|a, _| a.cmp(c));
2626
}
27+
28+
// Should not be linted to avoid hitting https://github.com/rust-lang/rust/issues/34162
29+
mod issue_5754 {
30+
struct Test(String);
31+
32+
#[derive(PartialOrd, Ord, PartialEq, Eq)]
33+
struct Wrapper<'a>(&'a str);
34+
35+
impl Test {
36+
fn name(&self) -> &str {
37+
&self.0
38+
}
39+
40+
fn wrapped(&self) -> Wrapper<'_> {
41+
Wrapper(&self.0)
42+
}
43+
}
44+
45+
pub fn test() {
46+
let mut args: Vec<Test> = vec![];
47+
48+
// Forward
49+
args.sort_by(|a, b| a.name().cmp(b.name()));
50+
args.sort_by(|a, b| a.wrapped().cmp(&b.wrapped()));
51+
args.sort_unstable_by(|a, b| a.name().cmp(b.name()));
52+
args.sort_unstable_by(|a, b| a.wrapped().cmp(&b.wrapped()));
53+
// Reverse
54+
args.sort_by(|a, b| b.name().cmp(a.name()));
55+
args.sort_by(|a, b| b.wrapped().cmp(&a.wrapped()));
56+
args.sort_unstable_by(|a, b| b.name().cmp(a.name()));
57+
args.sort_unstable_by(|a, b| b.wrapped().cmp(&a.wrapped()));
58+
}
59+
}
60+
61+
fn main() {
62+
unnecessary_sort_by();
63+
issue_5754::test();
64+
}

‎tests/ui/unnecessary_sort_by.rs

+42-4
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@
22

33
use std::cmp::Reverse;
44

5-
fn id(x: isize) -> isize {
6-
x
7-
}
5+
fn unnecessary_sort_by() {
6+
fn id(x: isize) -> isize {
7+
x
8+
}
89

9-
fn main() {
1010
let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5];
1111
// Forward examples
1212
vec.sort_by(|a, b| a.cmp(b));
@@ -24,3 +24,41 @@ fn main() {
2424
vec.sort_by(|_, b| b.cmp(c));
2525
vec.sort_unstable_by(|a, _| a.cmp(c));
2626
}
27+
28+
// Should not be linted to avoid hitting https://github.com/rust-lang/rust/issues/34162
29+
mod issue_5754 {
30+
struct Test(String);
31+
32+
#[derive(PartialOrd, Ord, PartialEq, Eq)]
33+
struct Wrapper<'a>(&'a str);
34+
35+
impl Test {
36+
fn name(&self) -> &str {
37+
&self.0
38+
}
39+
40+
fn wrapped(&self) -> Wrapper<'_> {
41+
Wrapper(&self.0)
42+
}
43+
}
44+
45+
pub fn test() {
46+
let mut args: Vec<Test> = vec![];
47+
48+
// Forward
49+
args.sort_by(|a, b| a.name().cmp(b.name()));
50+
args.sort_by(|a, b| a.wrapped().cmp(&b.wrapped()));
51+
args.sort_unstable_by(|a, b| a.name().cmp(b.name()));
52+
args.sort_unstable_by(|a, b| a.wrapped().cmp(&b.wrapped()));
53+
// Reverse
54+
args.sort_by(|a, b| b.name().cmp(a.name()));
55+
args.sort_by(|a, b| b.wrapped().cmp(&a.wrapped()));
56+
args.sort_unstable_by(|a, b| b.name().cmp(a.name()));
57+
args.sort_unstable_by(|a, b| b.wrapped().cmp(&a.wrapped()));
58+
}
59+
}
60+
61+
fn main() {
62+
unnecessary_sort_by();
63+
issue_5754::test();
64+
}

0 commit comments

Comments
 (0)