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>), + 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>) -> 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 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, + 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 { 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( @@ -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` 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` 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", "", "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 @@ -1865,6 +1865,13 @@ pub static ref ALL_LINTS: Vec = 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", 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, + bad2: Rc, + bad3: Rc>, + bad4: Rc, + // does not trigger lint + good1: Rc>, +} + +// triggers lint +fn func_bad1(_: Rc) {} +fn func_bad2(_: Rc) {} +fn func_bad3(_: Rc>) {} +fn func_bad4(_: Rc) {} +// does not trigger lint +fn func_good1(_: Rc>) {} + +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` when T is a buffer type + --> $DIR/rc_buffer.rs:10:11 + | +LL | bad1: 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:11:11 + | +LL | bad2: Rc, + | ^^^^^^^^^^^ help: try: `Rc` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:12:11 + | +LL | bad3: Rc>, + | ^^^^^^^^^^^ help: try: `Rc<[u8]>` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:13:11 + | +LL | bad4: Rc, + | ^^^^^^^^^^^^ help: try: `Rc` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:19:17 + | +LL | fn func_bad1(_: Rc) {} + | ^^^^^^^^^^ help: try: `Rc` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:20:17 + | +LL | fn func_bad2(_: Rc) {} + | ^^^^^^^^^^^ help: try: `Rc` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:21:17 + | +LL | fn func_bad3(_: Rc>) {} + | ^^^^^^^^^^^ help: try: `Rc<[u8]>` + +error: usage of `Rc` when T is a buffer type + --> $DIR/rc_buffer.rs:22:17 + | +LL | fn func_bad4(_: Rc) {} + | ^^^^^^^^^^^^ help: try: `Rc` + +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, + bad2: Arc, + bad3: Arc>, + bad4: Arc, + // does not trigger lint + good1: Arc>, +} + +// triggers lint +fn func_bad1(_: Arc) {} +fn func_bad2(_: Arc) {} +fn func_bad3(_: Arc>) {} +fn func_bad4(_: Arc) {} +// does not trigger lint +fn func_good1(_: Arc>) {} + +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` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:9:11 + | +LL | bad1: Arc, + | ^^^^^^^^^^^ help: try: `Arc` + | + = note: `-D clippy::rc-buffer` implied by `-D warnings` + +error: usage of `Arc` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:10:11 + | +LL | bad2: Arc, + | ^^^^^^^^^^^^ help: try: `Arc` + +error: usage of `Arc` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:11:11 + | +LL | bad3: Arc>, + | ^^^^^^^^^^^^ help: try: `Arc<[u8]>` + +error: usage of `Arc` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:12:11 + | +LL | bad4: Arc, + | ^^^^^^^^^^^^^ help: try: `Arc` + +error: usage of `Arc` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:18:17 + | +LL | fn func_bad1(_: Arc) {} + | ^^^^^^^^^^^ help: try: `Arc` + +error: usage of `Arc` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:19:17 + | +LL | fn func_bad2(_: Arc) {} + | ^^^^^^^^^^^^ help: try: `Arc` + +error: usage of `Arc` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:20:17 + | +LL | fn func_bad3(_: Arc>) {} + | ^^^^^^^^^^^^ help: try: `Arc<[u8]>` + +error: usage of `Arc` when T is a buffer type + --> $DIR/rc_buffer_arc.rs:21:17 + | +LL | fn func_bad4(_: Arc) {} + | ^^^^^^^^^^^^^ help: try: `Arc` + +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, +} + +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