Skip to content

Commit ca4c3ee

Browse files
Centri3Centri3
committed
new lint let_else_on_result_ok 
1 parent 06b444b commit ca4c3ee

File tree

10 files changed

+271
-18
lines changed

10 files changed

+271
-18
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4890,6 +4890,7 @@ Released 2018-09-13
48904890
[`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
48914891
[`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
48924892
[`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
4893+
[`let_else_on_result_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_else_on_result_ok
48934894
[`let_underscore_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_drop
48944895
[`let_underscore_future`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_future
48954896
[`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock

‎clippy_dev/src/setup/intellij.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ impl ClippyProjectInfo {
3636
}
3737

3838
pub fn setup_rustc_src(rustc_path: &str) {
39-
let Ok(rustc_source_dir) = check_and_get_rustc_dir(rustc_path) else {
39+
let Some(rustc_source_dir) = check_and_get_rustc_dir(rustc_path).ok() else {
4040
return
4141
};
4242

@@ -171,7 +171,7 @@ pub fn remove_rustc_src() {
171171
}
172172

173173
fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> bool {
174-
let Ok(mut cargo_content) = read_project_file(project.cargo_file) else {
174+
let Some(mut cargo_content) = read_project_file(project.cargo_file).ok() else {
175175
return false;
176176
};
177177
let Some(section_start) = cargo_content.find(RUSTC_PATH_SECTION) else {

‎clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
233233
crate::len_zero::COMPARISON_TO_EMPTY_INFO,
234234
crate::len_zero::LEN_WITHOUT_IS_EMPTY_INFO,
235235
crate::len_zero::LEN_ZERO_INFO,
236+
crate::let_else_on_result_ok::LET_ELSE_ON_RESULT_OK_INFO,
236237
crate::let_if_seq::USELESS_LET_IF_SEQ_INFO,
237238
crate::let_underscore::LET_UNDERSCORE_FUTURE_INFO,
238239
crate::let_underscore::LET_UNDERSCORE_LOCK_INFO,

‎clippy_lints/src/let_else_on_result_ok.rs

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
use clippy_utils::{
2+
diagnostics::span_lint_and_note,
3+
is_from_proc_macro, is_lang_item_or_ctor,
4+
msrvs::{self, Msrv},
5+
};
6+
use rustc_hir::{LangItem, PatKind, Stmt, StmtKind};
7+
use rustc_lint::{LateContext, LateLintPass, LintContext};
8+
use rustc_middle::lint::in_external_macro;
9+
use rustc_session::{declare_tool_lint, impl_lint_pass};
10+
11+
declare_clippy_lint! {
12+
/// ### What it does
13+
/// Checks for the usage of `Ok` in a `let...else` statement.
14+
///
15+
/// ### Why is this bad?
16+
/// This will ignore the contents of the `Err` variant, which is generally unintended and not
17+
/// desired. Alternatively, it can be propagated to the caller.
18+
///
19+
/// ### Example
20+
/// ```rust
21+
/// # fn foo() -> Result<(), ()> {
22+
/// # Ok(())
23+
/// # }
24+
/// let Ok(foo) = foo() else {
25+
/// return;
26+
/// };
27+
/// ```
28+
/// Use instead:
29+
/// ```rust
30+
/// # fn foo() -> Result<(), ()> {
31+
/// # Err(())
32+
/// # }
33+
/// // If you want the contents of the `Err` variant:
34+
/// let foo = match foo() {
35+
/// Ok(foo) => foo,
36+
/// Err(e) => eprintln!("{e:#?}"),
37+
/// };
38+
/// ```
39+
/// ```rust
40+
/// # fn foo() -> Result<(), ()> {
41+
/// # Ok(())
42+
/// # }
43+
/// // If you want to propagate it to the caller:
44+
/// let foo = foo()?;
45+
/// # Ok::<(), ()>(())
46+
/// ```
47+
/// ```rust
48+
/// # fn foo() -> Result<(), ()> {
49+
/// # Err(())
50+
/// # }
51+
/// // If you want to ignore the contents of the `Err` variant:
52+
/// let Some(foo) = foo().ok() else {
53+
/// return;
54+
/// };
55+
/// ```
56+
#[clippy::version = "1.72.0"]
57+
pub LET_ELSE_ON_RESULT_OK,
58+
pedantic,
59+
"checks for usage of `Ok` in `let...else` statements"
60+
}
61+
impl_lint_pass!(LetElseOnResultOk => [LET_ELSE_ON_RESULT_OK]);
62+
63+
pub struct LetElseOnResultOk {
64+
msrv: Msrv,
65+
}
66+
67+
impl LetElseOnResultOk {
68+
#[must_use]
69+
pub fn new(msrv: Msrv) -> Self {
70+
Self { msrv }
71+
}
72+
}
73+
74+
impl<'tcx> LateLintPass<'tcx> for LetElseOnResultOk {
75+
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &Stmt<'tcx>) {
76+
if !self.msrv.meets(msrvs::LET_ELSE) || in_external_macro(cx.sess(), stmt.span) {
77+
return;
78+
}
79+
80+
if let StmtKind::Local(local) = stmt.kind && let Some(els) = local.els {
81+
let spans = {
82+
let mut spans = vec![];
83+
local.pat.walk_always(|pat| {
84+
if let PatKind::TupleStruct(qpath, _, _) = pat.kind
85+
&& let Some(def_id) = cx.qpath_res(&qpath, pat.hir_id).opt_def_id()
86+
&& is_lang_item_or_ctor(cx, def_id, LangItem::ResultOk)
87+
{
88+
spans.push(pat.span);
89+
}
90+
});
91+
spans
92+
};
93+
94+
if !spans.is_empty() && is_from_proc_macro(cx, els) {
95+
return;
96+
};
97+
98+
for span in spans {
99+
span_lint_and_note(
100+
cx,
101+
LET_ELSE_ON_RESULT_OK,
102+
span,
103+
"usage of `let...else` on `Err`",
104+
None,
105+
"consider handling the `Err` variant gracefully or propagating it to the caller",
106+
);
107+
}
108+
}
109+
}
110+
extract_msrv_attr!(LateContext);
111+
}

‎clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ mod large_include_file;
171171
mod large_stack_arrays;
172172
mod large_stack_frames;
173173
mod len_zero;
174+
mod let_else_on_result_ok;
174175
mod let_if_seq;
175176
mod let_underscore;
176177
mod let_with_type_underscore;
@@ -1062,6 +1063,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10621063
def_id_to_usage: rustc_data_structures::fx::FxHashMap::default(),
10631064
})
10641065
});
1066+
store.register_late_pass(move |_| Box::new(let_else_on_result_ok::LetElseOnResultOk::new(msrv())));
10651067
// add lints here, do not remove this comment, it's used in `new_lint`
10661068
}
10671069

‎clippy_lints/src/utils/conf.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ define_Conf! {
293293
///
294294
/// Suppress lints whenever the suggested change would cause breakage for other crates.
295295
(avoid_breaking_exported_api: bool = true),
296-
/// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN.
296+
/// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, LET_ELSE_ON_RETURN.
297297
///
298298
/// The minimum rust version that the project supports
299299
(msrv: Option<String> = None),

‎clippy_utils/src/check_proc_macro.rs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -154,14 +154,7 @@ fn expr_search_pat(tcx: TyCtxt<'_>, e: &Expr<'_>) -> (Pat, Pat) {
154154
(expr_search_pat(tcx, e).0, Pat::Str("await"))
155155
},
156156
ExprKind::Closure(&Closure { body, .. }) => (Pat::Str(""), expr_search_pat(tcx, tcx.hir().body(body).value).1),
157-
ExprKind::Block(
158-
Block {
159-
rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
160-
..
161-
},
162-
None,
163-
) => (Pat::Str("unsafe"), Pat::Str("}")),
164-
ExprKind::Block(_, None) => (Pat::Str("{"), Pat::Str("}")),
157+
ExprKind::Block(block, _) => block_search_pat(block),
165158
ExprKind::Field(e, name) => (expr_search_pat(tcx, e).0, Pat::Sym(name.name)),
166159
ExprKind::Index(e, _) => (expr_search_pat(tcx, e).0, Pat::Str("]")),
167160
ExprKind::Path(ref path) => qpath_search_pat(path),
@@ -348,6 +341,16 @@ fn ident_search_pat(ident: Ident) -> (Pat, Pat) {
348341
(Pat::OwnedStr(ident.name.as_str().to_owned()), Pat::Str(""))
349342
}
350343

344+
fn block_search_pat(block: &Block<'_>) -> (Pat, Pat) {
345+
let start = if matches!(block.rules, BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided)) {
346+
"unsafe"
347+
} else {
348+
"{"
349+
};
350+
351+
(Pat::Str(start), Pat::Str("}"))
352+
}
353+
351354
pub trait WithSearchPat<'cx> {
352355
type Context: LintContext;
353356
fn search_pat(&self, cx: &Self::Context) -> (Pat, Pat);
@@ -375,6 +378,7 @@ impl_with_search_pat!(LateContext: ImplItem with impl_item_search_pat);
375378
impl_with_search_pat!(LateContext: FieldDef with field_def_search_pat);
376379
impl_with_search_pat!(LateContext: Variant with variant_search_pat);
377380
impl_with_search_pat!(LateContext: Ty with ty_search_pat);
381+
impl_with_search_pat!(LateContext: Block with block_search_pat);
378382

379383
impl<'cx> WithSearchPat<'cx> for (&FnKind<'cx>, &Body<'cx>, HirId, Span) {
380384
type Context = LateContext<'cx>;

‎clippy_utils/src/lib.rs

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2506,13 +2506,16 @@ pub fn tokenize_with_text(s: &str) -> impl Iterator<Item = (TokenKind, &str)> {
25062506
/// Checks whether a given span has any comment token
25072507
/// This checks for all types of comment: line "//", block "/**", doc "///" "//!"
25082508
pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool {
2509-
let Ok(snippet) = sm.span_to_snippet(span) else { return false };
2510-
return tokenize(&snippet).any(|token| {
2511-
matches!(
2512-
token.kind,
2513-
TokenKind::BlockComment { .. } | TokenKind::LineComment { .. }
2514-
)
2515-
});
2509+
if let Ok(snippet) = sm.span_to_snippet(span) {
2510+
return tokenize(&snippet).any(|token| {
2511+
matches!(
2512+
token.kind,
2513+
TokenKind::BlockComment { .. } | TokenKind::LineComment { .. }
2514+
)
2515+
});
2516+
}
2517+
2518+
false
25162519
}
25172520

25182521
/// Return all the comments a given span contains

‎tests/ui/let_else_on_result_ok.rs

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
//@aux-build:proc_macros.rs
2+
#![allow(clippy::redundant_pattern_matching, unused)]
3+
#![warn(clippy::let_else_on_result_ok)]
4+
5+
#[macro_use]
6+
extern crate proc_macros;
7+
8+
struct A(Result<&'static A, ()>);
9+
10+
enum AnEnum {
11+
A(Result<&'static A, ()>),
12+
}
13+
14+
fn a() -> Result<(), ()> {
15+
Ok(())
16+
}
17+
18+
fn a_constructor() -> A {
19+
todo!();
20+
}
21+
22+
fn an_enum_constructor() -> AnEnum {
23+
todo!();
24+
}
25+
26+
fn main() {
27+
// Lint
28+
let Ok(_) = a() else {
29+
return;
30+
};
31+
let (Ok(_), true) = (a(), true) else {
32+
return;
33+
};
34+
let [Ok(_), Ok(_)] = [a(), Err(())] else {
35+
return;
36+
};
37+
let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
38+
return;
39+
};
40+
let AnEnum::A(Ok(A(Err(_)))) = an_enum_constructor() else {
41+
return;
42+
};
43+
// Don't lint
44+
let Err(_) = a() else {
45+
return;
46+
};
47+
match a() {
48+
Ok(a) => a,
49+
Err(e) => eprintln!("{e:#?}"),
50+
};
51+
external! {
52+
let Ok(_) = a() else {
53+
return;
54+
};
55+
}
56+
}

‎tests/ui/let_else_on_result_ok.stderr

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
error: usage of `let...else` on `Err`
2+
--> $DIR/let_else_on_result_ok.rs:28:9
3+
|
4+
LL | let Ok(_) = a() else {
5+
| ^^^^^
6+
|
7+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
8+
= note: `-D clippy::let-else-on-result-ok` implied by `-D warnings`
9+
10+
error: usage of `let...else` on `Err`
11+
--> $DIR/let_else_on_result_ok.rs:31:10
12+
|
13+
LL | let (Ok(_), true) = (a(), true) else {
14+
| ^^^^^
15+
|
16+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
17+
18+
error: usage of `let...else` on `Err`
19+
--> $DIR/let_else_on_result_ok.rs:34:10
20+
|
21+
LL | let [Ok(_), Ok(_)] = [a(), Err(())] else {
22+
| ^^^^^
23+
|
24+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
25+
26+
error: usage of `let...else` on `Err`
27+
--> $DIR/let_else_on_result_ok.rs:34:17
28+
|
29+
LL | let [Ok(_), Ok(_)] = [a(), Err(())] else {
30+
| ^^^^^
31+
|
32+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
33+
34+
error: usage of `let...else` on `Err`
35+
--> $DIR/let_else_on_result_ok.rs:37:11
36+
|
37+
LL | let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
38+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
39+
|
40+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
41+
42+
error: usage of `let...else` on `Err`
43+
--> $DIR/let_else_on_result_ok.rs:37:16
44+
|
45+
LL | let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
46+
| ^^^^^^^^^^^^^^^^^^^
47+
|
48+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
49+
50+
error: usage of `let...else` on `Err`
51+
--> $DIR/let_else_on_result_ok.rs:37:21
52+
|
53+
LL | let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
54+
| ^^^^^^^^^^^^
55+
|
56+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
57+
58+
error: usage of `let...else` on `Err`
59+
--> $DIR/let_else_on_result_ok.rs:37:26
60+
|
61+
LL | let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
62+
| ^^^^^
63+
|
64+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
65+
66+
error: usage of `let...else` on `Err`
67+
--> $DIR/let_else_on_result_ok.rs:40:19
68+
|
69+
LL | let AnEnum::A(Ok(A(Err(_)))) = an_enum_constructor() else {
70+
| ^^^^^^^^^^^^^
71+
|
72+
= note: consider handling the `Err` variant gracefully or propagating it to the caller
73+
74+
error: aborting due to 9 previous errors
75+

0 commit comments

Comments
 (0)