diff --git a/CHANGELOG.md b/CHANGELOG.md index 285a2ff80..8922f5e70 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c017c5cb5..239eeb10b 100644 --- a/clippy_lints/src/lib.rs +++ b/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), ]); } diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index b6d405cca..5f3a2e0b6 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 and Arc when T is a mutable buffer type such as String or Vec + /// + /// **Why is this bad?** Expressions such as Rc have no advantage over Rc, since + /// it is larger and involves an extra level of indirection, and doesn't implement Borrow. + /// + /// 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) { ... } + /// ``` + /// + /// Better: + /// + /// ```rust,ignore + /// fn foo(interned: Rc) { ... } + /// ``` + 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 { 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` 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` 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( diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 65320d6a0..2df11d2ef 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 SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index a7d38c934..d0c6a1d63 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1851,6 +1851,13 @@ pub static ref ALL_LINTS: Vec = 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", diff --git a/tests/ui/rc_buffer.rs b/tests/ui/rc_buffer.rs new file mode 100644 index 000000000..c8c2bec67 --- /dev/null +++ b/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, + b: Rc, + c: Rc>, + d: Rc, +} + +fn main() {} diff --git a/tests/ui/rc_buffer.stderr b/tests/ui/rc_buffer.stderr new file mode 100644 index 000000000..641a13a22 --- /dev/null +++ b/tests/ui/rc_buffer.stderr @@ -0,0 +1,28 @@ +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:7:8 + | +LL | a: Rc, + | ^^^^^^^^^^ help: try: `Rc` + | + = note: `-D clippy::rc-buffer` implied by `-D warnings` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:8:8 + | +LL | b: Rc, + | ^^^^^^^^^^^ help: try: `Rc` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:9:8 + | +LL | c: Rc>, + | ^^^^^^^^^^^ help: try: `Rc<[u8]>` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:10:8 + | +LL | d: Rc, + | ^^^^^^^^^^^^ help: try: `Rc` + +error: aborting due to 4 previous errors +