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: bad suggestion #121330

Closed
matthiaskrgr opened this issue Feb 20, 2024 · 6 comments · Fixed by #121338
Closed

ambiguous_wide_pointer_comparisons: bad suggestion #121330

matthiaskrgr opened this issue Feb 20, 2024 · 6 comments · Fixed by #121338
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@matthiaskrgr
Copy link
Member

matthiaskrgr commented Feb 20, 2024

Code

//@ check-pass

use std::cmp::PartialEq;
use std::rc::Rc;
use std::sync::Arc;

struct A;
struct B;

trait T {}
impl T for A {}
impl T for B {}

fn main() {
    fn cmp<T: ?Sized>(a: *mut T, b: *mut T) -> bool {
        let _ = a == b;
        //~^ WARN ambiguous wide pointer comparison

        panic!();
    }
}

Current output

warning: unused import: `std::cmp::PartialEq`
 --> src/main.rs:3:5
  |
3 | use std::cmp::PartialEq;
  |     ^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: unused import: `std::rc::Rc`
 --> src/main.rs:4:5
  |
4 | use std::rc::Rc;
  |     ^^^^^^^^^^^

warning: unused import: `std::sync::Arc`
 --> src/main.rs:5:5
  |
5 | use std::sync::Arc;
  |     ^^^^^^^^^^^^^^

warning: struct `A` is never constructed
 --> src/main.rs:7:8
  |
7 | struct A;
  |        ^
  |
  = note: `#[warn(dead_code)]` on by default

warning: struct `B` is never constructed
 --> src/main.rs:8:8
  |
8 | struct B;
  |        ^

warning: trait `T` is never used
  --> src/main.rs:10:7
   |
10 | trait T {}
   |       ^

warning: function `cmp` is never used
  --> src/main.rs:15:8
   |
15 |     fn cmp<T: ?Sized>(a: *mut T, b: *mut T) -> bool {
   |        ^^^

warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
  --> src/main.rs:16:17
   |
16 |         let _ = 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
   |
16 |         let _ = std::ptr::addr_eq(a, b);
   |                 ++++++++++++++++++ ~  +
help: use explicit `std::ptr::eq` method to compare metadata and addresses
   |
16 |         let _ = std::ptr::eq(a, b);
   |                 +++++++++++++ ~  +

Desired output

No response

Rationale and extra context

The lint is marked cargo-fix able but applying the suggestion fails:

after fixes were automatically applied the compiler reported errors within these files:

  * src/main.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error[E0061]: this function takes 2 arguments but 1 argument was supplied
    --> src/main.rs:16:17
     |
16   |         let _ = std::ptr::eq(std::ptr::addr_eq(a, b));
     |                 ^^^^^^^^^^^^------------------------- an argument of type `*const _` is missing
     |
note: expected `*const _`, found `bool`
    --> src/main.rs:16:30
     |
16   |         let _ = std::ptr::eq(std::ptr::addr_eq(a, b));
     |                              ^^^^^^^^^^^^^^^^^^^^^^^
     = note: expected raw pointer `*const _`
                       found type `bool`
note: function defined here
    --> /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1932:8
     |
1932 | pub fn eq<T: ?Sized>(a: *const T, b: *const T) -> bool {
     |        ^^
help: provide the argument
     |
16   |         let _ = std::ptr::eq(/* a */, /* b */);
     |                             ~~~~~~~~~~~~~~~~~~

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0061`.
Original diagnostics will follow.

warning: `wiiide` (bin "wiiide" test) generated 7 warnings (7 duplicates)

Other cases

No response

Rust Version

rustc 1.78.0-nightly (3246e7951 2024-02-19)
binary: rustc
commit-hash: 3246e79513cb89ddbfc0f21cb5a877e5b321dcc5
commit-date: 2024-02-19
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

Anything else?

No response

@matthiaskrgr matthiaskrgr 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 20, 2024
@jieyouxu jieyouxu added A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels Feb 20, 2024
@matthiaskrgr
Copy link
Member Author

Same problem with

pub fn main() {
   pub fn cmp<T: ?Sized>(a: *mut T, b: *mut T) -> bool {
        let _ = a.eq(&b);
        //~^ WARN ambiguous wide pointer comparison

        todo!();
    }
}


warning: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
 --> src/main.rs:3:17
  |
3 |         let _ = a.eq(&b);
  |                 ^^^^^^^^
  |
  = note: `#[warn(ambiguous_wide_pointer_comparisons)]` on by default
help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
  |
3 |         let _ = std::ptr::addr_eq(a, b);
  |                 ++++++++++++++++++ ~  ~
help: use explicit `std::ptr::eq` method to compare metadata and addresses
  |
3 |         let _ = std::ptr::eq(a, b);
  |                 +++++++++++++ ~  ~

warning: `wiiide` (bin "wiiide" test) generated 1 warning (run `cargo fix --bin "wiiide" --tests` to apply 1 suggestion)
warning: failed to automatically apply fixes suggested by rustc to crate `wiiide`

after fixes were automatically applied the compiler reported errors within these files:

  * src/main.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error: mismatched closing delimiter: `}`
 --> src/main.rs:3:29
  |
3 |         let _ = std::ptr::eq(std::ptr::addr_eq(a, b);
  |                             ^ unclosed delimiter
...
7 |     }
  |     ^ mismatched closing delimiter

error: aborting due to 1 previous error

Original diagnostics will follow.

@jieyouxu
Copy link
Member

@rustbot claim

@Urgau
Copy link
Member

Urgau commented Feb 20, 2024

@matthiaskrgr @jieyouxu I think this is a known issue with cargo fix with applying all suggestions even if they are multually exclusive. I'm pretty sure there's nothing to do in the lint, it correctly suggests the two (valid) suggestions, only one of them should be applied.

@matthiaskrgr
Copy link
Member Author

matthiaskrgr commented Feb 20, 2024

Well we can either remove one of the suggestions or make the lint MaybeIncorrect or something?

@Urgau
Copy link
Member

Urgau commented Feb 20, 2024

I think this is #53934.

Well we can either remove one of the suggestions or make the lint MaybeIncorrect or something?

We can't remove one of the suggestions, they are both necessary. We can however make them MaybeIncorrect for the time being.

@jieyouxu
Copy link
Member

I think this is #53934.

That is indeed unfortunate.

@bors bors closed this as completed in e10b3b8 Feb 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 21, 2024
Rollup merge of rust-lang#121338 - jieyouxu:ambiguous_wide_pointer_comparisons_suggestion, r=Nadrieril

Downgrade ambiguous_wide_pointer_comparisons suggestions to MaybeIncorrect

In certain cases like rust-lang#121330, it is possible to have more than one suggestion from the `ambiguous_wide_pointer_comparisons` lint (which before this PR are `MachineApplicable`). When this gets passed to rustfix, rustfix makes *multiple* changes according to the suggestions which result in incorrect code.

This is a temporary workaround. The real long term solution to problems like these is to address <rust-lang#53934>.

This PR also includes a drive-by edit to the panic message emitted by compiletest because "ui" test suite now uses `//`@`` directives.

Fixes rust-lang#121330.
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-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. 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