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

ambiguous_wide_pointer_comparisons lint does not trigger for NonNull comparisons #121264

Closed
rj00a opened this issue Feb 18, 2024 · 3 comments · Fixed by #123207
Closed

ambiguous_wide_pointer_comparisons lint does not trigger for NonNull comparisons #121264

rj00a opened this issue Feb 18, 2024 · 3 comments · Fixed by #123207
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rj00a
Copy link

rj00a commented Feb 18, 2024

Code

#![allow(dead_code)]

use std::ptr::NonNull;

trait Foo {}

fn test(a: NonNull<dyn Foo>, b: NonNull<dyn Foo>) -> bool {
    a == b
}

fn main() {}

Current output

<no warnings>

Desired output

warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
 --> src/main.rs:8:5
  |
8 |     a == b
  |     ^^^^^^
  |
  = note: `#[warn(ambiguous_wide_pointer_comparisons)]` on by default
help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
  |
8 |     std::ptr::addr_eq(a.as_ptr(), b.as_ptr())
  |     ++++++++++++++++++ ++++++++++  ++++++++++

Rationale and extra context

#117758 added the ambiguous_wide_pointer_comparisons lint, but the implementation doesn't consider pointer types beyond *const T and *mut T.

Other cases

I've also found that explicit PartialOrd::partial_cmp and Ord::cmp calls are not warned with ordinary pointers.

fn test_1(a: *const dyn Foo, b: *const dyn Foo) -> Option<std::cmp::Ordering> {
    a.partial_cmp(&b) // No warning.
}

fn test_2(a: *const dyn Foo, b: *const dyn Foo) -> std::cmp::Ordering {
    a.cmp(&b) // No warning.
}

Rust Version

rustc 1.78.0-nightly (6672c16af 2024-02-17)
binary: rustc
commit-hash: 6672c16afcd4db8acdf08a6984fd4107bf07632c
commit-date: 2024-02-17
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

Anything else?

No response

@rj00a rj00a added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 18, 2024
@Urgau
Copy link
Member

Urgau commented Feb 18, 2024

Generically detecting inner ADT comparisons cases is not going do be easy since non automatically-derived PartialOrd/PartialEq implementation can do anything in it.

However detecting the other cases with PartialOrd methods is easy.

@rustbot claim

@rj00a
Copy link
Author

rj00a commented Feb 18, 2024

I figured a special case could be made for NonNull, but I understand if there's no appetite for that.

@Urgau
Copy link
Member

Urgau commented Feb 18, 2024

I figured a special case could be made for NonNull, but I understand if there's no appetite for that.

This could indeed be a temporary solution.

The more general solution would probably involved a custom rustc attribute that would "bubble-up" the warning, since internally the NonNull methods do get the warnings!

@jieyouxu jieyouxu added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Feb 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 29, 2024
…Nadrieril

Add detection of [Partial]Ord methods in the `ambiguous_wide_pointer_comparisons` lint

Partially addresses rust-lang#121264 by adding diagnostics items for PartialOrd and Ord methods, detecting such diagnostics items as "binary operation" and suggesting the correct replacement.

I also took the opportunity to change the suggestion to use new methods `.cast()` on `*mut T` an d `*const T`.
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 30, 2024
Add detection of [Partial]Ord methods in the `ambiguous_wide_pointer_comparisons` lint

Partially addresses rust-lang/rust#121264 by adding diagnostics items for PartialOrd and Ord methods, detecting such diagnostics items as "binary operation" and suggesting the correct replacement.

I also took the opportunity to change the suggestion to use new methods `.cast()` on `*mut T` an d `*const T`.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 30, 2024
…eril

Add support for `NonNull`s in the `ambiguous_wide_ptr_comparisions` lint

This PR add support for `NonNull` pointers in the `ambiguous_wide_ptr_comparisions` lint.

Fixes rust-lang#121264
r? `@Nadrieril` (since you just reviewed rust-lang#121268, feel free to reassign)
@bors bors closed this as completed in ef49365 Mar 30, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 31, 2024
Add support for `NonNull`s in the `ambiguous_wide_ptr_comparisions` lint

This PR add support for `NonNull` pointers in the `ambiguous_wide_ptr_comparisions` lint.

Fixes rust-lang/rust#121264
r? `@Nadrieril` (since you just reviewed #121268, feel free to reassign)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants