Add rc_buffer lint for checking Rc<String> and friends

CHANGELOG.md

@@ -1775,6 +1775,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

@@ -837,6 +837,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,
@@ -1479,6 +1480,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),
@@ -1779,6 +1781,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 SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"];

src/lintlist/mod.rs

@@ -1851,6 +1851,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() {}