Skip to content

Commit

Permalink
Implement manual_and and manual_or lints
Browse files Browse the repository at this point in the history
Suggests replacing if-else expressions where
one branch is a boolean literal with && / || operators.

Co-authored-by: Francisco Salgueiro <[email protected]>
  • Loading branch information
mira-eanda and franciscoBSalgueiro committed May 24, 2024
1 parent 680256f commit 66758bc
Show file tree
Hide file tree
Showing 29 changed files with 487 additions and 146 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5451,6 +5451,7 @@ Released 2018-09-13
[`macro_metavars_in_unsafe`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_metavars_in_unsafe
[`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports
[`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
[`manual_and`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_and
[`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
[`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
Expand All @@ -5474,6 +5475,7 @@ Released 2018-09-13
[`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
[`manual_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_or
[`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
[`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns
[`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::macro_metavars_in_unsafe::MACRO_METAVARS_IN_UNSAFE_INFO,
crate::macro_use::MACRO_USE_IMPORTS_INFO,
crate::main_recursion::MAIN_RECURSION_INFO,
crate::manual_and_or::MANUAL_AND_INFO,
crate::manual_and_or::MANUAL_OR_INFO,
crate::manual_assert::MANUAL_ASSERT_INFO,
crate::manual_async_fn::MANUAL_ASYNC_FN_INFO,
crate::manual_bits::MANUAL_BITS_INFO,
Expand Down
6 changes: 1 addition & 5 deletions clippy_lints/src/escape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,6 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
impl<'a, 'tcx> EscapeDelegate<'a, 'tcx> {
fn is_large_box(&self, ty: Ty<'tcx>) -> bool {
// Large types need to be boxed to avoid stack overflows.
if ty.is_box() {
self.cx.layout_of(ty.boxed_ty()).map_or(0, |l| l.size.bytes()) > self.too_large_for_stack
} else {
false
}
ty.is_box() && self.cx.layout_of(ty.boxed_ty()).map_or(0, |l| l.size.bytes()) > self.too_large_for_stack
}
}
7 changes: 2 additions & 5 deletions clippy_lints/src/functions/must_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,8 @@ fn is_mutable_pat(cx: &LateContext<'_>, pat: &hir::Pat<'_>, tys: &mut DefIdSet)
if let hir::PatKind::Wild = pat.kind {
return false; // ignore `_` patterns
}
if cx.tcx.has_typeck_results(pat.hir_id.owner.def_id) {
is_mutable_ty(cx, cx.tcx.typeck(pat.hir_id.owner.def_id).pat_ty(pat), tys)
} else {
false
}
cx.tcx.has_typeck_results(pat.hir_id.owner.def_id)
&& is_mutable_ty(cx, cx.tcx.typeck(pat.hir_id.owner.def_id).pat_ty(pat), tys)
}

static KNOWN_WRAPPER_TYS: &[Symbol] = &[sym::Rc, sym::Arc];
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ mod loops;
mod macro_metavars_in_unsafe;
mod macro_use;
mod main_recursion;
mod manual_and_or;
mod manual_assert;
mod manual_async_fn;
mod manual_bits;
Expand Down Expand Up @@ -1165,6 +1166,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
..Default::default()
})
});
store.register_late_pass(|_| Box::new(manual_and_or::ManualAndOr));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
6 changes: 1 addition & 5 deletions clippy_lints/src/literal_representation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,7 @@ impl LiteralDigitGrouping {
}

let group_sizes: Vec<usize> = num_lit.integer.split('_').map(str::len).collect();
if UUID_GROUP_LENS.len() == group_sizes.len() {
iter::zip(&UUID_GROUP_LENS, &group_sizes).all(|(&a, &b)| a == b)
} else {
false
}
UUID_GROUP_LENS.len() == group_sizes.len() && iter::zip(&UUID_GROUP_LENS, &group_sizes).all(|(&a, &b)| a == b)
}

/// Given the sizes of the digit groups of both integral and fractional
Expand Down
193 changes: 193 additions & 0 deletions clippy_lints/src/manual_and_or.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::visitors::for_each_expr;
use clippy_utils::{get_parent_expr, higher, peel_blocks};
use core::ops::ControlFlow;
use rustc_ast::ast::LitKind;
use rustc_ast::BinOpKind;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_session::declare_lint_pass;

declare_clippy_lint! {
/// ### What it does
/// Detects `if`-then-`else` that can be replaced with `&&`.
///
/// ### Why is this bad?
/// `&&` is simpler than `if`-then-`else`.
///
/// ### Example
/// ```ignore
/// if a {
/// b
/// } else {
/// false
/// }
/// ```
/// Use instead:
/// ```ignore
/// a && b
/// ```
#[clippy::version = "1.80.0"]
pub MANUAL_AND,
complexity,
"this `if`-then-`else` that can be replaced with `&&`."
}

declare_clippy_lint! {
/// ### What it does
/// Detects `if`-then-`else` that can be replaced with `||`.
///
/// ### Why is this bad?
/// `||` is simpler than `if`-then-`else`.
///
/// ### Example
/// ```ignore
/// if a {
/// true
/// } else {
/// b
/// }
/// ```
/// Use instead:
/// ```ignore
/// a || b
/// ```
#[clippy::version = "1.80.0"]
pub MANUAL_OR,
complexity,
"this `if`-then-`else` expression can be simplified with `||`"
}

declare_lint_pass!(ManualAndOr => [MANUAL_AND, MANUAL_OR]);

fn extract_final_expression_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<String> {
if let ExprKind::Block(block, _) = expr.kind {
if let Some(final_expr) = block.expr {
return cx.sess().source_map().span_to_snippet(final_expr.span).ok();
}
}
cx.sess().source_map().span_to_snippet(expr.span).ok()
}

fn fetch_bool_expr(expr: &Expr<'_>) -> Option<bool> {
if let ExprKind::Lit(lit_ptr) = peel_blocks(expr).kind {
if let LitKind::Bool(value) = lit_ptr.node {
return Some(value);
}
}
None
}

fn contains_let(cond: &Expr<'_>) -> bool {
for_each_expr(cond, |e| {
if matches!(e.kind, ExprKind::Let(_)) {
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
})
.is_some()
}

fn contains_or(cond: &Expr<'_>) -> bool {
for_each_expr(cond, |e| {
if let ExprKind::Binary(ref n, _, _) = e.kind {
if n.node == BinOpKind::Or {
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
} else {
ControlFlow::Continue(())
}
})
.is_some()
}

fn check_and<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, cond: &Expr<'tcx>, then: &Expr<'tcx>) {
if let Some(parent) = get_parent_expr(cx, expr) {
if let ExprKind::If(_, _, _) = parent.kind {
return;
}
}
if contains_or(cond) || contains_or(then) || fetch_bool_expr(then).is_some() {
return;
}
if match then.kind {
ExprKind::Block(block, _) => !block.stmts.is_empty(),
_ => false,
} {
return;
}

let applicability = Applicability::MachineApplicable;
let cond_snippet = cx
.sess()
.source_map()
.span_to_snippet(cond.span)
.unwrap_or_else(|_| "..".to_string());

let then_snippet = extract_final_expression_snippet(cx, then).unwrap_or_else(|| "..".to_string());
let suggestion = format!("{cond_snippet} && {then_snippet}");
span_lint_and_sugg(
cx,
MANUAL_AND,
expr.span,
"this `if`-then-`else` expression can be simplified with `&&`",
"try",
suggestion,
applicability,
);
}

fn check_or<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, cond: &Expr<'tcx>, else_expr: &Expr<'tcx>) {
if matches!(else_expr.kind, ExprKind::If(..)) || fetch_bool_expr(else_expr).is_some() {
return;
}
if match else_expr.kind {
ExprKind::Block(block, _) => !block.stmts.is_empty(),
_ => false,
} {
return;
}

let applicability = Applicability::MachineApplicable;
let cond_snippet = cx
.sess()
.source_map()
.span_to_snippet(cond.span)
.unwrap_or_else(|_| "..".to_string());

let else_snippet = extract_final_expression_snippet(cx, else_expr).unwrap_or_else(|| "..".to_string());
let suggestion = format!("{cond_snippet} || {else_snippet}");
span_lint_and_sugg(
cx,
MANUAL_OR,
expr.span,
"this `if`-then-`else` expression can be simplified with `||`",
"try",
suggestion,
applicability,
);
}

impl<'tcx> LateLintPass<'tcx> for ManualAndOr {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if let Some(higher::If {
cond,
then,
r#else: Some(else_expr),
}) = higher::If::hir(expr)
{
if contains_let(cond) {
return;
}
if let Some(false) = fetch_bool_expr(else_expr) {
check_and(cx, expr, cond, then);
} else if let Some(true) = fetch_bool_expr(then) {
check_or(cx, expr, cond, else_expr);
}
}
}
}
7 changes: 2 additions & 5 deletions clippy_lints/src/methods/bind_instead_of_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,8 @@ pub(crate) trait BindInsteadOfMap {
let closure_body = cx.tcx.hir().body(body);
let closure_expr = peel_blocks(closure_body.value);

if Self::lint_closure_autofixable(cx, expr, recv, closure_expr, fn_decl_span) {
true
} else {
Self::lint_closure(cx, expr, closure_expr)
}
Self::lint_closure_autofixable(cx, expr, recv, closure_expr, fn_decl_span)
|| Self::lint_closure(cx, expr, closure_expr)
},
// `_.and_then(Some)` case, which is no-op.
hir::ExprKind::Path(QPath::Resolved(_, path)) if Self::is_variant(cx, path.res) => {
Expand Down
7 changes: 2 additions & 5 deletions clippy_lints/src/methods/chars_last_cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ use super::CHARS_LAST_CMP;

/// Checks for the `CHARS_LAST_CMP` lint.
pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool {
if chars_cmp::check(cx, info, &["chars", "last"], CHARS_LAST_CMP, "ends_with") {
true
} else {
chars_cmp::check(cx, info, &["chars", "next_back"], CHARS_LAST_CMP, "ends_with")
}
chars_cmp::check(cx, info, &["chars", "last"], CHARS_LAST_CMP, "ends_with")
|| chars_cmp::check(cx, info, &["chars", "next_back"], CHARS_LAST_CMP, "ends_with")
}
7 changes: 2 additions & 5 deletions clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ use super::CHARS_LAST_CMP;

/// Checks for the `CHARS_LAST_CMP` lint with `unwrap()`.
pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool {
if chars_cmp_with_unwrap::check(cx, info, &["chars", "last", "unwrap"], CHARS_LAST_CMP, "ends_with") {
true
} else {
chars_cmp_with_unwrap::check(cx, info, &["chars", "next_back", "unwrap"], CHARS_LAST_CMP, "ends_with")
}
chars_cmp_with_unwrap::check(cx, info, &["chars", "last", "unwrap"], CHARS_LAST_CMP, "ends_with")
|| chars_cmp_with_unwrap::check(cx, info, &["chars", "next_back", "unwrap"], CHARS_LAST_CMP, "ends_with")
}
7 changes: 2 additions & 5 deletions clippy_lints/src/methods/search_is_some.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,8 @@ pub(super) fn check<'tcx>(
else if search_method == "find" {
let is_string_or_str_slice = |e| {
let self_ty = cx.typeck_results().expr_ty(e).peel_refs();
if is_type_lang_item(cx, self_ty, hir::LangItem::String) {
true
} else {
self_ty.is_str()
}

is_type_lang_item(cx, self_ty, hir::LangItem::String) || self_ty.is_str()
};
if is_string_or_str_slice(search_recv) && is_string_or_str_slice(search_arg) {
let msg = format!("called `{option_check_method}()` after calling `find()` on a string");
Expand Down
7 changes: 2 additions & 5 deletions clippy_lints/src/non_send_fields_in_send_ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,16 +207,13 @@ fn ty_allowed_with_raw_pointer_heuristic<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'t
.all(|ty| ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait)),
ty::Array(ty, _) | ty::Slice(ty) => ty_allowed_with_raw_pointer_heuristic(cx, *ty, send_trait),
ty::Adt(_, args) => {
if contains_pointer_like(cx, ty) {
contains_pointer_like(cx, ty)
// descends only if ADT contains any raw pointers
args.iter().all(|generic_arg| match generic_arg.unpack() {
&& args.iter().all(|generic_arg| match generic_arg.unpack() {
GenericArgKind::Type(ty) => ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait),
// Lifetimes and const generics are not solid part of ADT and ignored
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => true,
})
} else {
false
}
},
// Raw pointers are `!Send` but allowed by the heuristic
ty::RawPtr(_, _) => true,
Expand Down
Loading

0 comments on commit 66758bc

Please sign in to comment.