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

rustc suggests adding a PartialEq bound where another trait is actually required #93927

Closed
nsunderland1 opened this issue Feb 12, 2022 · 5 comments · Fixed by #94034
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints 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

@nsunderland1
Copy link
Contributor

nsunderland1 commented Feb 12, 2022

Given the following code (EDIT: see the comment below for a more concise snippet):

struct MyType<T: PartialEq>(T);

impl<T> PartialEq for MyType<T>
where
    T: PartialEq + Eq,
{
    fn eq(&self, other: &Self) -> bool {
        true
    }
}

struct MyStruct<T: PartialEq> {
    a: MyType<T>,
    b: MyType<T>,
}

impl<T> MyStruct<T>
where
    T: PartialEq,
{
    fn cond(&self) -> bool {
        self.b != self.a
    }
}

fn main() {
    let tst = MyStruct {
        a: MyType(1),
        b: MyType(2),
    };
    tst.cond();
}

https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=a69190abd7a2b258c588d8e994e69871

The current output is:

error[[E0369]](https://doc.rust-lang.org/stable/error-index.html#E0369): binary operation `!=` cannot be applied to type `MyType<T>`
  [--> src/main.rs:22:16
](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=1dacb528973e6547b8284f3039fadbb1#)   |
22 |         self.b != self.a
   |         ------ ^^ ------ MyType<T>
   |         |
   |         MyType<T>
   |
help: consider further restricting this bound
   |
19 |     T: PartialEq + std::cmp::PartialEq,
   |                  +++++++++++++++++++++

For more information about this error, try `rustc --explain E0369`.

Ideally the output should look like:

error[[E0369]](https://doc.rust-lang.org/stable/error-index.html#E0369): binary operation `!=` cannot be applied to type `MyType<T>`
  [--> src/main.rs:22:16
](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=1dacb528973e6547b8284f3039fadbb1#)   |
22 |         self.b != self.a
   |         ------ ^^ ------ MyType<T>
   |         |
   |         MyType<T>
   |
help: consider further restricting this bound
   |
19 |     T: PartialEq + std::cmp::Eq,
   |                  ++++++++++++++

For more information about this error, try `rustc --explain E0369`.

!= can't be used here because MyType<T> only implements PartialEq where T implements Eq, but instead of suggesting we add an Eq bound, rustc suggests adding an additional PartialEq bound, despite T already being PartialEq.

This bug is present on both stable and nightly.

@nsunderland1 nsunderland1 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 12, 2022
@nsunderland1
Copy link
Contributor Author

@rustbot label +D-incorrect +D-invalid-suggestion

@rustbot rustbot added D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Feb 12, 2022
@nsunderland1
Copy link
Contributor Author

nsunderland1 commented Feb 12, 2022

Reduced further: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=c6fde80b66900487eee5bd69cbde0970

struct MyType<T>(T);

impl<T> PartialEq for MyType<T>
where
    T: Eq,
{
    fn eq(&self, other: &Self) -> bool {
        true
    }
}

fn cond<T: PartialEq>(val: MyType<T>) -> bool {
    val == val
}

fn main() {
    cond(MyType(0));
}
error[[E0369]](https://doc.rust-lang.org/stable/error-index.html#E0369): binary operation `==` cannot be applied to type `MyType<T>`
  [--> src/main.rs:13:9
](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=a69190abd7a2b258c588d8e994e69871#)   |
13 |     val == val
   |     --- ^^ --- MyType<T>
   |     |
   |     MyType<T>
   |
help: consider further restricting this bound
   |
12 | fn cond<T: PartialEq + std::cmp::PartialEq>(val: MyType<T>) -> bool {
   |                      +++++++++++++++++++++

For more information about this error, try `rustc --explain E0369`.

@nsunderland1 nsunderland1 changed the title rustc suggests adding a PartialEq bound where Eq is actually required rustc suggests adding a PartialEq bound where another trait is actually required Feb 12, 2022
@nsunderland1
Copy link
Contributor Author

This issue is more general than the original title suggested: if we switch the Eq bound to Clone, the issue is there all the same

https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=6d6ad37bfa2e10e8d1ef096296bf2982

@nsunderland1
Copy link
Contributor Author

Likely a duplicate of #92347, and might be related to #93744 as well

@willcrichton
Copy link
Contributor

The relevant code is in op.rs, primarily added in this commit from @estebank: 8f40dae

I believe the issue is that the high level missing_trait is not the same as the trait needed to fulfill the actual obligation. One possible solution is to avoid using suggest_constraining_param (and getting rid of it!), and instead use suggest_restricting_param_bound like so:

 if errors.len() == 1 {
    if let Some(pred) =
        errors[0].obligation.predicate.to_opt_poly_trait_pred()
    {
        self.infcx.suggest_restricting_param_bound(
            &mut err,
            pred,
            self.body_id,
        );
    } else if *ty != lhs_ty {
        // When we know that a missing bound is responsible, we don't show
        // this note as it is redundant.
        err.note(&format!(
            "the trait `{}` is not implemented for `{}`",
            missing_trait, lhs_ty
        ));
    }
}

The upside is that this fixes the issue at hand, and it consolidates related code. The downside is that the generic suggest_restricting_param_bound does not contain as much helpful information, e.g. to suggest an Output= associated type.

If y'all think this is an appropriate fix, I am happy to file a PR.

This fixes at least these issues:

Issue #93927

Input:

struct MyType<T>(T);

impl<T> PartialEq for MyType<T>
where
    T: Eq,
{
    fn eq(&self, other: &Self) -> bool {
        true
    }
}

fn cond<T: PartialEq>(val: MyType<T>) -> bool {
    val == val
}

fn main() {
    cond(MyType(0));
}

Output:

error[E0369]: binary operation `==` cannot be applied to type `MyType<T>`
  --> test.rs:13:9
   |
13 |     val == val
   |     --- ^^ --- MyType<T>
   |     |
   |     MyType<T>
   |
help: consider further restricting this bound
   |
12 | fn cond<T: PartialEq + std::cmp::Eq>(val: MyType<T>) -> bool {
   |                      ++++++++++++++

Issue #92347

Input:

fn rot<T : std::ops::Shl<Output = T>>(x:&T, k:u32) -> T {
    let bit_count = std::mem::size_of::<T>() << 3;
    (x << k as T) | (x >> (bit_count - k as T))
}

Output:

error[E0277]: cannot subtract `T` from `usize`
 --> test4.rs:3:38
  |
3 |     (x << k as T) | (x >> (bit_count - k as T))
  |                                      ^ no implementation for `usize - T`
  |
  = help: the trait `Sub<T>` is not implemented for `usize`
help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
  |
1 | fn rot<T : std::ops::Shl<Output = T>>(x:&T, k:u32) -> T where usize: Sub<T> {
  |                                                         +++++++++++++++++++


error[E0369]: no implementation for `&T >> _`
 --> test4.rs:3:24
  |
3 |     (x << k as T) | (x >> (bit_count - k as T))
  |                      - ^^ -------------------- _
  |                      |
  |                      &T
  |
help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
  |
1 | fn rot<T : std::ops::Shl<Output = T>>(x:&T, k:u32) -> T where &T: Shr<_> {
  |

Issue #93744

Input:

use core::ops::Add;

fn add<A, B, C>(a: A, b: B) -> C {
    a + b
}

Output:

 --> test3.rs:4:7
  |
4 |     a + b
  |     - ^ - B
  |     |
  |     A
  |
help: consider restricting type parameter `A`
  |
3 | fn add<A: std::ops::Add<_>, B, C>(a: A, b: B) -> C {
  |         ++++++++++++++++++

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 26, 2022
…-binops, r=estebank

Fix incorrect suggestion for trait bounds involving binary operators

This PR fixes rust-lang#93927, rust-lang#92347, rust-lang#93744 by replacing the bespoke trait-suggestion logic in `op.rs` with a more common code path.

The downside is that this fix causes some suggestions to not include an `Output=` type, reducing their usefulness.

Note that this causes one case in the `missing-bounds.rs` test to fail rustfix. So I would need to move that code into a separate non-fix test if this PR is otherwise acceptable.
@bors bors closed this as completed in 4d0fe27 Apr 26, 2022
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 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