From 73a16c10dba19efbd5b3363be5f1d37a60d6436c Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 3 Sep 2024 00:21:02 -0400 Subject: [PATCH 1/3] Suggest `Option<&T>` instead of `&Option` --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 1 + clippy_config/src/conf.rs | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/functions/mod.rs | 57 ++++++ clippy_lints/src/functions/ref_option.rs | 122 +++++++++++++ tests/ui/ref_option/all/clippy.toml | 1 + tests/ui/ref_option/private/clippy.toml | 1 + tests/ui/ref_option/ref_option.all.fixed | 62 +++++++ tests/ui/ref_option/ref_option.all.stderr | 162 ++++++++++++++++++ tests/ui/ref_option/ref_option.private.fixed | 62 +++++++ tests/ui/ref_option/ref_option.private.stderr | 108 ++++++++++++ tests/ui/ref_option/ref_option.rs | 62 +++++++ .../ref_option/ref_option_traits.all.stderr | 37 ++++ .../ref_option_traits.private.stderr | 21 +++ tests/ui/ref_option/ref_option_traits.rs | 37 ++++ 16 files changed, 736 insertions(+) create mode 100644 clippy_lints/src/functions/ref_option.rs create mode 100644 tests/ui/ref_option/all/clippy.toml create mode 100644 tests/ui/ref_option/private/clippy.toml create mode 100644 tests/ui/ref_option/ref_option.all.fixed create mode 100644 tests/ui/ref_option/ref_option.all.stderr create mode 100644 tests/ui/ref_option/ref_option.private.fixed create mode 100644 tests/ui/ref_option/ref_option.private.stderr create mode 100644 tests/ui/ref_option/ref_option.rs create mode 100644 tests/ui/ref_option/ref_option_traits.all.stderr create mode 100644 tests/ui/ref_option/ref_option_traits.private.stderr create mode 100644 tests/ui/ref_option/ref_option_traits.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 41a86e8ce510..5d253d52531e 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 91159bc79c51..07a56fb33df1 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 757620341ccc..e4e2c97fdc1d 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 2b229d2fe6ac..9cec672beb00 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 ba8a459c917e..cfa63d3befce 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,50 @@ declare_clippy_lint! { "renamed function parameters in trait implementation" } +declare_clippy_lint! { + /// ### What it does + /// Warns when a function signature uses `&Option` instead of `Option<&T>`. + /// + /// ### Why is this bad? + /// More flexibility, better memory optimization, and more idiomatic Rust code. + /// + /// `&Option` 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` in order to return it. + /// At a lower level `&Option` points to memory that has `presence` bit flag + value, + /// whereas `Option<&T>` is always optimized to a single pointer. + /// + /// 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) {} + /// # struct Unit {} + /// # impl Unit { + /// fn bar(&self) -> &Option { &None } + /// # } + /// ``` + /// Use instead: + /// ```no_run + /// // caller should use foo(opt.as_ref()) + /// fn foo(a: Option<&String>) {} + /// # struct Unit {} + /// # impl Unit { + /// fn bar(&self) -> Option<&String> { None } + /// # } + /// ``` + #[clippy::version = "1.82.0"] + pub REF_OPTION, + nursery, + "function signature uses `&Option` instead of `Option<&T>`" +} + pub struct Functions { too_many_arguments_threshold: u64, too_many_lines_threshold: u64, @@ -437,6 +482,7 @@ impl_lint_pass!(Functions => [ MISNAMED_GETTERS, IMPL_TRAIT_IN_PARAMS, RENAMED_FUNCTION_PARAMS, + REF_OPTION, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -455,6 +501,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 +531,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 000000000000..373ecd74cb37 --- /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`", + |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 000000000000..cda8d17eed44 --- /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 000000000000..5f304987aa94 --- /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 000000000000..47781a97c983 --- /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(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>) {} +fn ret_box<'a>() -> Option<&'a Box> { + panic!() +} + +pub fn pub_opt_string(a: Option<&String>) {} +pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec>) {} + +pub trait PubTrait { + fn pub_trait_opt(&self, a: Option<&Vec>); + fn pub_trait_ret(&self) -> Option<&Vec>; +} + +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) {} +pub fn pub_mut_u8(a: &mut Option) {} + +// might be good to catch in the future +fn mut_u8_ref(a: &mut &Option) {} +pub fn pub_mut_u8_ref(a: &mut &Option) {} +fn lambdas() { + // Not handled for now, not sure if we should + let x = |a: &Option| {}; + let x = |a: &Option| -> &Option { 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 000000000000..b4c69ac62962 --- /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` + --> tests/ui/ref_option/ref_option.rs:8:1 + | +LL | fn opt_u8(a: &Option) {} + | ^^^^^^^^^^^^^-----------^^^^ + | | + | 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` + --> tests/ui/ref_option/ref_option.rs:9:1 + | +LL | fn opt_gen(a: &Option) {} + | ^^^^^^^^^^^^^^^^^----------^^^^ + | | + | help: change this to: `Option<&T>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:10:1 + | +LL | fn opt_string(a: &std::option::Option) {} + | ^^^^^^^^^^^^^^^^^----------------------------^^^^ + | | + | help: change this to: `std::option::Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:11:1 + | +LL | fn ret_string<'a>(p: &'a str) -> &'a Option { + | ^ -------------- help: change this to: `Option<&'a u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:14:1 + | +LL | fn ret_string_static() -> &'static Option { + | ^ ------------------- help: change this to: `Option<&'static u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:17:1 + | +LL | fn mult_string(a: &Option, b: &Option>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL | fn mult_string(a: Option<&String>, b: Option<&Vec>) {} + | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:18:1 + | +LL | fn ret_box<'a>() -> &'a Option> { + | ^ ------------------- help: change this to: `Option<&'a Box>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:22:1 + | +LL | pub fn pub_opt_string(a: &Option) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^---------------^^^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:23:1 + | +LL | pub fn pub_mult_string(a: &Option, b: &Option>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL | pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec>) {} + | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:26:5 + | +LL | fn pub_trait_opt(&self, a: &Option>); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^^ + | | + | help: change this to: `Option<&Vec>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:27:5 + | +LL | fn pub_trait_ret(&self) -> &Option>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^ + | | + | help: change this to: `Option<&Vec>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:31:5 + | +LL | fn trait_opt(&self, a: &Option); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:32:5 + | +LL | fn trait_ret(&self) -> &Option; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> 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` + --> tests/ui/ref_option/ref_option.rs:39:5 + | +LL | pub fn pub_opt_ret(&self) -> &Option { + | ^ --------------- help: change this to: `Option<&String>` + | _____| + | | +LL | | panic!() +LL | | } + | |_____^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> 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` + --> tests/ui/ref_option/ref_option.rs:44:5 + | +LL | fn private_opt_ret(&self) -> &Option { + | ^ --------------- 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 000000000000..8c42556e9b0d --- /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(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>) {} +fn ret_box<'a>() -> Option<&'a Box> { + panic!() +} + +pub fn pub_opt_string(a: &Option) {} +pub fn pub_mult_string(a: &Option, b: &Option>) {} + +pub trait PubTrait { + fn pub_trait_opt(&self, a: &Option>); + fn pub_trait_ret(&self) -> &Option>; +} + +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 { + 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) {} +pub fn pub_mut_u8(a: &mut Option) {} + +// might be good to catch in the future +fn mut_u8_ref(a: &mut &Option) {} +pub fn pub_mut_u8_ref(a: &mut &Option) {} +fn lambdas() { + // Not handled for now, not sure if we should + let x = |a: &Option| {}; + let x = |a: &Option| -> &Option { 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 000000000000..17c90536da34 --- /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` + --> tests/ui/ref_option/ref_option.rs:8:1 + | +LL | fn opt_u8(a: &Option) {} + | ^^^^^^^^^^^^^-----------^^^^ + | | + | 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` + --> tests/ui/ref_option/ref_option.rs:9:1 + | +LL | fn opt_gen(a: &Option) {} + | ^^^^^^^^^^^^^^^^^----------^^^^ + | | + | help: change this to: `Option<&T>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:10:1 + | +LL | fn opt_string(a: &std::option::Option) {} + | ^^^^^^^^^^^^^^^^^----------------------------^^^^ + | | + | help: change this to: `std::option::Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:11:1 + | +LL | fn ret_string<'a>(p: &'a str) -> &'a Option { + | ^ -------------- help: change this to: `Option<&'a u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:14:1 + | +LL | fn ret_string_static() -> &'static Option { + | ^ ------------------- help: change this to: `Option<&'static u8>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:17:1 + | +LL | fn mult_string(a: &Option, b: &Option>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: change this to + | +LL | fn mult_string(a: Option<&String>, b: Option<&Vec>) {} + | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:18:1 + | +LL | fn ret_box<'a>() -> &'a Option> { + | ^ ------------------- help: change this to: `Option<&'a Box>` + | _| + | | +LL | | panic!() +LL | | } + | |_^ + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:31:5 + | +LL | fn trait_opt(&self, a: &Option); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:32:5 + | +LL | fn trait_ret(&self) -> &Option; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> 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` + --> tests/ui/ref_option/ref_option.rs:44:5 + | +LL | fn private_opt_ret(&self) -> &Option { + | ^ --------------- 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 000000000000..05251bcf12cd --- /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) {} +fn opt_gen(a: &Option) {} +fn opt_string(a: &std::option::Option) {} +fn ret_string<'a>(p: &'a str) -> &'a Option { + panic!() +} +fn ret_string_static() -> &'static Option { + panic!() +} +fn mult_string(a: &Option, b: &Option>) {} +fn ret_box<'a>() -> &'a Option> { + panic!() +} + +pub fn pub_opt_string(a: &Option) {} +pub fn pub_mult_string(a: &Option, b: &Option>) {} + +pub trait PubTrait { + fn pub_trait_opt(&self, a: &Option>); + fn pub_trait_ret(&self) -> &Option>; +} + +trait PrivateTrait { + fn trait_opt(&self, a: &Option); + fn trait_ret(&self) -> &Option; +} + +pub struct PubStruct; + +impl PubStruct { + pub fn pub_opt_params(&self, a: &Option<()>) {} + pub fn pub_opt_ret(&self) -> &Option { + panic!() + } + + fn private_opt_params(&self, a: &Option<()>) {} + fn private_opt_ret(&self) -> &Option { + panic!() + } +} + +// valid, don't change +fn mut_u8(a: &mut Option) {} +pub fn pub_mut_u8(a: &mut Option) {} + +// might be good to catch in the future +fn mut_u8_ref(a: &mut &Option) {} +pub fn pub_mut_u8_ref(a: &mut &Option) {} +fn lambdas() { + // Not handled for now, not sure if we should + let x = |a: &Option| {}; + let x = |a: &Option| -> &Option { 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 000000000000..a9967168c12e --- /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` + --> tests/ui/ref_option/ref_option_traits.rs:10:5 + | +LL | fn pub_trait_opt(&self, a: &Option>); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^^ + | | + | help: change this to: `Option<&Vec>` + | + = 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` + --> tests/ui/ref_option/ref_option_traits.rs:11:5 + | +LL | fn pub_trait_ret(&self) -> &Option>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^ + | | + | help: change this to: `Option<&Vec>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option_traits.rs:15:5 + | +LL | fn trait_opt(&self, a: &Option); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | help: change this to: `Option<&String>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option_traits.rs:16:5 + | +LL | fn trait_ret(&self) -> &Option; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | 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 000000000000..36d0833af8a2 --- /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` + --> tests/ui/ref_option/ref_option_traits.rs:15:5 + | +LL | fn trait_opt(&self, a: &Option); + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^^ + | | + | 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` + --> tests/ui/ref_option/ref_option_traits.rs:16:5 + | +LL | fn trait_ret(&self) -> &Option; + | ^^^^^^^^^^^^^^^^^^^^^^^---------------^ + | | + | 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 000000000000..5d5f113c83db --- /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>); + fn pub_trait_ret(&self) -> &Option>; +} + +trait PrivateTrait { + fn trait_opt(&self, a: &Option); + fn trait_ret(&self) -> &Option; +} + +pub struct PubStruct; + +impl PubTrait for PubStruct { + fn pub_trait_opt(&self, a: &Option>) {} + fn pub_trait_ret(&self) -> &Option> { + panic!() + } +} + +struct PrivateStruct; + +impl PrivateTrait for PrivateStruct { + fn trait_opt(&self, a: &Option) {} + fn trait_ret(&self) -> &Option { + panic!() + } +} + +fn main() {} From 38295a0d8acf04a55567cbc1bfa3eddd45e5e702 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 28 Sep 2024 13:01:51 -0400 Subject: [PATCH 2/3] update docs --- clippy_lints/src/functions/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index cfa63d3befce..50b3d039ed3a 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -410,8 +410,9 @@ declare_clippy_lint! { /// `&Option` 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` in order to return it. - /// At a lower level `&Option` points to memory that has `presence` bit flag + value, - /// whereas `Option<&T>` is always optimized to a single pointer. + /// At a lower level, `&Option` 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. From 10e02cf8e3e8b46d3bc7f8194c9224755604eac4 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 28 Sep 2024 13:17:29 -0400 Subject: [PATCH 3/3] suggest &str --- clippy_lints/src/functions/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index 50b3d039ed3a..203c3ce9eec0 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -432,8 +432,10 @@ declare_clippy_lint! { /// ``` /// Use instead: /// ```no_run - /// // caller should use foo(opt.as_ref()) - /// fn foo(a: Option<&String>) {} + /// // 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 }