-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Suggest
Option<&T> instead of &Option<T>
- Loading branch information
Showing
13 changed files
with
566 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| 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::intravisit::FnKind; | ||
| use rustc_hir::{FnDecl, 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<'_>, decl: &FnDecl<'_>, span: Span) { | ||
| // dbg!(snippet(cx, span, "..")); | ||
| let mut fixes = Vec::new(); | ||
| // Check function arguments' types | ||
| for param in decl.inputs { | ||
| check_ty(cx, param, &mut fixes); | ||
| } | ||
| // Check return type | ||
| if let hir::FnRetTy::Return(ty) = &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, | ||
| span, | ||
| "it is more idiomatic to use `Option<&T>` instead of `&Option<T>`", | ||
| |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) { | ||
| 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.decl, sig.span); | ||
| } | ||
| } | ||
|
|
||
| pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, avoid_breaking_exported_api: bool) { | ||
| 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.decl, sig.span); | ||
| } | ||
| } | ||
|
|
||
| pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>, 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)) | ||
| { | ||
| check_fn_sig(cx, sig.decl, sig.span); | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn check_fn(cx: &LateContext<'_>, kind: FnKind<'_>, decl: &FnDecl<'_>, span: Span) { | ||
| 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; | ||
| }; | ||
|
|
||
| check_fn_sig(cx, decl, span); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| avoid-breaking-exported-api = false |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| avoid-breaking-exported-api = true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| //@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)] | ||
|
|
||
| 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<T>(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<u8>>) {} | ||
| pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} | ||
|
|
||
| pub fn pub_opt_string(a: Option<&String>) {} | ||
|
|
||
| pub trait PubTrait { | ||
| fn trait_opt(&self, a: Option<&Vec<u8>>); | ||
| fn trait_ret(&self) -> Option<&Vec<u8>>; | ||
| } | ||
|
|
||
| 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!() | ||
| } | ||
| } | ||
|
|
||
| fn lambdas() { | ||
| let s = Some("hello".to_string()); | ||
| let x = |a: Option<&String>| {}; | ||
| let x = |a: Option<&String>| -> Option<&String> { panic!() }; | ||
| } |
Oops, something went wrong.