Suggest Option<&T> instead of &Option<T>

Author: nyurik

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

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)

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,

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,

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<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 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<String>) {}
+/// # struct Unit {}
+/// # impl Unit {
+/// fn bar(&self) -> &Option<String> { &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"]
+pubREF_OPTION,
+ nursery,
+"function signature uses `&Option<T>` instead of `Option<&T>`"
+}
+
 pubstructFunctions{
 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>forFunctions{
@@ -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,
+);
 }
 
 fncheck_item(&mutself,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);
 }
 }

clippy_lints/src/functions/ref_option.rs

@@ -0,0 +1,122 @@
+usecrate::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};
+
+fncheck_ty<'a>(cx:&LateContext<'a>,param:&rustc_hir::Ty<'a>,param_ty:Ty<'a>,fixes:&mutVec<(Span,String)>){
+iflet 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()
+ && letGenericArgKind::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())
+ && letSome(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,"..")
+),
+));
+}
+}
+
+fncheck_fn_sig<'a>(cx:&LateContext<'a>,decl:&FnDecl<'a>,span:Span,sig: ty::FnSig<'a>){
+letmut 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
+iflet 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)fncheck_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;
+}
+
+ifletFnKind::Closure = kind {
+// Compute the span of the closure parameters + return type if set
+let span = iflet hir::FnRetTy::Return(out_ty) = &decl.output{
+if decl.inputs.is_empty(){
+ out_ty.span
+}else{
+ span.with_hi(out_ty.span.hi())
+}
+}elseiflet(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);
+}elseif !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)fncheck_trait_item<'a>(
+cx:&LateContext<'a>,
+trait_item:&hir::TraitItem<'a>,
+avoid_breaking_exported_api:bool,
+){
+iflet 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);
+}
+}

tests/ui/ref_option/all/clippy.toml

@@ -0,0 +1 @@
+avoid-breaking-exported-api = false

tests/ui/ref_option/private/clippy.toml

@@ -0,0 +1 @@
+avoid-breaking-exported-api = true

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() {}