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

impl PartialEq<Option<&OsStr>> for Option<&str> #74369

Closed
wants to merge 1 commit into from

Conversation

pickfire
Copy link
Contributor

Allows path.extension() == Some("ext")
Long alternative path.extension().map_or(false, |ext| ext == "ext")
I wish if let Some("ext") = path.extension() works too

Allows `path.extension() == Some("ext")`
Long alternative `path.extension().map_or(false, |ext| ext == "ext")`
I wish `if let Some("ext") = path.extension()` works too
@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 15, 2020
@meithecatte
Copy link
Contributor

This seems oddly specific. Perhaps we should implement Option<T>: PartialEq<Option<U>> where T: PartialEq<U> instead?

@pickfire
Copy link
Contributor Author

Ah, that does seem possible. I thought about that but I am not sure if that's useful. Should I change it?

@meithecatte
Copy link
Contributor

It's definitely useful in more situations :)

@pickfire pickfire closed this Jul 19, 2020
@pickfire pickfire deleted the patch-1 branch July 19, 2020 14:22
@pickfire pickfire restored the patch-1 branch July 19, 2020 14:41
@pickfire
Copy link
Contributor Author

pickfire commented Jul 19, 2020

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=116c68acaf357ee257833ed6d4ea4915

   Compiling playground v0.0.1 (/playground)
error[E0119]: conflicting implementations of trait `std::cmp::PartialEq<Op<_>>` for type `Op<_>`:
  --> src/lib.rs:1:10
   |
1  |   #[derive(PartialEq, Eq)]
   |            ^^^^^^^^^ conflicting implementation for `Op<_>`
...
17 | / impl<T, U> PartialEq<Op<T>> for Op<U>
18 | | where
19 | |     T: PartialEq<U>,
20 | | {
...  |
26 | |     }
27 | | }
   | |_- first implementation here
   |
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0119`.
error: could not compile `playground`.

To learn more, run the command again with --verbose.

It doesn't seemed to work though.

@pickfire pickfire reopened this Jul 19, 2020
@meithecatte
Copy link
Contributor

That's because the manually-implemented PartialEq also covers the case that derive(PartialEq) implements. You would need to remove the derive.

@pickfire
Copy link
Contributor Author

That's because the manually-implemented PartialEq also covers the case that derive(PartialEq) implements. You would need to remove the derive.

Ah, I didn't know that. Let me check again.

@pickfire
Copy link
Contributor Author

@NieDzejkob It seemed to be breaking chance to do that.

impl<T, U> PartialEq<Op<T>> for Op<U>
where
    T: PartialEq<U>,
    U: PartialEq<T>,
{
    #[inline]
    fn eq(&self, other: &Op<T>) -> bool {
        self.as_ref()
            .zip(other.as_ref())
            .map_or(false, |(x, y)| x == y)
    }
}

#[test]
fn derive_partialeq() {
    use std::ffi::OsStr;
    assert_eq!(Op::Some("a"), Op::Some("a"));
    assert_eq!(Op::Some("a"), Op::Some(OsStr::new("a")));
    // assert_ne!(Op::Some("a"), Op::None); // does not compile
    assert_ne!(Op::Some("a"), Op::None::<&str>);
}

@meithecatte
Copy link
Contributor

Hmm, I'm out of my depth here, but perhaps there is a special feature that could be used to disambiguate this.

@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 11, 2020
@JohnCSimon
Copy link
Member

@pickfire Ping from triage: can you please address the merge conflict and build failures?

@pickfire
Copy link
Contributor Author

@JohnCSimon No, I will be creating a new patch for this in another pull request.

@pickfire pickfire closed this Aug 11, 2020
@pickfire pickfire deleted the patch-1 branch August 11, 2020 06:33
@pickfire pickfire restored the patch-1 branch August 11, 2020 06:36
@pickfire pickfire reopened this Aug 11, 2020
@pickfire
Copy link
Contributor Author

pickfire commented Aug 11, 2020

The build breaks because I cannot impl for Option<&OsStr>. I am not sure how to implement stuff not in core but in std. How do I do that?

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2020
@pickfire
Copy link
Contributor Author

@crlf0710 I need help on this, I don't know how to fix it.

@jonas-schievink
Copy link
Contributor

I don't think this is implementable without the breaking change pointed out in #74369 (comment)

@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 13, 2020
@pickfire
Copy link
Contributor Author

So I need to close this?

@crlf0710
Copy link
Member

crlf0710 commented Sep 18, 2020

IIRC unfortunately i don't think this is currently possible without Option being #[fundamental], which it is not.

IIRC if let Some("ext") = path.extension() would be supported after RFC 2500 is implemented. I've created a topic on zulip. RFC 2295 is implemented(zulip topic), which seems to depend on the implementation of RFC 2500 (zulip topic).

@Dylan-DPC-zz
Copy link

Closing this based on the above comments. If you have a better way to do this you can submit a new PR :)

@pickfire pickfire deleted the patch-1 branch October 8, 2020 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants