diff --git a/CHANGELOG.md b/CHANGELOG.md index 11a3e6a613f68..afd31179098bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1920,6 +1920,7 @@ Released 2018-09-13 [`redundant_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate [`redundant_static_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes [`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref +[`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro [`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 643e56671db5a..f08a32ee553bb 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -294,6 +294,7 @@ mod redundant_closure_call; mod redundant_field_names; mod redundant_pub_crate; mod redundant_static_lifetimes; +mod ref_option_ref; mod reference; mod regex; mod repeat_once; @@ -810,6 +811,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &redundant_field_names::REDUNDANT_FIELD_NAMES, &redundant_pub_crate::REDUNDANT_PUB_CRATE, &redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES, + &ref_option_ref::REF_OPTION_REF, &reference::DEREF_ADDROF, &reference::REF_IN_DEREF, ®ex::INVALID_REGEX, @@ -1033,6 +1035,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &sess.target, ); store.register_late_pass(move || box pass_by_ref_or_value); + store.register_late_pass(|| box ref_option_ref::RefOptionRef); store.register_late_pass(|| box try_err::TryErr); store.register_late_pass(|| box use_self::UseSelf); store.register_late_pass(|| box bytecount::ByteCount); @@ -1261,6 +1264,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF), LintId::of(&ranges::RANGE_MINUS_ONE), LintId::of(&ranges::RANGE_PLUS_ONE), + LintId::of(&ref_option_ref::REF_OPTION_REF), LintId::of(&shadow::SHADOW_UNRELATED), LintId::of(&strings::STRING_ADD_ASSIGN), LintId::of(&trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS), diff --git a/clippy_lints/src/ref_option_ref.rs b/clippy_lints/src/ref_option_ref.rs new file mode 100644 index 0000000000000..a914a77d48b4e --- /dev/null +++ b/clippy_lints/src/ref_option_ref.rs @@ -0,0 +1,66 @@ +use crate::utils::{last_path_segment, snippet, span_lint_and_sugg}; +use rustc_hir::{GenericArg, Mutability, Ty, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +use if_chain::if_chain; +use rustc_errors::Applicability; + +declare_clippy_lint! { + /// **What it does:** Checks for usage of `&Option<&T>`. + /// + /// **Why is this bad?** Since `&` is Copy, it's useless to have a + /// reference on `Option<&T>`. + /// + /// **Known problems:** It may be irrevelent to use this lint on + /// public API code as it will make a breaking change to apply it. + /// + /// **Example:** + /// + /// ```rust,ignore + /// let x: &Option<&u32> = &Some(&0u32); + /// ``` + /// Use instead: + /// ```rust,ignore + /// let x: Option<&u32> = Some(&0u32); + /// ``` + pub REF_OPTION_REF, + pedantic, + "use `Option<&T>` instead of `&Option<&T>`" +} + +declare_lint_pass!(RefOptionRef => [REF_OPTION_REF]); + +impl<'tcx> LateLintPass<'tcx> for RefOptionRef { + fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx>) { + if_chain! { + if let TyKind::Rptr(_, ref mut_ty) = ty.kind; + if mut_ty.mutbl == Mutability::Not; + if let TyKind::Path(ref qpath) = &mut_ty.ty.kind; + let last = last_path_segment(qpath); + if let Some(res) = last.res; + if let Some(def_id) = res.opt_def_id(); + + if cx.tcx.is_diagnostic_item(sym!(option_type), def_id); + if let Some(ref params) = last_path_segment(qpath).args ; + if !params.parenthesized; + if let Some(inner_ty) = params.args.iter().find_map(|arg| match arg { + GenericArg::Type(inner_ty) => Some(inner_ty), + _ => None, + }); + if let TyKind::Rptr(_, _) = inner_ty.kind; + + then { + span_lint_and_sugg( + cx, + REF_OPTION_REF, + ty.span, + "since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`", + "try", + format!("Option<{}>", &snippet(cx, inner_ty.span, "..")), + Applicability::MaybeIncorrect, + ); + } + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 44d2dd746d590..6f3a4b1b013d1 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2034,6 +2034,13 @@ vec![ deprecation: None, module: "reference", }, + Lint { + name: "ref_option_ref", + group: "pedantic", + desc: "use `Option<&T>` instead of `&Option<&T>`", + deprecation: None, + module: "ref_option_ref", + }, Lint { name: "repeat_once", group: "complexity", diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index a7fb00a270577..47e7460fa7a44 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -1,6 +1,7 @@ // run-rustfix #![warn(clippy::option_if_let_else)] #![allow(clippy::redundant_closure)] +#![allow(clippy::ref_option_ref)] fn bad1(string: Option<&str>) -> (bool, &str) { string.map_or((false, "hello"), |x| (true, x)) diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index 895fd86321faf..e2f8dec3b930c 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -1,6 +1,7 @@ // run-rustfix #![warn(clippy::option_if_let_else)] #![allow(clippy::redundant_closure)] +#![allow(clippy::ref_option_ref)] fn bad1(string: Option<&str>) -> (bool, &str) { if let Some(x) = string { diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index b69fe76768270..7aab068800a02 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -1,5 +1,5 @@ error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:6:5 + --> $DIR/option_if_let_else.rs:7:5 | LL | / if let Some(x) = string { LL | | (true, x) @@ -11,7 +11,7 @@ LL | | } = note: `-D clippy::option-if-let-else` implied by `-D warnings` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:16:12 + --> $DIR/option_if_let_else.rs:17:12 | LL | } else if let Some(x) = string { | ____________^ @@ -22,19 +22,19 @@ LL | | } | |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:24:13 + --> $DIR/option_if_let_else.rs:25:13 | LL | let _ = if let Some(s) = *string { s.len() } else { 0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:25:13 + --> $DIR/option_if_let_else.rs:26:13 | LL | let _ = if let Some(s) = &num { s } else { &0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:26:13 + --> $DIR/option_if_let_else.rs:27:13 | LL | let _ = if let Some(s) = &mut num { | _____________^ @@ -54,13 +54,13 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:32:13 + --> $DIR/option_if_let_else.rs:33:13 | LL | let _ = if let Some(ref s) = num { s } else { &0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:33:13 + --> $DIR/option_if_let_else.rs:34:13 | LL | let _ = if let Some(mut s) = num { | _____________^ @@ -80,7 +80,7 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:39:13 + --> $DIR/option_if_let_else.rs:40:13 | LL | let _ = if let Some(ref mut s) = num { | _____________^ @@ -100,7 +100,7 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:48:5 + --> $DIR/option_if_let_else.rs:49:5 | LL | / if let Some(x) = arg { LL | | let y = x * x; @@ -119,7 +119,7 @@ LL | }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:61:13 + --> $DIR/option_if_let_else.rs:62:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -131,7 +131,7 @@ LL | | }; | |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)` error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:70:13 + --> $DIR/option_if_let_else.rs:71:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -154,7 +154,7 @@ LL | }, |x| x * x * x * x); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:99:13 + --> $DIR/option_if_let_else.rs:100:13 | LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` diff --git a/tests/ui/ref_option_ref.rs b/tests/ui/ref_option_ref.rs new file mode 100644 index 0000000000000..b2c275d68afa1 --- /dev/null +++ b/tests/ui/ref_option_ref.rs @@ -0,0 +1,47 @@ +#![allow(unused)] +#![warn(clippy::ref_option_ref)] + +// This lint is not tagged as run-rustfix because automatically +// changing the type of a variable would also means changing +// all usages of this variable to match and This is not handled +// by this lint. + +static THRESHOLD: i32 = 10; +static REF_THRESHOLD: &Option<&i32> = &Some(&THRESHOLD); +const CONST_THRESHOLD: &i32 = &10; +const REF_CONST: &Option<&i32> = &Some(&CONST_THRESHOLD); + +type RefOptRefU32<'a> = &'a Option<&'a u32>; +type RefOptRef<'a, T> = &'a Option<&'a T>; + +fn foo(data: &Option<&u32>) {} + +fn bar(data: &u32) -> &Option<&u32> { + &None +} + +struct StructRef<'a> { + data: &'a Option<&'a u32>, +} + +struct StructTupleRef<'a>(u32, &'a Option<&'a u32>); + +enum EnumRef<'a> { + Variant1(u32), + Variant2(&'a Option<&'a u32>), +} + +trait RefOptTrait { + type A; + fn foo(&self, _: Self::A); +} + +impl RefOptTrait for u32 { + type A = &'static Option<&'static Self>; + + fn foo(&self, _: Self::A) {} +} + +fn main() { + let x: &Option<&u32> = &None; +} diff --git a/tests/ui/ref_option_ref.stderr b/tests/ui/ref_option_ref.stderr new file mode 100644 index 0000000000000..4e7fc80006118 --- /dev/null +++ b/tests/ui/ref_option_ref.stderr @@ -0,0 +1,70 @@ +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:10:23 + | +LL | static REF_THRESHOLD: &Option<&i32> = &Some(&THRESHOLD); + | ^^^^^^^^^^^^^ help: try: `Option<&i32>` + | + = note: `-D clippy::ref-option-ref` implied by `-D warnings` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:12:18 + | +LL | const REF_CONST: &Option<&i32> = &Some(&CONST_THRESHOLD); + | ^^^^^^^^^^^^^ help: try: `Option<&i32>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:14:25 + | +LL | type RefOptRefU32<'a> = &'a Option<&'a u32>; + | ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:15:25 + | +LL | type RefOptRef<'a, T> = &'a Option<&'a T>; + | ^^^^^^^^^^^^^^^^^ help: try: `Option<&'a T>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:17:14 + | +LL | fn foo(data: &Option<&u32>) {} + | ^^^^^^^^^^^^^ help: try: `Option<&u32>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:19:23 + | +LL | fn bar(data: &u32) -> &Option<&u32> { + | ^^^^^^^^^^^^^ help: try: `Option<&u32>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:24:11 + | +LL | data: &'a Option<&'a u32>, + | ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:27:32 + | +LL | struct StructTupleRef<'a>(u32, &'a Option<&'a u32>); + | ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:31:14 + | +LL | Variant2(&'a Option<&'a u32>), + | ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:40:14 + | +LL | type A = &'static Option<&'static Self>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'static Self>` + +error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>` + --> $DIR/ref_option_ref.rs:46:12 + | +LL | let x: &Option<&u32> = &None; + | ^^^^^^^^^^^^^ help: try: `Option<&u32>` + +error: aborting due to 11 previous errors +