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/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,
@@ -1804,6 +1805,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),
         LintId::of(&redundant_pub_crate::REDUNDANT_PUB_CRATE),
         LintId::of(&transmute::USELESS_TRANSMUTE),
+        LintId::of(&types::RC_BUFFER),
         LintId::of(&use_self::USE_SELF),
     ]);
 }

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,
+    nursery,
+    "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: "nursery",
+        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,13 @@
+use std::ffi::OsString;
+use std::path::PathBuf;
+use std::rc::Rc;
+
+#[warn(clippy::rc_buffer)]
+struct S {
+    a: Rc<String>,
+    b: Rc<PathBuf>,
+    c: Rc<Vec<u8>>,
+    d: Rc<OsString>,
+}
+
+fn main() {}

tests/ui/rc_buffer.stderr

@@ -0,0 +1,28 @@
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:7:8
+   |
+LL |     a: 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:8:8
+   |
+LL |     b: Rc<PathBuf>,
+   |        ^^^^^^^^^^^ help: try: `Rc<std::path::Path>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:9:8
+   |
+LL |     c: Rc<Vec<u8>>,
+   |        ^^^^^^^^^^^ help: try: `Rc<[u8]>`
+
+error: usage of `Rc<T>` when T is a buffer type
+  --> $DIR/rc_buffer.rs:10:8
+   |
+LL |     d: Rc<OsString>,
+   |        ^^^^^^^^^^^^ help: try: `Rc<std::ffi::OsStr>`
+
+error: aborting due to 4 previous errors
+

tests/ui/rc_buffer_arc.rs

@@ -0,0 +1,13 @@
+use std::ffi::OsString;
+use std::path::PathBuf;
+use std::sync::Arc;
+
+#[warn(clippy::rc_buffer)]
+struct S {
+    a: Arc<String>,
+    b: Arc<PathBuf>,
+    c: Arc<Vec<u8>>,
+    d: Arc<OsString>,
+}
+
+fn main() {}

tests/ui/rc_buffer_arc.stderr

@@ -0,0 +1,28 @@
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:7:8
+   |
+LL |     a: 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:8:8
+   |
+LL |     b: Arc<PathBuf>,
+   |        ^^^^^^^^^^^^ help: try: `Arc<std::path::Path>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:9:8
+   |
+LL |     c: Arc<Vec<u8>>,
+   |        ^^^^^^^^^^^^ help: try: `Arc<[u8]>`
+
+error: usage of `Arc<T>` when T is a buffer type
+  --> $DIR/rc_buffer_arc.rs:10:8
+   |
+LL |     d: Arc<OsString>,
+   |        ^^^^^^^^^^^^^ help: try: `Arc<std::ffi::OsStr>`
+
+error: aborting due to 4 previous errors
+