From 9486f728790e1fb15bb88db672d2f164078a19eb Mon Sep 17 00:00:00 2001 From: CDirkx Date: Mon, 31 Aug 2020 02:11:48 +0200 Subject: [PATCH 1/4] Stabilize some Option methods as const Stabilize the following methods of `Option` as const: - `is_some` - `is_none` - `as_ref` Possible because of stabilization of #49146 (Allow if and match in constants). --- library/core/src/option.rs | 6 +++--- src/test/ui/consts/const-option.rs | 2 -- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/library/core/src/option.rs b/library/core/src/option.rs index 7e560d63fe23b..b1589008be073 100644 --- a/library/core/src/option.rs +++ b/library/core/src/option.rs @@ -175,7 +175,7 @@ impl Option { /// ``` #[must_use = "if you intended to assert that this has a value, consider `.unwrap()` instead"] #[inline] - #[rustc_const_unstable(feature = "const_option", issue = "67441")] + #[rustc_const_stable(feature = "const_option", since = "1.48.0")] #[stable(feature = "rust1", since = "1.0.0")] pub const fn is_some(&self) -> bool { matches!(*self, Some(_)) @@ -195,7 +195,7 @@ impl Option { #[must_use = "if you intended to assert that this doesn't have a value, consider \ `.and_then(|| panic!(\"`Option` had a value when expected `None`\"))` instead"] #[inline] - #[rustc_const_unstable(feature = "const_option", issue = "67441")] + #[rustc_const_stable(feature = "const_option", since = "1.48.0")] #[stable(feature = "rust1", since = "1.0.0")] pub const fn is_none(&self) -> bool { !self.is_some() @@ -254,7 +254,7 @@ impl Option { /// println!("still can print text: {:?}", text); /// ``` #[inline] - #[rustc_const_unstable(feature = "const_option", issue = "67441")] + #[rustc_const_stable(feature = "const_option", since = "1.48.0")] #[stable(feature = "rust1", since = "1.0.0")] pub const fn as_ref(&self) -> Option<&T> { match *self { diff --git a/src/test/ui/consts/const-option.rs b/src/test/ui/consts/const-option.rs index fbf20b9db6741..793f78c8d20fa 100644 --- a/src/test/ui/consts/const-option.rs +++ b/src/test/ui/consts/const-option.rs @@ -1,7 +1,5 @@ // run-pass -#![feature(const_option)] - const X: Option = Some(32); const Y: Option<&i32> = X.as_ref(); From 4f859fbcfc9080687f4a52a96e0ec98290e02f19 Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Fri, 4 Sep 2020 00:13:25 +0200 Subject: [PATCH 2/4] Move const tests for `Option` to `library\core` Part of #76268 --- library/core/tests/option.rs | 16 ++++++++++++++++ src/test/ui/consts/const-option.rs | 12 ------------ 2 files changed, 16 insertions(+), 12 deletions(-) delete mode 100644 src/test/ui/consts/const-option.rs diff --git a/library/core/tests/option.rs b/library/core/tests/option.rs index b46bcfd16d283..9e86e07dd91a3 100644 --- a/library/core/tests/option.rs +++ b/library/core/tests/option.rs @@ -356,3 +356,19 @@ fn test_replace() { assert_eq!(x, Some(3)); assert_eq!(old, None); } + +#[test] +fn option_const() { + // test that the methods of `Option` are usable in a const context + + const OPTION: Option = Some(32); + + const REF: Option<&usize> = OPTION.as_ref(); + assert_eq!(REF, Some(&32)); + + const IS_SOME: bool = OPTION.is_some(); + assert!(IS_SOME); + + const IS_NONE: bool = OPTION.is_none(); + assert!(!IS_NONE); +} diff --git a/src/test/ui/consts/const-option.rs b/src/test/ui/consts/const-option.rs deleted file mode 100644 index 793f78c8d20fa..0000000000000 --- a/src/test/ui/consts/const-option.rs +++ /dev/null @@ -1,12 +0,0 @@ -// run-pass - -const X: Option = Some(32); -const Y: Option<&i32> = X.as_ref(); - -const IS_SOME: bool = X.is_some(); -const IS_NONE: bool = Y.is_none(); - -fn main() { - assert!(IS_SOME); - assert!(!IS_NONE) -} From e58d0627b8e12644bdf4e915b5c5bc0d3c37be29 Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Sun, 20 Sep 2020 23:59:34 +0200 Subject: [PATCH 3/4] Update Clippy testcases Update the test `redundant_pattern_matching`: check if `is_some` and `is_none` are suggested within const contexts. --- .../tests/ui/redundant_pattern_matching.fixed | 39 ++++------ .../tests/ui/redundant_pattern_matching.rs | 45 ++++++------ .../ui/redundant_pattern_matching.stderr | 72 +++++++++++++++---- 3 files changed, 91 insertions(+), 65 deletions(-) diff --git a/src/tools/clippy/tests/ui/redundant_pattern_matching.fixed b/src/tools/clippy/tests/ui/redundant_pattern_matching.fixed index 08bfe7c78d38b..8084fdefdc23e 100644 --- a/src/tools/clippy/tests/ui/redundant_pattern_matching.fixed +++ b/src/tools/clippy/tests/ui/redundant_pattern_matching.fixed @@ -76,7 +76,6 @@ fn main() { takes_bool(x); issue5504(); - issue5697(); issue6067(); let _ = if gen_opt().is_some() { @@ -129,41 +128,31 @@ fn issue5504() { 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 Some(_) = Some(42) {} - - if let None = None::<()> {} - - while let Some(_) = Some(42) {} - - while let None = None::<()> {} - - match Some(42) { - Some(_) => true, - None => false, - }; - - match None::<()> { - Some(_) => false, - None => true, - }; -} - // Methods that are unstable const should not be suggested within a const context, see issue #5697. -// However, in Rust 1.48.0 the methods `is_ok` and `is_err` of `Result` were stabilized as const, -// so the following should be linted. +// However, in Rust 1.48.0 the methods `is_ok` and `is_err` of `Result`, and `is_some` and `is_none` +// of `Option` were stabilized as const, so the following should be linted. const fn issue6067() { if Ok::(42).is_ok() {} if Err::(42).is_err() {} + if Some(42).is_some() {} + + if None::<()>.is_none() {} + while Ok::(10).is_ok() {} while Ok::(10).is_err() {} + while Some(42).is_some() {} + + while None::<()>.is_none() {} + Ok::(42).is_ok(); Err::(42).is_err(); + + Some(42).is_some(); + + None::<()>.is_none(); } diff --git a/src/tools/clippy/tests/ui/redundant_pattern_matching.rs b/src/tools/clippy/tests/ui/redundant_pattern_matching.rs index c0660c6ac3947..48a32cb1c7b7d 100644 --- a/src/tools/clippy/tests/ui/redundant_pattern_matching.rs +++ b/src/tools/clippy/tests/ui/redundant_pattern_matching.rs @@ -97,7 +97,6 @@ fn main() { takes_bool(x); issue5504(); - issue5697(); issue6067(); let _ = if let Some(_) = gen_opt() { @@ -150,40 +149,26 @@ fn issue5504() { 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 Some(_) = Some(42) {} - - if let None = None::<()> {} - - while let Some(_) = Some(42) {} - - while let None = None::<()> {} - - match Some(42) { - Some(_) => true, - None => false, - }; - - match None::<()> { - Some(_) => false, - None => true, - }; -} - // Methods that are unstable const should not be suggested within a const context, see issue #5697. -// However, in Rust 1.48.0 the methods `is_ok` and `is_err` of `Result` were stabilized as const, -// so the following should be linted. +// However, in Rust 1.48.0 the methods `is_ok` and `is_err` of `Result`, and `is_some` and `is_none` +// of `Option` were stabilized as const, so the following should be linted. const fn issue6067() { if let Ok(_) = Ok::(42) {} if let Err(_) = Err::(42) {} + if let Some(_) = Some(42) {} + + if let None = None::<()> {} + while let Ok(_) = Ok::(10) {} while let Err(_) = Ok::(10) {} + while let Some(_) = Some(42) {} + + while let None = None::<()> {} + match Ok::(42) { Ok(_) => true, Err(_) => false, @@ -193,4 +178,14 @@ const fn issue6067() { Ok(_) => false, Err(_) => true, }; + + match Some(42) { + Some(_) => true, + None => false, + }; + + match None::<()> { + Some(_) => false, + None => true, + }; } diff --git a/src/tools/clippy/tests/ui/redundant_pattern_matching.stderr b/src/tools/clippy/tests/ui/redundant_pattern_matching.stderr index efd2f9903ec9c..17185217e8950 100644 --- a/src/tools/clippy/tests/ui/redundant_pattern_matching.stderr +++ b/src/tools/clippy/tests/ui/redundant_pattern_matching.stderr @@ -149,79 +149,103 @@ LL | let x = if let Some(_) = opt { true } else { false }; | -------^^^^^^^------ help: try this: `if opt.is_some()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/redundant_pattern_matching.rs:103:20 + --> $DIR/redundant_pattern_matching.rs:102:20 | LL | let _ = if let Some(_) = gen_opt() { | -------^^^^^^^------------ help: try this: `if gen_opt().is_some()` error: redundant pattern matching, consider using `is_none()` - --> $DIR/redundant_pattern_matching.rs:105:19 + --> $DIR/redundant_pattern_matching.rs:104:19 | LL | } else if let None = gen_opt() { | -------^^^^------------ help: try this: `if gen_opt().is_none()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:107:19 + --> $DIR/redundant_pattern_matching.rs:106:19 | LL | } else if let Ok(_) = gen_res() { | -------^^^^^------------ help: try this: `if gen_res().is_ok()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/redundant_pattern_matching.rs:109:19 + --> $DIR/redundant_pattern_matching.rs:108:19 | LL | } else if let Err(_) = gen_res() { | -------^^^^^^------------ help: try this: `if gen_res().is_err()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/redundant_pattern_matching.rs:142:19 + --> $DIR/redundant_pattern_matching.rs:141:19 | LL | while let Some(_) = r#try!(result_opt()) {} | ----------^^^^^^^----------------------- help: try this: `while r#try!(result_opt()).is_some()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/redundant_pattern_matching.rs:143:16 + --> $DIR/redundant_pattern_matching.rs:142:16 | LL | if let Some(_) = r#try!(result_opt()) {} | -------^^^^^^^----------------------- help: try this: `if r#try!(result_opt()).is_some()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/redundant_pattern_matching.rs:149:12 + --> $DIR/redundant_pattern_matching.rs:148:12 | LL | if let Some(_) = m!() {} | -------^^^^^^^------- help: try this: `if m!().is_some()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/redundant_pattern_matching.rs:150:15 + --> $DIR/redundant_pattern_matching.rs:149:15 | LL | while let Some(_) = m!() {} | ----------^^^^^^^------- help: try this: `while m!().is_some()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:179:12 + --> $DIR/redundant_pattern_matching.rs:156:12 | LL | if let Ok(_) = Ok::(42) {} | -------^^^^^--------------------- help: try this: `if Ok::(42).is_ok()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/redundant_pattern_matching.rs:181:12 + --> $DIR/redundant_pattern_matching.rs:158:12 | LL | if let Err(_) = Err::(42) {} | -------^^^^^^---------------------- help: try this: `if Err::(42).is_err()` +error: redundant pattern matching, consider using `is_some()` + --> $DIR/redundant_pattern_matching.rs:160:12 + | +LL | if let Some(_) = Some(42) {} + | -------^^^^^^^----------- help: try this: `if Some(42).is_some()` + +error: redundant pattern matching, consider using `is_none()` + --> $DIR/redundant_pattern_matching.rs:162:12 + | +LL | if let None = None::<()> {} + | -------^^^^------------- help: try this: `if None::<()>.is_none()` + error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:183:15 + --> $DIR/redundant_pattern_matching.rs:164:15 | LL | while let Ok(_) = Ok::(10) {} | ----------^^^^^--------------------- help: try this: `while Ok::(10).is_ok()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/redundant_pattern_matching.rs:185:15 + --> $DIR/redundant_pattern_matching.rs:166:15 | LL | while let Err(_) = Ok::(10) {} | ----------^^^^^^--------------------- help: try this: `while Ok::(10).is_err()` +error: redundant pattern matching, consider using `is_some()` + --> $DIR/redundant_pattern_matching.rs:168:15 + | +LL | while let Some(_) = Some(42) {} + | ----------^^^^^^^----------- help: try this: `while Some(42).is_some()` + +error: redundant pattern matching, consider using `is_none()` + --> $DIR/redundant_pattern_matching.rs:170:15 + | +LL | while let None = None::<()> {} + | ----------^^^^------------- help: try this: `while None::<()>.is_none()` + error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:187:5 + --> $DIR/redundant_pattern_matching.rs:172:5 | LL | / match Ok::(42) { LL | | Ok(_) => true, @@ -230,7 +254,7 @@ LL | | }; | |_____^ help: try this: `Ok::(42).is_ok()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/redundant_pattern_matching.rs:192:5 + --> $DIR/redundant_pattern_matching.rs:177:5 | LL | / match Err::(42) { LL | | Ok(_) => false, @@ -238,5 +262,23 @@ LL | | Err(_) => true, LL | | }; | |_____^ help: try this: `Err::(42).is_err()` -error: aborting due to 35 previous errors +error: redundant pattern matching, consider using `is_some()` + --> $DIR/redundant_pattern_matching.rs:182:5 + | +LL | / match Some(42) { +LL | | Some(_) => true, +LL | | None => false, +LL | | }; + | |_____^ help: try this: `Some(42).is_some()` + +error: redundant pattern matching, consider using `is_none()` + --> $DIR/redundant_pattern_matching.rs:187:5 + | +LL | / match None::<()> { +LL | | Some(_) => false, +LL | | None => true, +LL | | }; + | |_____^ help: try this: `None::<()>.is_none()` + +error: aborting due to 41 previous errors From 43cba349bdf9472eafccbff2542287a1f6580c5e Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Mon, 21 Sep 2020 00:00:33 +0200 Subject: [PATCH 4/4] Remove `can_suggest` from Clippy. Removes `can_suggest` from as it is no longer used. Reverts rust-clippy#5724. --- src/tools/clippy/clippy_lints/src/matches.rs | 78 ++++---------------- 1 file changed, 16 insertions(+), 62 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/matches.rs b/src/tools/clippy/clippy_lints/src/matches.rs index db52c97475fa7..819846ebc793b 100644 --- a/src/tools/clippy/clippy_lints/src/matches.rs +++ b/src/tools/clippy/clippy_lints/src/matches.rs @@ -1440,15 +1440,12 @@ where mod redundant_pattern_match { use super::REDUNDANT_PATTERN_MATCHING; - use crate::utils::{in_constant, match_qpath, match_trait_method, paths, snippet, span_lint_and_then}; + use crate::utils::{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, HirId, MatchSource, PatKind, QPath}; + use rustc_hir::{Arm, Expr, ExprKind, MatchSource, PatKind, QPath}; use rustc_lint::LateContext; - use rustc_middle::ty; - use rustc_mir::const_eval::is_const_fn; - use rustc_span::source_map::Symbol; pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if let ExprKind::Match(op, arms, ref match_source) = &expr.kind { @@ -1468,37 +1465,24 @@ mod redundant_pattern_match { 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) { - return Some("is_ok()"); - } - if match_qpath(path, &paths::RESULT_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 { - find_suggestion(cx, hir_id, path) + 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; + } } else { - None + return; } }, - PatKind::Path(ref path) => find_suggestion(cx, hir_id, path), - _ => None, - }; - let good_method = match good_method { - Some(method) => method, - None => return, + PatKind::Path(ref path) if match_qpath(path, &paths::OPTION_NONE) => "is_none()", + _ => return, }; // check that `while_let_on_iterator` lint does not trigger @@ -1547,7 +1531,6 @@ mod redundant_pattern_match { 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, _), @@ -1562,8 +1545,6 @@ mod redundant_pattern_match { &paths::RESULT_ERR, "is_ok()", "is_err()", - || true, - || true, ) } else { None @@ -1582,8 +1563,6 @@ mod redundant_pattern_match { &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 @@ -1616,7 +1595,6 @@ mod redundant_pattern_match { } } - #[allow(clippy::too_many_arguments)] fn find_good_method_for_match<'a>( arms: &[Arm<'_>], path_left: &QPath<'_>, @@ -1625,8 +1603,6 @@ mod redundant_pattern_match { 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) @@ -1638,35 +1614,13 @@ mod redundant_pattern_match { 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)) if can_suggest_left() => Some(should_be_left), - (LitKind::Bool(false), LitKind::Bool(true)) if can_suggest_right() => Some(should_be_right), + (LitKind::Bool(true), LitKind::Bool(false)) => Some(should_be_left), + (LitKind::Bool(false), LitKind::Bool(true)) => 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)) - } } #[test]