Skip to content

Commit 67cb5ec

Browse files
committed
Move TryErr into Matches lint pass
1 parent dbc7753 commit 67cb5ec

File tree

6 files changed

+188
-190
lines changed

6 files changed

+188
-190
lines changed

clippy_lints/src/lib.register_lints.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,7 @@ store.register_lints(&[
279279
matches::SIGNIFICANT_DROP_IN_SCRUTINEE,
280280
matches::SINGLE_MATCH,
281281
matches::SINGLE_MATCH_ELSE,
282+
matches::TRY_ERR,
282283
matches::WILDCARD_ENUM_MATCH_ARM,
283284
matches::WILDCARD_IN_OR_PATTERNS,
284285
mem_forget::MEM_FORGET,
@@ -521,7 +522,6 @@ store.register_lints(&[
521522
transmute::USELESS_TRANSMUTE,
522523
transmute::WRONG_TRANSMUTE,
523524
transmuting_null::TRANSMUTING_NULL,
524-
try_err::TRY_ERR,
525525
types::BORROWED_BOX,
526526
types::BOX_COLLECTION,
527527
types::LINKEDLIST,

clippy_lints/src/lib.register_restriction.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
3030
LintId::of(literal_representation::DECIMAL_LITERAL_REPRESENTATION),
3131
LintId::of(map_err_ignore::MAP_ERR_IGNORE),
3232
LintId::of(matches::REST_PAT_IN_FULLY_BOUND_STRUCTS),
33+
LintId::of(matches::TRY_ERR),
3334
LintId::of(matches::WILDCARD_ENUM_MATCH_ARM),
3435
LintId::of(mem_forget::MEM_FORGET),
3536
LintId::of(methods::CLONE_ON_REF_PTR),
@@ -67,7 +68,6 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
6768
LintId::of(strings::STRING_SLICE),
6869
LintId::of(strings::STRING_TO_STRING),
6970
LintId::of(strings::STR_TO_STRING),
70-
LintId::of(try_err::TRY_ERR),
7171
LintId::of(types::RC_BUFFER),
7272
LintId::of(types::RC_MUTEX),
7373
LintId::of(undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS),

clippy_lints/src/lib.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,6 @@ mod trailing_empty_array;
385385
mod trait_bounds;
386386
mod transmute;
387387
mod transmuting_null;
388-
mod try_err;
389388
mod types;
390389
mod undocumented_unsafe_blocks;
391390
mod unicode;
@@ -700,7 +699,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
700699
);
701700
store.register_late_pass(move || Box::new(pass_by_ref_or_value));
702701
store.register_late_pass(|| Box::new(ref_option_ref::RefOptionRef));
703-
store.register_late_pass(|| Box::new(try_err::TryErr));
704702
store.register_late_pass(|| Box::new(bytecount::ByteCount));
705703
store.register_late_pass(|| Box::new(infinite_iter::InfiniteIter));
706704
store.register_late_pass(|| Box::new(inline_fn_without_body::InlineFnWithoutBody));

clippy_lints/src/matches/mod.rs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ mod redundant_pattern_match;
2727
mod rest_pat_in_fully_bound_struct;
2828
mod significant_drop_in_scrutinee;
2929
mod single_match;
30+
mod try_err;
3031
mod wild_in_or_pats;
3132

3233
declare_clippy_lint! {
@@ -825,6 +826,41 @@ declare_clippy_lint! {
825826
"warns when a temporary of a type with a drop with a significant side-effect might have a surprising lifetime"
826827
}
827828

829+
declare_clippy_lint! {
830+
/// ### What it does
831+
/// Checks for usages of `Err(x)?`.
832+
///
833+
/// ### Why is this bad?
834+
/// The `?` operator is designed to allow calls that
835+
/// can fail to be easily chained. For example, `foo()?.bar()` or
836+
/// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will
837+
/// always return), it is more clear to write `return Err(x)`.
838+
///
839+
/// ### Example
840+
/// ```rust
841+
/// fn foo(fail: bool) -> Result<i32, String> {
842+
/// if fail {
843+
/// Err("failed")?;
844+
/// }
845+
/// Ok(0)
846+
/// }
847+
/// ```
848+
/// Could be written:
849+
///
850+
/// ```rust
851+
/// fn foo(fail: bool) -> Result<i32, String> {
852+
/// if fail {
853+
/// return Err("failed".into());
854+
/// }
855+
/// Ok(0)
856+
/// }
857+
/// ```
858+
#[clippy::version = "1.38.0"]
859+
pub TRY_ERR,
860+
restriction,
861+
"return errors explicitly rather than hiding them behind a `?`"
862+
}
863+
828864
#[derive(Default)]
829865
pub struct Matches {
830866
msrv: Option<RustcVersion>,
@@ -864,6 +900,7 @@ impl_lint_pass!(Matches => [
864900
MATCH_ON_VEC_ITEMS,
865901
MATCH_STR_CASE_MISMATCH,
866902
SIGNIFICANT_DROP_IN_SCRUTINEE,
903+
TRY_ERR,
867904
]);
868905

869906
impl<'tcx> LateLintPass<'tcx> for Matches {
@@ -888,6 +925,10 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
888925
wild_in_or_pats::check(cx, arms);
889926
}
890927

928+
if source == MatchSource::TryDesugar {
929+
try_err::check(cx, expr, ex);
930+
}
931+
891932
if !from_expansion && !contains_cfg_arm(cx, expr, ex, arms) {
892933
if source == MatchSource::Normal {
893934
if !(meets_msrv(self.msrv, msrvs::MATCHES_MACRO)

clippy_lints/src/matches/try_err.rs

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::source::snippet_with_applicability;
3+
use clippy_utils::ty::is_type_diagnostic_item;
4+
use clippy_utils::{get_parent_expr, is_lang_ctor, match_def_path, paths};
5+
use if_chain::if_chain;
6+
use rustc_errors::Applicability;
7+
use rustc_hir::LangItem::ResultErr;
8+
use rustc_hir::{Expr, ExprKind, LangItem, MatchSource, QPath};
9+
use rustc_lint::LateContext;
10+
use rustc_middle::ty::{self, Ty};
11+
use rustc_span::{hygiene, sym};
12+
13+
use super::TRY_ERR;
14+
15+
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, scrutinee: &'tcx Expr<'_>) {
16+
// Looks for a structure like this:
17+
// match ::std::ops::Try::into_result(Err(5)) {
18+
// ::std::result::Result::Err(err) =>
19+
// #[allow(unreachable_code)]
20+
// return ::std::ops::Try::from_error(::std::convert::From::from(err)),
21+
// ::std::result::Result::Ok(val) =>
22+
// #[allow(unreachable_code)]
23+
// val,
24+
// };
25+
if_chain! {
26+
if let ExprKind::Call(match_fun, try_args) = scrutinee.kind;
27+
if let ExprKind::Path(ref match_fun_path) = match_fun.kind;
28+
if matches!(match_fun_path, QPath::LangItem(LangItem::TryTraitBranch, ..));
29+
if let Some(try_arg) = try_args.get(0);
30+
if let ExprKind::Call(err_fun, err_args) = try_arg.kind;
31+
if let Some(err_arg) = err_args.get(0);
32+
if let ExprKind::Path(ref err_fun_path) = err_fun.kind;
33+
if is_lang_ctor(cx, err_fun_path, ResultErr);
34+
if let Some(return_ty) = find_return_type(cx, &expr.kind);
35+
then {
36+
let prefix;
37+
let suffix;
38+
let err_ty;
39+
40+
if let Some(ty) = result_error_type(cx, return_ty) {
41+
prefix = "Err(";
42+
suffix = ")";
43+
err_ty = ty;
44+
} else if let Some(ty) = poll_result_error_type(cx, return_ty) {
45+
prefix = "Poll::Ready(Err(";
46+
suffix = "))";
47+
err_ty = ty;
48+
} else if let Some(ty) = poll_option_result_error_type(cx, return_ty) {
49+
prefix = "Poll::Ready(Some(Err(";
50+
suffix = ")))";
51+
err_ty = ty;
52+
} else {
53+
return;
54+
};
55+
56+
let expr_err_ty = cx.typeck_results().expr_ty(err_arg);
57+
let span = hygiene::walk_chain(err_arg.span, try_arg.span.ctxt());
58+
let mut applicability = Applicability::MachineApplicable;
59+
let origin_snippet = snippet_with_applicability(cx, span, "_", &mut applicability);
60+
let ret_prefix = if get_parent_expr(cx, expr).map_or(false, |e| matches!(e.kind, ExprKind::Ret(_))) {
61+
"" // already returns
62+
} else {
63+
"return "
64+
};
65+
let suggestion = if err_ty == expr_err_ty {
66+
format!("{}{}{}{}", ret_prefix, prefix, origin_snippet, suffix)
67+
} else {
68+
format!("{}{}{}.into(){}", ret_prefix, prefix, origin_snippet, suffix)
69+
};
70+
71+
span_lint_and_sugg(
72+
cx,
73+
TRY_ERR,
74+
expr.span,
75+
"returning an `Err(_)` with the `?` operator",
76+
"try this",
77+
suggestion,
78+
applicability,
79+
);
80+
}
81+
}
82+
}
83+
84+
/// Finds function return type by examining return expressions in match arms.
85+
fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option<Ty<'tcx>> {
86+
if let ExprKind::Match(_, arms, MatchSource::TryDesugar) = expr {
87+
for arm in arms.iter() {
88+
if let ExprKind::Ret(Some(ret)) = arm.body.kind {
89+
return Some(cx.typeck_results().expr_ty(ret));
90+
}
91+
}
92+
}
93+
None
94+
}
95+
96+
/// Extracts the error type from Result<T, E>.
97+
fn result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
98+
if_chain! {
99+
if let ty::Adt(_, subst) = ty.kind();
100+
if is_type_diagnostic_item(cx, ty, sym::Result);
101+
then {
102+
Some(subst.type_at(1))
103+
} else {
104+
None
105+
}
106+
}
107+
}
108+
109+
/// Extracts the error type from Poll<Result<T, E>>.
110+
fn poll_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
111+
if_chain! {
112+
if let ty::Adt(def, subst) = ty.kind();
113+
if match_def_path(cx, def.did(), &paths::POLL);
114+
let ready_ty = subst.type_at(0);
115+
116+
if let ty::Adt(ready_def, ready_subst) = ready_ty.kind();
117+
if cx.tcx.is_diagnostic_item(sym::Result, ready_def.did());
118+
then {
119+
Some(ready_subst.type_at(1))
120+
} else {
121+
None
122+
}
123+
}
124+
}
125+
126+
/// Extracts the error type from Poll<Option<Result<T, E>>>.
127+
fn poll_option_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
128+
if_chain! {
129+
if let ty::Adt(def, subst) = ty.kind();
130+
if match_def_path(cx, def.did(), &paths::POLL);
131+
let ready_ty = subst.type_at(0);
132+
133+
if let ty::Adt(ready_def, ready_subst) = ready_ty.kind();
134+
if cx.tcx.is_diagnostic_item(sym::Option, ready_def.did());
135+
let some_ty = ready_subst.type_at(0);
136+
137+
if let ty::Adt(some_def, some_subst) = some_ty.kind();
138+
if cx.tcx.is_diagnostic_item(sym::Result, some_def.did());
139+
then {
140+
Some(some_subst.type_at(1))
141+
} else {
142+
None
143+
}
144+
}
145+
}

0 commit comments

Comments
 (0)