Skip to content

Commit 6b40c38

Browse files
committed
Suggest Option<&T> instead of &Option<T>
1 parent e8ba5d1 commit 6b40c38

File tree

13 files changed

+494
-0
lines changed

13 files changed

+494
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5814,6 +5814,7 @@ Released 2018-09-13
58145814
[`ref_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr
58155815
[`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference
58165816
[`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref
5817+
[`ref_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option
58175818
[`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref
58185819
[`ref_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_patterns
58195820
[`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro

book/src/lint_configuration.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,7 @@ Suppress lints whenever the suggested change would cause breakage for other crat
353353
* [`rc_buffer`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer)
354354
* [`rc_mutex`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex)
355355
* [`redundant_allocation`](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation)
356+
* [`ref_option`](https://rust-lang.github.io/rust-clippy/master/index.html#ref_option)
356357
* [`single_call_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#single_call_fn)
357358
* [`trivially_copy_pass_by_ref`](https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref)
358359
* [`unnecessary_box_returns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns)

clippy_config/src/conf.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,7 @@ define_Conf! {
378378
rc_buffer,
379379
rc_mutex,
380380
redundant_allocation,
381+
ref_option,
381382
single_call_fn,
382383
trivially_copy_pass_by_ref,
383384
unnecessary_box_returns,

clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
202202
crate::functions::MUST_USE_CANDIDATE_INFO,
203203
crate::functions::MUST_USE_UNIT_INFO,
204204
crate::functions::NOT_UNSAFE_PTR_ARG_DEREF_INFO,
205+
crate::functions::REF_OPTION_INFO,
205206
crate::functions::RENAMED_FUNCTION_PARAMS_INFO,
206207
crate::functions::RESULT_LARGE_ERR_INFO,
207208
crate::functions::RESULT_UNIT_ERR_INFO,

clippy_lints/src/functions/mod.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ mod impl_trait_in_params;
22
mod misnamed_getters;
33
mod must_use;
44
mod not_unsafe_ptr_arg_deref;
5+
mod ref_option;
56
mod renamed_function_params;
67
mod result;
78
mod too_many_arguments;
@@ -399,6 +400,26 @@ declare_clippy_lint! {
399400
"renamed function parameters in trait implementation"
400401
}
401402

403+
declare_clippy_lint! {
404+
/// ### What it does
405+
/// Warns when a function signature uses `&Option<T>` instead of `Option<&T>`.
406+
/// ### Why is this bad?
407+
/// More flexibility, better memory optimization, and more idiomatic Rust code.
408+
/// See [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE)
409+
/// ### Example
410+
/// ```no_run
411+
/// fn foo(a: &Option<String>) {}
412+
/// ```
413+
/// Use instead:
414+
/// ```no_run
415+
/// fn foo(a: Option<&String>) {}
416+
/// ```
417+
#[clippy::version = "1.82.0"]
418+
pub REF_OPTION,
419+
nursery,
420+
"function signature uses `&Option<T>` instead of `Option<&T>`"
421+
}
422+
402423
pub struct Functions {
403424
too_many_arguments_threshold: u64,
404425
too_many_lines_threshold: u64,
@@ -437,6 +458,7 @@ impl_lint_pass!(Functions => [
437458
MISNAMED_GETTERS,
438459
IMPL_TRAIT_IN_PARAMS,
439460
RENAMED_FUNCTION_PARAMS,
461+
REF_OPTION,
440462
]);
441463

442464
impl<'tcx> LateLintPass<'tcx> for Functions {
@@ -460,13 +482,15 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
460482
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
461483
must_use::check_item(cx, item);
462484
result::check_item(cx, item, self.large_error_threshold);
485+
ref_option::check_item(cx, item, self.avoid_breaking_exported_api);
463486
}
464487

465488
fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) {
466489
must_use::check_impl_item(cx, item);
467490
result::check_impl_item(cx, item, self.large_error_threshold);
468491
impl_trait_in_params::check_impl_item(cx, item);
469492
renamed_function_params::check_impl_item(cx, item, &self.trait_ids);
493+
ref_option::check_impl_item(cx, item, self.avoid_breaking_exported_api);
470494
}
471495

472496
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
@@ -475,5 +499,6 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
475499
must_use::check_trait_item(cx, item);
476500
result::check_trait_item(cx, item, self.large_error_threshold);
477501
impl_trait_in_params::check_trait_item(cx, item, self.avoid_breaking_exported_api);
502+
ref_option::check_trait_item(cx, item, self.avoid_breaking_exported_api);
478503
}
479504
}
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
use crate::functions::hir::GenericArg;
2+
use crate::functions::REF_OPTION;
3+
use clippy_utils::diagnostics::span_lint_and_then;
4+
use clippy_utils::source::snippet;
5+
use rustc_errors::Applicability;
6+
use rustc_hir as hir;
7+
use rustc_hir::{GenericArgs, ImplItem, MutTy, QPath, TraitItem, Ty, TyKind};
8+
use rustc_lint::LateContext;
9+
use rustc_span::{sym, Span};
10+
11+
fn check_ty(cx: &LateContext<'_>, param: &Ty<'_>, fixes: &mut Vec<(Span, String)>) {
12+
// TODO: is this the right way to check if ty is an Option?
13+
if let TyKind::Ref(lifetime, MutTy { ty, .. }) = param.kind
14+
&& let TyKind::Path(QPath::Resolved(_, path)) = ty.kind
15+
&& path.segments.len() == 1
16+
&& let seg = &path.segments[0]
17+
&& seg.ident.name == sym::Option
18+
// check if option contains a regular type, not a reference
19+
// TODO: Should this instead just check that opt_ty is a TyKind::Path?
20+
&& let Some(GenericArgs { args: [GenericArg::Type(opt_ty)], .. }) = seg.args
21+
&& !matches!(opt_ty.kind, TyKind::Ref(..))
22+
{
23+
// FIXME: Should this use the Option path from the original type?
24+
// FIXME: Should reference be added in some other way to the snippet?
25+
// FIXME: What should the lifetime be of the reference in Option<&T>?
26+
let lifetime = snippet(cx, lifetime.ident.span, "..");
27+
fixes.push((
28+
param.span,
29+
format!(
30+
"Option<&{}{}{}>",
31+
lifetime,
32+
if lifetime.is_empty() { "" } else { " " },
33+
snippet(cx, opt_ty.span, "..")
34+
),
35+
));
36+
}
37+
}
38+
39+
fn check_fn_sig(cx: &LateContext<'_>, sig: &hir::FnSig<'_>) {
40+
let mut fixes = Vec::new();
41+
// Check function arguments' types
42+
for param in sig.decl.inputs {
43+
check_ty(cx, param, &mut fixes);
44+
}
45+
// Check return type
46+
if let hir::FnRetTy::Return(ty) = &sig.decl.output {
47+
check_ty(cx, ty, &mut fixes);
48+
}
49+
if !fixes.is_empty() {
50+
// FIXME: These changes will often result in broken code that will need to be fixed manually
51+
// What should the applicability be?
52+
span_lint_and_then(
53+
cx,
54+
REF_OPTION,
55+
sig.span,
56+
"it is more idiomatic to use `Option<&T>` instead of `&Option<T>`",
57+
|diag| {
58+
diag.multipart_suggestion("change this to", fixes, Applicability::HasPlaceholders);
59+
},
60+
);
61+
}
62+
}
63+
64+
pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>, avoid_breaking_exported_api: bool) {
65+
// dbg!(avoid_breaking_exported_api);
66+
if let hir::ItemKind::Fn(ref sig, _, _) = item.kind
67+
&& !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(item.owner_id.def_id))
68+
{
69+
check_fn_sig(cx, sig);
70+
}
71+
}
72+
73+
pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, avoid_breaking_exported_api: bool) {
74+
// dbg!(avoid_breaking_exported_api);
75+
if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind
76+
&& !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(impl_item.owner_id.def_id))
77+
{
78+
check_fn_sig(cx, sig);
79+
}
80+
}
81+
82+
pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>, avoid_breaking_exported_api: bool) {
83+
// dbg!(avoid_breaking_exported_api);
84+
if let hir::TraitItemKind::Fn(ref sig, _) = trait_item.kind
85+
&& !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(trait_item.owner_id.def_id))
86+
{
87+
check_fn_sig(cx, sig);
88+
}
89+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
avoid-breaking-exported-api = false
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
avoid-breaking-exported-api = true
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
//@revisions: enabled disabled
2+
//@[enabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/enabled
3+
//@[disabled] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/disabled
4+
5+
#![allow(unused, clippy::all)]
6+
#![warn(clippy::ref_option)]
7+
8+
fn main() {}
9+
10+
// FIXME: Using `&None` instead of `unimplemented!()` is a better test, but it results in an error:
11+
// `after rustfix is applied, all errors should be gone, but weren't`
12+
13+
fn opt_u8(a: Option<&u8>) {}
14+
fn opt_gen<T>(a: Option<&T>) {}
15+
fn opt_string(a: Option<&String>) {}
16+
fn ret_string<'a>(p: &'a String) -> Option<&'a u8> {
17+
panic!()
18+
}
19+
fn ret_string_static() -> Option<&'static u8> {
20+
panic!()
21+
}
22+
fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
23+
pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
24+
25+
pub fn pub_opt_string(a: Option<&String>) {}
26+
27+
pub trait PubTrait {
28+
fn trait_opt(&self, a: Option<&Vec<u8>>);
29+
fn trait_ret(&self) -> Option<&Vec<u8>>;
30+
}
31+
32+
trait PrivateTrait {
33+
fn private_trait_opt(&self, a: Option<&String>);
34+
fn private_trait_ret(&self) -> Option<&String>;
35+
}
36+
37+
pub struct PubStruct;
38+
39+
impl PubStruct {
40+
pub fn pub_opt_params(&self, a: Option<&()>) {}
41+
pub fn pub_opt_ret(&self) -> Option<&String> {
42+
panic!()
43+
}
44+
45+
fn private_opt_params(&self, a: Option<&()>) {}
46+
fn private_opt_ret(&self) -> Option<&String> {
47+
panic!()
48+
}
49+
}
Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
2+
--> tests/ui/ref_option/ref_option.rs:13:1
3+
|
4+
LL | fn opt_u8(a: &Option<u8>) {}
5+
| ^^^^^^^^^^^^^-----------^
6+
| |
7+
| help: change this to: `Option<&u8>`
8+
|
9+
= note: `-D clippy::ref-option` implied by `-D warnings`
10+
= help: to override `-D warnings` add `#[allow(clippy::ref_option)]`
11+
12+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
13+
--> tests/ui/ref_option/ref_option.rs:14:1
14+
|
15+
LL | fn opt_gen<T>(a: &Option<T>) {}
16+
| ^^^^^^^^^^^^^^^^^----------^
17+
| |
18+
| help: change this to: `Option<&T>`
19+
20+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
21+
--> tests/ui/ref_option/ref_option.rs:15:1
22+
|
23+
LL | fn opt_string(a: &Option<String>) {}
24+
| ^^^^^^^^^^^^^^^^^---------------^
25+
| |
26+
| help: change this to: `Option<&String>`
27+
28+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
29+
--> tests/ui/ref_option/ref_option.rs:16:1
30+
|
31+
LL | fn ret_string<'a>(p: &'a String) -> &'a Option<u8> {
32+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------
33+
| |
34+
| help: change this to: `Option<&'a u8>`
35+
36+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
37+
--> tests/ui/ref_option/ref_option.rs:19:1
38+
|
39+
LL | fn ret_string_static() -> &'static Option<u8> {
40+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^-------------------
41+
| |
42+
| help: change this to: `Option<&'static u8>`
43+
44+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
45+
--> tests/ui/ref_option/ref_option.rs:22:1
46+
|
47+
LL | fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {}
48+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
49+
|
50+
help: change this to
51+
|
52+
LL | fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
53+
| ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~
54+
55+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
56+
--> tests/ui/ref_option/ref_option.rs:23:1
57+
|
58+
LL | pub fn pub_mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {}
59+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
60+
|
61+
help: change this to
62+
|
63+
LL | pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
64+
| ~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~
65+
66+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
67+
--> tests/ui/ref_option/ref_option.rs:25:1
68+
|
69+
LL | pub fn pub_opt_string(a: &Option<String>) {}
70+
| ^^^^^^^^^^^^^^^^^^^^^^^^^---------------^
71+
| |
72+
| help: change this to: `Option<&String>`
73+
74+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
75+
--> tests/ui/ref_option/ref_option.rs:28:5
76+
|
77+
LL | fn trait_opt(&self, a: &Option<Vec<u8>>);
78+
| ^^^^^^^^^^^^^^^^^^^^^^^----------------^^
79+
| |
80+
| help: change this to: `Option<&Vec<u8>>`
81+
82+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
83+
--> tests/ui/ref_option/ref_option.rs:29:5
84+
|
85+
LL | fn trait_ret(&self) -> &Option<Vec<u8>>;
86+
| ^^^^^^^^^^^^^^^^^^^^^^^----------------^
87+
| |
88+
| help: change this to: `Option<&Vec<u8>>`
89+
90+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
91+
--> tests/ui/ref_option/ref_option.rs:33:5
92+
|
93+
LL | fn private_trait_opt(&self, a: &Option<String>);
94+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------^^
95+
| |
96+
| help: change this to: `Option<&String>`
97+
98+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
99+
--> tests/ui/ref_option/ref_option.rs:34:5
100+
|
101+
LL | fn private_trait_ret(&self) -> &Option<String>;
102+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------^
103+
| |
104+
| help: change this to: `Option<&String>`
105+
106+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
107+
--> tests/ui/ref_option/ref_option.rs:40:5
108+
|
109+
LL | pub fn pub_opt_params(&self, a: &Option<()>) {}
110+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^
111+
| |
112+
| help: change this to: `Option<&()>`
113+
114+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
115+
--> tests/ui/ref_option/ref_option.rs:41:5
116+
|
117+
LL | pub fn pub_opt_ret(&self) -> &Option<String> {
118+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------
119+
| |
120+
| help: change this to: `Option<&String>`
121+
122+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
123+
--> tests/ui/ref_option/ref_option.rs:45:5
124+
|
125+
LL | fn private_opt_params(&self, a: &Option<()>) {}
126+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^
127+
| |
128+
| help: change this to: `Option<&()>`
129+
130+
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
131+
--> tests/ui/ref_option/ref_option.rs:46:5
132+
|
133+
LL | fn private_opt_ret(&self) -> &Option<String> {
134+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------
135+
| |
136+
| help: change this to: `Option<&String>`
137+
138+
error: aborting due to 16 previous errors
139+

0 commit comments

Comments
 (0)