Skip to content

Overly specific PartialEq impl for Option and Result #76483

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

Open
bugaevc opened this issue Sep 8, 2020 · 3 comments
Open

Overly specific PartialEq impl for Option and Result #76483

bugaevc opened this issue Sep 8, 2020 · 3 comments
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@bugaevc
Copy link

bugaevc commented Sep 8, 2020

I tried this code:

// This works:
let s1: String = "foo".to_owned();
let s2: &str = "foo";
s1 == s2;

// But these don't:
let o1: Option<String> = Some("foo".to_owned());
let o2: Option<&str> = Some("foo");
o1 == o2;

let r1: Result<String, ()> = Ok("foo".to_owned());
let r2: Result<&str, ()> = Ok("foo");
r1 == r2;

(playground)

This is because PartialEq impls are #[derive]d for Option and Result, which gives them conservative bounds.

Instead of current

impl<T> PartialEq<Option<T>> for Option<T>
where
    T: PartialEq<T>

I'd expect

impl<A, B> PartialEq<Option<B>> for Option<A>
where
    A: PartialEq<B>

and likewise, for Result

impl<T1, T2, E1, E2> PartialEq<Result<T2, E2>> for Result<T1, E1>
where
    T1: PartialEq<T2>,
    E1: PartialEq<E2>,

Note that this is similar to what Vec<T> already implements:

impl<A, B> PartialEq<Vec<B>> for Vec<A>
where
    A: PartialEq<B>

Meta

rustc --version --verbose:

rustc 1.45.2 (d3fb005a3 2020-07-31)
binary: rustc
commit-hash: d3fb005a39e62501b8b0b356166e515ae24e2e54
commit-date: 2020-07-31
host: x86_64-unknown-linux-gnu
release: 1.45.2
LLVM version: 10.0
@bugaevc bugaevc added the C-bug Category: This is a bug. label Sep 8, 2020
@bugaevc bugaevc changed the title Too specific PartialEq impl for Option and Result Overly specific PartialEq impl for Option and Result Sep 8, 2020
@cuviper
Copy link
Member

cuviper commented Sep 9, 2020

I just tried it for Option, and it immediately showed a type-inference issue:

error[E0282]: type annotations needed
   --> library/test/src/cli.rs:293:66
    |
293 |     let mut report_time_colored = report_time && colored_opt_str == Some("colored".into());
    |                                                                  ^^      ---------------- this method call resolves to `T`
    |                                                                  |
    |                                                                  cannot infer type for type parameter `B`

The left is Option<String>, so this wouldn't even need to convert .into() anymore, but it's a bad sign that we didn't get far. I changed that, and got an even more troubling one:

error[E0282]: type annotations needed
  --> library/term/src/terminfo/searcher/tests.rs:15:37
   |
15 |     assert!(get_dbpath_for_term("") == None);
   |                                     ^^ cannot infer type for type parameter `B`

It doesn't know what kind of None this is! I changed that to is_none(), but then I hit a whole bunch of assert_eq!(something, None) in the core tests. These may not want just is_none() because assert_eq! will debug-print the different values.

So, I think this is an infeasible change.

@jyn514 jyn514 added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed C-bug Category: This is a bug. labels Sep 15, 2020
@tamird
Copy link
Contributor

tamird commented Jul 16, 2021

Reached this as well and found the same problem with Result; not even the standard library compiles because of the same inference failure that @cuviper reports above.

error[E0282]: type annotations needed
   --> /home/tamird/.cargo/registry/src/github.com-1ecc6299db9ec823/tracing-subscriber-0.2.16/src/filter/env/field.rs:285:60
    |
285 |             Some((ValueMatch::U64(ref e), ref matched)) if Ok(value) == (*e).try_into() => {
    |                                                            ^^ cannot infer type for type parameter `E` declared on the enum `Result`

@Dylan-DPC Dylan-DPC added C-bug Category: This is a bug. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Jan 5, 2024
@chris-morgan
Copy link
Member

Duplicate of #20063 (and several more exact duplicates and similar cases which have been closed as duplicate of that isue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants