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

clippy beta 1.87 lints all pointer comparisons, possibly too much? #14525

Open
davidhewitt opened this issue Apr 2, 2025 · 9 comments · May be fixed by #14526
Open

clippy beta 1.87 lints all pointer comparisons, possibly too much? #14525

davidhewitt opened this issue Apr 2, 2025 · 9 comments · May be fixed by #14526
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@davidhewitt
Copy link

davidhewitt commented Apr 2, 2025

Summary

As of clippy beta 1.87 the ptr_eq lint seems to be forbidding all pointer equality.

I think this seems like a regression, as far as I can tell the ptr_eq lint is intended to help in cases where &T references are being cast to pointers and then compared. Using std::ptr::eq can coerce those references and conveys intention cleanly.

On the other hand, if you already have pointers, calling std::ptr::eq doesn't seem necessary to me?

Reproducer

I tried this code:

pub fn foo() -> bool {
    let a: *const i32 = std::ptr::null();
    let b: *const i32 = std::ptr::null();
    
    a == b
}

I expected to see this happen:

Lint ok

Instead, this happened:

Clippy suggests replacing a == b with a call to std::ptr::eq.

Version

0.1.87 (2025-04-01 45165c82a4)

Additional Labels

No response

@davidhewitt davidhewitt added the C-bug Category: Clippy is not doing the correct thing label Apr 2, 2025
@davidhewitt
Copy link
Author

cc @samueltardieu @Manishearth

My guess is that #14339 introduced this change, which is titled "lint more cases" but the ticket it closed seems to be a bug rather than a proposal to make this lint different. The PR also didn't update the motivation for "why is this bad" at all.

In particular this expansion of the lint is going to hit code using raw pointers quite hard, e.g. we noticed this in pyo3-ffi.

Maybe this change of behaviour to more aggressively forbid == on pointers really is intended. The motivation for it and the churn it may cause when this hits stable doesn't seem justified from what I can see in those discussions.

For what it's worth, I'd argue against this change of behaviour. In my opinion a == b is cleaner than special casing eq(a, b) if a and b are already pointers. Why add the cognitive overhead of a special case?

(On the other hand, I can completely support the lint as it currently functions on stable to make comparison of references by pointer much clearer.)

@Manishearth
Copy link
Member

I think the change was intentional: there's a couple footguns in this area, and potentially more with provenance, so encouraging this style overall seems good.

That said, perhaps we can make it not lint for simple cases where there is no casting and the types are both raw? @samueltardieu , what do you think

cc @rust-lang/clippy on whether we should back this out, do a fix, or leave as is

@mejrs
Copy link

mejrs commented Apr 2, 2025

See https://github.com/PyO3/pyo3/pull/5032/files for what the difference looks like. Personally I find it a bit too churn-y.

@samueltardieu samueltardieu self-assigned this Apr 2, 2025
@samueltardieu
Copy link
Contributor

I think the change was intentional: there's a couple footguns in this area, and potentially more with provenance, so encouraging this style overall seems good.

That said, perhaps we can make it not lint for simple cases where there is no casting and the types are both raw? @samueltardieu , what do you think

cc @rust-lang/clippy on whether we should back this out, do a fix, or leave as is

I'll do some experiments to see how less linting would look like.

@Manishearth
Copy link
Member

See https://github.com/PyO3/pyo3/pull/5032/files for what the difference looks like. Personally I find it a bit too churn-y.

Generally we're careful about churning, but churn in and of itself isn't bad, especially with cargo clippy --fix. The question is more about whether this change is positive when compared to the amount of churn. I think it comes out mostly even from my view: the clarity in unsafe code is basically always good. But still, perhaps it doesn't actually help much here.

@samueltardieu
Copy link
Contributor

I don't find the churning bad either, but I also understand the rationale of wanting to compare two raw pointers with ==. I have proposed a change for discussion in #14526. As stated by @Manishearth it would be great to have other people's input as well.

@Alexendoo
Copy link
Member

I agree with only linting if we can remove a cast

@flip1995
Copy link
Member

flip1995 commented Apr 3, 2025

If we should change something with this lint, please label the PR with beta-nominates, so that the current state doesn't hit stable.

@davidhewitt
Copy link
Author

To add a counter-argument, in the PyO3 thread it was pointed out to me that using ptr::eq does have the upside of making it very clear that the operation is using pointer equality, not value equality.

I could accept that motivation for preferring ptr::eq. if that is decided as preferred I think it would be worth updating the lint documentation to suggest that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants