eq_op: allow optionally check fn calls

[klensy]

Add option to allow eq_op lint to check function calls too.

changelog: [eq_op]: allow optionally check fn calls too

fixes: #13827

TODO: cleanup test

[rustbot]

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[klensy]

Okay, to test lint with config i need to place it into tests/ui-toml as other tests, but should i copypaste full tests for this lint (from tests/ui)?

[Jarcho]

Just allowing all function calls is not a great way to do this. You'll need some way kind of heuristic to determine whether the function is pure or not. You can use this as a rough example:

rust-clippy/clippy_lints/src/operators/float_cmp.rs

Lines 130 to 190 in 847e7d6

	// This is a best effort guess and may have false positives and negatives.
	fn is_pure_expr<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
	match e.kind {
	ExprKind::Path(_) | ExprKind::Lit(_) => true,
	ExprKind::Field(e, _) | ExprKind::Cast(e, _) | ExprKind::Repeat(e, _) => is_pure_expr(cx, e),
	ExprKind::Tup(args) => args.iter().all(|arg| is_pure_expr(cx, arg)),
	ExprKind::Struct(_, fields, base) => {
	base.map_or(true, |base| is_pure_expr(cx, base)) && fields.iter().all(|f| is_pure_expr(cx, f.expr))
	},
	// Since rust doesn't actually have the concept of a pure function we
	// have to guess whether it's likely pure from the signature of the
	// function.
	ExprKind::Unary(_, e) => is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(e)) && is_pure_expr(cx, e),
	ExprKind::Binary(_, x, y) | ExprKind::Index(x, y, _) => {
	is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(x))
	&& is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(y))
	&& is_pure_expr(cx, x)
	&& is_pure_expr(cx, y)
	},
	ExprKind::MethodCall(_, recv, args, _) => {
	is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(recv))
	&& is_pure_expr(cx, recv)
	&& cx
	.typeck_results()
	.type_dependent_def_id(e.hir_id)
	.is_some_and(|did| matches!(cx.tcx.fn_sig(did).skip_binder().skip_binder().safety, Safety::Safe))
	&& args
	.iter()
	.all(|arg| is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(arg)) && is_pure_expr(cx, arg))
	},
	ExprKind::Call(f, args @ [_, ..]) => {
	is_pure_expr(cx, f)
	&& is_pure_fn_ty(cx, cx.typeck_results().expr_ty_adjusted(f))
	&& args
	.iter()
	.all(|arg| is_pure_arg_ty(cx, cx.typeck_results().expr_ty_adjusted(arg)) && is_pure_expr(cx, arg))
	},
	_ => false,
	}
	}
	fn is_pure_fn_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
	let sig = match *ty.peel_refs().kind() {
	ty::FnDef(did, _) => cx.tcx.fn_sig(did).skip_binder(),
	ty::FnPtr(sig) => sig,
	ty::Closure(_, args) => {
	return args.as_closure().upvar_tys().iter().all(|ty| is_pure_arg_ty(cx, ty));
	},
	_ => return false,
	};
	matches!(sig.skip_binder().safety, Safety::Safe)
	}
	fn is_pure_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
	!ty.is_mutable_ptr()
	&& ty.is_copy_modulo_regions(cx.tcx, cx.param_env)
	&& (ty.peel_refs().is_freeze(cx.tcx, cx.param_env)
	|| !ty.has_type_flags(TypeFlags::HAS_FREE_REGIONS | TypeFlags::HAS_RE_ERASED | TypeFlags::HAS_RE_BOUND))
	}

Okay, to test lint with config i need to place it into tests/ui-toml as other tests, but should i copypaste full tests for this lint (from tests/ui)?

I would move the whole test over and use multiple revisions. You can use the absolute_paths test as an example of how that's set up.

[klensy]

Just allowing all function calls is not a great way to do this. You'll need some way kind of heuristic

This is actual intent to find all places with calls, so user can review them manually, rather than have some heuristic. I've checked top 200 crates via lintcheck and found 0 hits for eq_op lint, plus, it is disabled by default. See also linked issue for examples.

One FP over rustc compiler code.

[Jarcho]

Reasonable heuristics would catch the case you gave as an example. Given that it is, in all likelihood, possible to make a heuristic that could be both enabled by default and catch almost all desired cases, I'm not convinced this configuration is worth adding.