add manual_abs_diff lint

[yotamofek]

changelog: [manual_abs_diff]: Initial implementation

Hey, first time writing a new lint for clippy, hope I got it right. I think it's pretty self-explanatory!
Added a few fixme test cases, where the lint can be improved to catch more (probably rare) patterns, but opening a PR with this initial implementation to make sure I'm on the right track, and that this lint is acceptable at all.

😁

[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

[samueltardieu]

This should probably be restricted to standard integral types. This will be a false positive, with a suggestion causing a compilation error:

#[derive(Eq, PartialEq, PartialOrd)]
struct S(i32);

impl std::ops::Sub for S {
    type Output = S;

    fn sub(self, rhs: Self) -> Self::Output {
        Self(self.0 - rhs.0)
    }
}

fn main() {
    let (a, b) = (S(10), S(20));
    let _ = if a < b { b - a } else { a - b };
}

[yotamofek]

Right, I'll fix that.
Thanks for the quick review!

[samueltardieu]

Since I started the review already and have more to say:
r? @samueltardieu

[samueltardieu]

This is starting to be in a good shape. My issues right now are mostly with style, which feels a bit too verbose compared to most of the Clippy code. It is good not to have huge chunks of code, but creating intermediary types just for the sake of separating functions and using the types once seems a bit too much.

For example, the suggestions I make here should allow you to remove about 60 lines of code from this short lint (more than 35%) while keeping the intent clear and having the check_expr() method go straight to the point.

[samueltardieu]

Given the relatively low complexity of the lint, separating the check and emitting phase does not bring a lot of clarity. Moreover, it makes you introduce one additional function, one additional method, and two intermediate Suggestion and Input types.

You can inline the calls, and rewrite them a bit to be more concise. For example, it may not be needed to match all names of the If struct right away, I you can later refer to them by field access. Also, expansing the Spanned { OpKind, … } is often not done, preferring to refer to the .node attribute to get the operator kind later.

That could give something like:

Suggested change

	if !expr.span.from_expansion()
	&& let Some(suggestion) = self.is_manual_abs_diff_pattern(cx, expr)
	{
	emit_suggestion(cx, &suggestion);
	}
	if !expr.span.from_expansion()
	&& let Some(if_expr) = If::hir(expr)
	&& let Some(r#else) = if_expr.r#else
	&& let ExprKind::Binary(op, rhs, lhs) = if_expr.cond.kind
	&& let (BinOpKind::Gt | BinOpKind::Ge, a, b) | (BinOpKind::Lt | BinOpKind::Le, b, a) = (op.node, rhs, lhs)
	&& self.are_ty_eligible(cx, a, b)
	&& Self::is_sub_expr(cx, if_expr.then, a, b)
	&& Self::is_sub_expr(cx, r#else, b, a)
	{
	let a = Sugg::hir(cx, a, "..").maybe_paren();
	let b = Sugg::hir(cx, b, "..");
	span_lint_and_sugg(
	cx,
	MANUAL_ABS_DIFF,
	expr.span,
	"manual absolute difference pattern without using `abs_diff`",
	"replace with `abs_diff`",
	format!("{a}.abs_diff({b})"),
	Applicability::MachineApplicable,
	);
	}

which is readable, and more inline with the code usually found in Clippy. You can then remove is_manual_abs_diff_pattern(), emit_suggestion(), and the Suggestion and Input types.

I'll comment about are_ty_eligible() in another comment.

[samueltardieu]

Rather than checking each type individually, you can first make sure that they are identical, then check for their eligibility for the lint:

Suggested change

	fn is_ty_applicable(&self, cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
	let expr_ty = cx.typeck_results().expr_ty(expr).peel_refs();
	(matches!(expr_ty.kind(), ty::Uint(_)) && self.msrv.meets(cx, msrvs::ABS_DIFF))
	|| (is_type_diagnostic_item(cx, expr_ty, sym::Duration) && self.msrv.meets(cx, msrvs::DURATION_ABS_DIFF))
	}
	fn are_ty_eligible(&self, cx: &LateContext<'_>, a: &Expr<'_>, b: &Expr<'_>) -> bool {
	let a_ty = cx.typeck_results().expr_ty(a).peel_refs();
	a_ty == cx.typeck_results().expr_ty(b).peel_refs()
	&& ((matches!(a_ty.kind(), ty::Uint(_)) && self.msrv.meets(cx, msrvs::ABS_DIFF))
	|| (is_type_diagnostic_item(cx, a_ty, sym::Duration) && self.msrv.meets(cx, msrvs::DURATION_ABS_DIFF)))
	}

[yotamofek]

@samueltardieu applied all your suggestions, looks much better now, thanks!
I started out from manual_clamp as a "template", so a lot of the complexity was a remnant of that, but yeah - this lint is much simpler, no need for all that verbosity.

[yotamofek]

Also got rid of matching against the destructuring of Spanned in is_sub_expr.

[samueltardieu]

I'll have a new look later, thanks for the quick reaction. Can you also check for the presence of comments within the original expression? In this case, I think this is still a good idea to lint, but we should make the suggestion MaybeIncorrect as the user might not want to get rid of the comments they wrote.

[yotamofek]

Took comments into account, plus fixed the lint for literals (I hope...)

[yotamofek]

Thanks for the review and thorough explanation, much appreciated!
I'll try to use expr_type_is_certain() in the meantime, expecting some wrong results.

Just a small question about this:

if neither types are certain, you can still emit the lint, but omit the suggestion, and let the user fix it themselves

wouldn't the compiler just error out because of the ambiguous type in that case, meaning the lint won't even run?

[samueltardieu]

Thanks for the review and thorough explanation, much appreciated! I'll try to use expr_type_is_certain() in the meantime, expecting some wrong results.

Just a small question about this:

if neither types are certain, you can still emit the lint, but omit the suggestion, and let the user fix it themselves

wouldn't the compiler just error out because of the ambiguous type in that case, meaning the lint won't even run?

Unless we do the same complete type inference as the compiler, we will not see that some types may be certain. For example in

fn f(_: &mut u32) {}

fn main() {
    let mut b = 3;
    f(&mut b);
    let _ = if 5 < b {
        5 - b
    } else {
        b - 5
    };
}

the compiler knows that b is u32 because of the call to f, but we won't see that and mark both 5 and b as uncertain. And 5.abs_diff(b) won't compile, so we better not suggest it.

[Alexendoo]

IMO we should still suggest it in the face of likely inference errors, just as MaybeIncorrect. Fixing inference is generally easy to do by hand so having the starting point is a nice time saver

[samueltardieu]

IMO we should still suggest it in the face of likely inference errors, just as MaybeIncorrect. Fixing inference is generally easy to do by hand so having the starting point is a nice time saver

Agreed. I'll let just pass a few days to see if I have time to better inference detection, as I have some reservations in not putting a test with integers when I know it will fail. And:

• The problem with the current state of type certainty is that it will say that those cases are inferable (they will be false positive, not false negative)
• MaybeIncorrect suggestions are supposed to compile, so we would have to use Unspecified – and because of the first point we would do that at many places

[Alexendoo]

MaybeIncorrect is for something that could either change the behaviour of the program or result in a compile error (but doesn't include placeholders). In rust-lang/cargo#13028 it's proposed to split the two cases

New cases of Unspecified shouldn't be added

[samueltardieu]

MaybeIncorrect is for something that could either change the behaviour of the program or result in a compile error (but doesn't include placeholders). In rust-lang/cargo#13028 it's proposed to split the two cases

New cases of Unspecified shouldn't be added

As of today, the definition for MaybeIncorrect explicitly says: "The suggestion may be what the user intended, but it is uncertain. The suggestion should result in valid Rust code if it is applied.".

Also, our testsuite applies MaybeIncorrect suggestions and ensures they compile.

[Alexendoo]

In clippy as long as it produces valid syntax we use MaybeIncorrect, I don't know what the convention is on the rustc side. Practically speaking until rust-lang/cargo#13028 is resolved there's little difference between the non MachineApplicable variants

Our tests apply every suggestion no matter the applicability, suggestions that don't compile go in a separate file with rustfix disabled

[yotamofek]

I'll wait a bit with this PR to see if @samueltardieu can fix the issues with #14492, because that's obviously the best solution to the problem with ambiguous types,
but just wanted to mention that we could maybe return HasPlaceholders suggestions that look something like 2/* _type */.abs_diff(rhs)

[samueltardieu]

I'll wait a bit with this PR to see if @samueltardieu can fix the issues with #14492, because that's obviously the best solution to the problem with ambiguous types

This appears to be a can of worm: every time I add some new uncertainty resolution, it gets too conservative, and when I add some new, I find edge cases. I will continue working on this in the background, but it may take time.

What I propose so that you can go forward is to check if the left hand side is an unsuffixed literal. If it is, and the right hand side is not, swap them. We may get some suggestions that will cause errors, but they should be very rare and easy to fix for the user, and then when the expr type certainty check is better, we may switch to it, as we could in many lints.

Could you do the change, and let's see what the lintcheck run says? You may also want to rebase your commit, because of recent fixes in lintcheck handling in the CI.

we could maybe return HasPlaceholders suggestions that look something like 2/* _type */.abs_diff(rhs)

I don't think this would be beneficial.

[samueltardieu]

In parallel, I've started the "final comment period" thread for this lint.

[samueltardieu]

@pitaj suggested in the Zulip thread to also check for (a - b).abs() where a and b are of the same signed primitive integer type. Indeed, .abs_diff() does this on signed types, and has been added with the same MSRV as on unsigned types.

@yotamofek would you be willing to add this as a second commit, so that it can be squashed if appropriate or separated into a future PR if not? If you are not interested in doing this, no problem, we can open a new issue for enhancing this lint.

[yotamofek]

What I propose so that you can go forward is to check if the left hand side is an unsuffixed literal. If it is, and the right hand side is not, swap them. We may get some suggestions that will cause errors, but they should be very rare and easy to fix for the user, and then when the expr type certainty check is better, we may switch to it, as we could in many lints.

Could you do the change, and let's see what the lintcheck run says? You may also want to rebase your commit, because of recent fixes in lintcheck handling in the CI.

Sure, sounds good @samueltardieu ! Thanks!!

re: (a - b).abs(), I don't mind implementing in this PR, but I have some qualms about it. I'll explain on Zulip.

[samueltardieu]

What I propose so that you can go forward is to check if the left hand side is an unsuffixed literal. If it is, and the right hand side is not, swap them. We may get some suggestions that will cause errors, but they should be very rare and easy to fix for the user, and then when the expr type certainty check is better, we may switch to it, as we could in many lints.
Could you do the change, and let's see what the lintcheck run says? You may also want to rebase your commit, because of recent fixes in lintcheck handling in the CI.

Sure, sounds good @samueltardieu ! Thanks!!

re: (a - b).abs(), I don't mind implementing in this PR, but I have some qualms about it. I'll explain on Zulip.

Yes, it was a silly suggestion, forget about it.

[yotamofek]

@samueltardieu I think this is ready for review, hope I didn't miss anything