Skip to content

Commit 472768a

Browse files
samueltardieusamueltardieu
authored
fix(ref_option): don't lint in external and proc-macros (#15668)
Fixes #14063 changelog: [`ref_option`]: don't lint in external and proc-macros
2 parents 9f02cbc + b901ba7 commit 472768a

15 files changed

+481
-381
lines changed

‎clippy_lints/src/functions/ref_option.rs‎

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,20 @@
11
use crate::functions::REF_OPTION;
22
use clippy_utils::diagnostics::span_lint_and_then;
3-
use clippy_utils::is_trait_impl_item;
43
use clippy_utils::source::snippet;
5-
use clippy_utils::ty::is_type_diagnostic_item;
4+
use clippy_utils::ty::option_arg_ty;
5+
use clippy_utils::{is_from_proc_macro, is_trait_impl_item};
66
use rustc_errors::Applicability;
7-
use rustc_hir as hir;
87
use rustc_hir::intravisit::FnKind;
9-
use rustc_hir::{FnDecl, HirId};
10-
use rustc_lint::LateContext;
11-
use rustc_middle::ty::{self, GenericArgKind, Mutability, Ty};
8+
use rustc_hir::{self as hir, FnDecl, HirId};
9+
use rustc_lint::{LateContext, LintContext};
10+
use rustc_middle::ty::{self, Mutability, Ty};
11+
use rustc_span::Span;
1212
use rustc_span::def_id::LocalDefId;
13-
use rustc_span::{Span, sym};
1413

15-
fn check_ty<'a>(cx: &LateContext<'a>, param: &rustc_hir::Ty<'a>, param_ty: Ty<'a>, fixes: &mut Vec<(Span, String)>) {
16-
if let ty::Ref(_, opt_ty, Mutability::Not) = param_ty.kind()
17-
&& is_type_diagnostic_item(cx, *opt_ty, sym::Option)
18-
&& let ty::Adt(_, opt_gen_args) = opt_ty.kind()
19-
&& let [gen_arg] = opt_gen_args.as_slice()
20-
&& let GenericArgKind::Type(gen_ty) = gen_arg.kind()
14+
fn check_ty<'a>(cx: &LateContext<'a>, param: &hir::Ty<'a>, param_ty: Ty<'a>, fixes: &mut Vec<(Span, String)>) {
15+
if !param.span.in_external_macro(cx.sess().source_map())
16+
&& let ty::Ref(_, opt_ty, Mutability::Not) = param_ty.kind()
17+
&& let Some(gen_ty) = option_arg_ty(cx, *opt_ty)
2118
&& !gen_ty.is_ref()
2219
// Need to gen the original spans, so first parsing mid, and hir parsing afterward
2320
&& let hir::TyKind::Ref(lifetime, hir::MutTy { ty, .. }) = param.kind
@@ -27,6 +24,7 @@ fn check_ty<'a>(cx: &LateContext<'a>, param: &rustc_hir::Ty<'a>, param_ty: Ty<'a
2724
args: [hir::GenericArg::Type(opt_ty)],
2825
..
2926
}) = last.args
27+
&& !is_from_proc_macro(cx, param)
3028
{
3129
let lifetime = snippet(cx, lifetime.ident.span, "..");
3230
fixes.push((
@@ -67,21 +65,24 @@ fn check_fn_sig<'a>(cx: &LateContext<'a>, decl: &FnDecl<'a>, span: Span, sig: ty
6765
#[allow(clippy::too_many_arguments)]
6866
pub(crate) fn check_fn<'a>(
6967
cx: &LateContext<'a>,
70-
kind: FnKind<'_>,
68+
kind: FnKind<'a>,
7169
decl: &FnDecl<'a>,
7270
span: Span,
7371
hir_id: HirId,
7472
def_id: LocalDefId,
75-
body: &hir::Body<'_>,
73+
body: &hir::Body<'a>,
7674
avoid_breaking_exported_api: bool,
7775
) {
7876
if avoid_breaking_exported_api && cx.effective_visibilities.is_exported(def_id) {
7977
return;
8078
}
79+
if span.in_external_macro(cx.sess().source_map()) {
80+
return;
81+
}
8182

8283
if let FnKind::Closure = kind {
8384
// Compute the span of the closure parameters + return type if set
84-
let span = if let hir::FnRetTy::Return(out_ty) = &decl.output {
85+
let inputs_output_span = if let hir::FnRetTy::Return(out_ty) = &decl.output {
8586
if decl.inputs.is_empty() {
8687
out_ty.span
8788
} else {
@@ -100,9 +101,18 @@ pub(crate) fn check_fn<'a>(
100101
};
101102
let sig = args.as_closure().sig().skip_binder();
102103

103-
check_fn_sig(cx, decl, span, sig);
104+
if is_from_proc_macro(cx, &(&kind, body, hir_id, span)) {
105+
return;
106+
}
107+
108+
check_fn_sig(cx, decl, inputs_output_span, sig);
104109
} else if !is_trait_impl_item(cx, hir_id) {
105110
let sig = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder();
111+
112+
if is_from_proc_macro(cx, &(&kind, body, hir_id, span)) {
113+
return;
114+
}
115+
106116
check_fn_sig(cx, decl, span, sig);
107117
}
108118
}
@@ -112,8 +122,10 @@ pub(super) fn check_trait_item<'a>(
112122
trait_item: &hir::TraitItem<'a>,
113123
avoid_breaking_exported_api: bool,
114124
) {
115-
if let hir::TraitItemKind::Fn(ref sig, _) = trait_item.kind
125+
if !trait_item.span.in_external_macro(cx.sess().source_map())
126+
&& let hir::TraitItemKind::Fn(ref sig, _) = trait_item.kind
116127
&& !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(trait_item.owner_id.def_id))
128+
&& !is_from_proc_macro(cx, trait_item)
117129
{
118130
let def_id = trait_item.owner_id.def_id;
119131
let ty_sig = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder();

‎tests/ui/ref_option/all/clippy.toml‎ renamed to ‎tests/ui-toml/ref_option/all/clippy.toml‎

File renamed without changes.

‎tests/ui/ref_option/private/clippy.toml‎ renamed to ‎tests/ui-toml/ref_option/private/clippy.toml‎

File renamed without changes.

‎tests/ui-toml/ref_option/ref_option.all.fixed‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
//@aux-build:../../ui/auxiliary/proc_macros.rs
2+
//@revisions: private all
3+
//@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/private
4+
//@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/all
5+
6+
#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)]
7+
#![warn(clippy::ref_option)]
8+
9+
fn opt_u8(a: Option<&u8>) {}
10+
//~^ ref_option
11+
fn opt_gen<T>(a: Option<&T>) {}
12+
//~^ ref_option
13+
fn opt_string(a: std::option::Option<&String>) {}
14+
//~^ ref_option
15+
fn ret_u8<'a>(p: &'a str) -> Option<&'a u8> {
16+
//~^ ref_option
17+
panic!()
18+
}
19+
fn ret_u8_static() -> Option<&'static u8> {
20+
//~^ ref_option
21+
panic!()
22+
}
23+
fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
24+
//~^ ref_option
25+
fn ret_box<'a>() -> Option<&'a Box<u8>> {
26+
//~^ ref_option
27+
panic!()
28+
}
29+
30+
pub fn pub_opt_string(a: Option<&String>) {}
31+
//~[all]^ ref_option
32+
pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
33+
//~[all]^ ref_option
34+
35+
pub struct PubStruct;
36+
37+
impl PubStruct {
38+
pub fn pub_opt_params(&self, a: Option<&()>) {}
39+
//~[all]^ ref_option
40+
pub fn pub_opt_ret(&self) -> Option<&String> {
41+
//~[all]^ ref_option
42+
panic!()
43+
}
44+
45+
fn private_opt_params(&self, a: Option<&()>) {}
46+
//~^ ref_option
47+
fn private_opt_ret(&self) -> Option<&String> {
48+
//~^ ref_option
49+
panic!()
50+
}
51+
}
52+
53+
// valid, don't change
54+
fn mut_u8(a: &mut Option<u8>) {}
55+
pub fn pub_mut_u8(a: &mut Option<String>) {}
56+
57+
// might be good to catch in the future
58+
fn mut_u8_ref(a: &mut &Option<u8>) {}
59+
pub fn pub_mut_u8_ref(a: &mut &Option<String>) {}
60+
fn lambdas() {
61+
// Not handled for now, not sure if we should
62+
let x = |a: &Option<String>| {};
63+
let x = |a: &Option<String>| -> &Option<String> { panic!() };
64+
}
65+
66+
pub mod external {
67+
proc_macros::external!(
68+
fn opt_u8(a: &Option<u8>) {}
69+
fn ret_u8<'a>(p: &'a str) -> &'a Option<u8> {
70+
panic!()
71+
}
72+
pub fn pub_opt_u8(a: &Option<u8>) {}
73+
74+
pub struct PubStruct;
75+
impl PubStruct {
76+
pub fn pub_opt_params(&self, a: &Option<()>) {}
77+
pub fn pub_opt_ret(&self) -> &Option<String> {
78+
panic!()
79+
}
80+
81+
fn private_opt_params(&self, a: &Option<()>) {}
82+
fn private_opt_ret(&self) -> &Option<String> {
83+
panic!()
84+
}
85+
}
86+
);
87+
}
88+
89+
pub mod proc_macros {
90+
proc_macros::with_span!(
91+
span
92+
93+
fn opt_u8(a: &Option<u8>) {}
94+
fn ret_u8<'a>(p: &'a str) -> &'a Option<u8> {
95+
panic!()
96+
}
97+
pub fn pub_opt_u8(a: &Option<u8>) {}
98+
99+
pub struct PubStruct;
100+
impl PubStruct {
101+
pub fn pub_opt_params(&self, a: &Option<()>) {}
102+
pub fn pub_opt_ret(&self) -> &Option<String> {
103+
panic!()
104+
}
105+
106+
fn private_opt_params(&self, a: &Option<()>) {}
107+
fn private_opt_ret(&self) -> &Option<String> {
108+
panic!()
109+
}
110+
}
111+
);
112+
}
113+
114+
fn main() {}

‎tests/ui/ref_option/ref_option.all.stderr‎ renamed to ‎tests/ui-toml/ref_option/ref_option.all.stderr‎

Lines changed: 18 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
2-
--> tests/ui/ref_option/ref_option.rs:8:1
2+
--> tests/ui-toml/ref_option/ref_option.rs:9:1
33
|
44
LL | fn opt_u8(a: &Option<u8>) {}
55
| ^^^^^^^^^^^^^-----------^^^^
@@ -10,26 +10,26 @@ LL | fn opt_u8(a: &Option<u8>) {}
1010
= help: to override `-D warnings` add `#[allow(clippy::ref_option)]`
1111

1212
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
13-
--> tests/ui/ref_option/ref_option.rs:10:1
13+
--> tests/ui-toml/ref_option/ref_option.rs:11:1
1414
|
1515
LL | fn opt_gen<T>(a: &Option<T>) {}
1616
| ^^^^^^^^^^^^^^^^^----------^^^^
1717
| |
1818
| help: change this to: `Option<&T>`
1919

2020
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
21-
--> tests/ui/ref_option/ref_option.rs:12:1
21+
--> tests/ui-toml/ref_option/ref_option.rs:13:1
2222
|
2323
LL | fn opt_string(a: &std::option::Option<String>) {}
2424
| ^^^^^^^^^^^^^^^^^----------------------------^^^^
2525
| |
2626
| help: change this to: `std::option::Option<&String>`
2727

2828
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
29-
--> tests/ui/ref_option/ref_option.rs:14:1
29+
--> tests/ui-toml/ref_option/ref_option.rs:15:1
3030
|
31-
LL | fn ret_string<'a>(p: &'a str) -> &'a Option<u8> {
32-
| ^ -------------- help: change this to: `Option<&'a u8>`
31+
LL | fn ret_u8<'a>(p: &'a str) -> &'a Option<u8> {
32+
| ^ -------------- help: change this to: `Option<&'a u8>`
3333
| _|
3434
| |
3535
LL | |
@@ -38,10 +38,10 @@ LL | | }
3838
| |_^
3939

4040
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
41-
--> tests/ui/ref_option/ref_option.rs:18:1
41+
--> tests/ui-toml/ref_option/ref_option.rs:19:1
4242
|
43-
LL | fn ret_string_static() -> &'static Option<u8> {
44-
| ^ ------------------- help: change this to: `Option<&'static u8>`
43+
LL | fn ret_u8_static() -> &'static Option<u8> {
44+
| ^ ------------------- help: change this to: `Option<&'static u8>`
4545
| _|
4646
| |
4747
LL | |
@@ -50,7 +50,7 @@ LL | | }
5050
| |_^
5151

5252
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
53-
--> tests/ui/ref_option/ref_option.rs:22:1
53+
--> tests/ui-toml/ref_option/ref_option.rs:23:1
5454
|
5555
LL | fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {}
5656
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -62,7 +62,7 @@ LL + fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
6262
|
6363

6464
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
65-
--> tests/ui/ref_option/ref_option.rs:24:1
65+
--> tests/ui-toml/ref_option/ref_option.rs:25:1
6666
|
6767
LL | fn ret_box<'a>() -> &'a Option<Box<u8>> {
6868
| ^ ------------------- help: change this to: `Option<&'a Box<u8>>`
@@ -74,15 +74,15 @@ LL | | }
7474
| |_^
7575

7676
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
77-
--> tests/ui/ref_option/ref_option.rs:29:1
77+
--> tests/ui-toml/ref_option/ref_option.rs:30:1
7878
|
7979
LL | pub fn pub_opt_string(a: &Option<String>) {}
8080
| ^^^^^^^^^^^^^^^^^^^^^^^^^---------------^^^^
8181
| |
8282
| help: change this to: `Option<&String>`
8383

8484
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
85-
--> tests/ui/ref_option/ref_option.rs:31:1
85+
--> tests/ui-toml/ref_option/ref_option.rs:32:1
8686
|
8787
LL | pub fn pub_mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {}
8888
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -94,47 +94,15 @@ LL + pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
9494
|
9595

9696
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
97-
--> tests/ui/ref_option/ref_option.rs:35:5
98-
|
99-
LL | fn pub_trait_opt(&self, a: &Option<Vec<u8>>);
100-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^^
101-
| |
102-
| help: change this to: `Option<&Vec<u8>>`
103-
104-
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
105-
--> tests/ui/ref_option/ref_option.rs:37:5
106-
|
107-
LL | fn pub_trait_ret(&self) -> &Option<Vec<u8>>;
108-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^
109-
| |
110-
| help: change this to: `Option<&Vec<u8>>`
111-
112-
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
113-
--> tests/ui/ref_option/ref_option.rs:42:5
114-
|
115-
LL | fn trait_opt(&self, a: &Option<String>);
116-
| ^^^^^^^^^^^^^^^^^^^^^^^---------------^^
117-
| |
118-
| help: change this to: `Option<&String>`
119-
120-
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
121-
--> tests/ui/ref_option/ref_option.rs:44:5
122-
|
123-
LL | fn trait_ret(&self) -> &Option<String>;
124-
| ^^^^^^^^^^^^^^^^^^^^^^^---------------^
125-
| |
126-
| help: change this to: `Option<&String>`
127-
128-
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
129-
--> tests/ui/ref_option/ref_option.rs:51:5
97+
--> tests/ui-toml/ref_option/ref_option.rs:38:5
13098
|
13199
LL | pub fn pub_opt_params(&self, a: &Option<()>) {}
132100
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^
133101
| |
134102
| help: change this to: `Option<&()>`
135103

136104
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
137-
--> tests/ui/ref_option/ref_option.rs:53:5
105+
--> tests/ui-toml/ref_option/ref_option.rs:40:5
138106
|
139107
LL | pub fn pub_opt_ret(&self) -> &Option<String> {
140108
| ^ --------------- help: change this to: `Option<&String>`
@@ -146,15 +114,15 @@ LL | | }
146114
| |_____^
147115

148116
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
149-
--> tests/ui/ref_option/ref_option.rs:58:5
117+
--> tests/ui-toml/ref_option/ref_option.rs:45:5
150118
|
151119
LL | fn private_opt_params(&self, a: &Option<()>) {}
152120
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^
153121
| |
154122
| help: change this to: `Option<&()>`
155123

156124
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
157-
--> tests/ui/ref_option/ref_option.rs:60:5
125+
--> tests/ui-toml/ref_option/ref_option.rs:47:5
158126
|
159127
LL | fn private_opt_ret(&self) -> &Option<String> {
160128
| ^ --------------- help: change this to: `Option<&String>`
@@ -165,5 +133,5 @@ LL | | panic!()
165133
LL | | }
166134
| |_____^
167135

168-
error: aborting due to 17 previous errors
136+
error: aborting due to 13 previous errors
169137

0 commit comments

Comments
 (0)