Skip to content

Commit 7d9eb34

Browse files
J-ZhengLiJ-ZhengLi
committed
reduce code repetition
1 parent fb94992 commit 7d9eb34

File tree

4 files changed

+304
-172
lines changed

4 files changed

+304
-172
lines changed

‎clippy_lints/src/question_mark.rs

Lines changed: 170 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,18 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::higher;
2+
use clippy_utils::higher::{If, IfLet};
33
use clippy_utils::source::snippet_with_applicability;
4-
use clippy_utils::sugg::Sugg;
54
use clippy_utils::ty::is_type_diagnostic_item;
6-
use clippy_utils::{eq_expr_value, is_lang_ctor, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt};
5+
use clippy_utils::{
6+
eq_expr_value, is_else_clause, is_lang_ctor, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt,
7+
};
78
use if_chain::if_chain;
89
use rustc_errors::Applicability;
9-
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultOk};
10-
use rustc_hir::{BindingAnnotation, Expr, ExprKind, PatKind};
10+
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
11+
use rustc_hir::{BindingAnnotation, Expr, ExprKind, PatKind, PathSegment, QPath};
1112
use rustc_lint::{LateContext, LateLintPass};
13+
use rustc_middle::ty::Ty;
1214
use rustc_session::{declare_lint_pass, declare_tool_lint};
13-
use rustc_span::sym;
15+
use rustc_span::{sym, symbol::Symbol};
1416

1517
declare_clippy_lint! {
1618
/// ### What it does
@@ -39,135 +41,183 @@ declare_clippy_lint! {
3941

4042
declare_lint_pass!(QuestionMark => [QUESTION_MARK]);
4143

42-
impl QuestionMark {
43-
/// Checks if the given expression on the given context matches the following structure:
44-
///
45-
/// ```ignore
46-
/// if option.is_none() {
47-
/// return None;
48-
/// }
49-
/// ```
50-
///
51-
/// ```ignore
52-
/// if result.is_err() {
53-
/// return result;
54-
/// }
55-
/// ```
56-
///
57-
/// If it matches, it will suggest to use the question mark operator instead
58-
fn check_is_none_or_err_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>) {
59-
if_chain! {
60-
if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr);
61-
if let ExprKind::MethodCall(segment, args, _) = &cond.kind;
62-
if let Some(subject) = args.get(0);
63-
if (Self::option_check_and_early_return(cx, subject, then) && segment.ident.name == sym!(is_none)) ||
64-
(Self::result_check_and_early_return(cx, subject, then) && segment.ident.name == sym!(is_err));
65-
then {
66-
let mut applicability = Applicability::MachineApplicable;
67-
let receiver_str = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability);
68-
let mut replacement: Option<String> = None;
69-
if let Some(else_inner) = r#else {
70-
if eq_expr_value(cx, subject, peel_blocks(else_inner)) {
71-
replacement = Some(format!("Some({}?)", receiver_str));
72-
}
73-
} else if Self::moves_by_default(cx, subject)
74-
&& !matches!(subject.kind, ExprKind::Call(..) | ExprKind::MethodCall(..))
75-
{
76-
replacement = Some(format!("{}.as_ref()?;", receiver_str));
77-
} else {
78-
replacement = Some(format!("{}?;", receiver_str));
79-
}
44+
impl<'tcx> LateLintPass<'tcx> for QuestionMark {
45+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
46+
let mut suggestion: Option<String> = None;
47+
let mut applicability = Applicability::MachineApplicable;
8048

81-
if let Some(replacement_str) = replacement {
82-
span_lint_and_sugg(
83-
cx,
84-
QUESTION_MARK,
85-
expr.span,
86-
"this block may be rewritten with the `?` operator",
87-
"replace it with",
88-
replacement_str,
89-
applicability,
90-
);
91-
}
49+
if let Some(if_expr) = If::hir(expr) {
50+
if !is_else_clause(cx.tcx, expr) {
51+
suggestion = get_suggestion_for_if_struct(cx, &if_expr, &mut applicability);
9252
}
93-
}
94-
}
95-
96-
fn check_if_let_some_or_err_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>) {
97-
if_chain! {
98-
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) })
99-
= higher::IfLet::hir(cx, expr);
100-
if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind;
101-
if (Self::option_check_and_early_return(cx, let_expr, if_else) && is_lang_ctor(cx, path1, OptionSome)) ||
102-
(Self::result_check_and_early_return(cx, let_expr, if_else) && is_lang_ctor(cx, path1, ResultOk));
103-
104-
if let PatKind::Binding(annot, bind_id, _, _) = fields[0].kind;
105-
let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
106-
if path_to_local_id(peel_blocks(if_then), bind_id);
107-
then {
108-
let mut applicability = Applicability::MachineApplicable;
109-
let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
110-
let replacement = format!("{}{}?", receiver_str, if by_ref { ".as_ref()" } else { "" },);
111-
112-
span_lint_and_sugg(
113-
cx,
114-
QUESTION_MARK,
115-
expr.span,
116-
"this if-let-else may be rewritten with the `?` operator",
117-
"replace it with",
118-
replacement,
119-
applicability,
120-
);
53+
} else if let Some(if_let_expr) = IfLet::hir(cx, expr) {
54+
if !is_else_clause(cx.tcx, expr) {
55+
suggestion = get_suggestion_for_if_let_struct(cx, &if_let_expr, &mut applicability);
12156
}
12257
}
123-
}
124-
125-
fn result_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
126-
Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr)
127-
}
12858

129-
fn option_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
130-
Self::is_option(cx, expr) && Self::expression_returns_none(cx, nested_expr)
131-
}
132-
133-
fn moves_by_default(cx: &LateContext<'_>, expression: &Expr<'_>) -> bool {
134-
let expr_ty = cx.typeck_results().expr_ty(expression);
135-
136-
!expr_ty.is_copy_modulo_regions(cx.tcx.at(expression.span), cx.param_env)
137-
}
138-
139-
fn is_option(cx: &LateContext<'_>, expression: &Expr<'_>) -> bool {
140-
let expr_ty = cx.typeck_results().expr_ty(expression);
141-
142-
is_type_diagnostic_item(cx, expr_ty, sym::Option)
59+
if let Some(suggestion_str) = suggestion {
60+
span_lint_and_sugg(
61+
cx,
62+
QUESTION_MARK,
63+
expr.span,
64+
"this block may be rewritten with the `?` operator",
65+
"replace it with",
66+
suggestion_str,
67+
applicability,
68+
);
69+
}
14370
}
71+
}
14472

145-
fn is_result(cx: &LateContext<'_>, expression: &Expr<'_>) -> bool {
146-
let expr_ty = cx.typeck_results().expr_ty(expression);
147-
148-
is_type_diagnostic_item(cx, expr_ty, sym::Result)
73+
/// Receive an `clippy_utils::higher::If` struct, if it matches following syntax,
74+
/// this function will return an suggestion string wrapped in option.
75+
///
76+
/// ```ignore
77+
/// if option.is_none() {
78+
/// return None;
79+
/// }
80+
/// ```
81+
///
82+
/// ```ignore
83+
/// if result.is_err() {
84+
/// return result;
85+
/// }
86+
/// ```
87+
fn get_suggestion_for_if_struct(cx: &LateContext<'_>, if_expr: &If<'_>, apl: &mut Applicability) -> Option<String> {
88+
if_chain! {
89+
if let ExprKind::MethodCall(segment, args, _) = if_expr.cond.kind;
90+
if let Some(subject) = args.get(0);
91+
let subject_ty = cx.typeck_results().expr_ty(subject);
92+
if (is_return_none_or_err(cx, subject_ty, sym::Option, if_expr.then, subject, None) &&
93+
segment.ident.name == sym!(is_none)) ||
94+
(is_return_none_or_err(cx, subject_ty, sym::Result, if_expr.then, subject, None) &&
95+
segment.ident.name == sym!(is_err));
96+
then {
97+
let receiver_str = snippet_with_applicability(cx, subject.span, "..", apl);
98+
let by_ref = !subject_ty.is_copy_modulo_regions(cx.tcx.at(subject.span), cx.param_env)
99+
&& !matches!(subject.kind, ExprKind::Call(..) | ExprKind::MethodCall(..));
100+
if let Some(else_inner) = if_expr.r#else {
101+
if eq_expr_value(cx, subject, peel_blocks(else_inner)) {
102+
return Some(format!("Some({}?)", receiver_str));
103+
}
104+
} else {
105+
return Some(format!("{}{}?;", receiver_str, if by_ref { ".as_ref()" } else { "" }));
106+
}
107+
}
149108
}
109+
None
110+
}
150111

151-
fn expression_returns_none(cx: &LateContext<'_>, expression: &Expr<'_>) -> bool {
152-
match peel_blocks_with_stmt(expression).kind {
153-
ExprKind::Ret(Some(expr)) => Self::expression_returns_none(cx, expr),
154-
ExprKind::Path(ref qpath) => is_lang_ctor(cx, qpath, OptionNone),
155-
_ => false,
112+
/// Receive an `clippy_utils::higher::IfLet` struct, if it has similar syntax as below,
113+
/// this function will return an suggestion string wrapped in option.
114+
///
115+
/// ```ignore
116+
/// if let Err(err) = result {
117+
/// Err(err);
118+
/// }
119+
///
120+
/// let _ = if let Ok(a) = result {
121+
/// a
122+
/// } else {
123+
/// return result;
124+
/// }
125+
///
126+
/// let _ = if let Some(a) = option {
127+
/// a
128+
/// } else {
129+
/// return None;
130+
/// }
131+
/// ```
132+
fn get_suggestion_for_if_let_struct(
133+
cx: &LateContext<'_>,
134+
if_let_expr: &IfLet<'_>,
135+
apl: &mut Applicability,
136+
) -> Option<String> {
137+
let IfLet {
138+
let_pat,
139+
let_expr,
140+
if_then,
141+
if_else,
142+
} = if_let_expr;
143+
144+
if_chain! {
145+
if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind;
146+
let nested_expr = if_else.unwrap_or(if_then);
147+
if let PatKind::Binding(annot, bind_id, ident, _) = fields[0].kind;
148+
let let_expr_ty = cx.typeck_results().expr_ty(let_expr);
149+
if (is_return_none_or_err(cx, let_expr_ty, sym::Result, nested_expr, let_expr, Some(ident.name)) &&
150+
(is_lang_ctor(cx, path1, ResultOk) || is_lang_ctor(cx, path1, ResultErr))) ||
151+
(is_return_none_or_err(cx, let_expr_ty, sym::Option, nested_expr, let_expr, Some(ident.name)) &&
152+
is_lang_ctor(cx, path1, OptionSome) && path_to_local_id(peel_blocks(if_then), bind_id));
153+
then {
154+
let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
155+
let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", apl);
156+
return Some(format!(
157+
"{}{}?{}",
158+
receiver_str,
159+
if by_ref { ".as_ref()" } else { "" },
160+
if is_semicolon_needed(if_then, if_else) { ";" } else { "" }
161+
));
156162
}
157163
}
164+
None
165+
}
158166

159-
fn expression_returns_unmodified_err(cx: &LateContext<'_>, expr: &Expr<'_>, cond_expr: &Expr<'_>) -> bool {
160-
match peel_blocks_with_stmt(expr).kind {
161-
ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr),
162-
ExprKind::Path(_) => path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
167+
/// Check if the given expression matches any one of the following syntax.
168+
///
169+
/// ```ignore
170+
/// return None;
171+
/// return Err(err);
172+
/// None
173+
/// Err(err)
174+
/// x // Where x is evalued as None or Err
175+
/// ```
176+
fn is_return_none_or_err(
177+
cx: &LateContext<'_>,
178+
ty: Ty<'_>,
179+
ret_sym: Symbol,
180+
expr: &Expr<'_>,
181+
cond_expr: &Expr<'_>,
182+
let_pat_sym: Option<Symbol>,
183+
) -> bool {
184+
is_type_diagnostic_item(cx, ty, ret_sym)
185+
&& match &peel_blocks_with_stmt(expr).kind {
186+
ExprKind::Ret(Some(ret_expr)) => is_return_none_or_err(cx, ty, ret_sym, ret_expr, cond_expr, let_pat_sym),
187+
ExprKind::Path(ref qpath) => match ret_sym {
188+
sym::Result => path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
189+
sym::Option => is_lang_ctor(cx, qpath, OptionNone),
190+
_ => false,
191+
},
192+
ExprKind::Call(call_expr, args_expr) => {
193+
if_chain! {
194+
if ret_sym == sym::Result;
195+
if let ExprKind::Path(QPath::Resolved(_, path)) = &call_expr.kind;
196+
if let Some(segment) = path.segments.first();
197+
if let Some(pat_sym) = let_pat_sym;
198+
if let Some(arg) = args_expr.first();
199+
if let ExprKind::Path(QPath::Resolved(_, arg_path)) = &arg.kind;
200+
if let Some(PathSegment { ident, .. }) = arg_path.segments.first();
201+
then {
202+
return segment.ident.name == sym::Err && pat_sym == ident.name;
203+
}
204+
}
205+
false
206+
},
163207
_ => false,
164208
}
165-
}
166209
}
167210

168-
impl<'tcx> LateLintPass<'tcx> for QuestionMark {
169-
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
170-
Self::check_is_none_or_err_and_early_return(cx, expr);
171-
Self::check_if_let_some_or_err_and_early_return(cx, expr);
211+
/// Check whether the fixed code needs semicolon after `?`
212+
fn is_semicolon_needed(if_then: &Expr<'_>, if_else: &'_ Option<&'_ Expr<'_>>) -> bool {
213+
let if_then_kind = &peel_blocks_with_stmt(if_then).kind;
214+
let if_then_is_ret_stmt = matches!(if_then_kind, ExprKind::Ret(..));
215+
216+
if if_else.is_none() {
217+
return if_then_is_ret_stmt;
218+
}
219+
if let ExprKind::Ret(_) = peel_blocks_with_stmt(if_else.unwrap()).kind {
220+
return if_then_is_ret_stmt;
172221
}
222+
false
173223
}

0 commit comments

Comments
 (0)