Auto merge of #6044 - rschoon:rc-buffer, r=yaahc

Add `rc_buffer` lint for checking Rc<String> and friends

Fixes #2623

This is a bit different from the original PR attempting to implement this type of lint.   Rather than linting against converting into the unwanted types, this PR lints against declaring the unwanted type in a struct or function definition.

I'm reasonably happy with what I have here, although I used the fully qualified type names for the Path and OsString suggestions, and I'm not sure if I should have just used the short versions instead, even if they might not have been declared via use.

Also, I don't know if "buffer type" is the best way to put it or not.  Alternatively I could call it a "growable type" or "growable buffer type", but I was thinking of PathBuf when I started making the lint.

changelog: Add `rc_buffer` lint

Author: bors

CHANGELOG.md

@@ -1777,6 +1777,7 @@ Released 2018-09-13
 [`range_plus_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one
 [`range_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_step_by_zero
 [`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len
+[`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
 [`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation
 [`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
 [`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

clippy_lints/src/consts.rs

@@ -21,7 +21,7 @@ pub enum Constant {
     /// A `String` (e.g., "abc").
     Str(String),
     /// A binary string (e.g., `b"abc"`).
-    Binary(Lrc<Vec<u8>>),
+    Binary(Lrc<[u8]>),
     /// A single `char` (e.g., `'a'`).
     Char(char),
     /// An integer's bit representation.
@@ -155,7 +155,7 @@ pub fn lit_to_constant(lit: &LitKind, ty: Option<Ty<'_>>) -> Constant {
     match *lit {
         LitKind::Str(ref is, _) => Constant::Str(is.to_string()),
         LitKind::Byte(b) => Constant::Int(u128::from(b)),
-        LitKind::ByteStr(ref s) => Constant::Binary(Lrc::clone(s)),
+        LitKind::ByteStr(ref s) => Constant::Binary(Lrc::from(s.as_slice())),
         LitKind::Char(c) => Constant::Char(c),
         LitKind::Int(n, _) => Constant::Int(n),
         LitKind::Float(ref is, LitFloatType::Suffixed(fty)) => match fty {

clippy_lints/src/lib.rs

@@ -841,6 +841,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &types::LET_UNIT_VALUE,
         &types::LINKEDLIST,
         &types::OPTION_OPTION,
+        &types::RC_BUFFER,
         &types::REDUNDANT_ALLOCATION,
         &types::TYPE_COMPLEXITY,
         &types::UNIT_ARG,
@@ -1490,6 +1491,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&types::CHAR_LIT_AS_U8),
         LintId::of(&types::FN_TO_NUMERIC_CAST),
         LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
+        LintId::of(&types::RC_BUFFER),
         LintId::of(&types::REDUNDANT_ALLOCATION),
         LintId::of(&types::TYPE_COMPLEXITY),
         LintId::of(&types::UNIT_ARG),
@@ -1791,6 +1793,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
         LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE),
         LintId::of(&types::BOX_VEC),
+        LintId::of(&types::RC_BUFFER),
         LintId::of(&types::REDUNDANT_ALLOCATION),
         LintId::of(&vec::USELESS_VEC),
     ]);

clippy_lints/src/types.rs

@@ -215,11 +215,41 @@ declare_clippy_lint! {
     "redundant allocation"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for Rc<T> and Arc<T> when T is a mutable buffer type such as String or Vec
+    ///
+    /// **Why is this bad?** Expressions such as Rc<String> have no advantage over Rc<str>, since
+    /// it is larger and involves an extra level of indirection, and doesn't implement Borrow<str>.
+    ///
+    /// While mutating a buffer type would still be possible with Rc::get_mut(), it only
+    /// works if there are no additional references yet, which defeats the purpose of
+    /// enclosing it in a shared ownership type. Instead, additionally wrapping the inner
+    /// type with an interior mutable container (such as RefCell or Mutex) would normally
+    /// be used.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust,ignore
+    /// # use std::rc::Rc;
+    /// fn foo(interned: Rc<String>) { ... }
+    /// ```
+    ///
+    /// Better:
+    ///
+    /// ```rust,ignore
+    /// fn foo(interned: Rc<str>) { ... }
+    /// ```
+    pub RC_BUFFER,
+    perf,
+    "shared ownership of a buffer type"
+}
+
 pub struct Types {
     vec_box_size_threshold: u64,
 }
 
-impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION]);
+impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER]);
 
 impl<'tcx> LateLintPass<'tcx> for Types {
     fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _: &Body<'_>, _: Span, id: HirId) {
@@ -272,6 +302,19 @@ fn match_type_parameter(cx: &LateContext<'_>, qpath: &QPath<'_>, path: &[&str])
     None
 }
 
+fn match_buffer_type(cx: &LateContext<'_>, qpath: &QPath<'_>) -> Option<&'static str> {
+    if match_type_parameter(cx, qpath, &paths::STRING).is_some() {
+        return Some("str");
+    }
+    if match_type_parameter(cx, qpath, &paths::OS_STRING).is_some() {
+        return Some("std::ffi::OsStr");
+    }
+    if match_type_parameter(cx, qpath, &paths::PATH_BUF).is_some() {
+        return Some("std::path::Path");
+    }
+    None
+}
+
 fn match_borrows_parameter(_cx: &LateContext<'_>, qpath: &QPath<'_>) -> Option<Span> {
     let last = last_path_segment(qpath);
     if_chain! {
@@ -385,6 +428,45 @@ impl Types {
                             );
                             return; // don't recurse into the type
                         }
+                        if let Some(alternate) = match_buffer_type(cx, qpath) {
+                            span_lint_and_sugg(
+                                cx,
+                                RC_BUFFER,
+                                hir_ty.span,
+                                "usage of `Rc<T>` when T is a buffer type",
+                                "try",
+                                format!("Rc<{}>", alternate),
+                                Applicability::MachineApplicable,
+                            );
+                            return; // don't recurse into the type
+                        }
+                        if match_type_parameter(cx, qpath, &paths::VEC).is_some() {
+                            let vec_ty = match &last_path_segment(qpath).args.unwrap().args[0] {
+                                GenericArg::Type(ty) => match &ty.kind {
+                                    TyKind::Path(qpath) => qpath,
+                                    _ => return,
+                                },
+                                _ => return,
+                            };
+                            let inner_span = match &last_path_segment(&vec_ty).args.unwrap().args[0] {
+                                GenericArg::Type(ty) => ty.span,
+                                _ => return,
+                            };
+                            let mut applicability = Applicability::MachineApplicable;
+                            span_lint_and_sugg(
+                                cx,
+                                RC_BUFFER,
+                                hir_ty.span,
+                                "usage of `Rc<T>` when T is a buffer type",
+                                "try",
+                                format!(
+                                    "Rc<[{}]>",
+                                    snippet_with_applicability(cx, inner_span, "..", &mut applicability)
+                                ),
+                                Applicability::MachineApplicable,
+                            );
+                            return; // don't recurse into the type
+                        }
                         if let Some(span) = match_borrows_parameter(cx, qpath) {
                             let mut applicability = Applicability::MachineApplicable;
                             span_lint_and_sugg(
@@ -398,6 +480,46 @@ impl Types {
                             );
                             return; // don't recurse into the type
                         }
+                    } else if cx.tcx.is_diagnostic_item(sym::Arc, def_id) {
+                        if let Some(alternate) = match_buffer_type(cx, qpath) {
+                            span_lint_and_sugg(
+                                cx,
+                                RC_BUFFER,
+                                hir_ty.span,
+                                "usage of `Arc<T>` when T is a buffer type",
+                                "try",
+                                format!("Arc<{}>", alternate),
+                                Applicability::MachineApplicable,
+                            );
+                            return; // don't recurse into the type
+                        }
+                        if match_type_parameter(cx, qpath, &paths::VEC).is_some() {
+                            let vec_ty = match &last_path_segment(qpath).args.unwrap().args[0] {
+                                GenericArg::Type(ty) => match &ty.kind {
+                                    TyKind::Path(qpath) => qpath,
+                                    _ => return,
+                                },
+                                _ => return,
+                            };
+                            let inner_span = match &last_path_segment(&vec_ty).args.unwrap().args[0] {
+                                GenericArg::Type(ty) => ty.span,
+                                _ => return,
+                            };
+                            let mut applicability = Applicability::MachineApplicable;
+                            span_lint_and_sugg(
+                                cx,
+                                RC_BUFFER,
+                                hir_ty.span,
+                                "usage of `Arc<T>` when T is a buffer type",
+                                "try",
+                                format!(
+                                    "Arc<[{}]>",
+                                    snippet_with_applicability(cx, inner_span, "..", &mut applicability)
+                                ),
+                                Applicability::MachineApplicable,
+                            );
+                            return; // don't recurse into the type
+                        }
                     } else if cx.tcx.is_diagnostic_item(sym!(vec_type), def_id) {
                         if_chain! {
                             // Get the _ part of Vec<_>

clippy_lints/src/utils/paths.rs

@@ -113,6 +113,7 @@ pub const STD_CONVERT_IDENTITY: [&str; 3] = ["std", "convert", "identity"];
 pub const STD_FS_CREATE_DIR: [&str; 3] = ["std", "fs", "create_dir"];
 pub const STD_MEM_TRANSMUTE: [&str; 3] = ["std", "mem", "transmute"];
 pub const STD_PTR_NULL: [&str; 3] = ["std", "ptr", "null"];
+pub const STRING: [&str; 3] = ["alloc", "string", "String"];
 pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"];
 pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"];
 pub const STR_ENDS_WITH: [&str; 4] = ["core", "str", "<impl str>", "ends_with"];

src/lintlist/mod.rs

@@ -1865,6 +1865,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "ranges",
     },
+    Lint {
+        name: "rc_buffer",
+        group: "perf",
+        desc: "shared ownership of a buffer type",
+        deprecation: None,
+        module: "types",
+    },
     Lint {
         name: "redundant_allocation",
         group: "perf",

tests/ui/rc_buffer.rs

@@ -0,0 +1,26 @@
+#![warn(clippy::rc_buffer)]
+
+use std::cell::RefCell;
+use std::ffi::OsString;
+use std::path::PathBuf;
+use std::rc::Rc;
+
+struct S {
+    // triggers lint
+    bad1: Rc<String>,
+    bad2: Rc<PathBuf>,
+    bad3: Rc<Vec<u8>>,
+    bad4: Rc<OsString>,
+    // does not trigger lint
+    good1: Rc<RefCell<String>>,
+}
+
+// triggers lint
+fn func_bad1(_: Rc<String>) {}
+fn func_bad2(_: Rc<PathBuf>) {}
+fn func_bad3(_: Rc<Vec<u8>>) {}
+fn func_bad4(_: Rc<OsString>) {}
+// does not trigger lint
+fn func_good1(_: Rc<RefCell<String>>) {}
+
+fn main() {}

tests/ui/rc_buffer.stderr

@@ -0,0 +1,52 @@
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:10:11
+   |
+LL |     bad1: Rc<String>,
+   |           ^^^^^^^^^^ help: try: `Rc<str>`
+   |
+   = note: `-D clippy::rc-buffer` implied by `-D warnings`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:11:11
+   |
+LL |     bad2: Rc<PathBuf>,
+   |           ^^^^^^^^^^^ help: try: `Rc<std::path::Path>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:12:11
+   |
+LL |     bad3: Rc<Vec<u8>>,
+   |           ^^^^^^^^^^^ help: try: `Rc<[u8]>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:13:11
+   |
+LL |     bad4: Rc<OsString>,
+   |           ^^^^^^^^^^^^ help: try: `Rc<std::ffi::OsStr>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:19:17
+   |
+LL | fn func_bad1(_: Rc<String>) {}
+   |                 ^^^^^^^^^^ help: try: `Rc<str>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:20:17
+   |
+LL | fn func_bad2(_: Rc<PathBuf>) {}
+   |                 ^^^^^^^^^^^ help: try: `Rc<std::path::Path>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:21:17
+   |
+LL | fn func_bad3(_: Rc<Vec<u8>>) {}
+   |                 ^^^^^^^^^^^ help: try: `Rc<[u8]>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:22:17
+   |
+LL | fn func_bad4(_: Rc<OsString>) {}
+   |                 ^^^^^^^^^^^^ help: try: `Rc<std::ffi::OsStr>`
+
+error: aborting due to 8 previous errors
+

tests/ui/rc_buffer_arc.rs

@@ -0,0 +1,25 @@
+#![warn(clippy::rc_buffer)]
+
+use std::ffi::OsString;
+use std::path::PathBuf;
+use std::sync::{Arc, Mutex};
+
+struct S {
+    // triggers lint
+    bad1: Arc<String>,
+    bad2: Arc<PathBuf>,
+    bad3: Arc<Vec<u8>>,
+    bad4: Arc<OsString>,
+    // does not trigger lint
+    good1: Arc<Mutex<String>>,
+}
+
+// triggers lint
+fn func_bad1(_: Arc<String>) {}
+fn func_bad2(_: Arc<PathBuf>) {}
+fn func_bad3(_: Arc<Vec<u8>>) {}
+fn func_bad4(_: Arc<OsString>) {}
+// does not trigger lint
+fn func_good1(_: Arc<Mutex<String>>) {}
+
+fn main() {}

tests/ui/rc_buffer_arc.stderr

@@ -0,0 +1,52 @@
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:9:11
+   |
+LL |     bad1: Arc<String>,
+   |           ^^^^^^^^^^^ help: try: `Arc<str>`
+   |
+   = note: `-D clippy::rc-buffer` implied by `-D warnings`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:10:11
+   |
+LL |     bad2: Arc<PathBuf>,
+   |           ^^^^^^^^^^^^ help: try: `Arc<std::path::Path>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:11:11
+   |
+LL |     bad3: Arc<Vec<u8>>,
+   |           ^^^^^^^^^^^^ help: try: `Arc<[u8]>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:12:11
+   |
+LL |     bad4: Arc<OsString>,
+   |           ^^^^^^^^^^^^^ help: try: `Arc<std::ffi::OsStr>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:18:17
+   |
+LL | fn func_bad1(_: Arc<String>) {}
+   |                 ^^^^^^^^^^^ help: try: `Arc<str>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:19:17
+   |
+LL | fn func_bad2(_: Arc<PathBuf>) {}
+   |                 ^^^^^^^^^^^^ help: try: `Arc<std::path::Path>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:20:17
+   |
+LL | fn func_bad3(_: Arc<Vec<u8>>) {}
+   |                 ^^^^^^^^^^^^ help: try: `Arc<[u8]>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:21:17
+   |
+LL | fn func_bad4(_: Arc<OsString>) {}
+   |                 ^^^^^^^^^^^^^ help: try: `Arc<std::ffi::OsStr>`
+
+error: aborting due to 8 previous errors
+

tests/ui/rc_buffer_redefined_string.rs

@@ -0,0 +1,12 @@
+#![warn(clippy::rc_buffer)]
+
+use std::rc::Rc;
+
+struct String;
+
+struct S {
+    // does not trigger lint
+    good1: Rc<String>,
+}
+
+fn main() {}