comparison_to_empty: peel derefs in suggestions

[mdgaziur]

This PR fixes #9958. In order to fix the issue, the lint will now peel reference operators and enclose the expression with parentheses when necessary.

changelog: [comparison_to_empty]: Peel deref operators in suggestions when necessary

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) soon.

Please see the contribution instructions for more information.

[dswij]

Thanks for the PR, and welcome to Clippy!

Note that for the changelog, it's user-facing changelog. So we'd expect a short explanation of the change itself, rather than pointing it to an issue number/link. Could you help to change that?

Other than that, it's just a small comment on the change itself, and it should be good to go

[dswij]

I think this only covers 1 deref, so 2 derefs or more wouldn't be covered, i.e.

    let s = "Hello, world!";
    let s1 = &&s;
    println!("{}", **s1 == "");

[mdgaziur]

I think getting the expression that is deref'd in a loop can be a solution

[dswij]

That's one way to do it, but we have a util function that derefs expr

rust-clippy/clippy_utils/src/lib.rs

Lines 2276 to 2287 in 3cf2c17

	/// Removes `AddrOf` operators (`&`) or deref operators (`*`), but only if a reference type is
	/// dereferenced. An overloaded deref such as `Vec` to slice would not be removed.
	pub fn peel_ref_operators<'hir>(cx: &LateContext<'_>, mut expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> {
	loop {
	match expr.kind {
	ExprKind::AddrOf(_, _, e) => expr = e,
	ExprKind::Unary(UnOp::Deref, e) if cx.typeck_results().expr_ty(e).is_ref() => expr = e,
	_ => break,
	}
	}
	expr
	}

Maybe we can use that instead

[mdgaziur]

Another thing I noticed is that, parentheses aren't removed. So, something like the following code:

**(s1) == ""

will become:

(s1).is_empty()

Is there any way to remove these redundant parentheses?

[dswij]

We have another util to build suggestions, but it needs a little bit more digging:

https://github.com/rust-lang/rust-clippy/blob/78589ffad2c973f7863fdabc2df6e8ab2b54110f/clippy_utils/src/sugg.rs

IIRC, you can remove parentheses with this.

[mdgaziur]

Can you tell me what default parameter in Sugg::hir_with_applicability does?

[mdgaziur]

I found out that other lints don't remove the parentheses. Is this ok if I ignore parentheses?
AFAIK, the util you've suggested directly takes snippet from source code and has no method to remove unnecessary parentheses.

[dswij]

I think it depends on the lint whether it will suggest removing the parentheses or not. Ideally, we'd have the parentheses removed.

But it's fine if we merge it without removing it in this PR, since the change is already an improvement as it is.

Let me know if you decide not to include the parentheses change in this PR.

[mdgaziur]

I think we can ignore parentheses for now.

[dswij]

Let's also add a case for multiple derefs in the test.

[dswij]

Thanks for this! @bors r+

[bors]

📌 Commit 7a0f0c0 has been approved by dswij

It is now in the queue for this repository.

[bors]

⌛ Testing commit 7a0f0c0 with merge 56d5657...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 56d5657 to master...