redundant_pattern_matching: avoid non-const fn in const context

Author: ebroto

clippy_lints/src/redundant_pattern_matching.rs

@@ -1,10 +1,13 @@
-use crate::utils::{match_qpath, match_trait_method, paths, snippet, span_lint_and_then};
+use crate::utils::{in_constant, match_qpath, match_trait_method, paths, snippet, span_lint_and_then};
 use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
-use rustc_hir::{Arm, Expr, ExprKind, MatchSource, PatKind, QPath};
+use rustc_hir::{Arm, Expr, ExprKind, HirId, MatchSource, PatKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
+use rustc_mir::const_eval::is_const_fn;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::source_map::Symbol;
 
 declare_clippy_lint! {
     /// **What it does:** Lint for redundant pattern matching over `Result` or
@@ -64,26 +67,37 @@ fn find_sugg_for_if_let<'a, 'tcx>(
     arms: &[Arm<'_>],
     keyword: &'static str,
 ) {
+    fn find_suggestion(cx: &LateContext<'_, '_>, hir_id: HirId, path: &QPath<'_>) -> Option<&'static str> {
+        if match_qpath(path, &paths::RESULT_OK) && can_suggest(cx, hir_id, sym!(result_type), "is_ok") {
+            return Some("is_ok()");
+        }
+        if match_qpath(path, &paths::RESULT_ERR) && can_suggest(cx, hir_id, sym!(result_type), "is_err") {
+            return Some("is_err()");
+        }
+        if match_qpath(path, &paths::OPTION_SOME) && can_suggest(cx, hir_id, sym!(option_type), "is_some") {
+            return Some("is_some()");
+        }
+        if match_qpath(path, &paths::OPTION_NONE) && can_suggest(cx, hir_id, sym!(option_type), "is_none") {
+            return Some("is_none()");
+        }
+        None
+    }
+
+    let hir_id = expr.hir_id;
     let good_method = match arms[0].pat.kind {
         PatKind::TupleStruct(ref path, ref patterns, _) if patterns.len() == 1 => {
             if let PatKind::Wild = patterns[0].kind {
-                if match_qpath(path, &paths::RESULT_OK) {
-                    "is_ok()"
-                } else if match_qpath(path, &paths::RESULT_ERR) {
-                    "is_err()"
-                } else if match_qpath(path, &paths::OPTION_SOME) {
-                    "is_some()"
-                } else {
-                    return;
-                }
+                find_suggestion(cx, hir_id, path)
             } else {
-                return;
+                None
             }
         },
-
-        PatKind::Path(ref path) if match_qpath(path, &paths::OPTION_NONE) => "is_none()",
-
-        _ => return,
+        PatKind::Path(ref path) => find_suggestion(cx, hir_id, path),
+        _ => None,
+    };
+    let good_method = match good_method {
+        Some(method) => method,
+        None => return,
     };
 
     // check that `while_let_on_iterator` lint does not trigger
@@ -128,6 +142,7 @@ fn find_sugg_for_match<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_
     if arms.len() == 2 {
         let node_pair = (&arms[0].pat.kind, &arms[1].pat.kind);
 
+        let hir_id = expr.hir_id;
         let found_good_method = match node_pair {
             (
                 PatKind::TupleStruct(ref path_left, ref patterns_left, _),
@@ -142,6 +157,8 @@ fn find_sugg_for_match<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_
                         &paths::RESULT_ERR,
                         "is_ok()",
                         "is_err()",
+                        || can_suggest(cx, hir_id, sym!(result_type), "is_ok"),
+                        || can_suggest(cx, hir_id, sym!(result_type), "is_err"),
                     )
                 } else {
                     None
@@ -160,6 +177,8 @@ fn find_sugg_for_match<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_
                         &paths::OPTION_NONE,
                         "is_some()",
                         "is_none()",
+                        || can_suggest(cx, hir_id, sym!(option_type), "is_some"),
+                        || can_suggest(cx, hir_id, sym!(option_type), "is_none"),
                     )
                 } else {
                     None
@@ -188,6 +207,7 @@ fn find_sugg_for_match<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_
     }
 }
 
+#[allow(clippy::too_many_arguments)]
 fn find_good_method_for_match<'a>(
     arms: &[Arm<'_>],
     path_left: &QPath<'_>,
@@ -196,6 +216,8 @@ fn find_good_method_for_match<'a>(
     expected_right: &[&str],
     should_be_left: &'a str,
     should_be_right: &'a str,
+    can_suggest_left: impl Fn() -> bool,
+    can_suggest_right: impl Fn() -> bool,
 ) -> Option<&'a str> {
     let body_node_pair = if match_qpath(path_left, expected_left) && match_qpath(path_right, expected_right) {
         (&(*arms[0].body).kind, &(*arms[1].body).kind)
@@ -207,10 +229,32 @@ fn find_good_method_for_match<'a>(
 
     match body_node_pair {
         (ExprKind::Lit(ref lit_left), ExprKind::Lit(ref lit_right)) => match (&lit_left.node, &lit_right.node) {
-            (LitKind::Bool(true), LitKind::Bool(false)) => Some(should_be_left),
-            (LitKind::Bool(false), LitKind::Bool(true)) => Some(should_be_right),
+            (LitKind::Bool(true), LitKind::Bool(false)) if can_suggest_left() => Some(should_be_left),
+            (LitKind::Bool(false), LitKind::Bool(true)) if can_suggest_right() => Some(should_be_right),
             _ => None,
         },
         _ => None,
     }
 }
+
+fn can_suggest(cx: &LateContext<'_, '_>, hir_id: HirId, diag_item: Symbol, name: &str) -> bool {
+    if !in_constant(cx, hir_id) {
+        return true;
+    }
+
+    // Avoid suggesting calls to non-`const fn`s in const contexts, see #5697.
+    cx.tcx
+        .get_diagnostic_item(diag_item)
+        .and_then(|def_id| {
+            cx.tcx.inherent_impls(def_id).iter().find_map(|imp| {
+                cx.tcx
+                    .associated_items(*imp)
+                    .in_definition_order()
+                    .find_map(|item| match item.kind {
+                        ty::AssocKind::Fn if item.ident.name.as_str() == name => Some(item.def_id),
+                        _ => None,
+                    })
+            })
+        })
+        .map_or(false, |def_id| is_const_fn(cx.tcx, def_id))
+}

tests/ui/redundant_pattern_matching.fixed

@@ -1,5 +1,7 @@
 // run-rustfix
 
+#![feature(const_if_match)]
+#![feature(const_loop)]
 #![warn(clippy::all)]
 #![warn(clippy::redundant_pattern_matching)]
 #![allow(clippy::unit_arg, unused_must_use, clippy::needless_bool, deprecated)]
@@ -67,6 +69,7 @@ fn main() {
     takes_bool(x);
 
     issue5504();
+    issue5697();
 
     let _ = if gen_opt().is_some() {
         1
@@ -117,3 +120,42 @@ fn issue5504() {
     if m!().is_some() {}
     while m!().is_some() {}
 }
+
+// None of these should be linted because none of the suggested methods
+// are `const fn` without toggling a feature.
+const fn issue5697() {
+    if let Ok(_) = Ok::<i32, i32>(42) {}
+
+    if let Err(_) = Err::<i32, i32>(42) {}
+
+    if let Some(_) = Some(42) {}
+
+    if let None = None::<()> {}
+
+    while let Ok(_) = Ok::<i32, i32>(10) {}
+
+    while let Err(_) = Ok::<i32, i32>(10) {}
+
+    while let Some(_) = Some(42) {}
+
+    while let None = None::<()> {}
+
+    match Ok::<i32, i32>(42) {
+        Ok(_) => true,
+        Err(_) => false,
+    };
+
+    match Err::<i32, i32>(42) {
+        Ok(_) => false,
+        Err(_) => true,
+    };
+    match Some(42) {
+        Some(_) => true,
+        None => false,
+    };
+
+    match None::<()> {
+        Some(_) => false,
+        None => true,
+    };
+}

tests/ui/redundant_pattern_matching.rs

@@ -1,5 +1,7 @@
 // run-rustfix
 
+#![feature(const_if_match)]
+#![feature(const_loop)]
 #![warn(clippy::all)]
 #![warn(clippy::redundant_pattern_matching)]
 #![allow(clippy::unit_arg, unused_must_use, clippy::needless_bool, deprecated)]
@@ -88,6 +90,7 @@ fn main() {
     takes_bool(x);
 
     issue5504();
+    issue5697();
 
     let _ = if let Some(_) = gen_opt() {
         1
@@ -138,3 +141,42 @@ fn issue5504() {
     if let Some(_) = m!() {}
     while let Some(_) = m!() {}
 }
+
+// None of these should be linted because none of the suggested methods
+// are `const fn` without toggling a feature.
+const fn issue5697() {
+    if let Ok(_) = Ok::<i32, i32>(42) {}
+
+    if let Err(_) = Err::<i32, i32>(42) {}
+
+    if let Some(_) = Some(42) {}
+
+    if let None = None::<()> {}
+
+    while let Ok(_) = Ok::<i32, i32>(10) {}
+
+    while let Err(_) = Ok::<i32, i32>(10) {}
+
+    while let Some(_) = Some(42) {}
+
+    while let None = None::<()> {}
+
+    match Ok::<i32, i32>(42) {
+        Ok(_) => true,
+        Err(_) => false,
+    };
+
+    match Err::<i32, i32>(42) {
+        Ok(_) => false,
+        Err(_) => true,
+    };
+    match Some(42) {
+        Some(_) => true,
+        None => false,
+    };
+
+    match None::<()> {
+        Some(_) => false,
+        None => true,
+    };
+}