From 6b40c388c2fd0e17862778d275c6d63f72525294 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 3 Sep 2024 00:21:02 -0400 Subject: [PATCH] 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 | 25 ++++ clippy_lints/src/functions/ref_option.rs | 89 +++++++++++ tests/ui/ref_option/disabled/clippy.toml | 1 + tests/ui/ref_option/enabled/clippy.toml | 1 + tests/ui/ref_option/ref_option.disabled.fixed | 49 ++++++ .../ui/ref_option/ref_option.disabled.stderr | 139 ++++++++++++++++++ tests/ui/ref_option/ref_option.enabled.fixed | 49 ++++++ tests/ui/ref_option/ref_option.enabled.stderr | 88 +++++++++++ tests/ui/ref_option/ref_option.rs | 49 ++++++ 13 files changed, 494 insertions(+) create mode 100644 clippy_lints/src/functions/ref_option.rs create mode 100644 tests/ui/ref_option/disabled/clippy.toml create mode 100644 tests/ui/ref_option/enabled/clippy.toml create mode 100644 tests/ui/ref_option/ref_option.disabled.fixed create mode 100644 tests/ui/ref_option/ref_option.disabled.stderr create mode 100644 tests/ui/ref_option/ref_option.enabled.fixed create mode 100644 tests/ui/ref_option/ref_option.enabled.stderr create mode 100644 tests/ui/ref_option/ref_option.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index fc51694ff57f..e1e78a63c4e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5814,6 +5814,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 78348797588a..9c453dabd26c 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 a6f1b958bfb1..7ac8f1d78b82 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 e478ab330e8b..7d4c7ff3c03d 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 0f48941783b2..ae46b633235b 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,26 @@ 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. + /// See [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE) + /// ### Example + /// ```no_run + /// fn foo(a: &Option) {} + /// ``` + /// Use instead: + /// ```no_run + /// fn foo(a: Option<&String>) {} + /// ``` + #[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 +458,7 @@ impl_lint_pass!(Functions => [ MISNAMED_GETTERS, IMPL_TRAIT_IN_PARAMS, RENAMED_FUNCTION_PARAMS, + REF_OPTION, ]); impl<'tcx> LateLintPass<'tcx> for Functions { @@ -460,6 +482,7 @@ impl<'tcx> LateLintPass<'tcx> for Functions { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { must_use::check_item(cx, item); result::check_item(cx, item, self.large_error_threshold); + ref_option::check_item(cx, item, self.avoid_breaking_exported_api); } fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) { @@ -467,6 +490,7 @@ impl<'tcx> LateLintPass<'tcx> for Functions { result::check_impl_item(cx, item, self.large_error_threshold); impl_trait_in_params::check_impl_item(cx, item); renamed_function_params::check_impl_item(cx, item, &self.trait_ids); + ref_option::check_impl_item(cx, item, self.avoid_breaking_exported_api); } fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { @@ -475,5 +499,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..b8551fdebc75 --- /dev/null +++ b/clippy_lints/src/functions/ref_option.rs @@ -0,0 +1,89 @@ +use crate::functions::hir::GenericArg; +use crate::functions::REF_OPTION; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::{GenericArgs, ImplItem, MutTy, QPath, TraitItem, Ty, TyKind}; +use rustc_lint::LateContext; +use rustc_span::{sym, Span}; + +fn check_ty(cx: &LateContext<'_>, param: &Ty<'_>, fixes: &mut Vec<(Span, String)>) { + // TODO: is this the right way to check if ty is an Option? + if let TyKind::Ref(lifetime, MutTy { ty, .. }) = param.kind + && let TyKind::Path(QPath::Resolved(_, path)) = ty.kind + && path.segments.len() == 1 + && let seg = &path.segments[0] + && seg.ident.name == sym::Option + // check if option contains a regular type, not a reference + // TODO: Should this instead just check that opt_ty is a TyKind::Path? + && let Some(GenericArgs { args: [GenericArg::Type(opt_ty)], .. }) = seg.args + && !matches!(opt_ty.kind, TyKind::Ref(..)) + { + // FIXME: Should this use the Option path from the original type? + // FIXME: Should reference be added in some other way to the snippet? + // FIXME: What should the lifetime be of the reference in Option<&T>? + let lifetime = snippet(cx, lifetime.ident.span, ".."); + fixes.push(( + param.span, + format!( + "Option<&{}{}{}>", + lifetime, + if lifetime.is_empty() { "" } else { " " }, + snippet(cx, opt_ty.span, "..") + ), + )); + } +} + +fn check_fn_sig(cx: &LateContext<'_>, sig: &hir::FnSig<'_>) { + let mut fixes = Vec::new(); + // Check function arguments' types + for param in sig.decl.inputs { + check_ty(cx, param, &mut fixes); + } + // Check return type + if let hir::FnRetTy::Return(ty) = &sig.decl.output { + check_ty(cx, ty, &mut fixes); + } + if !fixes.is_empty() { + // FIXME: These changes will often result in broken code that will need to be fixed manually + // What should the applicability be? + span_lint_and_then( + cx, + REF_OPTION, + sig.span, + "it is more idiomatic to use `Option<&T>` instead of `&Option`", + |diag| { + diag.multipart_suggestion("change this to", fixes, Applicability::HasPlaceholders); + }, + ); + } +} + +pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>, avoid_breaking_exported_api: bool) { + // dbg!(avoid_breaking_exported_api); + if let hir::ItemKind::Fn(ref sig, _, _) = item.kind + && !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(item.owner_id.def_id)) + { + check_fn_sig(cx, sig); + } +} + +pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, avoid_breaking_exported_api: bool) { + // dbg!(avoid_breaking_exported_api); + if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind + && !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(impl_item.owner_id.def_id)) + { + check_fn_sig(cx, sig); + } +} + +pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>, avoid_breaking_exported_api: bool) { + // dbg!(avoid_breaking_exported_api); + 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)) + { + check_fn_sig(cx, sig); + } +} diff --git a/tests/ui/ref_option/disabled/clippy.toml b/tests/ui/ref_option/disabled/clippy.toml new file mode 100644 index 000000000000..cda8d17eed44 --- /dev/null +++ b/tests/ui/ref_option/disabled/clippy.toml @@ -0,0 +1 @@ +avoid-breaking-exported-api = false diff --git a/tests/ui/ref_option/enabled/clippy.toml b/tests/ui/ref_option/enabled/clippy.toml new file mode 100644 index 000000000000..5f304987aa94 --- /dev/null +++ b/tests/ui/ref_option/enabled/clippy.toml @@ -0,0 +1 @@ +avoid-breaking-exported-api = true diff --git a/tests/ui/ref_option/ref_option.disabled.fixed b/tests/ui/ref_option/ref_option.disabled.fixed new file mode 100644 index 000000000000..d8faf7ec24a2 --- /dev/null +++ b/tests/ui/ref_option/ref_option.disabled.fixed @@ -0,0 +1,49 @@ +//@revisions: enabled disabled +//@[enabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/enabled +//@[disabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/disabled + +#![allow(unused, clippy::all)] +#![warn(clippy::ref_option)] + +fn main() {} + +// FIXME: Using `&None` instead of `unimplemented!()` is a better test, but it results in an error: +// `after rustfix is applied, all errors should be gone, but weren't` + +fn opt_u8(a: Option<&u8>) {} +fn opt_gen(a: Option<&T>) {} +fn opt_string(a: Option<&String>) {} +fn ret_string<'a>(p: &'a String) -> Option<&'a u8> { + panic!() +} +fn ret_string_static() -> Option<&'static u8> { + panic!() +} +fn mult_string(a: Option<&String>, b: Option<&Vec>) {} +pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec>) {} + +pub fn pub_opt_string(a: Option<&String>) {} + +pub trait PubTrait { + fn trait_opt(&self, a: Option<&Vec>); + fn trait_ret(&self) -> Option<&Vec>; +} + +trait PrivateTrait { + fn private_trait_opt(&self, a: Option<&String>); + fn private_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!() + } +} diff --git a/tests/ui/ref_option/ref_option.disabled.stderr b/tests/ui/ref_option/ref_option.disabled.stderr new file mode 100644 index 000000000000..861d21d329c2 --- /dev/null +++ b/tests/ui/ref_option/ref_option.disabled.stderr @@ -0,0 +1,139 @@ +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:13: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:14: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:15:1 + | +LL | fn 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:16:1 + | +LL | fn ret_string<'a>(p: &'a String) -> &'a Option { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-------------- + | | + | help: change this to: `Option<&'a u8>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:19:1 + | +LL | fn ret_string_static() -> &'static Option { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^------------------- + | | + | help: change this to: `Option<&'static u8>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:22: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: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:25: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:28:5 + | +LL | fn 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:29:5 + | +LL | fn 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:33:5 + | +LL | fn private_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:34:5 + | +LL | fn private_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:40: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:41:5 + | +LL | pub fn pub_opt_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:45: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:46:5 + | +LL | fn private_opt_ret(&self) -> &Option { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------- + | | + | help: change this to: `Option<&String>` + +error: aborting due to 16 previous errors + diff --git a/tests/ui/ref_option/ref_option.enabled.fixed b/tests/ui/ref_option/ref_option.enabled.fixed new file mode 100644 index 000000000000..0ff1e7a4cb9d --- /dev/null +++ b/tests/ui/ref_option/ref_option.enabled.fixed @@ -0,0 +1,49 @@ +//@revisions: enabled disabled +//@[enabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/enabled +//@[disabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/disabled + +#![allow(unused, clippy::all)] +#![warn(clippy::ref_option)] + +fn main() {} + +// FIXME: Using `&None` instead of `unimplemented!()` is a better test, but it results in an error: +// `after rustfix is applied, all errors should be gone, but weren't` + +fn opt_u8(a: Option<&u8>) {} +fn opt_gen(a: Option<&T>) {} +fn opt_string(a: Option<&String>) {} +fn ret_string<'a>(p: &'a String) -> Option<&'a u8> { + panic!() +} +fn ret_string_static() -> Option<&'static u8> { + panic!() +} +fn mult_string(a: Option<&String>, b: Option<&Vec>) {} +pub fn pub_mult_string(a: &Option, b: &Option>) {} + +pub fn pub_opt_string(a: &Option) {} + +pub trait PubTrait { + fn trait_opt(&self, a: &Option>); + fn trait_ret(&self) -> &Option>; +} + +trait PrivateTrait { + fn private_trait_opt(&self, a: Option<&String>); + fn private_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!() + } +} diff --git a/tests/ui/ref_option/ref_option.enabled.stderr b/tests/ui/ref_option/ref_option.enabled.stderr new file mode 100644 index 000000000000..5644e6b42431 --- /dev/null +++ b/tests/ui/ref_option/ref_option.enabled.stderr @@ -0,0 +1,88 @@ +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:13: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:14: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:15:1 + | +LL | fn 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:16:1 + | +LL | fn ret_string<'a>(p: &'a String) -> &'a Option { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-------------- + | | + | help: change this to: `Option<&'a u8>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:19:1 + | +LL | fn ret_string_static() -> &'static Option { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^------------------- + | | + | help: change this to: `Option<&'static u8>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option/ref_option.rs:22: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:33:5 + | +LL | fn private_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:34:5 + | +LL | fn private_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:45: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:46:5 + | +LL | fn private_opt_ret(&self) -> &Option { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------- + | | + | help: change this to: `Option<&String>` + +error: aborting due to 10 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..c15cdb619573 --- /dev/null +++ b/tests/ui/ref_option/ref_option.rs @@ -0,0 +1,49 @@ +//@revisions: enabled disabled +//@[enabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/enabled +//@[disabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/disabled + +#![allow(unused, clippy::all)] +#![warn(clippy::ref_option)] + +fn main() {} + +// FIXME: Using `&None` instead of `unimplemented!()` is a better test, but it results in an error: +// `after rustfix is applied, all errors should be gone, but weren't` + +fn opt_u8(a: &Option) {} +fn opt_gen(a: &Option) {} +fn opt_string(a: &Option) {} +fn ret_string<'a>(p: &'a String) -> &'a Option { + panic!() +} +fn ret_string_static() -> &'static Option { + panic!() +} +fn mult_string(a: &Option, b: &Option>) {} +pub fn pub_mult_string(a: &Option, b: &Option>) {} + +pub fn pub_opt_string(a: &Option) {} + +pub trait PubTrait { + fn trait_opt(&self, a: &Option>); + fn trait_ret(&self) -> &Option>; +} + +trait PrivateTrait { + fn private_trait_opt(&self, a: &Option); + fn private_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!() + } +}