diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | book/src/lint_configuration.md | 1 | ||||
| -rw-r--r-- | clippy_config/src/conf.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/functions/mod.rs | 60 | ||||
| -rw-r--r-- | clippy_lints/src/functions/ref_option.rs | 122 | ||||
| -rw-r--r-- | tests/ui/ref_option/all/clippy.toml | 1 | ||||
| -rw-r--r-- | tests/ui/ref_option/private/clippy.toml | 1 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option.all.fixed | 62 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option.all.stderr | 162 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option.private.fixed | 62 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option.private.stderr | 108 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option.rs | 62 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option_traits.all.stderr | 37 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option_traits.private.stderr | 21 | ||||
| -rw-r--r-- | tests/ui/ref_option/ref_option_traits.rs | 37 |
16 files changed, 739 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 41a86e8ce51..5d253d52531 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5871,6 +5871,7 @@ Released 2018-09-13 [`ref_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference [`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref +[`ref_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option [`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref [`ref_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_patterns [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 91159bc79c5..07a56fb33df 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -353,6 +353,7 @@ Suppress lints whenever the suggested change would cause breakage for other crat * [`rc_buffer`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer) * [`rc_mutex`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex) * [`redundant_allocation`](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation) +* [`ref_option`](https://rust-lang.github.io/rust-clippy/master/index.html#ref_option) * [`single_call_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#single_call_fn) * [`trivially_copy_pass_by_ref`](https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref) * [`unnecessary_box_returns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 757620341cc..e4e2c97fdc1 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -378,6 +378,7 @@ define_Conf! { rc_buffer, rc_mutex, redundant_allocation, + ref_option, single_call_fn, trivially_copy_pass_by_ref, unnecessary_box_returns, diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2b229d2fe6a..9cec672beb0 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -202,6 +202,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::functions::MUST_USE_CANDIDATE_INFO, crate::functions::MUST_USE_UNIT_INFO, crate::functions::NOT_UNSAFE_PTR_ARG_DEREF_INFO, + crate::functions::REF_OPTION_INFO, crate::functions::RENAMED_FUNCTION_PARAMS_INFO, crate::functions::RESULT_LARGE_ERR_INFO, crate::functions::RESULT_UNIT_ERR_INFO, diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index ba8a459c917..203c3ce9eec 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -2,6 +2,7 @@ mod impl_trait_in_params; mod misnamed_getters; mod must_use; mod not_unsafe_ptr_arg_deref; +mod ref_option; mod renamed_function_params; mod result; mod too_many_arguments; @@ -399,6 +400,53 @@ declare_clippy_lint! { "renamed function parameters in trait implementation" } +declare_clippy_lint! { + /// ### What it does + /// Warns when a function signature uses `&Option<T>` instead of `Option<&T>`. + /// + /// ### Why is this bad? + /// More flexibility, better memory optimization, and more idiomatic Rust code. + /// + /// `&Option<T>` in a function signature breaks encapsulation because the caller must own T + /// and move it into an Option to call with it. When returned, the owner must internally store + /// it as `Option<T>` in order to return it. + /// At a lower level, `&Option<T>` points to memory with the `presence` bit flag plus the `T` value, + /// whereas `Option<&T>` is usually [optimized](https://doc.rust-lang.org/1.81.0/std/option/index.html#representation) + /// to a single pointer, so it may be more optimal. + /// + /// See this [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE) by + /// Logan Smith for an in-depth explanation of why this is important. + /// + /// ### Known problems + /// This lint recommends changing the function signatures, but it cannot + /// automatically change the function calls or the function implementations. + /// + /// ### Example + /// ```no_run + /// // caller uses foo(&opt) + /// fn foo(a: &Option<String>) {} + /// # struct Unit {} + /// # impl Unit { + /// fn bar(&self) -> &Option<String> { &None } + /// # } + /// ``` + /// Use instead: + /// ```no_run + /// // caller should use `foo1(opt.as_ref())` + /// fn foo1(a: Option<&String>) {} + /// // better yet, use string slice `foo2(opt.as_deref())` + /// fn foo2(a: Option<&str>) {} + /// # struct Unit {} + /// # impl Unit { + /// fn bar(&self) -> Option<&String> { None } + /// # } + /// ``` + #[clippy::version = "1.82.0"] + pub REF_OPTION, + nursery, + "function signature uses `&Option<T>` instead of `Option<&T>`" +} + pub struct Functions { too_many_arguments_threshold: u64, too_many_lines_threshold: u64, @@ -437,6 +485,7 @@ impl_lint_pass!(Functions => [ MISNAMED_GETTERS, IMPL_TRAIT_IN_PARAMS, RENAMED_FUNCTION_PARAMS, + REF_OPTION, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -455,6 +504,16 @@ impl<'tcx> LateLintPass<'tcx> for Functions { not_unsafe_ptr_arg_deref::check_fn(cx, kind, decl, body, def_id); misnamed_getters::check_fn(cx, kind, decl, body, span); impl_trait_in_params::check_fn(cx, &kind, body, hir_id); + ref_option::check_fn( + cx, + kind, + decl, + span, + hir_id, + def_id, + body, + self.avoid_breaking_exported_api, + ); } fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { @@ -475,5 +534,6 @@ impl<'tcx> LateLintPass<'tcx> for Functions { must_use::check_trait_item(cx, item); result::check_trait_item(cx, item, self.large_error_threshold); impl_trait_in_params::check_trait_item(cx, item, self.avoid_breaking_exported_api); + ref_option::check_trait_item(cx, item, self.avoid_breaking_exported_api); } } diff --git a/clippy_lints/src/functions/ref_option.rs b/clippy_lints/src/functions/ref_option.rs new file mode 100644 index 00000000000..373ecd74cb3 --- /dev/null +++ b/clippy_lints/src/functions/ref_option.rs @@ -0,0 +1,122 @@ +use crate::functions::REF_OPTION; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::is_trait_impl_item; +use clippy_utils::source::snippet; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::intravisit::FnKind; +use rustc_hir::{FnDecl, HirId}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, GenericArgKind, Mutability, Ty}; +use rustc_span::def_id::LocalDefId; +use rustc_span::{Span, sym}; + +fn check_ty<'a>(cx: &LateContext<'a>, param: &rustc_hir::Ty<'a>, param_ty: Ty<'a>, fixes: &mut Vec<(Span, String)>) { + if let ty::Ref(_, opt_ty, Mutability::Not) = param_ty.kind() + && is_type_diagnostic_item(cx, *opt_ty, sym::Option) + && let ty::Adt(_, opt_gen) = opt_ty.kind() + && let [gen] = opt_gen.as_slice() + && let GenericArgKind::Type(gen_ty) = gen.unpack() + && !gen_ty.is_ref() + // Need to gen the original spans, so first parsing mid, and hir parsing afterward + && let hir::TyKind::Ref(lifetime, hir::MutTy { ty, .. }) = param.kind + && let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = ty.kind + && let (Some(first), Some(last)) = (path.segments.first(), path.segments.last()) + && let Some(hir::GenericArgs { + args: [hir::GenericArg::Type(opt_ty)], + .. + }) = last.args + { + let lifetime = snippet(cx, lifetime.ident.span, ".."); + fixes.push(( + param.span, + format!( + "{}<&{lifetime}{}{}>", + snippet(cx, first.ident.span.to(last.ident.span), ".."), + if lifetime.is_empty() { "" } else { " " }, + snippet(cx, opt_ty.span, "..") + ), + )); + } +} + +fn check_fn_sig<'a>(cx: &LateContext<'a>, decl: &FnDecl<'a>, span: Span, sig: ty::FnSig<'a>) { + let mut fixes = Vec::new(); + // Check function arguments' types + for (param, param_ty) in decl.inputs.iter().zip(sig.inputs()) { + check_ty(cx, param, *param_ty, &mut fixes); + } + // Check return type + if let hir::FnRetTy::Return(ty) = &decl.output { + check_ty(cx, ty, sig.output(), &mut fixes); + } + if !fixes.is_empty() { + span_lint_and_then( + cx, + REF_OPTION, + span, + "it is more idiomatic to use `Option<&T>` instead of `&Option<T>`", + |diag| { + diag.multipart_suggestion("change this to", fixes, Applicability::Unspecified); + }, + ); + } +} + +#[allow(clippy::too_many_arguments)] +pub(crate) fn check_fn<'a>( + cx: &LateContext<'a>, + kind: FnKind<'_>, + decl: &FnDecl<'a>, + span: Span, + hir_id: HirId, + def_id: LocalDefId, + body: &hir::Body<'_>, + avoid_breaking_exported_api: bool, +) { + if avoid_breaking_exported_api && cx.effective_visibilities.is_exported(def_id) { + return; + } + + if let FnKind::Closure = kind { + // Compute the span of the closure parameters + return type if set + let span = if let hir::FnRetTy::Return(out_ty) = &decl.output { + if decl.inputs.is_empty() { + out_ty.span + } else { + span.with_hi(out_ty.span.hi()) + } + } else if let (Some(first), Some(last)) = (decl.inputs.first(), decl.inputs.last()) { + first.span.to(last.span) + } else { + // No parameters - no point in checking + return; + }; + + // Figure out the signature of the closure + let ty::Closure(_, args) = cx.typeck_results().expr_ty(body.value).kind() else { + return; + }; + let sig = args.as_closure().sig().skip_binder(); + + check_fn_sig(cx, decl, span, sig); + } else if !is_trait_impl_item(cx, hir_id) { + let sig = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder(); + check_fn_sig(cx, decl, span, sig); + } +} + +pub(super) fn check_trait_item<'a>( + cx: &LateContext<'a>, + trait_item: &hir::TraitItem<'a>, + avoid_breaking_exported_api: bool, +) { + if let hir::TraitItemKind::Fn(ref sig, _) = trait_item.kind + && !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(trait_item.owner_id.def_id)) + { + let def_id = trait_item.owner_id.def_id; + let ty_sig = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder(); + check_fn_sig(cx, sig.decl, sig.span, ty_sig); + } +} diff --git a/tests/ui/ref_option/all/clippy.toml b/tests/ui/ref_option/all/clippy.toml new file mode 100644 index 00000000000..cda8d17eed4 --- /dev/null +++ b/tests/ui/ref_option/all/clippy.toml @@ -0,0 +1 @@ +avoid-breaking-exported-api = false diff --git a/tests/ui/ref_option/private/clippy.toml b/tests/ui/ref_option/private/clippy.toml new file mode 100644 index 00000000000..5f304987aa9 --- /dev/null +++ b/tests/ui/ref_option/private/clippy.toml @@ -0,0 +1 @@ +avoid-breaking-exported-api = true diff --git a/tests/ui/ref_option/ref_option.all.fixed b/tests/ui/ref_option/ref_option.all.fixed new file mode 100644 index 00000000000..47781a97c98 --- /dev/null +++ b/tests/ui/ref_option/ref_option.all.fixed @@ -0,0 +1,62 @@ +//@revisions: private all +//@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/private +//@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/all + +#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)] +#![warn(clippy::ref_option)] + +fn opt_u8(a: Option<&u8>) {} +fn opt_gen<T>(a: Option<&T>) {} +fn opt_string(a: std::option::Option<&String>) {} +fn ret_string<'a>(p: &'a str) -> Option<&'a u8> { + panic!() +} +fn ret_string_static() -> Option<&'static u8> { + panic!() +} +fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} +fn ret_box<'a>() -> Option<&'a Box<u8>> { + panic!() +} + +pub fn pub_opt_string(a: Option<&String>) {} +pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} + +pub trait PubTrait { + fn pub_trait_opt(&self, a: Option<&Vec<u8>>); + fn pub_trait_ret(&self) -> Option<&Vec<u8>>; +} + +trait PrivateTrait { + fn trait_opt(&self, a: Option<&String>); + fn trait_ret(&self) -> Option<&String>; +} + +pub struct PubStruct; + +impl PubStruct { + pub fn pub_opt_params(&self, a: Option<&()>) {} + pub fn pub_opt_ret(&self) -> Option<&String> { + panic!() + } + + fn private_opt_params(&self, a: Option<&()>) {} + fn private_opt_ret(&self) -> Option<&String> { + panic!() + } +} + +// valid, don't change +fn mut_u8(a: &mut Option<u8>) {} +pub fn pub_mut_u8(a: &mut Option<String>) {} + +// might be good to catch in the future +fn mut_u8_ref(a: &mut &Option<u8>) {} +pub fn pub_mut_u8_ref(a: &mut &Option<String>) {} +fn lambdas() { + // Not handled for now, not sure if we should + let x = |a: &Option<String>| {}; + let x = |a: &Option<String>| -> &Option<String> { panic!() }; +} + +fn main() {} diff --git a/tests/ui/ref_option/ref_option.all.stderr b/tests/ui/ref_option/ref_option.all.stderr new file mode 100644 index 00000000000..b4c69ac6296 --- /dev/null +++ b/tests/ui/ref_option/ref_option.all.stderr @@ -0,0 +1,162 @@ +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:8:1 + | +LL | fn opt_u8(a: &Option<u8>) {} + | ^^^^^^^^^^^^^-----------^^^^ + | | + | help: change this to: `Option<&u8>` + | + = note: `-D clippy::ref-option` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ref_option)]` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:9:1 + | +LL | fn opt_gen<T>(a: &Option<T>) {} + | ^^^^^^^^^^^^^^^^^----------^^^^ + | | + | help: change this to: `Option<&T>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:10:1 + | +LL | fn opt_string(a: &std::option::Option<String>) {} + | ^^^^^^^^^^^^^^^^^----------------------------^^^^ + | | + | help: change this to: `std::option::Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:11:1 + | +LL | fn ret_string<'a>(p: &'a str) -> &'a Option<u8> { + | ^ -------------- help: change this to: `Option<&'a u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:14:1 + | +LL | fn ret_string_static() -> &'static Option<u8> { + | ^ ------------------- help: change this to: `Option<&'static u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:17:1 + | +LL | fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL | fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} + | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:18:1 + | +LL | fn ret_box<'a>() -> &'a Option<Box<u8>> { + | ^ ------------------- help: change this to: `Option<&'a Box<u8>>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:22:1 + | +LL | pub fn pub_opt_string(a: &Option<String>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^---------------^^^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:23:1 + | +LL | pub fn pub_mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL | pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} + | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:26:5 + | +LL | fn pub_trait_opt(&self, a: &Option<Vec<u8>>); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^^ + | | + | help: change this to: `Option<&Vec<u8>>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:27:5 + | +LL | fn pub_trait_ret(&self) -> &Option<Vec<u8>>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^ + | | + | help: change this to: `Option<&Vec<u8>>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:31:5 + | +LL | fn trait_opt(&self, a: &Option<String>); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:32:5 + | +LL | fn trait_ret(&self) -> &Option<String>; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:38:5 + | +LL | pub fn pub_opt_params(&self, a: &Option<()>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^ + | | + | help: change this to: `Option<&()>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:39:5 + | +LL | pub fn pub_opt_ret(&self) -> &Option<String> { + | ^ --------------- help: change this to: `Option<&String>` + | _____| + | | +LL | | panic!() +LL | | } + | |_____^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:43:5 + | +LL | fn private_opt_params(&self, a: &Option<()>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^ + | | + | help: change this to: `Option<&()>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:44:5 + | +LL | fn private_opt_ret(&self) -> &Option<String> { + | ^ --------------- help: change this to: `Option<&String>` + | _____| + | | +LL | | panic!() +LL | | } + | |_____^ + +error: aborting due to 17 previous errors + diff --git a/tests/ui/ref_option/ref_option.private.fixed b/tests/ui/ref_option/ref_option.private.fixed new file mode 100644 index 00000000000..8c42556e9b0 --- /dev/null +++ b/tests/ui/ref_option/ref_option.private.fixed @@ -0,0 +1,62 @@ +//@revisions: private all +//@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/private +//@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/all + +#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)] +#![warn(clippy::ref_option)] + +fn opt_u8(a: Option<&u8>) {} +fn opt_gen<T>(a: Option<&T>) {} +fn opt_string(a: std::option::Option<&String>) {} +fn ret_string<'a>(p: &'a str) -> Option<&'a u8> { + panic!() +} +fn ret_string_static() -> Option<&'static u8> { + panic!() +} +fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} +fn ret_box<'a>() -> Option<&'a Box<u8>> { + panic!() +} + +pub fn pub_opt_string(a: &Option<String>) {} +pub fn pub_mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} + +pub trait PubTrait { + fn pub_trait_opt(&self, a: &Option<Vec<u8>>); + fn pub_trait_ret(&self) -> &Option<Vec<u8>>; +} + +trait PrivateTrait { + fn trait_opt(&self, a: Option<&String>); + fn trait_ret(&self) -> Option<&String>; +} + +pub struct PubStruct; + +impl PubStruct { + pub fn pub_opt_params(&self, a: &Option<()>) {} + pub fn pub_opt_ret(&self) -> &Option<String> { + panic!() + } + + fn private_opt_params(&self, a: Option<&()>) {} + fn private_opt_ret(&self) -> Option<&String> { + panic!() + } +} + +// valid, don't change +fn mut_u8(a: &mut Option<u8>) {} +pub fn pub_mut_u8(a: &mut Option<String>) {} + +// might be good to catch in the future +fn mut_u8_ref(a: &mut &Option<u8>) {} +pub fn pub_mut_u8_ref(a: &mut &Option<String>) {} +fn lambdas() { + // Not handled for now, not sure if we should + let x = |a: &Option<String>| {}; + let x = |a: &Option<String>| -> &Option<String> { panic!() }; +} + +fn main() {} diff --git a/tests/ui/ref_option/ref_option.private.stderr b/tests/ui/ref_option/ref_option.private.stderr new file mode 100644 index 00000000000..17c90536da3 --- /dev/null +++ b/tests/ui/ref_option/ref_option.private.stderr @@ -0,0 +1,108 @@ +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:8:1 + | +LL | fn opt_u8(a: &Option<u8>) {} + | ^^^^^^^^^^^^^-----------^^^^ + | | + | help: change this to: `Option<&u8>` + | + = note: `-D clippy::ref-option` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ref_option)]` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:9:1 + | +LL | fn opt_gen<T>(a: &Option<T>) {} + | ^^^^^^^^^^^^^^^^^----------^^^^ + | | + | help: change this to: `Option<&T>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:10:1 + | +LL | fn opt_string(a: &std::option::Option<String>) {} + | ^^^^^^^^^^^^^^^^^----------------------------^^^^ + | | + | help: change this to: `std::option::Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:11:1 + | +LL | fn ret_string<'a>(p: &'a str) -> &'a Option<u8> { + | ^ -------------- help: change this to: `Option<&'a u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:14:1 + | +LL | fn ret_string_static() -> &'static Option<u8> { + | ^ ------------------- help: change this to: `Option<&'static u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:17:1 + | +LL | fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL | fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} + | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:18:1 + | +LL | fn ret_box<'a>() -> &'a Option<Box<u8>> { + | ^ ------------------- help: change this to: `Option<&'a Box<u8>>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:31:5 + | +LL | fn trait_opt(&self, a: &Option<String>); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:32:5 + | +LL | fn trait_ret(&self) -> &Option<String>; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:43:5 + | +LL | fn private_opt_params(&self, a: &Option<()>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^ + | | + | help: change this to: `Option<&()>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option.rs:44:5 + | +LL | fn private_opt_ret(&self) -> &Option<String> { + | ^ --------------- help: change this to: `Option<&String>` + | _____| + | | +LL | | panic!() +LL | | } + | |_____^ + +error: aborting due to 11 previous errors + diff --git a/tests/ui/ref_option/ref_option.rs b/tests/ui/ref_option/ref_option.rs new file mode 100644 index 00000000000..05251bcf12c --- /dev/null +++ b/tests/ui/ref_option/ref_option.rs @@ -0,0 +1,62 @@ +//@revisions: private all +//@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/private +//@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/all + +#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)] +#![warn(clippy::ref_option)] + +fn opt_u8(a: &Option<u8>) {} +fn opt_gen<T>(a: &Option<T>) {} +fn opt_string(a: &std::option::Option<String>) {} +fn ret_string<'a>(p: &'a str) -> &'a Option<u8> { + panic!() +} +fn ret_string_static() -> &'static Option<u8> { + panic!() +} +fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} +fn ret_box<'a>() -> &'a Option<Box<u8>> { + panic!() +} + +pub fn pub_opt_string(a: &Option<String>) {} +pub fn pub_mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} + +pub trait PubTrait { + fn pub_trait_opt(&self, a: &Option<Vec<u8>>); + fn pub_trait_ret(&self) -> &Option<Vec<u8>>; +} + +trait PrivateTrait { + fn trait_opt(&self, a: &Option<String>); + fn trait_ret(&self) -> &Option<String>; +} + +pub struct PubStruct; + +impl PubStruct { + pub fn pub_opt_params(&self, a: &Option<()>) {} + pub fn pub_opt_ret(&self) -> &Option<String> { + panic!() + } + + fn private_opt_params(&self, a: &Option<()>) {} + fn private_opt_ret(&self) -> &Option<String> { + panic!() + } +} + +// valid, don't change +fn mut_u8(a: &mut Option<u8>) {} +pub fn pub_mut_u8(a: &mut Option<String>) {} + +// might be good to catch in the future +fn mut_u8_ref(a: &mut &Option<u8>) {} +pub fn pub_mut_u8_ref(a: &mut &Option<String>) {} +fn lambdas() { + // Not handled for now, not sure if we should + let x = |a: &Option<String>| {}; + let x = |a: &Option<String>| -> &Option<String> { panic!() }; +} + +fn main() {} diff --git a/tests/ui/ref_option/ref_option_traits.all.stderr b/tests/ui/ref_option/ref_option_traits.all.stderr new file mode 100644 index 00000000000..a9967168c12 --- /dev/null +++ b/tests/ui/ref_option/ref_option_traits.all.stderr @@ -0,0 +1,37 @@ +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option_traits.rs:10:5 + | +LL | fn pub_trait_opt(&self, a: &Option<Vec<u8>>); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^^ + | | + | help: change this to: `Option<&Vec<u8>>` + | + = note: `-D clippy::ref-option` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ref_option)]` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option_traits.rs:11:5 + | +LL | fn pub_trait_ret(&self) -> &Option<Vec<u8>>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^ + | | + | help: change this to: `Option<&Vec<u8>>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option_traits.rs:15:5 + | +LL | fn trait_opt(&self, a: &Option<String>); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option_traits.rs:16:5 + | +LL | fn trait_ret(&self) -> &Option<String>; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | help: change this to: `Option<&String>` + +error: aborting due to 4 previous errors + diff --git a/tests/ui/ref_option/ref_option_traits.private.stderr b/tests/ui/ref_option/ref_option_traits.private.stderr new file mode 100644 index 00000000000..36d0833af8a --- /dev/null +++ b/tests/ui/ref_option/ref_option_traits.private.stderr @@ -0,0 +1,21 @@ +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option_traits.rs:15:5 + | +LL | fn trait_opt(&self, a: &Option<String>); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | help: change this to: `Option<&String>` + | + = note: `-D clippy::ref-option` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ref_option)]` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` + --> tests/ui/ref_option/ref_option_traits.rs:16:5 + | +LL | fn trait_ret(&self) -> &Option<String>; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | help: change this to: `Option<&String>` + +error: aborting due to 2 previous errors + diff --git a/tests/ui/ref_option/ref_option_traits.rs b/tests/ui/ref_option/ref_option_traits.rs new file mode 100644 index 00000000000..5d5f113c83d --- /dev/null +++ b/tests/ui/ref_option/ref_option_traits.rs @@ -0,0 +1,37 @@ +//@no-rustfix: fixes are only done to traits, not the impls +//@revisions: private all +//@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/private +//@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/all + +#![allow(unused, clippy::all)] +#![warn(clippy::ref_option)] + +pub trait PubTrait { + fn pub_trait_opt(&self, a: &Option<Vec<u8>>); + fn pub_trait_ret(&self) -> &Option<Vec<u8>>; +} + +trait PrivateTrait { + fn trait_opt(&self, a: &Option<String>); + fn trait_ret(&self) -> &Option<String>; +} + +pub struct PubStruct; + +impl PubTrait for PubStruct { + fn pub_trait_opt(&self, a: &Option<Vec<u8>>) {} + fn pub_trait_ret(&self) -> &Option<Vec<u8>> { + panic!() + } +} + +struct PrivateStruct; + +impl PrivateTrait for PrivateStruct { + fn trait_opt(&self, a: &Option<String>) {} + fn trait_ret(&self) -> &Option<String> { + panic!() + } +} + +fn main() {} |