diff --git a/CHANGELOG.md b/CHANGELOG.md index 682c3cfe0151..79039ae81f6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5814,6 +5814,7 @@ Released 2018-09-13 [`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_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref +[`ref_option_sig`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_sig [`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 [`renamed_function_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#renamed_function_params diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index aded9e276c4a..33f70d460758 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_SIG_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..94d046f70531 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_sig; 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_SIG, + 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_SIG, ]); 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_sig::check_item(cx, item); } 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_sig::check_impl_item(cx, item); } 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_sig::check_trait_item(cx, item); } } diff --git a/clippy_lints/src/functions/ref_option_sig.rs b/clippy_lints/src/functions/ref_option_sig.rs new file mode 100644 index 000000000000..dafbd7bb84f6 --- /dev/null +++ b/clippy_lints/src/functions/ref_option_sig.rs @@ -0,0 +1,60 @@ +use crate::functions::hir::GenericArg; +use crate::functions::REF_OPTION_SIG; +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, TyKind}; +use rustc_lint::LateContext; +use rustc_span::{sym, Span}; + +fn check_fn_sig(cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, sig_span: Span) { + let mut fixes = Vec::new(); + for param in decl.inputs { + if let TyKind::Ref(_, MutTy { ty, .. }) = param.kind + // TODO: is this the right way to check if ty is an Option? + && 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? + fixes.push((param.span, format!("Option<&{}>", snippet(cx, opt_ty.span, "..")))); + } + } + + if !fixes.is_empty() { + span_lint_and_then( + cx, + REF_OPTION_SIG, + sig_span, + "it is more idiomatic to use `Option<&T>` instead of `&Option`", + |diag| { + diag.multipart_suggestion("change this to", fixes, Applicability::Unspecified); + }, + ); + } +} + +pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { + if let hir::ItemKind::Fn(ref sig, _, _) = item.kind { + check_fn_sig(cx, sig.decl, sig.span); + } +} + +pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { + if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind { + check_fn_sig(cx, sig.decl, sig.span); + } +} + +pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>) { + if let hir::TraitItemKind::Fn(ref sig, _) = trait_item.kind { + check_fn_sig(cx, sig.decl, sig.span); + } +} diff --git a/tests/ui/ref_option_sig.fixed b/tests/ui/ref_option_sig.fixed new file mode 100644 index 000000000000..b364aa626fdc --- /dev/null +++ b/tests/ui/ref_option_sig.fixed @@ -0,0 +1,25 @@ +#![allow(unused, clippy::all)] +#![warn(clippy::ref_option_sig)] + +fn main() {} + +fn opt_u8(a: Option<&u8>) {} +fn opt_gen(a: Option<&T>) {} +fn opt_string(a: Option<&String>) {} +fn mult_string(a: Option<&String>, b: Option<&Vec>) {} + +pub fn pub_opt_string(a: Option<&String>) {} + +pub trait HasOpt { + fn trait_opt(&self, a: Option<&Vec>); +} + +trait PrivateHasOpt { + fn private_trait_opt(&self, a: Option<&Option>); +} + +pub struct UnitOpt; + +impl UnitOpt { + pub fn opt_params(&self, a: Option<&()>) {} +} diff --git a/tests/ui/ref_option_sig.rs b/tests/ui/ref_option_sig.rs new file mode 100644 index 000000000000..69c53afcd35d --- /dev/null +++ b/tests/ui/ref_option_sig.rs @@ -0,0 +1,25 @@ +#![allow(unused, clippy::all)] +#![warn(clippy::ref_option_sig)] + +fn main() {} + +fn opt_u8(a: &Option) {} +fn opt_gen(a: &Option) {} +fn opt_string(a: &Option) {} +fn mult_string(a: &Option, b: &Option>) {} + +pub fn pub_opt_string(a: &Option) {} + +pub trait HasOpt { + fn trait_opt(&self, a: &Option>); +} + +trait PrivateHasOpt { + fn private_trait_opt(&self, a: &Option>); +} + +pub struct UnitOpt; + +impl UnitOpt { + pub fn opt_params(&self, a: &Option<()>) {} +} diff --git a/tests/ui/ref_option_sig.stderr b/tests/ui/ref_option_sig.stderr new file mode 100644 index 000000000000..42ca2a707ba1 --- /dev/null +++ b/tests/ui/ref_option_sig.stderr @@ -0,0 +1,72 @@ +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option_sig.rs:6:1 + | +LL | fn opt_u8(a: &Option) {} + | ^^^^^^^^^^^^^-----------^ + | | + | help: change this to: `Option<&u8>` + | + = note: `-D clippy::ref-option-sig` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ref_option_sig)]` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option_sig.rs:7: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_sig.rs:8: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_sig.rs:9: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_sig.rs:11: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_sig.rs:14: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_sig.rs:18:5 + | +LL | fn private_trait_opt(&self, a: &Option>); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------------^^ + | | + | help: change this to: `Option<&Option>` + +error: it is more idiomatic to use `Option<&T>` instead of `&Option` + --> tests/ui/ref_option_sig.rs:24:5 + | +LL | pub fn opt_params(&self, a: &Option<()>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^ + | | + | help: change this to: `Option<&()>` + +error: aborting due to 8 previous errors +