Auto merge of #7165 - camsteffen:question-mark, r=Manishearth
Fix needless_quesiton_mark false positive changelog: Fix [`needless_question_mark`] false positive where the inner value is implicity dereferenced by the question mark. Fixes #7107
This commit is contained in:
bors
commit
65951c969f
4 changed files with 30 additions and 94 deletions
|
|
@ -1,14 +1,13 @@
|
|||
use clippy_utils::diagnostics::span_lint_and_sugg;
|
||||
use clippy_utils::is_lang_ctor;
|
||||
use clippy_utils::source::snippet;
|
||||
use clippy_utils::ty::is_type_diagnostic_item;
|
||||
use clippy_utils::{differing_macro_contexts, is_lang_ctor};
|
||||
use if_chain::if_chain;
|
||||
use rustc_errors::Applicability;
|
||||
use rustc_hir::LangItem::{OptionSome, ResultOk};
|
||||
use rustc_hir::{Body, Expr, ExprKind, LangItem, MatchSource, QPath};
|
||||
use rustc_lint::{LateContext, LateLintPass};
|
||||
use rustc_middle::ty::TyS;
|
||||
use rustc_session::{declare_lint_pass, declare_tool_lint};
|
||||
use rustc_span::sym;
|
||||
|
||||
declare_clippy_lint! {
|
||||
/// **What it does:**
|
||||
|
|
@ -63,12 +62,6 @@ declare_clippy_lint! {
|
|||
|
||||
declare_lint_pass!(NeedlessQuestionMark => [NEEDLESS_QUESTION_MARK]);
|
||||
|
||||
#[derive(Debug)]
|
||||
enum SomeOkCall<'a> {
|
||||
SomeCall(&'a Expr<'a>, &'a Expr<'a>),
|
||||
OkCall(&'a Expr<'a>, &'a Expr<'a>),
|
||||
}
|
||||
|
||||
impl LateLintPass<'_> for NeedlessQuestionMark {
|
||||
/*
|
||||
* The question mark operator is compatible with both Result<T, E> and Option<T>,
|
||||
|
|
@ -90,104 +83,37 @@ impl LateLintPass<'_> for NeedlessQuestionMark {
|
|||
*/
|
||||
|
||||
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) {
|
||||
let e = match &expr.kind {
|
||||
ExprKind::Ret(Some(e)) => e,
|
||||
_ => return,
|
||||
};
|
||||
|
||||
if let Some(ok_some_call) = is_some_or_ok_call(cx, e) {
|
||||
emit_lint(cx, &ok_some_call);
|
||||
if let ExprKind::Ret(Some(e)) = expr.kind {
|
||||
check(cx, e);
|
||||
}
|
||||
}
|
||||
|
||||
fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) {
|
||||
// Function / Closure block
|
||||
let expr_opt = if let ExprKind::Block(block, _) = &body.value.kind {
|
||||
block.expr
|
||||
} else {
|
||||
// Single line closure
|
||||
Some(&body.value)
|
||||
};
|
||||
|
||||
if_chain! {
|
||||
if let Some(expr) = expr_opt;
|
||||
if let Some(ok_some_call) = is_some_or_ok_call(cx, expr);
|
||||
then {
|
||||
emit_lint(cx, &ok_some_call);
|
||||
}
|
||||
};
|
||||
check(cx, body.value.peel_blocks());
|
||||
}
|
||||
}
|
||||
|
||||
fn emit_lint(cx: &LateContext<'_>, expr: &SomeOkCall<'_>) {
|
||||
let (entire_expr, inner_expr) = match expr {
|
||||
SomeOkCall::OkCall(outer, inner) | SomeOkCall::SomeCall(outer, inner) => (outer, inner),
|
||||
fn check(cx: &LateContext<'_>, expr: &Expr<'_>) {
|
||||
let inner_expr = if_chain! {
|
||||
if let ExprKind::Call(path, [arg]) = &expr.kind;
|
||||
if let ExprKind::Path(ref qpath) = &path.kind;
|
||||
if is_lang_ctor(cx, qpath, OptionSome) || is_lang_ctor(cx, qpath, ResultOk);
|
||||
if let ExprKind::Match(inner_expr_with_q, _, MatchSource::TryDesugar) = &arg.kind;
|
||||
if let ExprKind::Call(called, [inner_expr]) = &inner_expr_with_q.kind;
|
||||
if let ExprKind::Path(QPath::LangItem(LangItem::TryIntoResult, _)) = &called.kind;
|
||||
if expr.span.ctxt() == inner_expr.span.ctxt();
|
||||
let expr_ty = cx.typeck_results().expr_ty(expr);
|
||||
let inner_ty = cx.typeck_results().expr_ty(inner_expr);
|
||||
if TyS::same_type(expr_ty, inner_ty);
|
||||
then { inner_expr } else { return; }
|
||||
};
|
||||
|
||||
span_lint_and_sugg(
|
||||
cx,
|
||||
NEEDLESS_QUESTION_MARK,
|
||||
entire_expr.span,
|
||||
expr.span,
|
||||
"question mark operator is useless here",
|
||||
"try",
|
||||
format!("{}", snippet(cx, inner_expr.span, r#""...""#)),
|
||||
Applicability::MachineApplicable,
|
||||
);
|
||||
}
|
||||
|
||||
fn is_some_or_ok_call<'a>(cx: &'a LateContext<'_>, expr: &'a Expr<'_>) -> Option<SomeOkCall<'a>> {
|
||||
if_chain! {
|
||||
// Check outer expression matches CALL_IDENT(ARGUMENT) format
|
||||
if let ExprKind::Call(path, args) = &expr.kind;
|
||||
if let ExprKind::Path(ref qpath) = &path.kind;
|
||||
if is_lang_ctor(cx, qpath, OptionSome) || is_lang_ctor(cx, qpath, ResultOk);
|
||||
|
||||
// Extract inner expression from ARGUMENT
|
||||
if let ExprKind::Match(inner_expr_with_q, _, MatchSource::TryDesugar) = &args[0].kind;
|
||||
if let ExprKind::Call(called, args) = &inner_expr_with_q.kind;
|
||||
if args.len() == 1;
|
||||
|
||||
if let ExprKind::Path(QPath::LangItem(LangItem::TryIntoResult, _)) = &called.kind;
|
||||
then {
|
||||
// Extract inner expr type from match argument generated by
|
||||
// question mark operator
|
||||
let inner_expr = &args[0];
|
||||
|
||||
// if the inner expr is inside macro but the outer one is not, do not lint (#6921)
|
||||
if differing_macro_contexts(expr.span, inner_expr.span) {
|
||||
return None;
|
||||
}
|
||||
|
||||
let inner_ty = cx.typeck_results().expr_ty(inner_expr);
|
||||
let outer_ty = cx.typeck_results().expr_ty(expr);
|
||||
|
||||
// Check if outer and inner type are Option
|
||||
let outer_is_some = is_type_diagnostic_item(cx, outer_ty, sym::option_type);
|
||||
let inner_is_some = is_type_diagnostic_item(cx, inner_ty, sym::option_type);
|
||||
|
||||
// Check for Option MSRV
|
||||
if outer_is_some && inner_is_some {
|
||||
return Some(SomeOkCall::SomeCall(expr, inner_expr));
|
||||
}
|
||||
|
||||
// Check if outer and inner type are Result
|
||||
let outer_is_result = is_type_diagnostic_item(cx, outer_ty, sym::result_type);
|
||||
let inner_is_result = is_type_diagnostic_item(cx, inner_ty, sym::result_type);
|
||||
|
||||
// Additional check: if the error type of the Result can be converted
|
||||
// via the From trait, then don't match
|
||||
let does_not_call_from = !has_implicit_error_from(cx, expr, inner_expr);
|
||||
|
||||
// Must meet Result MSRV
|
||||
if outer_is_result && inner_is_result && does_not_call_from {
|
||||
return Some(SomeOkCall::OkCall(expr, inner_expr));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
None
|
||||
}
|
||||
|
||||
fn has_implicit_error_from(cx: &LateContext<'_>, entire_expr: &Expr<'_>, inner_result_expr: &Expr<'_>) -> bool {
|
||||
return cx.typeck_results().expr_ty(entire_expr) != cx.typeck_results().expr_ty(inner_result_expr);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -94,6 +94,11 @@ where
|
|||
Ok(x?)
|
||||
}
|
||||
|
||||
// not quite needless
|
||||
fn deref_ref(s: Option<&String>) -> Option<&str> {
|
||||
Some(s?)
|
||||
}
|
||||
|
||||
fn main() {}
|
||||
|
||||
// #6921 if a macro wraps an expr in Some( ) and the ? is in the macro use,
|
||||
|
|
|
|||
|
|
@ -94,6 +94,11 @@ where
|
|||
Ok(x?)
|
||||
}
|
||||
|
||||
// not quite needless
|
||||
fn deref_ref(s: Option<&String>) -> Option<&str> {
|
||||
Some(s?)
|
||||
}
|
||||
|
||||
fn main() {}
|
||||
|
||||
// #6921 if a macro wraps an expr in Some( ) and the ? is in the macro use,
|
||||
|
|
|
|||
|
|
@ -67,7 +67,7 @@ LL | return Ok(t.magic?);
|
|||
| ^^^^^^^^^^^^ help: try: `t.magic`
|
||||
|
||||
error: question mark operator is useless here
|
||||
--> $DIR/needless_question_mark.rs:115:27
|
||||
--> $DIR/needless_question_mark.rs:120:27
|
||||
|
|
||||
LL | || -> Option<_> { Some(Some($expr)?) }()
|
||||
| ^^^^^^^^^^^^^^^^^^ help: try: `Some($expr)`
|
||||
|
|
|
|||
Loading…
Reference in a new issue