New lint [let_else_on_result_ok]

CHANGELOG.md

@@ -4890,6 +4890,7 @@ Released 2018-09-13
 [`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
 [`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
 [`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
+[`let_else_on_result_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_else_on_result_ok
 [`let_underscore_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_drop
 [`let_underscore_future`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_future
 [`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock

clippy_dev/src/setup/intellij.rs

@@ -36,7 +36,7 @@ impl ClippyProjectInfo {
 }
 
 pub fn setup_rustc_src(rustc_path: &str) {
-    let Ok(rustc_source_dir) = check_and_get_rustc_dir(rustc_path) else {
+    let Some(rustc_source_dir) = check_and_get_rustc_dir(rustc_path).ok() else {
         return
     };
 
@@ -171,7 +171,7 @@ pub fn remove_rustc_src() {
 }
 
 fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> bool {
-    let Ok(mut cargo_content) = read_project_file(project.cargo_file) else {
+    let Some(mut cargo_content) = read_project_file(project.cargo_file).ok() else {
         return false;
     };
     let Some(section_start) = cargo_content.find(RUSTC_PATH_SECTION) else {

clippy_lints/src/declared_lints.rs

@@ -233,6 +233,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::len_zero::COMPARISON_TO_EMPTY_INFO,
     crate::len_zero::LEN_WITHOUT_IS_EMPTY_INFO,
     crate::len_zero::LEN_ZERO_INFO,
+    crate::let_else_on_result_ok::LET_ELSE_ON_RESULT_OK_INFO,
     crate::let_if_seq::USELESS_LET_IF_SEQ_INFO,
     crate::let_underscore::LET_UNDERSCORE_FUTURE_INFO,
     crate::let_underscore::LET_UNDERSCORE_LOCK_INFO,

clippy_lints/src/let_else_on_result_ok.rs

@@ -0,0 +1,145 @@
+use clippy_utils::{
+    diagnostics::span_lint_and_then,
+    is_from_proc_macro, is_path_diagnostic_item,
+    msrvs::{self, Msrv},
+    ty::is_type_diagnostic_item,
+    visitors::for_each_expr,
+};
+use rustc_hir::{ExprKind, FnRetTy, ItemKind, OwnerNode, Stmt, StmtKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::{
+    lint::in_external_macro,
+    ty::{self, VariantDef},
+};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::sym;
+use std::ops::ControlFlow;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for the usage of `Ok` in a `let...else` statement in a function that returns
+    /// `Result`.
+    ///
+    /// ### Why is this bad?
+    /// This will ignore the contents of the `Err` variant, which is generally unintended and not
+    /// desired. Alternatively, it can be propagated to the caller.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # fn foo() -> Result<(), ()> {
+    /// #    Ok(())
+    /// # }
+    /// let Ok(foo) = foo() else {
+    ///     return;
+    /// };
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # fn foo() -> Result<(), ()> {
+    /// #    Err(())
+    /// # }
+    /// // If you want the contents of the `Err` variant:
+    /// let foo = match foo() {
+    ///     Ok(foo) => foo,
+    ///     Err(e) => eprintln!("{e:#?}"),
+    /// };
+    /// ```
+    /// ```rust
+    /// # fn foo() -> Result<(), ()> {
+    /// #    Ok(())
+    /// # }
+    /// // If you want to propagate it to the caller:
+    /// let foo = foo()?;
+    /// # Ok::<(), ()>(())
+    /// ```
+    /// ```rust
+    /// # fn foo() -> Result<(), ()> {
+    /// #    Err(())
+    /// # }
+    /// // If you want to explicitly ignore the contents of the `Err` variant:
+    /// let Some(foo) = foo().ok() else {
+    ///     return;
+    /// };
+    /// ```
+    #[clippy::version = "1.72.0"]
+    pub LET_ELSE_ON_RESULT_OK,
+    pedantic,
+    "checks for usage of `Ok` in `let...else` statements"
+}
+impl_lint_pass!(LetElseOnResultOk => [LET_ELSE_ON_RESULT_OK]);
+
+pub struct LetElseOnResultOk {
+    msrv: Msrv,
+}
+
+impl LetElseOnResultOk {
+    #[must_use]
+    pub fn new(msrv: Msrv) -> Self {
+        Self { msrv }
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for LetElseOnResultOk {
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &Stmt<'tcx>) {
+        if !self.msrv.meets(msrvs::LET_ELSE) || in_external_macro(cx.sess(), stmt.span) {
+            return;
+        }
+
+        if let StmtKind::Local(local) = stmt.kind
+            && let Some(els) = local.els
+            && let OwnerNode::Item(item) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(stmt.hir_id))
+            && let ItemKind::Fn(sig, _, _) = item.kind
+            && let FnRetTy::Return(ret_ty) = sig.decl.output
+            && is_path_diagnostic_item(cx, ret_ty, sym::Result)
+            // Only lint if we return from it
+            && for_each_expr(els, |expr| {
+                if matches!(expr.kind, ExprKind::Ret(..)) {
+                    return ControlFlow::Break(());
+                }
+
+                ControlFlow::Continue(())
+            })
+            .is_some()
+        {
+            let spans = {
+                let mut spans = vec![];
+                local.pat.walk_always(|pat| {
+                    let ty = cx.typeck_results().pat_ty(pat);
+                    if is_type_diagnostic_item(cx, ty, sym::Result)
+                        && let ty::Adt(_, substs) = ty.kind()
+                        && let [_, err_ty] = substs.as_slice()
+                        && let Some(err_ty) = err_ty.as_type()
+                        && let Some(err_def) = err_ty.ty_adt_def()
+                        && (err_def.all_fields().count() != 0
+                            || err_def
+                                .variants()
+                                .iter()
+                                .any(VariantDef::is_field_list_non_exhaustive)
+                            || err_def.is_variant_list_non_exhaustive())
+                    {
+                        spans.push(pat.span);
+                    }
+                });
+                spans
+            };
+
+            if !spans.is_empty() && is_from_proc_macro(cx, els) {
+                return;
+            };
+
+            for span in spans {
+                span_lint_and_then(
+                    cx,
+                    LET_ELSE_ON_RESULT_OK,
+                    span,
+                    "usage of `let...else` on `Ok`",
+                    |diag| {
+                        diag.note("this will ignore the contents of the `Err` variant");
+                        diag.help("consider using a `match` instead, or propagating it to the caller");
+                    }
+                );
+            }
+        }
+    }
+    extract_msrv_attr!(LateContext);
+}

clippy_lints/src/lib.rs

@@ -171,6 +171,7 @@ mod large_include_file;
 mod large_stack_arrays;
 mod large_stack_frames;
 mod len_zero;
+mod let_else_on_result_ok;
 mod let_if_seq;
 mod let_underscore;
 mod let_with_type_underscore;
@@ -1062,6 +1063,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
             def_id_to_usage: rustc_data_structures::fx::FxHashMap::default(),
         })
     });
+    store.register_late_pass(move |_| Box::new(let_else_on_result_ok::LetElseOnResultOk::new(msrv())));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/utils/conf.rs

@@ -293,7 +293,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// 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.
+    /// 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.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),

clippy_utils/src/check_proc_macro.rs

@@ -154,14 +154,7 @@ fn expr_search_pat(tcx: TyCtxt<'_>, e: &Expr<'_>) -> (Pat, Pat) {
             (expr_search_pat(tcx, e).0, Pat::Str("await"))
         },
         ExprKind::Closure(&Closure { body, .. }) => (Pat::Str(""), expr_search_pat(tcx, tcx.hir().body(body).value).1),
-        ExprKind::Block(
-            Block {
-                rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
-                ..
-            },
-            None,
-        ) => (Pat::Str("unsafe"), Pat::Str("}")),
-        ExprKind::Block(_, None) => (Pat::Str("{"), Pat::Str("}")),
+        ExprKind::Block(block, _) => block_search_pat(block),
         ExprKind::Field(e, name) => (expr_search_pat(tcx, e).0, Pat::Sym(name.name)),
         ExprKind::Index(e, _) => (expr_search_pat(tcx, e).0, Pat::Str("]")),
         ExprKind::Path(ref path) => qpath_search_pat(path),
@@ -348,6 +341,16 @@ fn ident_search_pat(ident: Ident) -> (Pat, Pat) {
     (Pat::OwnedStr(ident.name.as_str().to_owned()), Pat::Str(""))
 }
 
+fn block_search_pat(block: &Block<'_>) -> (Pat, Pat) {
+    let start = if matches!(block.rules, BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided)) {
+        "unsafe"
+    } else {
+        "{"
+    };
+
+    (Pat::Str(start), Pat::Str("}"))
+}
+
 pub trait WithSearchPat<'cx> {
     type Context: LintContext;
     fn search_pat(&self, cx: &Self::Context) -> (Pat, Pat);
@@ -375,6 +378,7 @@ impl_with_search_pat!(LateContext: ImplItem with impl_item_search_pat);
 impl_with_search_pat!(LateContext: FieldDef with field_def_search_pat);
 impl_with_search_pat!(LateContext: Variant with variant_search_pat);
 impl_with_search_pat!(LateContext: Ty with ty_search_pat);
+impl_with_search_pat!(LateContext: Block with block_search_pat);
 
 impl<'cx> WithSearchPat<'cx> for (&FnKind<'cx>, &Body<'cx>, HirId, Span) {
     type Context = LateContext<'cx>;

clippy_utils/src/lib.rs

@@ -2506,13 +2506,16 @@ pub fn tokenize_with_text(s: &str) -> impl Iterator<Item = (TokenKind, &str)> {
 /// Checks whether a given span has any comment token
 /// This checks for all types of comment: line "//", block "/**", doc "///" "//!"
 pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool {
-    let Ok(snippet) = sm.span_to_snippet(span) else { return false };
-    return tokenize(&snippet).any(|token| {
-        matches!(
-            token.kind,
-            TokenKind::BlockComment { .. } | TokenKind::LineComment { .. }
-        )
-    });
+    if let Ok(snippet) = sm.span_to_snippet(span) {
+        return tokenize(&snippet).any(|token| {
+            matches!(
+                token.kind,
+                TokenKind::BlockComment { .. } | TokenKind::LineComment { .. }
+            )
+        });
+    }
+
+    false
 }
 
 /// Return all the comments a given span contains