Auto merge of #5809 - JarredAllen:stable_sort_primitive, r=Manishearth

Stable sort primitive

changelog: Implements #5762

CHANGELOG.md

@@ -1701,6 +1701,7 @@ Released 2018-09-13
 [`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
 [`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
 [`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
+[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
 [`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
 [`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add
 [`string_add_assign`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add_assign

clippy_lints/src/lib.rs

@@ -288,6 +288,7 @@ mod serde_api;
 mod shadow;
 mod single_component_path_imports;
 mod slow_vector_initialization;
+mod stable_sort_primitive;
 mod strings;
 mod suspicious_trait_impl;
 mod swap;
@@ -776,6 +777,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &shadow::SHADOW_UNRELATED,
         &single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS,
         &slow_vector_initialization::SLOW_VECTOR_INITIALIZATION,
+        &stable_sort_primitive::STABLE_SORT_PRIMITIVE,
         &strings::STRING_ADD,
         &strings::STRING_ADD_ASSIGN,
         &strings::STRING_LIT_AS_BYTES,
@@ -1078,6 +1080,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box macro_use::MacroUseImports::default());
     store.register_late_pass(|| box map_identity::MapIdentity);
     store.register_late_pass(|| box pattern_type_mismatch::PatternTypeMismatch);
+    store.register_late_pass(|| box stable_sort_primitive::StableSortPrimitive);
     store.register_late_pass(|| box repeat_once::RepeatOnce);
 
     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:
         LintId::of(&serde_api::SERDE_API_MISUSE),
         LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS),
         LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
+        LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE),
         LintId::of(&strings::STRING_LIT_AS_BYTES),
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
@@ -1723,6 +1727,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&mutex_atomic::MUTEX_ATOMIC),
         LintId::of(&redundant_clone::REDUNDANT_CLONE),
         LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
+        LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE),
         LintId::of(&types::BOX_VEC),
         LintId::of(&types::REDUNDANT_ALLOCATION),
         LintId::of(&vec::USELESS_VEC),

clippy_lints/src/stable_sort_primitive.rs

@@ -0,0 +1,130 @@
+use crate::utils::{is_slice_of_primitives, span_lint_and_sugg, sugg::Sugg};
+
+use if_chain::if_chain;
+
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:**
+    /// When sorting primitive values (integers, bools, chars, as well
+    /// as arrays, slices, and tuples of such items), it is better to
+    /// use an unstable sort than a stable sort.
+    ///
+    /// **Why is this bad?**
+    /// Using a stable sort consumes more memory and cpu cycles. Because
+    /// values which compare equal are identical, preserving their
+    /// relative order (the guarantee that a stable sort provides) means
+    /// nothing, while the extra costs still apply.
+    ///
+    /// **Known problems:**
+    /// None
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let mut vec = vec![2, 1, 3];
+    /// vec.sort();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let mut vec = vec![2, 1, 3];
+    /// vec.sort_unstable();
+    /// ```
+    pub STABLE_SORT_PRIMITIVE,
+    perf,
+    "use of sort() when sort_unstable() is equivalent"
+}
+
+declare_lint_pass!(StableSortPrimitive => [STABLE_SORT_PRIMITIVE]);
+
+/// The three "kinds" of sorts
+enum SortingKind {
+    Vanilla,
+    // The other kinds of lint are currently commented out because they
+    // can map distinct values to equal ones. If the key function is
+    // provably one-to-one, or if the Cmp function conserves equality,
+    // then they could be linted on, but I don't know if we can check
+    // for that.
+
+    // ByKey,
+    // ByCmp,
+}
+impl SortingKind {
+    /// The name of the stable version of this kind of sort
+    fn stable_name(&self) -> &str {
+        match self {
+            SortingKind::Vanilla => "sort",
+            // SortingKind::ByKey => "sort_by_key",
+            // SortingKind::ByCmp => "sort_by",
+        }
+    }
+    /// The name of the unstable version of this kind of sort
+    fn unstable_name(&self) -> &str {
+        match self {
+            SortingKind::Vanilla => "sort_unstable",
+            // SortingKind::ByKey => "sort_unstable_by_key",
+            // SortingKind::ByCmp => "sort_unstable_by",
+        }
+    }
+    /// Takes the name of a function call and returns the kind of sort
+    /// that corresponds to that function name (or None if it isn't)
+    fn from_stable_name(name: &str) -> Option<SortingKind> {
+        match name {
+            "sort" => Some(SortingKind::Vanilla),
+            // "sort_by" => Some(SortingKind::ByCmp),
+            // "sort_by_key" => Some(SortingKind::ByKey),
+            _ => None,
+        }
+    }
+}
+
+/// A detected instance of this lint
+struct LintDetection {
+    slice_name: String,
+    method: SortingKind,
+    method_args: String,
+}
+
+fn detect_stable_sort_primitive(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintDetection> {
+    if_chain! {
+        if let ExprKind::MethodCall(method_name, _, args, _) = &expr.kind;
+        if let Some(slice) = &args.get(0);
+        if let Some(method) = SortingKind::from_stable_name(&method_name.ident.name.as_str());
+        if is_slice_of_primitives(cx, slice);
+        then {
+            let args_str = args.iter().skip(1).map(|arg| Sugg::hir(cx, arg, "..").to_string()).collect::<Vec<String>>().join(", ");
+            Some(LintDetection { slice_name: Sugg::hir(cx, slice, "..").to_string(), method, method_args: args_str })
+        } else {
+            None
+        }
+    }
+}
+
+impl LateLintPass<'_> for StableSortPrimitive {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if let Some(detection) = detect_stable_sort_primitive(cx, expr) {
+            span_lint_and_sugg(
+                cx,
+                STABLE_SORT_PRIMITIVE,
+                expr.span,
+                format!(
+                    "Use {} instead of {}",
+                    detection.method.unstable_name(),
+                    detection.method.stable_name()
+                )
+                .as_str(),
+                "try",
+                format!(
+                    "{}.{}({})",
+                    detection.slice_name,
+                    detection.method.unstable_name(),
+                    detection.method_args
+                ),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}

clippy_lints/src/utils/mod.rs

@@ -1378,6 +1378,36 @@ pub fn run_lints(cx: &LateContext<'_>, lints: &[&'static Lint], id: HirId) -> bo
     })
 }
 
+/// Returns true iff the given type is a primitive (a bool or char, any integer or floating-point
+/// number type, a str, or an array, slice, or tuple of those types).
+pub fn is_recursively_primitive_type(ty: Ty<'_>) -> bool {
+    match ty.kind {
+        ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => true,
+        ty::Ref(_, inner, _) if inner.kind == ty::Str => true,
+        ty::Array(inner_type, _) | ty::Slice(inner_type) => is_recursively_primitive_type(inner_type),
+        ty::Tuple(inner_types) => inner_types.types().all(is_recursively_primitive_type),
+        _ => false,
+    }
+}
+
+/// Returns true iff the given expression is a slice of primitives (as defined in the
+/// `is_recursively_primitive_type` function).
+pub fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    let expr_type = cx.typeck_results().expr_ty_adjusted(expr);
+    match expr_type.kind {
+        ty::Slice(ref element_type)
+        | ty::Ref(
+            _,
+            ty::TyS {
+                kind: ty::Slice(ref element_type),
+                ..
+            },
+            _,
+        ) => is_recursively_primitive_type(element_type),
+        _ => false,
+    }
+}
+
 #[macro_export]
 macro_rules! unwrap_cargo_metadata {
     ($cx: ident, $lint: ident, $deps: expr) => {{

src/lintlist/mod.rs

@@ -2026,6 +2026,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "slow_vector_initialization",
     },
+    Lint {
+        name: "stable_sort_primitive",
+        group: "perf",
+        desc: "use of sort() when sort_unstable() is equivalent",
+        deprecation: None,
+        module: "stable_sort_primitive",
+    },
     Lint {
         name: "string_add",
         group: "restriction",

tests/ui/stable_sort_primitive.fixed

@@ -0,0 +1,32 @@
+// run-rustfix
+#![warn(clippy::stable_sort_primitive)]
+
+fn main() {
+    // positive examples
+    let mut vec = vec![1, 3, 2];
+    vec.sort_unstable();
+    let mut vec = vec![false, false, true];
+    vec.sort_unstable();
+    let mut vec = vec!['a', 'A', 'c'];
+    vec.sort_unstable();
+    let mut vec = vec!["ab", "cd", "ab", "bc"];
+    vec.sort_unstable();
+    let mut vec = vec![(2, 1), (1, 2), (2, 5)];
+    vec.sort_unstable();
+    let mut vec = vec![[2, 1], [1, 2], [2, 5]];
+    vec.sort_unstable();
+    let mut arr = [1, 3, 2];
+    arr.sort_unstable();
+    // Negative examples: behavior changes if made unstable
+    let mut vec = vec![1, 3, 2];
+    vec.sort_by_key(|i| i / 2);
+    vec.sort_by(|a, b| (a + b).cmp(&b));
+    // negative examples - Not of a primitive type
+    let mut vec_of_complex = vec![String::from("hello"), String::from("world!")];
+    vec_of_complex.sort();
+    vec_of_complex.sort_by_key(String::len);
+    let mut vec = vec![(String::from("hello"), String::from("world"))];
+    vec.sort();
+    let mut vec = vec![[String::from("hello"), String::from("world")]];
+    vec.sort();
+}

tests/ui/stable_sort_primitive.rs

@@ -0,0 +1,32 @@
+// run-rustfix
+#![warn(clippy::stable_sort_primitive)]
+
+fn main() {
+    // positive examples
+    let mut vec = vec![1, 3, 2];
+    vec.sort();
+    let mut vec = vec![false, false, true];
+    vec.sort();
+    let mut vec = vec!['a', 'A', 'c'];
+    vec.sort();
+    let mut vec = vec!["ab", "cd", "ab", "bc"];
+    vec.sort();
+    let mut vec = vec![(2, 1), (1, 2), (2, 5)];
+    vec.sort();
+    let mut vec = vec![[2, 1], [1, 2], [2, 5]];
+    vec.sort();
+    let mut arr = [1, 3, 2];
+    arr.sort();
+    // Negative examples: behavior changes if made unstable
+    let mut vec = vec![1, 3, 2];
+    vec.sort_by_key(|i| i / 2);
+    vec.sort_by(|a, b| (a + b).cmp(&b));
+    // negative examples - Not of a primitive type
+    let mut vec_of_complex = vec![String::from("hello"), String::from("world!")];
+    vec_of_complex.sort();
+    vec_of_complex.sort_by_key(String::len);
+    let mut vec = vec![(String::from("hello"), String::from("world"))];
+    vec.sort();
+    let mut vec = vec![[String::from("hello"), String::from("world")]];
+    vec.sort();
+}

tests/ui/stable_sort_primitive.stderr

@@ -0,0 +1,46 @@
+error: Use sort_unstable instead of sort
+  --> $DIR/stable_sort_primitive.rs:7:5
+   |
+LL |     vec.sort();
+   |     ^^^^^^^^^^ help: try: `vec.sort_unstable()`
+   |
+   = note: `-D clippy::stable-sort-primitive` implied by `-D warnings`
+
+error: Use sort_unstable instead of sort
+  --> $DIR/stable_sort_primitive.rs:9:5
+   |
+LL |     vec.sort();
+   |     ^^^^^^^^^^ help: try: `vec.sort_unstable()`
+
+error: Use sort_unstable instead of sort
+  --> $DIR/stable_sort_primitive.rs:11:5
+   |
+LL |     vec.sort();
+   |     ^^^^^^^^^^ help: try: `vec.sort_unstable()`
+
+error: Use sort_unstable instead of sort
+  --> $DIR/stable_sort_primitive.rs:13:5
+   |
+LL |     vec.sort();
+   |     ^^^^^^^^^^ help: try: `vec.sort_unstable()`
+
+error: Use sort_unstable instead of sort
+  --> $DIR/stable_sort_primitive.rs:15:5
+   |
+LL |     vec.sort();
+   |     ^^^^^^^^^^ help: try: `vec.sort_unstable()`
+
+error: Use sort_unstable instead of sort
+  --> $DIR/stable_sort_primitive.rs:17:5
+   |
+LL |     vec.sort();
+   |     ^^^^^^^^^^ help: try: `vec.sort_unstable()`
+
+error: Use sort_unstable instead of sort
+  --> $DIR/stable_sort_primitive.rs:19:5
+   |
+LL |     arr.sort();
+   |     ^^^^^^^^^^ help: try: `arr.sort_unstable()`
+
+error: aborting due to 7 previous errors
+

tests/ui/unnecessary_sort_by.fixed

@@ -1,5 +1,7 @@
 // run-rustfix
 
+#![allow(clippy::stable_sort_primitive)]
+
 use std::cmp::Reverse;
 
 fn unnecessary_sort_by() {

tests/ui/unnecessary_sort_by.rs

@@ -1,5 +1,7 @@
 // run-rustfix
 
+#![allow(clippy::stable_sort_primitive)]
+
 use std::cmp::Reverse;
 
 fn unnecessary_sort_by() {