dnrops
/
rust
mirror of https://github.com/rust-lang/rust.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[`question_mark`]: don't lint inside of `try` block

This commit is contained in:
y21 2023-06-21 16:36:48 +02:00
parent 8fd021f504
commit 716305d4b6
5 changed files with 181 additions and 117 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
clippy_lints/src/lib.rs
View File

@ -770,7 +770,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|_| Box::<useless_conversion::UselessConversion>::default());
store.register_late_pass(|_| Box::new(implicit_hasher::ImplicitHasher));
store.register_late_pass(|_| Box::new(fallible_impl_from::FallibleImplFrom));
store.register_late_pass(|_| Box::new(question_mark::QuestionMark));
store.register_late_pass(|_| Box::<question_mark::QuestionMark>::default());
store.register_late_pass(|_| Box::new(question_mark_used::QuestionMarkUsed));
store.register_early_pass(|| Box::new(suspicious_operation_groupings::SuspiciousOperationGroupings));
store.register_late_pass(|_| Box::new(suspicious_trait_impl::SuspiciousImpl));

242
clippy_lints/src/question_mark.rs
View File

@ -1,19 +1,20 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::higher;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{
eq_expr_value, get_parent_node, in_constant, is_else_clause, is_res_lang_ctor, path_to_local, path_to_local_id,
peel_blocks, peel_blocks_with_stmt,
};
use clippy_utils::{higher, is_path_lang_item};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::def::Res;
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk};
use rustc_hir::{BindingAnnotation, ByRef, Expr, ExprKind, Node, PatKind, PathSegment, QPath};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::Ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_session::declare_tool_lint;
use rustc_session::impl_lint_pass;
use rustc_span::{sym, symbol::Symbol};
declare_clippy_lint! {
@ -41,7 +42,16 @@ declare_clippy_lint! {
"checks for expressions that could be replaced by the question mark operator"
}
declare_lint_pass!(QuestionMark => [QUESTION_MARK]);
#[derive(Default)]
pub struct QuestionMark {
/// Keeps track of how many try blocks we are in at any point during linting.
/// This allows us to answer the question "are we inside of a try block"
/// very quickly, without having to walk up the parent chain, by simply checking
/// if it is greater than zero.
/// As for why we need this in the first place: <https://github.com/rust-lang/rust-clippy/issues/8628>
try_block_depth: u32,
}
impl_lint_pass!(QuestionMark => [QUESTION_MARK]);
enum IfBlockType<'hir> {
/// An `if x.is_xxx() { a } else { b } ` expression.
@ -68,98 +78,6 @@ enum IfBlockType<'hir> {
),
}
/// Checks if the given expression on the given context matches the following structure:
///
/// ```ignore
/// if option.is_none() {
/// return None;
/// }
/// ```
///
/// ```ignore
/// if result.is_err() {
/// return result;
/// }
/// ```
///
/// If it matches, it will suggest to use the question mark operator instead
fn check_is_none_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if_chain! {
if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr);
if !is_else_clause(cx.tcx, expr);
if let ExprKind::MethodCall(segment, caller, ..) = &cond.kind;
let caller_ty = cx.typeck_results().expr_ty(caller);
let if_block = IfBlockType::IfIs(caller, caller_ty, segment.ident.name, then, r#else);
if is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block);
then {
let mut applicability = Applicability::MachineApplicable;
let receiver_str = snippet_with_applicability(cx, caller.span, "..", &mut applicability);
let by_ref = !caller_ty.is_copy_modulo_regions(cx.tcx, cx.param_env) &&
!matches!(caller.kind, ExprKind::Call(..) | ExprKind::MethodCall(..));
let sugg = if let Some(else_inner) = r#else {
if eq_expr_value(cx, caller, peel_blocks(else_inner)) {
format!("Some({receiver_str}?)")
} else {
return;
}
} else {
format!("{receiver_str}{}?;", if by_ref { ".as_ref()" } else { "" })
};
span_lint_and_sugg(
cx,
QUESTION_MARK,
expr.span,
"this block may be rewritten with the `?` operator",
"replace it with",
sugg,
applicability,
);
}
}
}
fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if_chain! {
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else }) = higher::IfLet::hir(cx, expr);
if !is_else_clause(cx.tcx, expr);
if let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind;
if ddpos.as_opt_usize().is_none();
if let PatKind::Binding(BindingAnnotation(by_ref, _), bind_id, ident, None) = field.kind;
let caller_ty = cx.typeck_results().expr_ty(let_expr);
let if_block = IfBlockType::IfLet(
cx.qpath_res(path1, let_pat.hir_id),
caller_ty,
ident.name,
let_expr,
if_then,
if_else
);
if (is_early_return(sym::Option, cx, &if_block) && path_to_local_id(peel_blocks(if_then), bind_id))
|| is_early_return(sym::Result, cx, &if_block);
if if_else.map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))).filter(|e| *e).is_none();
then {
let mut applicability = Applicability::MachineApplicable;
let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
let requires_semi = matches!(get_parent_node(cx.tcx, expr.hir_id), Some(Node::Stmt(_)));
let sugg = format!(
"{receiver_str}{}?{}",
if by_ref == ByRef::Yes { ".as_ref()" } else { "" },
if requires_semi { ";" } else { "" }
);
span_lint_and_sugg(
cx,
QUESTION_MARK,
expr.span,
"this block may be rewritten with the `?` operator",
"replace it with",
sugg,
applicability,
);
}
}
}
fn is_early_return(smbl: Symbol, cx: &LateContext<'_>, if_block: &IfBlockType<'_>) -> bool {
match *if_block {
IfBlockType::IfIs(caller, caller_ty, call_sym, if_then, _) => {
@ -230,11 +148,133 @@ fn expr_return_none_or_err(
}
}
impl<'tcx> LateLintPass<'tcx> for QuestionMark {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if !in_constant(cx, expr.hir_id) {
check_is_none_or_err_and_early_return(cx, expr);
check_if_let_some_or_err_and_early_return(cx, expr);
impl QuestionMark {
fn inside_try_block(&self) -> bool {
self.try_block_depth > 0
}
/// Checks if the given expression on the given context matches the following structure:
///
/// ```ignore
/// if option.is_none() {
/// return None;
/// }
/// ```
///
/// ```ignore
/// if result.is_err() {
/// return result;
/// }
/// ```
///
/// If it matches, it will suggest to use the question mark operator instead
fn check_is_none_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if_chain! {
if !self.inside_try_block();
if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr);
if !is_else_clause(cx.tcx, expr);
if let ExprKind::MethodCall(segment, caller, ..) = &cond.kind;
let caller_ty = cx.typeck_results().expr_ty(caller);
let if_block = IfBlockType::IfIs(caller, caller_ty, segment.ident.name, then, r#else);
if is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block);
then {
let mut applicability = Applicability::MachineApplicable;
let receiver_str = snippet_with_applicability(cx, caller.span, "..", &mut applicability);
let by_ref = !caller_ty.is_copy_modulo_regions(cx.tcx, cx.param_env) &&
!matches!(caller.kind, ExprKind::Call(..) | ExprKind::MethodCall(..));
let sugg = if let Some(else_inner) = r#else {
if eq_expr_value(cx, caller, peel_blocks(else_inner)) {
format!("Some({receiver_str}?)")
} else {
return;
}
} else {
format!("{receiver_str}{}?;", if by_ref { ".as_ref()" } else { "" })
};
span_lint_and_sugg(
cx,
QUESTION_MARK,
expr.span,
"this block may be rewritten with the `?` operator",
"replace it with",
sugg,
applicability,
);
}
}
}
fn check_if_let_some_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if_chain! {
if !self.inside_try_block();
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else }) = higher::IfLet::hir(cx, expr);
if !is_else_clause(cx.tcx, expr);
if let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind;
if ddpos.as_opt_usize().is_none();
if let PatKind::Binding(BindingAnnotation(by_ref, _), bind_id, ident, None) = field.kind;
let caller_ty = cx.typeck_results().expr_ty(let_expr);
let if_block = IfBlockType::IfLet(
cx.qpath_res(path1, let_pat.hir_id),
caller_ty,
ident.name,
let_expr,
if_then,
if_else
);
if (is_early_return(sym::Option, cx, &if_block) && path_to_local_id(peel_blocks(if_then), bind_id))
|| is_early_return(sym::Result, cx, &if_block);
if if_else.map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))).filter(|e| *e).is_none();
then {
let mut applicability = Applicability::MachineApplicable;
let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
let requires_semi = matches!(get_parent_node(cx.tcx, expr.hir_id), Some(Node::Stmt(_)));
let sugg = format!(
"{receiver_str}{}?{}",
if by_ref == ByRef::Yes { ".as_ref()" } else { "" },
if requires_semi { ";" } else { "" }
);
span_lint_and_sugg(
cx,
QUESTION_MARK,
expr.span,
"this block may be rewritten with the `?` operator",
"replace it with",
sugg,
applicability,
);
}
}
}
}
fn is_try_block(cx: &LateContext<'_>, bl: &rustc_hir::Block<'_>) -> bool {
if let Some(expr) = bl.expr
&& let rustc_hir::ExprKind::Call(callee, _) = expr.kind
{
is_path_lang_item(cx, callee, LangItem::TryTraitFromOutput)
} else {
false
}
}
impl<'tcx> LateLintPass<'tcx> for QuestionMark {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if !in_constant(cx, expr.hir_id) {
self.check_is_none_or_err_and_early_return(cx, expr);
self.check_if_let_some_or_err_and_early_return(cx, expr);
}
}
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx rustc_hir::Block<'tcx>) {
if is_try_block(cx, block) {
self.try_block_depth += 1;
}
}
fn check_block_post(&mut self, cx: &LateContext<'tcx>, block: &'tcx rustc_hir::Block<'tcx>) {
if is_try_block(cx, block) {
self.try_block_depth -= 1;
}
}
}

12
tests/ui/question_mark.fixed
View File

@ -1,4 +1,5 @@
//@run-rustfix
#![feature(try_blocks)]
#![allow(unreachable_code)]
#![allow(dead_code)]
#![allow(clippy::unnecessary_wraps)]
@ -227,6 +228,17 @@ fn pattern() -> Result<(), PatternedError> {
fn main() {}
// `?` is not the same as `return None;` if inside of a try block
fn issue8628(a: Option<u32>) -> Option<u32> {
let b: Option<u32> = try {
if a.is_none() {
return None;
}
32
};
b.or(Some(128))
}
// should not lint, `?` operator not available in const context
const fn issue9175(option: Option<()>) -> Option<()> {
if option.is_none() {

12
tests/ui/question_mark.rs
View File

@ -1,4 +1,5 @@
//@run-rustfix
#![feature(try_blocks)]
#![allow(unreachable_code)]
#![allow(dead_code)]
#![allow(clippy::unnecessary_wraps)]
@ -263,6 +264,17 @@ fn pattern() -> Result<(), PatternedError> {
fn main() {}
// `?` is not the same as `return None;` if inside of a try block
fn issue8628(a: Option<u32>) -> Option<u32> {
let b: Option<u32> = try {
if a.is_none() {
return None;
}
32
};
b.or(Some(128))
}
// should not lint, `?` operator not available in const context
const fn issue9175(option: Option<()>) -> Option<()> {
if option.is_none() {

30
tests/ui/question_mark.stderr
View File

@ -1,5 +1,5 @@
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:7:5
--> $DIR/question_mark.rs:8:5
|
LL | / if a.is_none() {
LL | | return None;
@ -9,7 +9,7 @@ LL | | }
= note: `-D clippy::question-mark` implied by `-D warnings`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:52:9
--> $DIR/question_mark.rs:53:9
|
LL | / if (self.opt).is_none() {
LL | | return None;
@ -17,7 +17,7 @@ LL | | }
| |_________^ help: replace it with: `(self.opt)?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:56:9
--> $DIR/question_mark.rs:57:9
|
LL | / if self.opt.is_none() {
LL | | return None
@ -25,7 +25,7 @@ LL | | }
| |_________^ help: replace it with: `self.opt?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:60:17
--> $DIR/question_mark.rs:61:17
|
LL | let _ = if self.opt.is_none() {
| _________________^
@ -36,7 +36,7 @@ LL | | };
| |_________^ help: replace it with: `Some(self.opt?)`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:66:17
--> $DIR/question_mark.rs:67:17
|
LL | let _ = if let Some(x) = self.opt {
| _________________^
@ -47,7 +47,7 @@ LL | | };
| |_________^ help: replace it with: `self.opt?`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:83:9
--> $DIR/question_mark.rs:84:9
|
LL | / if self.opt.is_none() {
LL | | return None;
@ -55,7 +55,7 @@ LL | | }
| |_________^ help: replace it with: `self.opt.as_ref()?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:91:9
--> $DIR/question_mark.rs:92:9
|
LL | / if self.opt.is_none() {
LL | | return None;
@ -63,7 +63,7 @@ LL | | }
| |_________^ help: replace it with: `self.opt.as_ref()?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:99:9
--> $DIR/question_mark.rs:100:9
|
LL | / if self.opt.is_none() {
LL | | return None;
@ -71,7 +71,7 @@ LL | | }
| |_________^ help: replace it with: `self.opt.as_ref()?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:106:26
--> $DIR/question_mark.rs:107:26
|
LL | let v: &Vec<_> = if let Some(ref v) = self.opt {
| __________________________^
@ -82,7 +82,7 @@ LL | | };
| |_________^ help: replace it with: `self.opt.as_ref()?`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:116:17
--> $DIR/question_mark.rs:117:17
|
LL | let v = if let Some(v) = self.opt {
| _________________^
@ -93,7 +93,7 @@ LL | | };
| |_________^ help: replace it with: `self.opt?`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:131:5
--> $DIR/question_mark.rs:132:5
|
LL | / if f().is_none() {
LL | | return None;
@ -101,13 +101,13 @@ LL | | }
| |_____^ help: replace it with: `f()?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:143:13
--> $DIR/question_mark.rs:144:13
|
LL | let _ = if let Ok(x) = x { x } else { return x };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:145:5
--> $DIR/question_mark.rs:146:5
|
LL | / if x.is_err() {
LL | | return x;
@ -115,7 +115,7 @@ LL | | }
| |_____^ help: replace it with: `x?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:196:5
--> $DIR/question_mark.rs:197:5
|
LL | / if let Err(err) = func_returning_result() {
LL | | return Err(err);
@ -123,7 +123,7 @@ LL | | }
| |_____^ help: replace it with: `func_returning_result()?;`
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:203:5
--> $DIR/question_mark.rs:204:5
|
LL | / if let Err(err) = func_returning_result() {
LL | | return Err(err);