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

Tweaks to the blanket impls of PartialEq #23521

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

NOTE: This is not ready to land yet (it's blocked by #23519), but I'd like to discuss the idea.

The idea is simple - currently while doing comparisons we effectively remove references from both operands until one of the operands become non-reference and then we perform the actual comparison.
After this patch we remove references from operands until both of the operands become non-references and then perform the comparison. After the change all the concrete impls of PartialEq will be written for non-reference types (see vec.rs for the most impressive consequences). The same tweak can be applied to PartialOrd if needed.

It was discussed shortly in #22320
r? @alexcrichton
cc @japaric

@alexcrichton
Copy link
Member

This does seem pretty nice to me, although I'd be somewhat wary signing off on the usage of negative trait impls here. cc @aturon, @nikomatsakis, seems coherence related?

@petrochenkov
Copy link
Contributor Author

Regarding coherence, the new blanket impls restrict user-defined impls no more than the old impls. Except for user-defined impls for references, but that is intended.
Ah, and yes, I had to use negative impls due to coherence issues between the blanket impls themselves. (And it wasn't possible before the yesterday's snapshot :)

@aturon
Copy link
Member

aturon commented Mar 20, 2015

@petrochenkov This is quite neat, and it's one of the strategies @nikomatsakis and I have discussed for encoding specialization/negative bounds.

However, we're in the midst of a (another) coherence overhaul, precisely related to negative reasoning. See #23086 for example. For that reason, I'd prefer to wait on this PR until we know for sure that this kind of negative reasoning can continue to be supported.

(This will hopefully be resolved in the next few days.)

@bors
Copy link
Contributor

bors commented Apr 1, 2015

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

@petrochenkov
Copy link
Contributor Author

@aturon
It doesn't work anymore after #23867 :(
Should I close it?

@petrochenkov
Copy link
Contributor Author

In addition to not fitting into the new implementation of coherence, after #23673 type inference for operator == is inhibited for some reason when PartialEq has these new blanket impls. I doubt these problems will be resolved before 1.0, so I'm closing this PR. Hopefully, in the future specialization will solve this problem better.

Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 3, 2015
…richton

Right now comparing a `&String` (or a `&Cow`) to a `&str` requires redundant borrowing of the latter. Implementing `PartialEq<str>` tries to avoid this limitation.

```rust
struct Foo (String);

fn main () {
    let s = Foo("foo".to_string());
    match s {
        Foo(ref x) if x == &"foo" => println!("foo!"),
        // avoid this -----^
        _ => {}
    }
}
```

I was hoping that rust-lang#23521 would solve this but it didn't work out.
bors added a commit that referenced this pull request Apr 3, 2015
Right now comparing a `&String` (or a `&Cow`) to a `&str` requires redundant borrowing of the latter. Implementing `PartialEq<str>` tries to avoid this limitation.

```rust
struct Foo (String);

fn main () {
    let s = Foo("foo".to_string());
    match s {
        Foo(ref x) if x == &"foo" => println!("foo!"),
        // avoid this -----^
        _ => {}
    }
}
```

I was hoping that #23521 would solve this but it didn't work out.
@petrochenkov petrochenkov deleted the cmp branch May 9, 2015 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants