-
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
494 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,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<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) { | ||
| // 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); | ||
| } | ||
| } |
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,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<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!() | ||
| } | ||
| } |
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,139 @@ | ||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:13:1 | ||
| | | ||
| LL | fn opt_u8(a: &Option<u8>) {} | ||
| | ^^^^^^^^^^^^^-----------^ | ||
| | | | ||
| | 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<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:14:1 | ||
| | | ||
| LL | fn opt_gen<T>(a: &Option<T>) {} | ||
| | ^^^^^^^^^^^^^^^^^----------^ | ||
| | | | ||
| | help: change this to: `Option<&T>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:15:1 | ||
| | | ||
| LL | fn opt_string(a: &Option<String>) {} | ||
| | ^^^^^^^^^^^^^^^^^---------------^ | ||
| | | | ||
| | help: change this to: `Option<&String>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:16:1 | ||
| | | ||
| LL | fn ret_string<'a>(p: &'a String) -> &'a Option<u8> { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-------------- | ||
| | | | ||
| | help: change this to: `Option<&'a u8>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:19:1 | ||
| | | ||
| LL | fn ret_string_static() -> &'static Option<u8> { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^------------------- | ||
| | | | ||
| | help: change this to: `Option<&'static u8>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:22:1 | ||
| | | ||
| LL | fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| help: change this to | ||
| | | ||
| LL | fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} | ||
| | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:23:1 | ||
| | | ||
| LL | pub fn pub_mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {} | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| help: change this to | ||
| | | ||
| LL | pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {} | ||
| | ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:25:1 | ||
| | | ||
| LL | pub fn pub_opt_string(a: &Option<String>) {} | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^---------------^ | ||
| | | | ||
| | help: change this to: `Option<&String>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:28:5 | ||
| | | ||
| LL | fn trait_opt(&self, a: &Option<Vec<u8>>); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^----------------^^ | ||
| | | | ||
| | help: change this to: `Option<&Vec<u8>>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:29:5 | ||
| | | ||
| LL | fn trait_ret(&self) -> &Option<Vec<u8>>; | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^----------------^ | ||
| | | | ||
| | help: change this to: `Option<&Vec<u8>>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:33:5 | ||
| | | ||
| LL | fn private_trait_opt(&self, a: &Option<String>); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------^^ | ||
| | | | ||
| | help: change this to: `Option<&String>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:34:5 | ||
| | | ||
| LL | fn private_trait_ret(&self) -> &Option<String>; | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------^ | ||
| | | | ||
| | help: change this to: `Option<&String>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> 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<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:41:5 | ||
| | | ||
| LL | pub fn pub_opt_ret(&self) -> &Option<String> { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------- | ||
| | | | ||
| | help: change this to: `Option<&String>` | ||
|
|
||
| error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>` | ||
| --> 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<T>` | ||
| --> tests/ui/ref_option/ref_option.rs:46:5 | ||
| | | ||
| LL | fn private_opt_ret(&self) -> &Option<String> { | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------- | ||
| | | | ||
| | help: change this to: `Option<&String>` | ||
|
|
||
| error: aborting due to 16 previous errors | ||
|
|
Oops, something went wrong.