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

12 files changed, 304 insertions, 3 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5a84333d6db..d1dfe36ffd8 100644
--- a/CHANGELOG.md
+++ b/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
diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs
index 3ee022e4e68..0000d39263e 100644
--- a/clippy_lints/src/consts.rs
+++ b/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 {
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index a6313127f69..58112ac8da5 100644
--- a/clippy_lints/src/lib.rs
+++ b/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),
     ]);
diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs
index b6d405cca77..a29a199b8c3 100644
--- a/clippy_lints/src/types.rs
+++ b/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<_>
diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
index 8736121c3b4..1583afad208 100644
--- a/clippy_lints/src/utils/paths.rs
+++ b/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"];
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index 2718b14e291..9603023ed06 100644
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -1866,6 +1866,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         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",
         desc: "redundant allocation",
diff --git a/tests/ui/rc_buffer.rs b/tests/ui/rc_buffer.rs
new file mode 100644
index 00000000000..1fa98643936
--- /dev/null
+++ b/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() {}
diff --git a/tests/ui/rc_buffer.stderr b/tests/ui/rc_buffer.stderr
new file mode 100644
index 00000000000..e4cc169af07
--- /dev/null
+++ b/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
+
diff --git a/tests/ui/rc_buffer_arc.rs b/tests/ui/rc_buffer_arc.rs
new file mode 100644
index 00000000000..5d586584817
--- /dev/null
+++ b/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() {}
diff --git a/tests/ui/rc_buffer_arc.stderr b/tests/ui/rc_buffer_arc.stderr
new file mode 100644
index 00000000000..8252270d2ac
--- /dev/null
+++ b/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
+
diff --git a/tests/ui/rc_buffer_redefined_string.rs b/tests/ui/rc_buffer_redefined_string.rs
new file mode 100644
index 00000000000..5d31a848cf7
--- /dev/null
+++ b/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() {}
diff --git a/tests/ui/rc_buffer_redefined_string.stderr b/tests/ui/rc_buffer_redefined_string.stderr
new file mode 100644
index 00000000000..e69de29bb2d
--- /dev/null
+++ b/tests/ui/rc_buffer_redefined_string.stderr