Skip to content

Simplify negative Option::{is_some_and,is_none_or} #13443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Simplify negative Option::{is_some_and,is_none_or} #13443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clippy_config/src/msrvs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ macro_rules! msrv_aliases {
msrv_aliases! {
1,83,0 { CONST_EXTERN_FN }
1,83,0 { CONST_FLOAT_BITS_CONV }
1,82,0 { IS_NONE_OR }
1,81,0 { LINT_REASONS_STABILIZATION }
1,80,0 { BOX_INTO_ITER}
1,77,0 { C_STR_LITERALS }
Expand Down
78 changes: 60 additions & 18 deletions clippy_lints/src/booleans.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use clippy_config::Conf;
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
use clippy_utils::eq_expr_value;
use clippy_utils::source::SpanRangeExt;
Expand All @@ -7,7 +9,7 @@ use rustc_errors::Applicability;
use rustc_hir::intravisit::{FnKind, Visitor, walk_expr};
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, UnOp};
use rustc_lint::{LateContext, LateLintPass, Level};
use rustc_session::declare_lint_pass;
use rustc_session::{RustcVersion, impl_lint_pass};
use rustc_span::def_id::LocalDefId;
use rustc_span::{Span, sym};

Expand Down Expand Up @@ -69,9 +71,25 @@ declare_clippy_lint! {
}

// For each pairs, both orders are considered.
const METHODS_WITH_NEGATION: [(&str, &str); 2] = [("is_some", "is_none"), ("is_err", "is_ok")];
const METHODS_WITH_NEGATION: [(Option<RustcVersion>, &str, &str); 3] = [
(None, "is_some", "is_none"),
(None, "is_err", "is_ok"),
(Some(msrvs::IS_NONE_OR), "is_some_and", "is_none_or"),
];

pub struct NonminimalBool {
msrv: Msrv,
}

impl NonminimalBool {
pub fn new(conf: &'static Conf) -> Self {
Self {
msrv: conf.msrv.clone(),
}
}
}

declare_lint_pass!(NonminimalBool => [NONMINIMAL_BOOL, OVERLY_COMPLEX_BOOL_EXPR]);
impl_lint_pass!(NonminimalBool => [NONMINIMAL_BOOL, OVERLY_COMPLEX_BOOL_EXPR]);

impl<'tcx> LateLintPass<'tcx> for NonminimalBool {
fn check_fn(
Expand All @@ -83,7 +101,7 @@ impl<'tcx> LateLintPass<'tcx> for NonminimalBool {
_: Span,
_: LocalDefId,
) {
NonminimalBoolVisitor { cx }.visit_body(body);
NonminimalBoolVisitor { cx, msrv: &self.msrv }.visit_body(body);
}

fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
Expand All @@ -100,6 +118,8 @@ impl<'tcx> LateLintPass<'tcx> for NonminimalBool {
_ => {},
}
}

extract_msrv_attr!(LateContext);
}

fn inverted_bin_op_eq_str(op: BinOpKind) -> Option<&'static str> {
Expand Down Expand Up @@ -176,11 +196,11 @@ fn check_inverted_bool_in_condition(
);
}

fn check_simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) {
fn check_simplify_not(cx: &LateContext<'_>, msrv: &Msrv, expr: &Expr<'_>) {
if let ExprKind::Unary(UnOp::Not, inner) = &expr.kind
&& !expr.span.from_expansion()
&& !inner.span.from_expansion()
&& let Some(suggestion) = simplify_not(cx, inner)
&& let Some(suggestion) = simplify_not(cx, msrv, inner)
&& cx.tcx.lint_level_at_node(NONMINIMAL_BOOL, expr.hir_id).0 != Level::Allow
{
span_lint_and_sugg(
Expand All @@ -197,6 +217,7 @@ fn check_simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) {

struct NonminimalBoolVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
msrv: &'a Msrv,
}

use quine_mc_cluskey::Bool;
Expand Down Expand Up @@ -289,6 +310,7 @@ impl<'a, 'tcx, 'v> Hir2Qmm<'a, 'tcx, 'v> {
struct SuggestContext<'a, 'tcx, 'v> {
terminals: &'v [&'v Expr<'v>],
cx: &'a LateContext<'tcx>,
msrv: &'a Msrv,
output: String,
}

Expand All @@ -311,7 +333,7 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
},
Term(n) => {
let terminal = self.terminals[n as usize];
if let Some(str) = simplify_not(self.cx, terminal) {
if let Some(str) = simplify_not(self.cx, self.msrv, terminal) {
self.output.push_str(&str);
} else {
self.output.push('!');
Expand Down Expand Up @@ -358,7 +380,7 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
}
}

fn simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> {
fn simplify_not(cx: &LateContext<'_>, curr_msrv: &Msrv, expr: &Expr<'_>) -> Option<String> {
match &expr.kind {
ExprKind::Binary(binop, lhs, rhs) => {
if !implements_ord(cx, lhs) {
Expand Down Expand Up @@ -389,7 +411,7 @@ fn simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> {
Some(format!("{lhs_snippet}{op}{rhs_snippet}"))
})
},
ExprKind::MethodCall(path, receiver, [], _) => {
ExprKind::MethodCall(path, receiver, args, _) => {
let type_of_receiver = cx.typeck_results().expr_ty(receiver);
if !is_type_diagnostic_item(cx, type_of_receiver, sym::Option)
&& !is_type_diagnostic_item(cx, type_of_receiver, sym::Result)
Expand All @@ -399,21 +421,41 @@ fn simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> {
METHODS_WITH_NEGATION
.iter()
.copied()
.flat_map(|(a, b)| vec![(a, b), (b, a)])
.find(|&(a, _)| {
let path: &str = path.ident.name.as_str();
a == path
.flat_map(|(msrv, a, b)| vec![(msrv, a, b), (msrv, b, a)])
.find(|&(msrv, a, _)| msrv.is_none_or(|msrv| curr_msrv.meets(msrv)) && a == path.ident.name.as_str())
.and_then(|(_, _, neg_method)| {
let negated_args = args
.iter()
.map(|arg| simplify_not(cx, curr_msrv, arg))
.collect::<Option<Vec<_>>>()?
.join(", ");
Some(format!(
"{}.{neg_method}({negated_args})",
receiver.span.get_source_text(cx)?
))
})
.and_then(|(_, neg_method)| Some(format!("{}.{neg_method}()", receiver.span.get_source_text(cx)?)))
},
ExprKind::Closure(closure) => {
let body = cx.tcx.hir().body(closure.body);
let params = body
.params
.iter()
.map(|param| param.span.get_source_text(cx).map(|t| t.to_string()))
.collect::<Option<Vec<_>>>()?
.join(", ");
let negated = simplify_not(cx, curr_msrv, body.value)?;
Some(format!("|{params}| {negated}"))
},
ExprKind::Unary(UnOp::Not, expr) => expr.span.get_source_text(cx).map(|t| t.to_string()),
_ => None,
}
}

fn suggest(cx: &LateContext<'_>, suggestion: &Bool, terminals: &[&Expr<'_>]) -> String {
fn suggest(cx: &LateContext<'_>, msrv: &Msrv, suggestion: &Bool, terminals: &[&Expr<'_>]) -> String {
let mut suggest_context = SuggestContext {
terminals,
cx,
msrv,
output: String::new(),
};
suggest_context.recurse(suggestion);
Expand Down Expand Up @@ -526,7 +568,7 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
diag.span_suggestion(
e.span,
"it would look like the following",
suggest(self.cx, suggestion, &h2q.terminals),
suggest(self.cx, self.msrv, suggestion, &h2q.terminals),
// nonminimal_bool can produce minimal but
// not human readable expressions (#3141)
Applicability::Unspecified,
Expand Down Expand Up @@ -569,12 +611,12 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
}
};
if improvements.is_empty() {
check_simplify_not(self.cx, e);
check_simplify_not(self.cx, self.msrv, e);
} else {
nonminimal_bool_lint(
improvements
.into_iter()
.map(|suggestion| suggest(self.cx, suggestion, &h2q.terminals))
.map(|suggestion| suggest(self.cx, self.msrv, suggestion, &h2q.terminals))
.collect(),
);
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(move |tcx| Box::new(await_holding_invalid::AwaitHolding::new(tcx, conf)));
store.register_late_pass(|_| Box::new(serde_api::SerdeApi));
store.register_late_pass(move |_| Box::new(types::Types::new(conf)));
store.register_late_pass(|_| Box::new(booleans::NonminimalBool));
store.register_late_pass(move |_| Box::new(booleans::NonminimalBool::new(conf)));
store.register_late_pass(|_| Box::new(enum_clike::UnportableVariant));
store.register_late_pass(|_| Box::new(float_literal::FloatLiteral));
store.register_late_pass(|_| Box::new(ptr::Ptr));
Expand Down
62 changes: 62 additions & 0 deletions tests/ui/nonminimal_bool_methods.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,66 @@ fn issue_12625() {
if a as u64 > b {} //~ ERROR: this boolean expression can be simplified
}

fn issue_13436() {
fn not_zero(x: i32) -> bool {
x != 0
}

let opt = Some(500);
_ = opt.is_some_and(|x| x < 1000);
_ = opt.is_some_and(|x| x <= 1000);
_ = opt.is_some_and(|x| x > 1000);
_ = opt.is_some_and(|x| x >= 1000);
_ = opt.is_some_and(|x| x == 1000);
_ = opt.is_some_and(|x| x != 1000);
_ = opt.is_some_and(not_zero);
_ = opt.is_none_or(|x| x >= 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x > 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x <= 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x < 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x != 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x == 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_some_and(not_zero);
_ = opt.is_none_or(|x| x < 1000);
_ = opt.is_none_or(|x| x <= 1000);
_ = opt.is_none_or(|x| x > 1000);
_ = opt.is_none_or(|x| x >= 1000);
_ = opt.is_none_or(|x| x == 1000);
_ = opt.is_none_or(|x| x != 1000);
_ = opt.is_none_or(not_zero);
_ = opt.is_some_and(|x| x >= 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_some_and(|x| x > 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_some_and(|x| x <= 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_some_and(|x| x < 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_some_and(|x| x != 1000); //~ ERROR: this boolean expression can be simplified
_ = opt.is_some_and(|x| x == 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(not_zero);

let opt = Some(true);
_ = opt.is_some_and(|x| x);
_ = opt.is_some_and(|x| !x);
_ = !opt.is_some_and(|x| x);
_ = opt.is_none_or(|x| x); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x);
_ = opt.is_none_or(|x| !x);
_ = !opt.is_none_or(|x| x);
_ = opt.is_some_and(|x| x); //~ ERROR: this boolean expression can be simplified

let opt: Option<Result<i32, i32>> = Some(Ok(123));
_ = opt.is_some_and(|x| x.is_ok());
_ = opt.is_some_and(|x| x.is_err());
_ = opt.is_none_or(|x| x.is_ok());
_ = opt.is_none_or(|x| x.is_err());
_ = opt.is_none_or(|x| x.is_err()); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x.is_ok()); //~ ERROR: this boolean expression can be simplified
_ = opt.is_some_and(|x| x.is_err()); //~ ERROR: this boolean expression can be simplified
_ = opt.is_some_and(|x| x.is_ok()); //~ ERROR: this boolean expression can be simplified

#[clippy::msrv = "1.81"]
fn before_stabilization() {
let opt = Some(500);
_ = !opt.is_some_and(|x| x < 1000);
}
}

fn main() {}
62 changes: 62 additions & 0 deletions tests/ui/nonminimal_bool_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,66 @@ fn issue_12625() {
if !(a as u64 <= b) {} //~ ERROR: this boolean expression can be simplified
}

fn issue_13436() {
fn not_zero(x: i32) -> bool {
x != 0
}

let opt = Some(500);
_ = opt.is_some_and(|x| x < 1000);
_ = opt.is_some_and(|x| x <= 1000);
_ = opt.is_some_and(|x| x > 1000);
_ = opt.is_some_and(|x| x >= 1000);
_ = opt.is_some_and(|x| x == 1000);
_ = opt.is_some_and(|x| x != 1000);
_ = opt.is_some_and(not_zero);
_ = !opt.is_some_and(|x| x < 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_some_and(|x| x <= 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_some_and(|x| x > 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_some_and(|x| x >= 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_some_and(|x| x == 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_some_and(|x| x != 1000); //~ ERROR: this boolean expression can be simplified
Copy link
Contributor

@xFrednet xFrednet Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since simplify_not is called recursively on closures, could you add a recursive test? Something like this:

let opt_opt = Some(opt);
!opt_opt.is_some_and(|x| !x.is_some_and(|y| y != 1000));

I just want to make sure that the output is correct

Copy link
Contributor Author

@SpriteOvO SpriteOvO Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! The test triggers an error cannot replace slice of data that was already replaced. I'm trying to fix it.

Copy link
Contributor

@xFrednet xFrednet Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, that happens if two suggestions overlap. Don't spend too much time on it. Overlapping suggestions are okay if they are still correct. You can also just create a second test file that doesn't run rustfix.

Copy link
Contributor Author

@SpriteOvO SpriteOvO Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learned it! It looks like for users, rustfix will apply the first fix and ignore the subsequent. So !opt_opt.is_some_and(|x| !x.is_some_and(|y| y != 1000)) will be fixed to opt_opt.is_none_or(|x| x.is_some_and(|y| y != 1000)).

I have added the test case to the PR.

_ = !opt.is_some_and(not_zero);
_ = opt.is_none_or(|x| x < 1000);
_ = opt.is_none_or(|x| x <= 1000);
_ = opt.is_none_or(|x| x > 1000);
_ = opt.is_none_or(|x| x >= 1000);
_ = opt.is_none_or(|x| x == 1000);
_ = opt.is_none_or(|x| x != 1000);
_ = opt.is_none_or(not_zero);
_ = !opt.is_none_or(|x| x < 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(|x| x <= 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(|x| x > 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(|x| x >= 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(|x| x == 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(|x| x != 1000); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(not_zero);

let opt = Some(true);
_ = opt.is_some_and(|x| x);
_ = opt.is_some_and(|x| !x);
_ = !opt.is_some_and(|x| x);
_ = !opt.is_some_and(|x| !x); //~ ERROR: this boolean expression can be simplified
_ = opt.is_none_or(|x| x);
_ = opt.is_none_or(|x| !x);
_ = !opt.is_none_or(|x| x);
_ = !opt.is_none_or(|x| !x); //~ ERROR: this boolean expression can be simplified

let opt: Option<Result<i32, i32>> = Some(Ok(123));
_ = opt.is_some_and(|x| x.is_ok());
_ = opt.is_some_and(|x| x.is_err());
_ = opt.is_none_or(|x| x.is_ok());
_ = opt.is_none_or(|x| x.is_err());
_ = !opt.is_some_and(|x| x.is_ok()); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_some_and(|x| x.is_err()); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(|x| x.is_ok()); //~ ERROR: this boolean expression can be simplified
_ = !opt.is_none_or(|x| x.is_err()); //~ ERROR: this boolean expression can be simplified

#[clippy::msrv = "1.81"]
fn before_stabilization() {
let opt = Some(500);
_ = !opt.is_some_and(|x| x < 1000);
}
}

fn main() {}
Loading