Skip to content
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.

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

eq_op: allow optionally check fn calls #13828

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,9 @@ define_Conf! {
/// For internal testing only, ignores the current `publish` settings in the Cargo manifest.
#[lints(cargo_common_metadata)]
cargo_ignore_publish: bool = false,
/// Check fn calls in eq_op lint
#[lints(expect_used)]
check_fn_call_in_eq_op: bool = true,
/// Whether to also run the listed lints on private items.
#[lints(missing_errors_doc, missing_panics_doc, missing_safety_doc, unnecessary_safety_doc)]
check_private_items: bool = false,
Expand Down
20 changes: 16 additions & 4 deletions clippy_lints/src/operators/eq_op.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use clippy_utils::ast_utils::is_useless_with_eq_exprs;
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
use clippy_utils::macros::{find_assert_eq_args, first_node_macro_backtrace};
use clippy_utils::{eq_expr_value, is_in_test_function};
use clippy_utils::{eq_expr_value, eq_expr_value_with_sideffects, is_in_test_function};
use rustc_hir::{BinOpKind, Expr};
use rustc_lint::LateContext;

use super::EQ_OP;

pub(crate) fn check_assert<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
pub(crate) fn check_assert<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, with_sideffects: bool) {
if let Some((macro_call, macro_name)) = first_node_macro_backtrace(cx, e).find_map(|macro_call| {
let name = cx.tcx.item_name(macro_call.def_id);
matches!(
Expand All @@ -16,7 +16,11 @@ pub(crate) fn check_assert<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
)
.then(|| (macro_call, name))
}) && let Some((lhs, rhs, _)) = find_assert_eq_args(cx, e, macro_call.expn)
&& eq_expr_value(cx, lhs, rhs)
&& if with_sideffects {
eq_expr_value_with_sideffects(cx, lhs, rhs)
} else {
eq_expr_value(cx, lhs, rhs)
}
&& macro_call.is_local()
&& !is_in_test_function(cx.tcx, e.hir_id)
{
Expand All @@ -35,8 +39,16 @@ pub(crate) fn check<'tcx>(
op: BinOpKind,
left: &'tcx Expr<'_>,
right: &'tcx Expr<'_>,
with_sideffects: bool,
) {
if is_useless_with_eq_exprs(op) && eq_expr_value(cx, left, right) && !is_in_test_function(cx.tcx, e.hir_id) {
if is_useless_with_eq_exprs(op)
&& if with_sideffects {
eq_expr_value_with_sideffects(cx, left, right)
} else {
eq_expr_value(cx, left, right)
}
&& !is_in_test_function(cx.tcx, e.hir_id)
{
span_lint_and_then(
cx,
EQ_OP,
Expand Down
6 changes: 4 additions & 2 deletions clippy_lints/src/operators/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -841,13 +841,15 @@ pub struct Operators {
arithmetic_context: numeric_arithmetic::Context,
verbose_bit_mask_threshold: u64,
modulo_arithmetic_allow_comparison_to_zero: bool,
check_fn_call_in_eq_op: bool,
}
impl Operators {
pub fn new(conf: &'static Conf) -> Self {
Self {
arithmetic_context: numeric_arithmetic::Context::default(),
verbose_bit_mask_threshold: conf.verbose_bit_mask_threshold,
modulo_arithmetic_allow_comparison_to_zero: conf.allow_comparison_to_zero,
check_fn_call_in_eq_op: conf.check_fn_call_in_eq_op,
}
}
}
Expand Down Expand Up @@ -883,13 +885,13 @@ impl_lint_pass!(Operators => [

impl<'tcx> LateLintPass<'tcx> for Operators {
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
eq_op::check_assert(cx, e);
eq_op::check_assert(cx, e, self.check_fn_call_in_eq_op);
match e.kind {
ExprKind::Binary(op, lhs, rhs) => {
if !e.span.from_expansion() {
absurd_extreme_comparisons::check(cx, e, op.node, lhs, rhs);
if !(macro_with_not_op(lhs) || macro_with_not_op(rhs)) {
eq_op::check(cx, e, op.node, lhs, rhs);
eq_op::check(cx, e, op.node, lhs, rhs, self.check_fn_call_in_eq_op);
op_ref::check(cx, e, op.node, lhs, rhs);
}
erasing_op::check(cx, e, op.node, lhs, rhs);
Expand Down
5 changes: 5 additions & 0 deletions clippy_utils/src/hir_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,11 @@ pub fn eq_expr_value(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) ->
SpanlessEq::new(cx).deny_side_effects().eq_expr(left, right)
}

/// Checks if two expressions evaluate to the same value, ignoring side effects.
pub fn eq_expr_value_with_sideffects(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) -> bool {
SpanlessEq::new(cx).eq_expr(left, right)
}

/// Returns the segments of a path that might have generic parameters.
/// Usually just the last segment for free items, except for when the path resolves to an associated
/// item, in which case it is the last two
Expand Down
3 changes: 2 additions & 1 deletion clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ pub mod visitors;
pub use self::attrs::*;
pub use self::check_proc_macro::{is_from_proc_macro, is_span_if, is_span_match};
pub use self::hir_utils::{
HirEqInterExpr, SpanlessEq, SpanlessHash, both, count_eq, eq_expr_value, hash_expr, hash_stmt, is_bool, over,
HirEqInterExpr, SpanlessEq, SpanlessHash, both, count_eq, eq_expr_value, eq_expr_value_with_sideffects, hash_expr,
hash_stmt, is_bool, over,
};

use core::mem;
Expand Down
Loading