Skip to content

Commit 2d4c337

Browse files
committed
Auto merge of #5809 - JarredAllen:stable_sort_primitive, r=Manishearth
Stable sort primitive changelog: Implements #5762
2 parents 2eab060 + 542740c commit 2d4c337

11 files changed

+294
-7
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -1702,6 +1702,7 @@ Released 2018-09-13
17021702
[`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
17031703
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
17041704
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
1705+
[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
17051706
[`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
17061707
[`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add
17071708
[`string_add_assign`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add_assign

clippy_lints/src/lib.rs

+5
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,7 @@ mod serde_api;
288288
mod shadow;
289289
mod single_component_path_imports;
290290
mod slow_vector_initialization;
291+
mod stable_sort_primitive;
291292
mod strings;
292293
mod suspicious_trait_impl;
293294
mod swap;
@@ -773,6 +774,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
773774
&shadow::SHADOW_UNRELATED,
774775
&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS,
775776
&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION,
777+
&stable_sort_primitive::STABLE_SORT_PRIMITIVE,
776778
&strings::STRING_ADD,
777779
&strings::STRING_ADD_ASSIGN,
778780
&strings::STRING_LIT_AS_BYTES,
@@ -1076,6 +1078,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10761078
store.register_late_pass(|| box macro_use::MacroUseImports::default());
10771079
store.register_late_pass(|| box map_identity::MapIdentity);
10781080
store.register_late_pass(|| box pattern_type_mismatch::PatternTypeMismatch);
1081+
store.register_late_pass(|| box stable_sort_primitive::StableSortPrimitive);
10791082
store.register_late_pass(|| box repeat_once::RepeatOnce);
10801083

10811084
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
@@ -1408,6 +1411,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
14081411
LintId::of(&serde_api::SERDE_API_MISUSE),
14091412
LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS),
14101413
LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
1414+
LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE),
14111415
LintId::of(&strings::STRING_LIT_AS_BYTES),
14121416
LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
14131417
LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
@@ -1724,6 +1728,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
17241728
LintId::of(&mutex_atomic::MUTEX_ATOMIC),
17251729
LintId::of(&redundant_clone::REDUNDANT_CLONE),
17261730
LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
1731+
LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE),
17271732
LintId::of(&types::BOX_VEC),
17281733
LintId::of(&types::REDUNDANT_ALLOCATION),
17291734
LintId::of(&vec::USELESS_VEC),
+130
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
use crate::utils::{is_slice_of_primitives, span_lint_and_sugg, sugg::Sugg};
2+
3+
use if_chain::if_chain;
4+
5+
use rustc_errors::Applicability;
6+
use rustc_hir::{Expr, ExprKind};
7+
use rustc_lint::{LateContext, LateLintPass};
8+
use rustc_session::{declare_lint_pass, declare_tool_lint};
9+
10+
declare_clippy_lint! {
11+
/// **What it does:**
12+
/// When sorting primitive values (integers, bools, chars, as well
13+
/// as arrays, slices, and tuples of such items), it is better to
14+
/// use an unstable sort than a stable sort.
15+
///
16+
/// **Why is this bad?**
17+
/// Using a stable sort consumes more memory and cpu cycles. Because
18+
/// values which compare equal are identical, preserving their
19+
/// relative order (the guarantee that a stable sort provides) means
20+
/// nothing, while the extra costs still apply.
21+
///
22+
/// **Known problems:**
23+
/// None
24+
///
25+
/// **Example:**
26+
///
27+
/// ```rust
28+
/// let mut vec = vec![2, 1, 3];
29+
/// vec.sort();
30+
/// ```
31+
/// Use instead:
32+
/// ```rust
33+
/// let mut vec = vec![2, 1, 3];
34+
/// vec.sort_unstable();
35+
/// ```
36+
pub STABLE_SORT_PRIMITIVE,
37+
perf,
38+
"use of sort() when sort_unstable() is equivalent"
39+
}
40+
41+
declare_lint_pass!(StableSortPrimitive => [STABLE_SORT_PRIMITIVE]);
42+
43+
/// The three "kinds" of sorts
44+
enum SortingKind {
45+
Vanilla,
46+
/* The other kinds of lint are currently commented out because they
47+
* can map distinct values to equal ones. If the key function is
48+
* provably one-to-one, or if the Cmp function conserves equality,
49+
* then they could be linted on, but I don't know if we can check
50+
* for that. */
51+
52+
/* ByKey,
53+
* ByCmp, */
54+
}
55+
impl SortingKind {
56+
/// The name of the stable version of this kind of sort
57+
fn stable_name(&self) -> &str {
58+
match self {
59+
SortingKind::Vanilla => "sort",
60+
/* SortingKind::ByKey => "sort_by_key",
61+
* SortingKind::ByCmp => "sort_by", */
62+
}
63+
}
64+
/// The name of the unstable version of this kind of sort
65+
fn unstable_name(&self) -> &str {
66+
match self {
67+
SortingKind::Vanilla => "sort_unstable",
68+
/* SortingKind::ByKey => "sort_unstable_by_key",
69+
* SortingKind::ByCmp => "sort_unstable_by", */
70+
}
71+
}
72+
/// Takes the name of a function call and returns the kind of sort
73+
/// that corresponds to that function name (or None if it isn't)
74+
fn from_stable_name(name: &str) -> Option<SortingKind> {
75+
match name {
76+
"sort" => Some(SortingKind::Vanilla),
77+
// "sort_by" => Some(SortingKind::ByCmp),
78+
// "sort_by_key" => Some(SortingKind::ByKey),
79+
_ => None,
80+
}
81+
}
82+
}
83+
84+
/// A detected instance of this lint
85+
struct LintDetection {
86+
slice_name: String,
87+
method: SortingKind,
88+
method_args: String,
89+
}
90+
91+
fn detect_stable_sort_primitive(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintDetection> {
92+
if_chain! {
93+
if let ExprKind::MethodCall(method_name, _, args, _) = &expr.kind;
94+
if let Some(slice) = &args.get(0);
95+
if let Some(method) = SortingKind::from_stable_name(&method_name.ident.name.as_str());
96+
if is_slice_of_primitives(cx, slice);
97+
then {
98+
let args_str = args.iter().skip(1).map(|arg| Sugg::hir(cx, arg, "..").to_string()).collect::<Vec<String>>().join(", ");
99+
Some(LintDetection { slice_name: Sugg::hir(cx, slice, "..").to_string(), method, method_args: args_str })
100+
} else {
101+
None
102+
}
103+
}
104+
}
105+
106+
impl LateLintPass<'_> for StableSortPrimitive {
107+
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
108+
if let Some(detection) = detect_stable_sort_primitive(cx, expr) {
109+
span_lint_and_sugg(
110+
cx,
111+
STABLE_SORT_PRIMITIVE,
112+
expr.span,
113+
format!(
114+
"Use {} instead of {}",
115+
detection.method.unstable_name(),
116+
detection.method.stable_name()
117+
)
118+
.as_str(),
119+
"try",
120+
format!(
121+
"{}.{}({})",
122+
detection.slice_name,
123+
detection.method.unstable_name(),
124+
detection.method_args
125+
),
126+
Applicability::MachineApplicable,
127+
);
128+
}
129+
}
130+
}

clippy_lints/src/utils/mod.rs

+30
Original file line numberDiff line numberDiff line change
@@ -1377,6 +1377,36 @@ pub fn run_lints(cx: &LateContext<'_>, lints: &[&'static Lint], id: HirId) -> bo
13771377
})
13781378
}
13791379

1380+
/// Returns true iff the given type is a primitive (a bool or char, any integer or floating-point
1381+
/// number type, a str, or an array, slice, or tuple of those types).
1382+
pub fn is_recursively_primitive_type(ty: Ty<'_>) -> bool {
1383+
match ty.kind {
1384+
ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => true,
1385+
ty::Ref(_, inner, _) if inner.kind == ty::Str => true,
1386+
ty::Array(inner_type, _) | ty::Slice(inner_type) => is_recursively_primitive_type(inner_type),
1387+
ty::Tuple(inner_types) => inner_types.types().all(is_recursively_primitive_type),
1388+
_ => false,
1389+
}
1390+
}
1391+
1392+
/// Returns true iff the given expression is a slice of primitives (as defined in the
1393+
/// `is_recursively_primitive_type` function).
1394+
pub fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
1395+
let expr_type = cx.typeck_results().expr_ty_adjusted(expr);
1396+
match expr_type.kind {
1397+
ty::Slice(ref element_type)
1398+
| ty::Ref(
1399+
_,
1400+
ty::TyS {
1401+
kind: ty::Slice(ref element_type),
1402+
..
1403+
},
1404+
_,
1405+
) => is_recursively_primitive_type(element_type),
1406+
_ => false,
1407+
}
1408+
}
1409+
13801410
#[macro_export]
13811411
macro_rules! unwrap_cargo_metadata {
13821412
($cx: ident, $lint: ident, $deps: expr) => {{

src/lintlist/mod.rs

+7
Original file line numberDiff line numberDiff line change
@@ -2033,6 +2033,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
20332033
deprecation: None,
20342034
module: "slow_vector_initialization",
20352035
},
2036+
Lint {
2037+
name: "stable_sort_primitive",
2038+
group: "perf",
2039+
desc: "use of sort() when sort_unstable() is equivalent",
2040+
deprecation: None,
2041+
module: "stable_sort_primitive",
2042+
},
20362043
Lint {
20372044
name: "string_add",
20382045
group: "restriction",

tests/ui/stable_sort_primitive.fixed

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// run-rustfix
2+
#![warn(clippy::stable_sort_primitive)]
3+
4+
fn main() {
5+
// positive examples
6+
let mut vec = vec![1, 3, 2];
7+
vec.sort_unstable();
8+
let mut vec = vec![false, false, true];
9+
vec.sort_unstable();
10+
let mut vec = vec!['a', 'A', 'c'];
11+
vec.sort_unstable();
12+
let mut vec = vec!["ab", "cd", "ab", "bc"];
13+
vec.sort_unstable();
14+
let mut vec = vec![(2, 1), (1, 2), (2, 5)];
15+
vec.sort_unstable();
16+
let mut vec = vec![[2, 1], [1, 2], [2, 5]];
17+
vec.sort_unstable();
18+
let mut arr = [1, 3, 2];
19+
arr.sort_unstable();
20+
// Negative examples: behavior changes if made unstable
21+
let mut vec = vec![1, 3, 2];
22+
vec.sort_by_key(|i| i / 2);
23+
vec.sort_by(|a, b| (a + b).cmp(&b));
24+
// negative examples - Not of a primitive type
25+
let mut vec_of_complex = vec![String::from("hello"), String::from("world!")];
26+
vec_of_complex.sort();
27+
vec_of_complex.sort_by_key(String::len);
28+
let mut vec = vec![(String::from("hello"), String::from("world"))];
29+
vec.sort();
30+
let mut vec = vec![[String::from("hello"), String::from("world")]];
31+
vec.sort();
32+
}

tests/ui/stable_sort_primitive.rs

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// run-rustfix
2+
#![warn(clippy::stable_sort_primitive)]
3+
4+
fn main() {
5+
// positive examples
6+
let mut vec = vec![1, 3, 2];
7+
vec.sort();
8+
let mut vec = vec![false, false, true];
9+
vec.sort();
10+
let mut vec = vec!['a', 'A', 'c'];
11+
vec.sort();
12+
let mut vec = vec!["ab", "cd", "ab", "bc"];
13+
vec.sort();
14+
let mut vec = vec![(2, 1), (1, 2), (2, 5)];
15+
vec.sort();
16+
let mut vec = vec![[2, 1], [1, 2], [2, 5]];
17+
vec.sort();
18+
let mut arr = [1, 3, 2];
19+
arr.sort();
20+
// Negative examples: behavior changes if made unstable
21+
let mut vec = vec![1, 3, 2];
22+
vec.sort_by_key(|i| i / 2);
23+
vec.sort_by(|a, b| (a + b).cmp(&b));
24+
// negative examples - Not of a primitive type
25+
let mut vec_of_complex = vec![String::from("hello"), String::from("world!")];
26+
vec_of_complex.sort();
27+
vec_of_complex.sort_by_key(String::len);
28+
let mut vec = vec![(String::from("hello"), String::from("world"))];
29+
vec.sort();
30+
let mut vec = vec![[String::from("hello"), String::from("world")]];
31+
vec.sort();
32+
}

tests/ui/stable_sort_primitive.stderr

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
error: Use sort_unstable instead of sort
2+
--> $DIR/stable_sort_primitive.rs:7:5
3+
|
4+
LL | vec.sort();
5+
| ^^^^^^^^^^ help: try: `vec.sort_unstable()`
6+
|
7+
= note: `-D clippy::stable-sort-primitive` implied by `-D warnings`
8+
9+
error: Use sort_unstable instead of sort
10+
--> $DIR/stable_sort_primitive.rs:9:5
11+
|
12+
LL | vec.sort();
13+
| ^^^^^^^^^^ help: try: `vec.sort_unstable()`
14+
15+
error: Use sort_unstable instead of sort
16+
--> $DIR/stable_sort_primitive.rs:11:5
17+
|
18+
LL | vec.sort();
19+
| ^^^^^^^^^^ help: try: `vec.sort_unstable()`
20+
21+
error: Use sort_unstable instead of sort
22+
--> $DIR/stable_sort_primitive.rs:13:5
23+
|
24+
LL | vec.sort();
25+
| ^^^^^^^^^^ help: try: `vec.sort_unstable()`
26+
27+
error: Use sort_unstable instead of sort
28+
--> $DIR/stable_sort_primitive.rs:15:5
29+
|
30+
LL | vec.sort();
31+
| ^^^^^^^^^^ help: try: `vec.sort_unstable()`
32+
33+
error: Use sort_unstable instead of sort
34+
--> $DIR/stable_sort_primitive.rs:17:5
35+
|
36+
LL | vec.sort();
37+
| ^^^^^^^^^^ help: try: `vec.sort_unstable()`
38+
39+
error: Use sort_unstable instead of sort
40+
--> $DIR/stable_sort_primitive.rs:19:5
41+
|
42+
LL | arr.sort();
43+
| ^^^^^^^^^^ help: try: `arr.sort_unstable()`
44+
45+
error: aborting due to 7 previous errors
46+

tests/ui/unnecessary_sort_by.fixed

+2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// run-rustfix
22

3+
#![allow(clippy::stable_sort_primitive)]
4+
35
use std::cmp::Reverse;
46

57
fn unnecessary_sort_by() {

tests/ui/unnecessary_sort_by.rs

+2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// run-rustfix
22

3+
#![allow(clippy::stable_sort_primitive)]
4+
35
use std::cmp::Reverse;
46

57
fn unnecessary_sort_by() {

0 commit comments

Comments
 (0)